[PATCH dwarves 0/4] BTF ELF writing changes

[PATCH dwarves 0/4] BTF ELF writing changes

[Giuliano Procida]

Hi.

This follows on from my change to improve the error handling around
llvm-objcopy in libbtf.c.

Note on recipients: Please let me know if I should adjust To or CC.

Note on style: I've generally placed declarations as allowed by C99,
closest to point of use. Let me know if you'd prefer otherwise.

1. Improve ELF error reporting

2. Add .BTF section using libelf

This shows the minimal amount of code needed to drive libelf. However,
it leaves layout up to libelf, which is almost certainly not wanted.

As an unexpcted side-effect, vmlinux is larger than before. It seems
llvm-objcopy likes to trim down .strtab.

3. Manually lay out updated ELF sections

This does full layout of new and updated ELF sections. If the update
ELF sections were not the last ones in the file by offset, then it can
leave gaps between sections.

4. Align .BTF section to 8 bytes

This was my original aim.

Regards.

Giuliano Procida (4):
  btf_encoder: Improve ELF error reporting
  btf_encoder: Add .BTF section using libelf
  btf_encoder: Manually lay out updated ELF sections
  btf_encoder: Align .BTF section to 8 bytes

 libbtf.c | 222 +++++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 175 insertions(+), 47 deletions(-)

-- 
2.30.0.280.ga3ce27912f-goog

[PATCH dwarves 1/4] btf_encoder: Improve ELF error reporting

[Giuliano Procida]

libelf provides an errno/strerror-like facility. This commit updates
various error messages to include helpful error text.

Signed-off-by: Giuliano Procida <gprocida@google.com>
---
 libbtf.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/libbtf.c b/libbtf.c
index 7552d8e..9f76283 100644
--- a/libbtf.c
+++ b/libbtf.c
@@ -110,8 +110,8 @@ try_as_raw_btf:
 
 		btfe->elf = elf_begin(btfe->in_fd, ELF_C_READ_MMAP, NULL);
 		if (!btfe->elf) {
-			fprintf(stderr, "%s: cannot read %s ELF file.\n",
-				__func__, filename);
+			fprintf(stderr, "%s: cannot read %s ELF file: %s.\n",
+				__func__, filename, elf_errmsg(elf_errno()));
 			goto errout;
 		}
 	}
@@ -707,13 +707,15 @@ static int btf_elf__write(const char *filename, struct btf *btf)
 	}
 
 	if (elf_version(EV_CURRENT) == EV_NONE) {
-		fprintf(stderr, "Cannot set libelf version.\n");
+		fprintf(stderr, "Cannot set libelf version: %s.\n",
+			elf_errmsg(elf_errno()));
 		goto out;
 	}
 
 	elf = elf_begin(fd, ELF_C_RDWR, NULL);
 	if (elf == NULL) {
-		fprintf(stderr, "Cannot update ELF file.\n");
+		fprintf(stderr, "Cannot update ELF file: %s.\n",
+			elf_errmsg(elf_errno()));
 		goto out;
 	}
 
@@ -721,7 +723,8 @@ static int btf_elf__write(const char *filename, struct btf *btf)
 
 	ehdr = gelf_getehdr(elf, &ehdr_mem);
 	if (ehdr == NULL) {
-		fprintf(stderr, "%s: elf_getehdr failed.\n", __func__);
+		fprintf(stderr, "%s: elf_getehdr failed: %s.\n", __func__,
+			elf_errmsg(elf_errno()));
 		goto out;
 	}
 
@@ -764,6 +767,9 @@ static int btf_elf__write(const char *filename, struct btf *btf)
 		if (elf_update(elf, ELF_C_NULL) >= 0 &&
 		    elf_update(elf, ELF_C_WRITE) >= 0)
 			err = 0;
