diff mbox series

[dwarves,v3,4/5] btf_encoder: Add .BTF section using libelf

Message ID 20210205134221.2953163-5-gprocida@google.com (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series ELF writing changes | expand

Checks

Context Check Description
netdev/tree_selection success Not a local patch

Commit Message

Giuliano Procida Feb. 5, 2021, 1:42 p.m. UTC
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.

Layout is left completely up to libelf and existing section offsets
are likely to change.

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

Comments

Andrii Nakryiko Feb. 8, 2021, 10:29 p.m. UTC | #1
On Fri, Feb 5, 2021 at 5:42 AM Giuliano Procida <gprocida@google.com> 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.
>
> Layout is left completely up to libelf and existing section offsets
> are likely to change.
>
> Signed-off-by: Giuliano Procida <gprocida@google.com>
> ---

Logic looks correct. One nit below.

Acked-by: Andrii Nakryiko <andrii@kernel.org>

>  libbtf.c | 127 +++++++++++++++++++++++++++++++++++--------------------
>  1 file changed, 81 insertions(+), 46 deletions(-)
>
> diff --git a/libbtf.c b/libbtf.c
> index 4ae7150..9f4abb3 100644
> --- a/libbtf.c
> +++ b/libbtf.c
> @@ -698,6 +698,7 @@ int32_t btf_elf__add_datasec_type(struct btf_elf *btfe, const char *section_name
>
>  static int btf_elf__write(const char *filename, struct btf *btf)
>  {
> +       const char dot_BTF[] = ".BTF";

it's a constant, so more appropriate name would be DOT_BTF, but that
"dot_" notation in the name of the variable throws me off, honestly.
libbpf is using BTF_SEC_NAME for this, which IMO makes more sense as a
name for the constant


>         GElf_Ehdr ehdr;
>         Elf_Data *btf_data = NULL;
>         Elf *elf = NULL;
> @@ -705,6 +706,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) {

[...]
Giuliano Procida Feb. 9, 2021, 3:04 p.m. UTC | #2
On Mon, 8 Feb 2021 at 22:29, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
>
> On Fri, Feb 5, 2021 at 5:42 AM Giuliano Procida <gprocida@google.com> 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.
> >
> > Layout is left completely up to libelf and existing section offsets
> > are likely to change.
> >
> > Signed-off-by: Giuliano Procida <gprocida@google.com>
> > ---
>
> Logic looks correct. One nit below.
>
> Acked-by: Andrii Nakryiko <andrii@kernel.org>
>
> >  libbtf.c | 127 +++++++++++++++++++++++++++++++++++--------------------
> >  1 file changed, 81 insertions(+), 46 deletions(-)
> >
> > diff --git a/libbtf.c b/libbtf.c
> > index 4ae7150..9f4abb3 100644
> > --- a/libbtf.c
> > +++ b/libbtf.c
> > @@ -698,6 +698,7 @@ int32_t btf_elf__add_datasec_type(struct btf_elf *btfe, const char *section_name
> >
> >  static int btf_elf__write(const char *filename, struct btf *btf)
> >  {
> > +       const char dot_BTF[] = ".BTF";
>
> it's a constant, so more appropriate name would be DOT_BTF, but that
> "dot_" notation in the name of the variable throws me off, honestly.
> libbpf is using BTF_SEC_NAME for this, which IMO makes more sense as a
> name for the constant
>

Ack. Changed. I'll wait a day for further comments before reposting.

Thanks again,
Giuliano.

>
> >         GElf_Ehdr ehdr;
> >         Elf_Data *btf_data = NULL;
> >         Elf *elf = NULL;
> > @@ -705,6 +706,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) {
>
> [...]
diff mbox series

Patch

diff --git a/libbtf.c b/libbtf.c
index 4ae7150..9f4abb3 100644
--- a/libbtf.c
+++ b/libbtf.c
@@ -698,6 +698,7 @@  int32_t btf_elf__add_datasec_type(struct btf_elf *btfe, const char *section_name
 
 static int btf_elf__write(const char *filename, struct btf *btf)
 {
+	const char dot_BTF[] = ".BTF";
 	GElf_Ehdr ehdr;
 	Elf_Data *btf_data = NULL;
 	Elf *elf = NULL;
@@ -705,6 +706,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) {
@@ -743,73 +745,106 @@  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 = NULL;
 	for (Elf_Scn *scn = elf_nextscn(elf, NULL); scn; scn = elf_nextscn(elf, scn)) {
 		GElf_Shdr shdr;
 		if (!gelf_getshdr(scn, &shdr))
 			continue;
 		char *secname = elf_strptr(elf, strndx, shdr.sh_name);
-		if (strcmp(secname, ".BTF") == 0) {
-			btf_data = elf_getdata(scn, btf_data);
+		if (strcmp(secname, dot_BTF) == 0) {
+			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) {
+		elf_error("elf_getscn(strndx) failed");
+		goto out;
+	}
 
-		if (elf_update(elf, ELF_C_NULL) >= 0 &&
-		    elf_update(elf, ELF_C_WRITE) >= 0)
-			err = 0;
-		else
-			elf_error("elf_update failed");
+	size_t dot_btf_offset = 0;
+	if (btf_scn) {
+		/* Existing .BTF section found */
+		btf_data = elf_getdata(btf_scn, NULL);
+		if (!btf_data) {
+			elf_error("elf_getdata failed");
+			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) {
+			elf_error("elf_getdata(str_scn) failed");
 			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 + sizeof(dot_BTF);
+		str_table = malloc(new_str_size);
+		if (!str_table) {
+			fprintf(stderr, "%s: malloc (strtab) failed\n", __func__);
+			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;
+		memcpy(str_table, str_data->d_buf, dot_btf_offset);
+		memcpy(str_table + dot_btf_offset, dot_BTF, sizeof(dot_BTF));
+		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) {
+			elf_error("elf_newscn failed");
+			goto out;
+		}
+		btf_data = elf_newdata(btf_scn);
+		if (!btf_data) {
+			elf_error("elf_newdata failed");
+			goto out;
 		}
+	}
+
+	/* (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;
+	if (!gelf_getshdr(btf_scn, &btf_shdr)) {
+		elf_error("elf_getshdr(btf_scn) failed");
+		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)) {
+		elf_error("gelf_update_shdr failed");
+		goto out;
+	}
 
-		err = 0;
-	unlink:
-		unlink(tmp_fn);
+	if (elf_update(elf, ELF_C_NULL) < 0) {
+		elf_error("elf_update (layout) failed");
+		goto out;
+	}
+
+	if (elf_update(elf, ELF_C_WRITE) < 0) {
+		elf_error("elf_update (write) failed");
+		goto out;
 	}
+	err = 0;
 
 out:
+	if (str_table)
+		free(str_table);
 	if (fd != -1)
 		close(fd);
 	if (elf)