+		else
+			fprintf(stderr, "%s: elf_update failed: %s.\n",
+				__func__, elf_errmsg(elf_errno()));
 	} else {
 		const char *llvm_objcopy;
 		char tmp_fn[PATH_MAX];
-- 
2.30.0.280.ga3ce27912f-goog

[PATCH dwarves 2/4] btf_encoder: Add .BTF section using libelf

[Giuliano Procida]

pahole -J uses libelf directly when updating a .BTF section. However,
it uses llvm-objcopy to add .BTF sections. This commit switches to
using libelf for both cases.

This eliminates pahole's dependency on llvm-objcopy. One unfortunate
side-effect is that vmlinux actually increases in size. It seems that
llvm-objcopy modifies the .strtab section, discarding many strings. I
speculate that is it discarding strings not referenced from .symtab
and updating the references therein.

In this initial version layout is left completely up to libelf which
may be OK for non-loadable object files, but is probably no good for
things like vmlinux where all the offsets may change. This is
addressed in a follow-up commit.

Signed-off-by: Giuliano Procida <gprocida@google.com>
---
 libbtf.c | 145 ++++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 100 insertions(+), 45 deletions(-)

diff --git a/libbtf.c b/libbtf.c
index 9f76283..fb8e043 100644
--- a/libbtf.c
+++ b/libbtf.c
@@ -699,6 +699,7 @@ static int btf_elf__write(const char *filename, struct btf *btf)
 	uint32_t raw_btf_size;
 	int fd, err = -1;
 	size_t strndx;
+	void *str_table = NULL;
 
 	fd = open(filename, O_RDWR);
 	if (fd < 0) {
@@ -741,74 +742,128 @@ static int btf_elf__write(const char *filename, struct btf *btf)
 	}
 
 	/*
-	 * First we look if there was already a .BTF section to overwrite.
+	 * First we check if there is already a .BTF section present.
 	 */
-
 	elf_getshdrstrndx(elf, &strndx);
+	Elf_Scn *btf_scn = 0;
 	while ((scn = elf_nextscn(elf, scn)) != NULL) {
 		shdr = gelf_getshdr(scn, &shdr_mem);
 		if (shdr == NULL)
 			continue;
 		char *secname = elf_strptr(elf, strndx, shdr->sh_name);
 		if (strcmp(secname, ".BTF") == 0) {
-			btf_data = elf_getdata(scn, btf_data);
+			btf_scn = scn;
 			break;
 		}
 	}
 
-	raw_btf_data = btf__get_raw_data(btf, &raw_btf_size);
-
-	if (btf_data) {
-		/* Exisiting .BTF section found */
-		btf_data->d_buf = (void *)raw_btf_data;
-		btf_data->d_size = raw_btf_size;
-		elf_flagdata(btf_data, ELF_C_SET, ELF_F_DIRTY);
+	Elf_Scn *str_scn = elf_getscn(elf, strndx);
+	if (!str_scn) {
+		fprintf(stderr, "%s: elf_getscn(strndx) failed\n", __func__);
+		goto out;
+	}
 
-		if (elf_update(elf, ELF_C_NULL) >= 0 &&
-		    elf_update(elf, ELF_C_WRITE) >= 0)
-			err = 0;
-		else
-			fprintf(stderr, "%s: elf_update failed: %s.\n",
-				__func__, elf_errmsg(elf_errno()));
+	size_t dot_btf_offset = 0;
+	if (btf_scn) {
+		/* Existing .BTF section found */
+		btf_data = elf_getdata(btf_scn, NULL);
+		if (!btf_data) {
+			fprintf(stderr, "%s: elf_getdata failed: %s\n", __func__,
+				elf_errmsg(elf_errno()));
+			goto out;
+		}
 	} else {
-		const char *llvm_objcopy;
-		char tmp_fn[PATH_MAX];
-		char cmd[PATH_MAX * 2];
-
-		llvm_objcopy = getenv("LLVM_OBJCOPY");
-		if (!llvm_objcopy)
-			llvm_objcopy = "llvm-objcopy";
-
-		/* Use objcopy to add a .BTF section */
-		snprintf(tmp_fn, sizeof(tmp_fn), "%s.btf", filename);
-		close(fd);
-		fd = creat(tmp_fn, S_IRUSR | S_IWUSR);
-		if (fd == -1) {
-			fprintf(stderr, "%s: open(%s) failed!\n", __func__,
-				tmp_fn);
+		/* Add ".BTF" to the section name string table */
+		Elf_Data *str_data = elf_getdata(str_scn, NULL);
+		if (!str_data) {
+			fprintf(stderr, "%s: elf_getdata(str_scn) failed: %s\n",
+				__func__, elf_errmsg(elf_errno()));
 			goto out;
 		}
-
-		if (write(fd, raw_btf_data, raw_btf_size) != raw_btf_size) {
-			fprintf(stderr, "%s: write of %d bytes to '%s' failed: %d!\n",
-				__func__, raw_btf_size, tmp_fn, errno);
-			goto unlink;
+		dot_btf_offset = str_data->d_size;
+		size_t new_str_size = dot_btf_offset + 5;
+		str_table = malloc(new_str_size);
+		if (!str_table) {
+			fprintf(stderr, "%s: malloc(%zu) failed: %s\n", __func__,
+				new_str_size, elf_errmsg(elf_errno()));
+			goto out;
 		}
+		memcpy(str_table, str_data->d_buf, dot_btf_offset);
+		memcpy(str_table + dot_btf_offset, ".BTF", 5);
+		str_data->d_buf = str_table;
+		str_data->d_size = new_str_size;
+		elf_flagdata(str_data, ELF_C_SET, ELF_F_DIRTY);
+
+		/* Create a new section */
+		btf_scn = elf_newscn(elf);
+		if (!btf_scn) {
+			fprintf(stderr, "%s: elf_newscn failed: %s\n",
+			__func__, elf_errmsg(elf_errno()));
+			goto out;
+		}
+		btf_data = elf_newdata(btf_scn);
+		if (!btf_data) {
+			fprintf(stderr, "%s: elf_newdata failed: %s\n",
+			__func__, elf_errmsg(elf_errno()));
+			goto out;
+		}
+	}
 
-		snprintf(cmd, sizeof(cmd), "%s --add-section .BTF=%s %s",
-			 llvm_objcopy, tmp_fn, filename);
-		if (system(cmd)) {
-			fprintf(stderr, "%s: failed to add .BTF section to '%s': %d!\n",
-				__func__, filename, errno);
-			goto unlink;
+	/* (Re)populate the BTF section data */
+	raw_btf_data = btf__get_raw_data(btf, &raw_btf_size);
+	btf_data->d_buf = (void *)raw_btf_data;
+	btf_data->d_size = raw_btf_size;
+	btf_data->d_type = ELF_T_BYTE;
+	btf_data->d_version = EV_CURRENT;
+	elf_flagdata(btf_data, ELF_C_SET, ELF_F_DIRTY);
+
+	/* Update .BTF section in the SHT */
+	GElf_Shdr btf_shdr_mem;
+	GElf_Shdr *btf_shdr = gelf_getshdr(btf_scn, &btf_shdr_mem);
+	if (!btf_shdr) {
+		fprintf(stderr, "%s: elf_getshdr(btf_scn) failed: %s\n",
+			__func__, elf_errmsg(elf_errno()));
+		goto out;
+	}
+	btf_shdr->sh_entsize = 0;
+	btf_shdr->sh_flags = 0;
+	if (dot_btf_offset)
+		btf_shdr->sh_name = dot_btf_offset;
+	btf_shdr->sh_type = SHT_PROGBITS;
+	if (!gelf_update_shdr(btf_scn, btf_shdr)) {
+		fprintf(stderr, "%s: gelf_update_shdr failed: %s\n",
+			__func__, elf_errmsg(elf_errno()));
+		goto out;
+	}
+
+	if (elf_update(elf, ELF_C_NULL) < 0) {
+		fprintf(stderr, "%s: elf_update (layout) failed: %s\n",
+			__func__, elf_errmsg(elf_errno()));
+		goto out;
+	}
+
+	size_t phnum = 0;
+	if (!elf_getphdrnum(elf, &phnum)) {
+		for (size_t ix = 0; ix < phnum; ++ix) {
+			GElf_Phdr phdr;
+			GElf_Phdr *elf_phdr = gelf_getphdr(elf, ix, &phdr);
+			size_t filesz = gelf_fsize(elf, ELF_T_PHDR, 1, EV_CURRENT);
+			fprintf(stderr, "type: %d %d\n", elf_phdr->p_type, PT_PHDR);
+			fprintf(stderr, "offset: %lu %lu\n", elf_phdr->p_offset, ehdr->e_phoff);
+			fprintf(stderr, "filesize: %lu %lu\n", elf_phdr->p_filesz, filesz);
 		}
+	}
 
-		err = 0;
-	unlink:
-		unlink(tmp_fn);
+	if (elf_update(elf, ELF_C_WRITE) < 0) {
+		fprintf(stderr, "%s: elf_update (write) failed: %s\n",
+			__func__, elf_errmsg(elf_errno()));
+		goto out;
 	}
+	err = 0;
 
 out:
+	if (str_table)
+		free(str_table);
 	if (fd != -1)
 		close(fd);
 	if (elf)
-- 
2.30.0.280.ga3ce27912f-goog

[PATCH dwarves 3/4] btf_encoder: Manually lay out updated ELF sections

[Giuliano Procida]

pahole -J needs to do the following to an ELF file:

* add or update the ".BTF" section
* maybe update the section name string table
* update the Section Header Table (SHT)

libelf either takes full control of layout or requires the user to
specify offset, size and alignment of all new and updated sections and
headers.

To avoid libelf moving program segments in particular, we position the
".BTF" and section name string table (typically named ".shstrtab")
sections after all others. The SHT always lives at the end of the file.

Note that the last section in an ELF file is normally the section name
string table and any ".BTF" section will normally be second last.
However, if these sections appear earlier, then we'll waste some space
in the ELF file when we rewrite them.

Signed-off-by: Giuliano Procida <gprocida@google.com>
---
 libbtf.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 62 insertions(+), 2 deletions(-)

diff --git a/libbtf.c b/libbtf.c
index fb8e043..4726e16 100644
--- a/libbtf.c
+++ b/libbtf.c
@@ -742,9 +742,28 @@ static int btf_elf__write(const char *filename, struct btf *btf)
 	}
 
 	/*
-	 * First we check if there is already a .BTF section present.
+	 * The SHT is at the very end of the ELF and gets re-written in any case.
+	 *
+	 * We'll always add or update the .BTF section and when adding have to
+	 * re-write the section name string table (usually named .shstrtab). In
+	 * fact, as good citizens, we'll always leave the string table last, in
+	 * case someone else wants to add a section.
+	 *
+	 * However, if .BTF or the section name string table are followed by
+	 * further sections, we'll not try to be clever about shuffling
+	 * everything else in the ELF file, we'll just leave some dead space.
+	 * This actually happens in practice with vmlinux which has .strtab
+	 * after .shstrtab, resulting in a (small) hole the size of the original
+	 * .shstrtab.
+	 */
+
+	/*
+	 * First we look if there was already a .BTF section present and
+	 * determine the first usable offset in the ELF (for .BTF and the
+	 * section name table).
 	 */
 	elf_getshdrstrndx(elf, &strndx);
+	size_t high_water_mark = 0;
 	Elf_Scn *btf_scn = 0;
 	while ((scn = elf_nextscn(elf, scn)) != NULL) {
 		shdr = gelf_getshdr(scn, &shdr_mem);
@@ -753,7 +772,10 @@ static int btf_elf__write(const char *filename, struct btf *btf)
 		char *secname = elf_strptr(elf, strndx, shdr->sh_name);
 		if (strcmp(secname, ".BTF") == 0) {
 			btf_scn = scn;
-			break;
+		} else if (elf_ndxscn(scn) != strndx) {
+			size_t limit = shdr->sh_offset + shdr->sh_size;
+			if (limit > high_water_mark)
+				high_water_mark = limit;
 		}
 	}
 
@@ -762,6 +784,12 @@ static int btf_elf__write(const char *filename, struct btf *btf)
 		fprintf(stderr, "%s: elf_getscn(strndx) failed\n", __func__);
 		goto out;
 	}
+	GElf_Shdr str_shdr_mem;
+	GElf_Shdr *str_shdr = gelf_getshdr(str_scn, &str_shdr_mem);
+	if (!str_shdr) {
+		fprintf(stderr, "%s: elf_getshdr(str_scn) failed\n", __func__);
+		goto out;
+	}
 
 	size_t dot_btf_offset = 0;
 	if (btf_scn) {
@@ -793,6 +821,7 @@ static int btf_elf__write(const char *filename, struct btf *btf)
 		str_data->d_buf = str_table;
 		str_data->d_size = new_str_size;
 		elf_flagdata(str_data, ELF_C_SET, ELF_F_DIRTY);
+		str_shdr->sh_size = new_str_size;
 
 		/* Create a new section */
 		btf_scn = elf_newscn(elf);
@@ -812,12 +841,15 @@ static int btf_elf__write(const char *filename, struct btf *btf)
 	/* (Re)populate the BTF section data */
 	raw_btf_data = btf__get_raw_data(btf, &raw_btf_size);
 	btf_data->d_buf = (void *)raw_btf_data;
+	btf_data->d_off = 0;
 	btf_data->d_size = raw_btf_size;
 	btf_data->d_type = ELF_T_BYTE;
 	btf_data->d_version = EV_CURRENT;
 	elf_flagdata(btf_data, ELF_C_SET, ELF_F_DIRTY);
 
 	/* Update .BTF section in the SHT */
+	size_t new_btf_offset = high_water_mark;
+	size_t new_btf_size = raw_btf_size;
 	GElf_Shdr btf_shdr_mem;
 	GElf_Shdr *btf_shdr = gelf_getshdr(btf_scn, &btf_shdr_mem);
 	if (!btf_shdr) {
@@ -829,6 +861,8 @@ static int btf_elf__write(const char *filename, struct btf *btf)
 	btf_shdr->sh_flags = 0;
 	if (dot_btf_offset)
 		btf_shdr->sh_name = dot_btf_offset;
+	btf_shdr->sh_offset = new_btf_offset;
+	btf_shdr->sh_size = new_btf_size;
 	btf_shdr->sh_type = SHT_PROGBITS;
 	if (!gelf_update_shdr(btf_scn, btf_shdr)) {
 		fprintf(stderr, "%s: gelf_update_shdr failed: %s\n",
@@ -836,6 +870,32 @@ static int btf_elf__write(const char *filename, struct btf *btf)
 		goto out;
 	}
 
+	/* Update section name string table */
+	size_t new_str_offset = new_btf_offset + new_btf_size;
+	str_shdr->sh_offset = new_str_offset;
+	if (!gelf_update_shdr(str_scn, str_shdr)) {
+		fprintf(stderr, "gelf_update_shdr failed\n");
+		goto out;
+	}
+
+	/* Update SHT, allowing for ELF64 alignment */
+	size_t sht_offset = roundup(new_str_offset + str_shdr->sh_size, 8);
+	ehdr->e_shoff = sht_offset;
+	if (!gelf_update_ehdr(elf, ehdr)) {
+		fprintf(stderr, "gelf_update_ehdr failed\n");
+		goto out;
+	}
+
+	if (btf_elf__verbose) {
+		fprintf(stderr, ".BTF [0x%lx, +0x%lx)\n",
+			btf_shdr->sh_offset, btf_shdr->sh_size);
+		fprintf(stderr, ".shstrtab [0x%lx, +0x%lx)\n",
+			str_shdr->sh_offset, str_shdr->sh_size);
+		fprintf(stderr, "SHT [0x%lx, +%d*0x%x)\n",
+			ehdr->e_shoff, ehdr->e_shnum, ehdr->e_shentsize);
+	}
+
+	elf_flagelf(elf, ELF_C_SET, ELF_F_LAYOUT);
 	if (elf_update(elf, ELF_C_NULL) < 0) {
 		fprintf(stderr, "%s: elf_update (layout) failed: %s\n",
 			__func__, elf_errmsg(elf_errno()));
-- 
2.30.0.280.ga3ce27912f-goog

[PATCH dwarves 4/4] btf_encoder: Align .BTF section to 8 bytes

[Giuliano Procida]

This is to avoid misaligned access to BTF type structs when
memory-mapping ELF sections.

Signed-off-by: Giuliano Procida <gprocida@google.com>
---
 libbtf.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/libbtf.c b/libbtf.c
index 4726e16..fa00053 100644
--- a/libbtf.c
+++ b/libbtf.c
@@ -755,7 +755,13 @@ static int btf_elf__write(const char *filename, struct btf *btf)
 	 * This actually happens in practice with vmlinux which has .strtab
 	 * after .shstrtab, resulting in a (small) hole the size of the original
 	 * .shstrtab.
+	 *
+	 * We'll align .BTF to 8 bytes to cater for all architectures. It'd be
+	 * nice if we could fetch this value from somewhere. The BTF
+	 * specification does not discuss alignment and its trailing string
+	 * table is not currently padded to any particular alignment.
 	 */
+	const size_t btf_alignment = 8;
 
 	/*
 	 * First we look if there was already a .BTF section present and
@@ -848,8 +854,8 @@ static int btf_elf__write(const char *filename, struct btf *btf)
 	elf_flagdata(btf_data, ELF_C_SET, ELF_F_DIRTY);
 
 	/* Update .BTF section in the SHT */
-	size_t new_btf_offset = high_water_mark;
-	size_t new_btf_size = raw_btf_size;
+	size_t new_btf_offset = roundup(high_water_mark, btf_alignment);
+	size_t new_btf_size = roundup(raw_btf_size, btf_alignment);
 	GElf_Shdr btf_shdr_mem;
 	GElf_Shdr *btf_shdr = gelf_getshdr(btf_scn, &btf_shdr_mem);
 	if (!btf_shdr) {
@@ -857,6 +863,7 @@ static int btf_elf__write(const char *filename, struct btf *btf)
 			__func__, elf_errmsg(elf_errno()));
 		goto out;
 	}
+	btf_shdr->sh_addralign = btf_alignment;
 	btf_shdr->sh_entsize = 0;
 	btf_shdr->sh_flags = 0;
 	if (dot_btf_offset)
-- 
2.30.0.280.ga3ce27912f-goog

Re: [PATCH dwarves 0/4] BTF ELF writing changes

[Jiri Olsa]

On Mon, Jan 25, 2021 at 01:06:21PM +0000, Giuliano Procida wrote:
> Hi.
> 
> This follows on from my change to improve the error handling around
> llvm-objcopy in libbtf.c.
> 
> Note on recipients: Please let me know if I should adjust To or CC.
> 
> Note on style: I've generally placed declarations as allowed by C99,
> closest to point of use. Let me know if you'd prefer otherwise.
> 
> 1. Improve ELF error reporting
> 
> 2. Add .BTF section using libelf
> 
> This shows the minimal amount of code needed to drive libelf. However,
> it leaves layout up to libelf, which is almost certainly not wanted.
> 
> As an unexpcted side-effect, vmlinux is larger than before. It seems
> llvm-objcopy likes to trim down .strtab.
> 
> 3. Manually lay out updated ELF sections
> 
> This does full layout of new and updated ELF sections. If the update
> ELF sections were not the last ones in the file by offset, then it can
> leave gaps between sections.
> 
> 4. Align .BTF section to 8 bytes
> 
> This was my original aim.
> 
> Regards.
> 
> Giuliano Procida (4):
>   btf_encoder: Improve ELF error reporting
>   btf_encoder: Add .BTF section using libelf
>   btf_encoder: Manually lay out updated ELF sections
>   btf_encoder: Align .BTF section to 8 bytes

hi,
I can't apply this on dwarves git master, which commit is it based on?

thanks,
jirka

> 
>  libbtf.c | 222 +++++++++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 175 insertions(+), 47 deletions(-)
> 
> -- 
> 2.30.0.280.ga3ce27912f-goog
> 

Re: [PATCH dwarves 0/4] BTF ELF writing changes

[Giuliano Procida]

Hi.

On Tue, 26 Jan 2021 at 19:56, Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Mon, Jan 25, 2021 at 01:06:21PM +0000, Giuliano Procida wrote:
> > Hi.
> >
> > This follows on from my change to improve the error handling around
> > llvm-objcopy in libbtf.c.
> >
> > Note on recipients: Please let me know if I should adjust To or CC.
> >
> > Note on style: I've generally placed declarations as allowed by C99,
> > closest to point of use. Let me know if you'd prefer otherwise.
> >
> > 1. Improve ELF error reporting
> >
> > 2. Add .BTF section using libelf
> >
> > This shows the minimal amount of code needed to drive libelf. However,
> > it leaves layout up to libelf, which is almost certainly not wanted.
> >
> > As an unexpcted side-effect, vmlinux is larger than before. It seems
> > llvm-objcopy likes to trim down .strtab.
> >
> > 3. Manually lay out updated ELF sections
> >
> > This does full layout of new and updated ELF sections. If the update
> > ELF sections were not the last ones in the file by offset, then it can
> > leave gaps between sections.
> >
> > 4. Align .BTF section to 8 bytes
> >
> > This was my original aim.
> >
> > Regards.
> >
> > Giuliano Procida (4):
> >   btf_encoder: Improve ELF error reporting
> >   btf_encoder: Add .BTF section using libelf
> >   btf_encoder: Manually lay out updated ELF sections
> >   btf_encoder: Align .BTF section to 8 bytes
>
> hi,
> I can't apply this on dwarves git master, which commit is it based on?
>

It's based on:
https://www.spinics.net/lists/dwarves/msg00775.html (0/3)
https://www.spinics.net/lists/dwarves/msg00774.html (1/3, unrelated fix)
https://www.spinics.net/lists/dwarves/msg00773.html (2/3, this is the
one you'll need for a clean git am; obsoleted by this new series)
(3/3 was abandoned)

Arnaldo did say the two commits were applied... but perhaps they
haven't been pushed to public master yet.

> thanks,
> jirka
>

You're welcome.
Giuliano.

> >
> >  libbtf.c | 222 +++++++++++++++++++++++++++++++++++++++++++------------
> >  1 file changed, 175 insertions(+), 47 deletions(-)
> >
> > --
> > 2.30.0.280.ga3ce27912f-goog
> >
>

Re: [PATCH dwarves 0/4] BTF ELF writing changes

[Arnaldo Carvalho de Melo]

Em Wed, Jan 27, 2021 at 01:10:40AM +0000, Giuliano Procida escreveu:
> Hi.
> 
> On Tue, 26 Jan 2021 at 19:56, Jiri Olsa <jolsa@redhat.com> wrote:
> >
> > On Mon, Jan 25, 2021 at 01:06:21PM +0000, Giuliano Procida wrote:
> > > Hi.
> > >
> > > This follows on from my change to improve the error handling around
> > > llvm-objcopy in libbtf.c.
> > >
> > > Note on recipients: Please let me know if I should adjust To or CC.
> > >
> > > Note on style: I've generally placed declarations as allowed by C99,
> > > closest to point of use. Let me know if you'd prefer otherwise.
> > >
> > > 1. Improve ELF error reporting
> > >
> > > 2. Add .BTF section using libelf
> > >
> > > This shows the minimal amount of code needed to drive libelf. However,
> > > it leaves layout up to libelf, which is almost certainly not wanted.
> > >
> > > As an unexpcted side-effect, vmlinux is larger than before. It seems
> > > llvm-objcopy likes to trim down .strtab.
> > >
> > > 3. Manually lay out updated ELF sections
> > >
> > > This does full layout of new and updated ELF sections. If the update
> > > ELF sections were not the last ones in the file by offset, then it can
> > > leave gaps between sections.
> > >
> > > 4. Align .BTF section to 8 bytes
> > >
> > > This was my original aim.
> > >
> > > Regards.
> > >
> > > Giuliano Procida (4):
> > >   btf_encoder: Improve ELF error reporting
> > >   btf_encoder: Add .BTF section using libelf
> > >   btf_encoder: Manually lay out updated ELF sections
> > >   btf_encoder: Align .BTF section to 8 bytes
> >
> > hi,
> > I can't apply this on dwarves git master, which commit is it based on?
> >
> 
> It's based on:
> https://www.spinics.net/lists/dwarves/msg00775.html (0/3)
> https://www.spinics.net/lists/dwarves/msg00774.html (1/3, unrelated fix)
> https://www.spinics.net/lists/dwarves/msg00773.html (2/3, this is the
> one you'll need for a clean git am; obsoleted by this new series)
> (3/3 was abandoned)
> 
> Arnaldo did say the two commits were applied... but perhaps they
> haven't been pushed to public master yet.

I pushed what I have now, please check if anything is missing.

I'm now working on DWARF4's DW_AT_data_bit_offset, that gcc uses when
dwarf-5 is asked for, I should have something usable tomorrow and
hopefully this will be the last stuff to get into 1.20.

dd

thanks,

- Arnaldo
 
> > thanks,
> > jirka
> >
> 
> You're welcome.
> Giuliano.
> 
> > >
> > >  libbtf.c | 222 +++++++++++++++++++++++++++++++++++++++++++------------
> > >  1 file changed, 175 insertions(+), 47 deletions(-)
> > >
> > > --
> > > 2.30.0.280.ga3ce27912f-goog
> > >
> >

-- 

- Arnaldo

Re: [PATCH dwarves 0/4] BTF ELF writing changes

[Arnaldo Carvalho de Melo]

Em Mon, Jan 25, 2021 at 01:06:21PM +0000, Giuliano Procida escreveu:
> Hi.
> 
> This follows on from my change to improve the error handling around
> llvm-objcopy in libbtf.c.
> 
> Note on recipients: Please let me know if I should adjust To or CC.
> 
> Note on style: I've generally placed declarations as allowed by C99,
> closest to point of use. Let me know if you'd prefer otherwise.
> 
> 1. Improve ELF error reporting

applied
 
> 2. Add .BTF section using libelf
> 
> This shows the minimal amount of code needed to drive libelf. However,
> it leaves layout up to libelf, which is almost certainly not wanted.
> 
> As an unexpcted side-effect, vmlinux is larger than before. It seems
> llvm-objcopy likes to trim down .strtab.

We have to test this thoroughly, I'm adding support to gcc's -gdwarf-5
DW_AT_data_bit_offset, I think I should get that done and release 1.20,
if some bug is still left on that new code, we can just fallback to
-gdwarf-4.

Then get back to the last 2 patches in your series, ok?

- Arnaldo
 
> 3. Manually lay out updated ELF sections
> 
> This does full layout of new and updated ELF sections. If the update
> ELF sections were not the last ones in the file by offset, then it can
> leave gaps between sections.
> 
> 4. Align .BTF section to 8 bytes
> 
> This was my original aim.
> 
> Regards.
> 
> Giuliano Procida (4):
>   btf_encoder: Improve ELF error reporting
>   btf_encoder: Add .BTF section using libelf
>   btf_encoder: Manually lay out updated ELF sections
>   btf_encoder: Align .BTF section to 8 bytes
> 
>  libbtf.c | 222 +++++++++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 175 insertions(+), 47 deletions(-)
> 
> -- 
> 2.30.0.280.ga3ce27912f-goog
> 

-- 

- Arnaldo

Re: [PATCH dwarves 0/4] BTF ELF writing changes

[Giuliano Procida]

Hi.

On Wed, 27 Jan 2021 at 14:06, Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
>
> Em Mon, Jan 25, 2021 at 01:06:21PM +0000, Giuliano Procida escreveu:
> > Hi.
> >
> > This follows on from my change to improve the error handling around
> > llvm-objcopy in libbtf.c.
> >
> > Note on recipients: Please let me know if I should adjust To or CC.
> >
> > Note on style: I've generally placed declarations as allowed by C99,
> > closest to point of use. Let me know if you'd prefer otherwise.
> >
> > 1. Improve ELF error reporting
>
> applied
>
> > 2. Add .BTF section using libelf
> >
> > This shows the minimal amount of code needed to drive libelf. However,
> > it leaves layout up to libelf, which is almost certainly not wanted.
> >
> > As an unexpcted side-effect, vmlinux is larger than before. It seems
> > llvm-objcopy likes to trim down .strtab.
>
> We have to test this thoroughly, I'm adding support to gcc's -gdwarf-5
> DW_AT_data_bit_offset, I think I should get that done and release 1.20,
> if some bug is still left on that new code, we can just fallback to
> -gdwarf-4.
>
> Then get back to the last 2 patches in your series, ok?
>

That's fine.

I've spent a little time digging into what llvm-objcopy (11.0.0) is
doing. It turns out it will rewrite an ELF file even if you just do
llvm-objcopy --dump-section .strtab=/dev/null elf_file, and even if
the file is not writable.

Our AOSP kernels have a lot symbols filtered out of the symbol table
and perhaps this is what makes such a difference to the size of
.strtab after llvm-objcopy has done its thing. I will try on a vanilla
kernel.

Giuliano.

> - Arnaldo
>
> > 3. Manually lay out updated ELF sections
> >
> > This does full layout of new and updated ELF sections. If the update
> > ELF sections were not the last ones in the file by offset, then it can
> > leave gaps between sections.
> >
> > 4. Align .BTF section to 8 bytes
> >
> > This was my original aim.
> >
> > Regards.
> >
> > Giuliano Procida (4):
> >   btf_encoder: Improve ELF error reporting
> >   btf_encoder: Add .BTF section using libelf
> >   btf_encoder: Manually lay out updated ELF sections
> >   btf_encoder: Align .BTF section to 8 bytes
> >
> >  libbtf.c | 222 +++++++++++++++++++++++++++++++++++++++++++------------
> >  1 file changed, 175 insertions(+), 47 deletions(-)
> >
> > --
> > 2.30.0.280.ga3ce27912f-goog
> >
>
> --
>
> - Arnaldo

Re: [PATCH dwarves 2/4] btf_encoder: Add .BTF section using libelf

[Jiri Olsa]

On Mon, Jan 25, 2021 at 01:06:23PM +0000, Giuliano Procida wrote:
> pahole -J uses libelf directly when updating a .BTF section. However,
> it uses llvm-objcopy to add .BTF sections. This commit switches to
> using libelf for both cases.
> 
> This eliminates pahole's dependency on llvm-objcopy. One unfortunate
> side-effect is that vmlinux actually increases in size. It seems that
> llvm-objcopy modifies the .strtab section, discarding many strings. I
> speculate that is it discarding strings not referenced from .symtab
> and updating the references therein.
> 
> In this initial version layout is left completely up to libelf which
> may be OK for non-loadable object files, but is probably no good for
> things like vmlinux where all the offsets may change. This is
> addressed in a follow-up commit.
> 
> Signed-off-by: Giuliano Procida <gprocida@google.com>
> ---
>  libbtf.c | 145 ++++++++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 100 insertions(+), 45 deletions(-)
> 
> diff --git a/libbtf.c b/libbtf.c
> index 9f76283..fb8e043 100644
> --- a/libbtf.c
> +++ b/libbtf.c
> @@ -699,6 +699,7 @@ static int btf_elf__write(const char *filename, struct btf *btf)
>  	uint32_t raw_btf_size;
>  	int fd, err = -1;
>  	size_t strndx;
> +	void *str_table = NULL;
>  
>  	fd = open(filename, O_RDWR);
>  	if (fd < 0) {
> @@ -741,74 +742,128 @@ static int btf_elf__write(const char *filename, struct btf *btf)
>  	}
>  
>  	/*
> -	 * First we look if there was already a .BTF section to overwrite.
> +	 * First we check if there is already a .BTF section present.
>  	 */
> -
>  	elf_getshdrstrndx(elf, &strndx);
> +	Elf_Scn *btf_scn = 0;

NULL


SNIP

> -		const char *llvm_objcopy;
> -		char tmp_fn[PATH_MAX];
> -		char cmd[PATH_MAX * 2];
> -
> -		llvm_objcopy = getenv("LLVM_OBJCOPY");
> -		if (!llvm_objcopy)
> -			llvm_objcopy = "llvm-objcopy";
> -
> -		/* Use objcopy to add a .BTF section */
> -		snprintf(tmp_fn, sizeof(tmp_fn), "%s.btf", filename);
> -		close(fd);
> -		fd = creat(tmp_fn, S_IRUSR | S_IWUSR);
> -		if (fd == -1) {
> -			fprintf(stderr, "%s: open(%s) failed!\n", __func__,
> -				tmp_fn);
> +		/* Add ".BTF" to the section name string table */
> +		Elf_Data *str_data = elf_getdata(str_scn, NULL);
> +		if (!str_data) {
> +			fprintf(stderr, "%s: elf_getdata(str_scn) failed: %s\n",
> +				__func__, elf_errmsg(elf_errno()));
>  			goto out;
>  		}
> -
> -		if (write(fd, raw_btf_data, raw_btf_size) != raw_btf_size) {
> -			fprintf(stderr, "%s: write of %d bytes to '%s' failed: %d!\n",
> -				__func__, raw_btf_size, tmp_fn, errno);
> -			goto unlink;
> +		dot_btf_offset = str_data->d_size;
> +		size_t new_str_size = dot_btf_offset + 5;
> +		str_table = malloc(new_str_size);
> +		if (!str_table) {
> +			fprintf(stderr, "%s: malloc(%zu) failed: %s\n", __func__,
> +				new_str_size, elf_errmsg(elf_errno()));
> +			goto out;
>  		}
> +		memcpy(str_table, str_data->d_buf, dot_btf_offset);
> +		memcpy(str_table + dot_btf_offset, ".BTF", 5);

hum, I wonder this will always copy the final zero byte

> +		str_data->d_buf = str_table;
> +		str_data->d_size = new_str_size;
> +		elf_flagdata(str_data, ELF_C_SET, ELF_F_DIRTY);
> +
> +		/* Create a new section */
> +		btf_scn = elf_newscn(elf);
> +		if (!btf_scn) {
> +			fprintf(stderr, "%s: elf_newscn failed: %s\n",
> +			__func__, elf_errmsg(elf_errno()));
> +			goto out;
> +		}
> +		btf_data = elf_newdata(btf_scn);
> +		if (!btf_data) {
> +			fprintf(stderr, "%s: elf_newdata failed: %s\n",
> +			__func__, elf_errmsg(elf_errno()));
> +			goto out;
> +		}
> +	}
>  
> -		snprintf(cmd, sizeof(cmd), "%s --add-section .BTF=%s %s",
> -			 llvm_objcopy, tmp_fn, filename);
> -		if (system(cmd)) {
> -			fprintf(stderr, "%s: failed to add .BTF section to '%s': %d!\n",
> -				__func__, filename, errno);
> -			goto unlink;
> +	/* (Re)populate the BTF section data */
> +	raw_btf_data = btf__get_raw_data(btf, &raw_btf_size);
> +	btf_data->d_buf = (void *)raw_btf_data;

doesn't this potentially leak btf_data->d_buf?

> +	btf_data->d_size = raw_btf_size;
> +	btf_data->d_type = ELF_T_BYTE;
> +	btf_data->d_version = EV_CURRENT;
> +	elf_flagdata(btf_data, ELF_C_SET, ELF_F_DIRTY);
> +
> +	/* Update .BTF section in the SHT */
> +	GElf_Shdr btf_shdr_mem;
> +	GElf_Shdr *btf_shdr = gelf_getshdr(btf_scn, &btf_shdr_mem);
> +	if (!btf_shdr) {
> +		fprintf(stderr, "%s: elf_getshdr(btf_scn) failed: %s\n",
> +			__func__, elf_errmsg(elf_errno()));
> +		goto out;
> +	}
> +	btf_shdr->sh_entsize = 0;
> +	btf_shdr->sh_flags = 0;
> +	if (dot_btf_offset)
> +		btf_shdr->sh_name = dot_btf_offset;
> +	btf_shdr->sh_type = SHT_PROGBITS;
> +	if (!gelf_update_shdr(btf_scn, btf_shdr)) {
> +		fprintf(stderr, "%s: gelf_update_shdr failed: %s\n",
> +			__func__, elf_errmsg(elf_errno()));
> +		goto out;
> +	}
> +
> +	if (elf_update(elf, ELF_C_NULL) < 0) {
> +		fprintf(stderr, "%s: elf_update (layout) failed: %s\n",
> +			__func__, elf_errmsg(elf_errno()));
> +		goto out;
> +	}
> +
> +	size_t phnum = 0;
> +	if (!elf_getphdrnum(elf, &phnum)) {
> +		for (size_t ix = 0; ix < phnum; ++ix) {
> +			GElf_Phdr phdr;
> +			GElf_Phdr *elf_phdr = gelf_getphdr(elf, ix, &phdr);
> +			size_t filesz = gelf_fsize(elf, ELF_T_PHDR, 1, EV_CURRENT);
> +			fprintf(stderr, "type: %d %d\n", elf_phdr->p_type, PT_PHDR);
> +			fprintf(stderr, "offset: %lu %lu\n", elf_phdr->p_offset, ehdr->e_phoff);
> +			fprintf(stderr, "filesize: %lu %lu\n", elf_phdr->p_filesz, filesz);

looks like s forgotten debug or you're missing
btf_elf__verbose check for fprintf calls above

jirka

Re: [PATCH dwarves 2/4] btf_encoder: Add .BTF section using libelf

[Giuliano Procida]

Hi.

On Wed, 27 Jan 2021 at 23:23, Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Mon, Jan 25, 2021 at 01:06:23PM +0000, Giuliano Procida wrote:
> > pahole -J uses libelf directly when updating a .BTF section. However,
> > it uses llvm-objcopy to add .BTF sections. This commit switches to
> > using libelf for both cases.
> >
> > This eliminates pahole's dependency on llvm-objcopy. One unfortunate
> > side-effect is that vmlinux actually increases in size. It seems that
> > llvm-objcopy modifies the .strtab section, discarding many strings. I
> > speculate that is it discarding strings not referenced from .symtab
> > and updating the references therein.
> >
> > In this initial version layout is left completely up to libelf which
> > may be OK for non-loadable object files, but is probably no good for
> > things like vmlinux where all the offsets may change. This is
> > addressed in a follow-up commit.
> >
> > Signed-off-by: Giuliano Procida <gprocida@google.com>
> > ---
> >  libbtf.c | 145 ++++++++++++++++++++++++++++++++++++++-----------------
> >  1 file changed, 100 insertions(+), 45 deletions(-)
> >
> > diff --git a/libbtf.c b/libbtf.c
> > index 9f76283..fb8e043 100644
> > --- a/libbtf.c
> > +++ b/libbtf.c
> > @@ -699,6 +699,7 @@ static int btf_elf__write(const char *filename, struct btf *btf)
> >       uint32_t raw_btf_size;
> >       int fd, err = -1;
> >       size_t strndx;
> > +     void *str_table = NULL;
> >
> >       fd = open(filename, O_RDWR);
> >       if (fd < 0) {
> > @@ -741,74 +742,128 @@ static int btf_elf__write(const char *filename, struct btf *btf)
> >       }
> >
> >       /*
> > -      * First we look if there was already a .BTF section to overwrite.
> > +      * First we check if there is already a .BTF section present.
> >        */
> > -
> >       elf_getshdrstrndx(elf, &strndx);
> > +     Elf_Scn *btf_scn = 0;
>
> NULL
>
>
> SNIP
>
> > -             const char *llvm_objcopy;
> > -             char tmp_fn[PATH_MAX];
> > -             char cmd[PATH_MAX * 2];
> > -
> > -             llvm_objcopy = getenv("LLVM_OBJCOPY");
> > -             if (!llvm_objcopy)
> > -                     llvm_objcopy = "llvm-objcopy";
> > -
> > -             /* Use objcopy to add a .BTF section */
> > -             snprintf(tmp_fn, sizeof(tmp_fn), "%s.btf", filename);
> > -             close(fd);
> > -             fd = creat(tmp_fn, S_IRUSR | S_IWUSR);
> > -             if (fd == -1) {
> > -                     fprintf(stderr, "%s: open(%s) failed!\n", __func__,
> > -                             tmp_fn);
> > +             /* Add ".BTF" to the section name string table */
> > +             Elf_Data *str_data = elf_getdata(str_scn, NULL);
> > +             if (!str_data) {
> > +                     fprintf(stderr, "%s: elf_getdata(str_scn) failed: %s\n",
> > +                             __func__, elf_errmsg(elf_errno()));
> >                       goto out;
> >               }
> > -
> > -             if (write(fd, raw_btf_data, raw_btf_size) != raw_btf_size) {
> > -                     fprintf(stderr, "%s: write of %d bytes to '%s' failed: %d!\n",
> > -                             __func__, raw_btf_size, tmp_fn, errno);
> > -                     goto unlink;
> > +             dot_btf_offset = str_data->d_size;
> > +             size_t new_str_size = dot_btf_offset + 5;
> > +             str_table = malloc(new_str_size);
> > +             if (!str_table) {
> > +                     fprintf(stderr, "%s: malloc(%zu) failed: %s\n", __func__,
> > +                             new_str_size, elf_errmsg(elf_errno()));
> > +                     goto out;
> >               }
> > +             memcpy(str_table, str_data->d_buf, dot_btf_offset);
> > +             memcpy(str_table + dot_btf_offset, ".BTF", 5);
>
> hum, I wonder this will always copy the final zero byte

It should, as strlen(".BTF") == 4.

> > +             str_data->d_buf = str_table;
> > +             str_data->d_size = new_str_size;
> > +             elf_flagdata(str_data, ELF_C_SET, ELF_F_DIRTY);
> > +
> > +             /* Create a new section */
> > +             btf_scn = elf_newscn(elf);
> > +             if (!btf_scn) {
> > +                     fprintf(stderr, "%s: elf_newscn failed: %s\n",
> > +                     __func__, elf_errmsg(elf_errno()));
> > +                     goto out;
> > +             }
> > +             btf_data = elf_newdata(btf_scn);
> > +             if (!btf_data) {
> > +                     fprintf(stderr, "%s: elf_newdata failed: %s\n",
> > +                     __func__, elf_errmsg(elf_errno()));
> > +                     goto out;
> > +             }
> > +     }
> >
> > -             snprintf(cmd, sizeof(cmd), "%s --add-section .BTF=%s %s",
> > -                      llvm_objcopy, tmp_fn, filename);
> > -             if (system(cmd)) {
> > -                     fprintf(stderr, "%s: failed to add .BTF section to '%s': %d!\n",
> > -                             __func__, filename, errno);
> > -                     goto unlink;
> > +     /* (Re)populate the BTF section data */
> > +     raw_btf_data = btf__get_raw_data(btf, &raw_btf_size);
> > +     btf_data->d_buf = (void *)raw_btf_data;
>
> doesn't this potentially leak btf_data->d_buf?

I believe libelf owns the original btf_data->d_buf and it could even
just be a pointer into mmaped memory,  but I will check.

> > +     btf_data->d_size = raw_btf_size;
> > +     btf_data->d_type = ELF_T_BYTE;
> > +     btf_data->d_version = EV_CURRENT;
> > +     elf_flagdata(btf_data, ELF_C_SET, ELF_F_DIRTY);
> > +
> > +     /* Update .BTF section in the SHT */
> > +     GElf_Shdr btf_shdr_mem;
> > +     GElf_Shdr *btf_shdr = gelf_getshdr(btf_scn, &btf_shdr_mem);
> > +     if (!btf_shdr) {
> > +             fprintf(stderr, "%s: elf_getshdr(btf_scn) failed: %s\n",
> > +                     __func__, elf_errmsg(elf_errno()));
> > +             goto out;
> > +     }
> > +     btf_shdr->sh_entsize = 0;
> > +     btf_shdr->sh_flags = 0;
> > +     if (dot_btf_offset)
> > +             btf_shdr->sh_name = dot_btf_offset;
> > +     btf_shdr->sh_type = SHT_PROGBITS;
> > +     if (!gelf_update_shdr(btf_scn, btf_shdr)) {
> > +             fprintf(stderr, "%s: gelf_update_shdr failed: %s\n",
> > +                     __func__, elf_errmsg(elf_errno()));
> > +             goto out;
> > +     }
> > +
> > +     if (elf_update(elf, ELF_C_NULL) < 0) {
> > +             fprintf(stderr, "%s: elf_update (layout) failed: %s\n",
> > +                     __func__, elf_errmsg(elf_errno()));
> > +             goto out;
> > +     }
> > +
> > +     size_t phnum = 0;
> > +     if (!elf_getphdrnum(elf, &phnum)) {
> > +             for (size_t ix = 0; ix < phnum; ++ix) {
> > +                     GElf_Phdr phdr;
> > +                     GElf_Phdr *elf_phdr = gelf_getphdr(elf, ix, &phdr);
> > +                     size_t filesz = gelf_fsize(elf, ELF_T_PHDR, 1, EV_CURRENT);
> > +                     fprintf(stderr, "type: %d %d\n", elf_phdr->p_type, PT_PHDR);
> > +                     fprintf(stderr, "offset: %lu %lu\n", elf_phdr->p_offset, ehdr->e_phoff);
> > +                     fprintf(stderr, "filesize: %lu %lu\n", elf_phdr->p_filesz, filesz);
>
> looks like s forgotten debug or you're missing
> btf_elf__verbose check for fprintf calls above

Oops, forgotten debug.

>
> jirka
>

Thanks,
Giuliano.

Re: [PATCH dwarves 2/4] btf_encoder: Add .BTF section using libelf

[Giuliano Procida]

Hi.

On Thu, 28 Jan 2021 at 13:35, Giuliano Procida <gprocida@google.com> wrote:
>
> Hi.
>
> On Wed, 27 Jan 2021 at 23:23, Jiri Olsa <jolsa@redhat.com> wrote:
> >
> > On Mon, Jan 25, 2021 at 01:06:23PM +0000, Giuliano Procida wrote:
> > > pahole -J uses libelf directly when updating a .BTF section. However,
> > > it uses llvm-objcopy to add .BTF sections. This commit switches to
> > > using libelf for both cases.
> > >
> > > This eliminates pahole's dependency on llvm-objcopy. One unfortunate
> > > side-effect is that vmlinux actually increases in size. It seems that
> > > llvm-objcopy modifies the .strtab section, discarding many strings. I
> > > speculate that is it discarding strings not referenced from .symtab
> > > and updating the references therein.
> > >
> > > In this initial version layout is left completely up to libelf which
> > > may be OK for non-loadable object files, but is probably no good for
> > > things like vmlinux where all the offsets may change. This is
> > > addressed in a follow-up commit.
> > >
> > > Signed-off-by: Giuliano Procida <gprocida@google.com>
> > > ---
> > >  libbtf.c | 145 ++++++++++++++++++++++++++++++++++++++-----------------
> > >  1 file changed, 100 insertions(+), 45 deletions(-)
> > >
> > > diff --git a/libbtf.c b/libbtf.c
> > > index 9f76283..fb8e043 100644
> > > --- a/libbtf.c
> > > +++ b/libbtf.c
> > > @@ -699,6 +699,7 @@ static int btf_elf__write(const char *filename, struct btf *btf)
> > >       uint32_t raw_btf_size;
> > >       int fd, err = -1;
> > >       size_t strndx;
> > > +     void *str_table = NULL;
> > >
> > >       fd = open(filename, O_RDWR);
> > >       if (fd < 0) {
> > > @@ -741,74 +742,128 @@ static int btf_elf__write(const char *filename, struct btf *btf)
> > >       }
> > >
> > >       /*
> > > -      * First we look if there was already a .BTF section to overwrite.
> > > +      * First we check if there is already a .BTF section present.
> > >        */
> > > -
> > >       elf_getshdrstrndx(elf, &strndx);
> > > +     Elf_Scn *btf_scn = 0;
> >
> > NULL
> >
> >
> > SNIP
> >
> > > -             const char *llvm_objcopy;
> > > -             char tmp_fn[PATH_MAX];
> > > -             char cmd[PATH_MAX * 2];
> > > -
> > > -             llvm_objcopy = getenv("LLVM_OBJCOPY");
> > > -             if (!llvm_objcopy)
> > > -                     llvm_objcopy = "llvm-objcopy";
> > > -
> > > -             /* Use objcopy to add a .BTF section */
> > > -             snprintf(tmp_fn, sizeof(tmp_fn), "%s.btf", filename);
> > > -             close(fd);
> > > -             fd = creat(tmp_fn, S_IRUSR | S_IWUSR);
> > > -             if (fd == -1) {
> > > -                     fprintf(stderr, "%s: open(%s) failed!\n", __func__,
> > > -                             tmp_fn);
> > > +             /* Add ".BTF" to the section name string table */
> > > +             Elf_Data *str_data = elf_getdata(str_scn, NULL);
> > > +             if (!str_data) {
> > > +                     fprintf(stderr, "%s: elf_getdata(str_scn) failed: %s\n",
> > > +                             __func__, elf_errmsg(elf_errno()));
> > >                       goto out;
> > >               }
> > > -
> > > -             if (write(fd, raw_btf_data, raw_btf_size) != raw_btf_size) {
> > > -                     fprintf(stderr, "%s: write of %d bytes to '%s' failed: %d!\n",
> > > -                             __func__, raw_btf_size, tmp_fn, errno);
> > > -                     goto unlink;
> > > +             dot_btf_offset = str_data->d_size;
> > > +             size_t new_str_size = dot_btf_offset + 5;
> > > +             str_table = malloc(new_str_size);
> > > +             if (!str_table) {
> > > +                     fprintf(stderr, "%s: malloc(%zu) failed: %s\n", __func__,
> > > +                             new_str_size, elf_errmsg(elf_errno()));
> > > +                     goto out;
> > >               }
> > > +             memcpy(str_table, str_data->d_buf, dot_btf_offset);
> > > +             memcpy(str_table + dot_btf_offset, ".BTF", 5);
> >
> > hum, I wonder this will always copy the final zero byte
>
> It should, as strlen(".BTF") == 4.
>
> > > +             str_data->d_buf = str_table;
> > > +             str_data->d_size = new_str_size;
> > > +             elf_flagdata(str_data, ELF_C_SET, ELF_F_DIRTY);
> > > +
> > > +             /* Create a new section */
> > > +             btf_scn = elf_newscn(elf);
> > > +             if (!btf_scn) {
> > > +                     fprintf(stderr, "%s: elf_newscn failed: %s\n",
> > > +                     __func__, elf_errmsg(elf_errno()));
> > > +                     goto out;
> > > +             }
> > > +             btf_data = elf_newdata(btf_scn);
> > > +             if (!btf_data) {
> > > +                     fprintf(stderr, "%s: elf_newdata failed: %s\n",
> > > +                     __func__, elf_errmsg(elf_errno()));
> > > +                     goto out;
> > > +             }
> > > +     }
> > >
> > > -             snprintf(cmd, sizeof(cmd), "%s --add-section .BTF=%s %s",
> > > -                      llvm_objcopy, tmp_fn, filename);
> > > -             if (system(cmd)) {
> > > -                     fprintf(stderr, "%s: failed to add .BTF section to '%s': %d!\n",
> > > -                             __func__, filename, errno);
> > > -                     goto unlink;
> > > +     /* (Re)populate the BTF section data */
> > > +     raw_btf_data = btf__get_raw_data(btf, &raw_btf_size);
> > > +     btf_data->d_buf = (void *)raw_btf_data;
> >
> > doesn't this potentially leak btf_data->d_buf?
>
> I believe libelf owns the original btf_data->d_buf and it could even
> just be a pointer into mmaped memory,  but I will check.
>

FTR, that pointer *is* owned by libelf. If I free it, I get a
double-free warning later on.

> > > +     btf_data->d_size = raw_btf_size;
> > > +     btf_data->d_type = ELF_T_BYTE;
> > > +     btf_data->d_version = EV_CURRENT;
> > > +     elf_flagdata(btf_data, ELF_C_SET, ELF_F_DIRTY);
> > > +
> > > +     /* Update .BTF section in the SHT */
> > > +     GElf_Shdr btf_shdr_mem;
> > > +     GElf_Shdr *btf_shdr = gelf_getshdr(btf_scn, &btf_shdr_mem);
> > > +     if (!btf_shdr) {
> > > +             fprintf(stderr, "%s: elf_getshdr(btf_scn) failed: %s\n",
> > > +                     __func__, elf_errmsg(elf_errno()));
> > > +             goto out;
> > > +     }
> > > +     btf_shdr->sh_entsize = 0;
> > > +     btf_shdr->sh_flags = 0;
> > > +     if (dot_btf_offset)
> > > +             btf_shdr->sh_name = dot_btf_offset;
> > > +     btf_shdr->sh_type = SHT_PROGBITS;
> > > +     if (!gelf_update_shdr(btf_scn, btf_shdr)) {
> > > +             fprintf(stderr, "%s: gelf_update_shdr failed: %s\n",
> > > +                     __func__, elf_errmsg(elf_errno()));
> > > +             goto out;
> > > +     }
> > > +
> > > +     if (elf_update(elf, ELF_C_NULL) < 0) {
> > > +             fprintf(stderr, "%s: elf_update (layout) failed: %s\n",
> > > +                     __func__, elf_errmsg(elf_errno()));
> > > +             goto out;
> > > +     }
> > > +
> > > +     size_t phnum = 0;
> > > +     if (!elf_getphdrnum(elf, &phnum)) {
> > > +             for (size_t ix = 0; ix < phnum; ++ix) {
> > > +                     GElf_Phdr phdr;
> > > +                     GElf_Phdr *elf_phdr = gelf_getphdr(elf, ix, &phdr);
> > > +                     size_t filesz = gelf_fsize(elf, ELF_T_PHDR, 1, EV_CURRENT);
> > > +                     fprintf(stderr, "type: %d %d\n", elf_phdr->p_type, PT_PHDR);
> > > +                     fprintf(stderr, "offset: %lu %lu\n", elf_phdr->p_offset, ehdr->e_phoff);
> > > +                     fprintf(stderr, "filesize: %lu %lu\n", elf_phdr->p_filesz, filesz);
> >
> > looks like s forgotten debug or you're missing
> > btf_elf__verbose check for fprintf calls above
>
> Oops, forgotten debug.
>
> >
> > jirka
> >
>
> Thanks,
> Giuliano.