[PATCH dwarves v2 0/4] BTF ELF writing changes

[PATCH dwarves v2 0/4] BTF ELF writing changes

[Giuliano Procida]

Note: The 4 patches in this series supersede patches 2-4 of the
previous version of this series (patch 1 was applied).

pahole -J currently uses llvm-objcopy to add .BTF as a section (and
segment) to an ELF object containing DWARF debug information. If a
.BTF section is already present it is rewritten but segment
information is not updated.

In terms of the current Linux linker script, all that's needed is
either a dump of the raw BTF or a file containing a .BTF section. This
is because the script dumps .BTF out to a file, creates a
single-section ELF file and then links this into a final vmlinux.

The first commit in this series removes the dependency on
llvm-objcopy.

The remaining commits attempt to roughly duplicate the desired end
result, namely adding a .BTF section which is also a loadable segment.
They may not be sufficient. In terms of non-kernel .BTF use, they are
probably not necessary.

Nevertheless, this was an interesting exercise and removing the
llvm-objcopy dependency is a worthwhile goal.

I'm not well-placed to test the viability of vmlinux with .BTF as
generated by these patches, so any feedback is very welcome.

Giuliano Procida (4):
  btf_encoder: Add .BTF section using libelf
  btf_encoder: Manually lay out updated ELF sections
  btf_encoder: Add .BTF as a loadable segment
  btf_encoder: Align .BTF section/segment to 8 bytes

 libbtf.c | 240 +++++++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 197 insertions(+), 43 deletions(-)

-- 
2.30.0.365.g02bc693789-goog

[PATCH dwarves v2 1/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 and
indeed offsets of existing sections are likely to change.

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

diff --git a/libbtf.c b/libbtf.c
index 81b1b36..5b91d3a 100644
--- a/libbtf.c
+++ b/libbtf.c
@@ -698,6 +698,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) {
@@ -740,74 +741,115 @@ 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 (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, ".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;
 		}
+	}
 
-		err = 0;
-	unlink:
-		unlink(tmp_fn);
+	/* (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 = SHF_ALLOC;
+	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;
+	}
+
+	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.365.g02bc693789-goog

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

[Andrii Nakryiko]

On Mon, Feb 1, 2021 at 9:26 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.
>
> In this initial version layout is left completely up to libelf and
> indeed offsets of existing sections are likely to change.
>
> Signed-off-by: Giuliano Procida <gprocida@google.com>
> ---
>  libbtf.c | 134 ++++++++++++++++++++++++++++++++++++-------------------
>  1 file changed, 88 insertions(+), 46 deletions(-)
>
> diff --git a/libbtf.c b/libbtf.c
> index 81b1b36..5b91d3a 100644
> --- a/libbtf.c
> +++ b/libbtf.c
> @@ -698,6 +698,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) {
> @@ -740,74 +741,115 @@ 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, not 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__);

no elf_errmsg(elf_errno()) here? BTW, this form is very common (and a
bit verbose), so how about having a local macro that would make this
shorter, e.g.:

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
> -                       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;

5 is a bit magical, maybe use sizeof(".BTF") or a dedicated constant?

> +               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, ".BTF", 5);

same about magical 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;
>                 }
> +       }
>
> -               err = 0;
> -       unlink:
> -               unlink(tmp_fn);
> +       /* (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) {


btf_shdr just points to btf_shdr_mem, no? This duplication is not
pretty, why not:

GElf_Shdr btf_shdr;
if (!gelf_getshdr(btf_scn, &btf_shdr_mem)) { ... }

And then use btf_shdr. everywhere below

> +               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 = SHF_ALLOC;

this is wrong, making .BTF allocatable should be an opt-in, not all
applications need to have a loadable .BTF section. Plus this patch
doesn't really make it loadable, so SHF_ALLOC should be updated in the
later patch. And I don't think we'll use that for vmlinux BTF or
kernel module BTFs either, because there is still going to be linker
script involved.


> +       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;
> +       }
> +
> +       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.365.g02bc693789-goog
>

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

[Giuliano Procida]

On Thu, 4 Feb 2021 at 04:10, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
>

I've addressed all the comments below, except for flag control over
whether .BTF is loadable, in pending commits.
I've also moved the simple alignment change to earlier in the series.
I'll review any other comments and see if I can work out what's wrong
with the segment creation code or if it's worth preserving. I think
it's likely that anything involving segments won't survive.

Thanks for reviewing!

> On Mon, Feb 1, 2021 at 9:26 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.
> >
> > In this initial version layout is left completely up to libelf and
> > indeed offsets of existing sections are likely to change.
> >
> > Signed-off-by: Giuliano Procida <gprocida@google.com>
> > ---
> >  libbtf.c | 134 ++++++++++++++++++++++++++++++++++++-------------------
> >  1 file changed, 88 insertions(+), 46 deletions(-)
> >
> > diff --git a/libbtf.c b/libbtf.c
> > index 81b1b36..5b91d3a 100644
> > --- a/libbtf.c
> > +++ b/libbtf.c
> > @@ -698,6 +698,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) {
> > @@ -740,74 +741,115 @@ 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, not 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__);
>
> no elf_errmsg(elf_errno()) here? BTW, this form is very common (and a
> bit verbose), so how about having a local macro that would make this
> shorter, e.g.:
>
> 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
> > -                       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;
>
> 5 is a bit magical, maybe use sizeof(".BTF") or a dedicated constant?
>
> > +               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, ".BTF", 5);
>
> same about magical 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;
> >                 }
> > +       }
> >
> > -               err = 0;
> > -       unlink:
> > -               unlink(tmp_fn);
> > +       /* (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) {
>
>
> btf_shdr just points to btf_shdr_mem, no? This duplication is not
> pretty, why not:
>
> GElf_Shdr btf_shdr;
> if (!gelf_getshdr(btf_scn, &btf_shdr_mem)) { ... }
>
> And then use btf_shdr. everywhere below
>
> > +               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 = SHF_ALLOC;
>
> this is wrong, making .BTF allocatable should be an opt-in, not all
> applications need to have a loadable .BTF section. Plus this patch
> doesn't really make it loadable, so SHF_ALLOC should be updated in the
> later patch. And I don't think we'll use that for vmlinux BTF or
> kernel module BTFs either, because there is still going to be linker
> script involved.
>
>
> > +       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;
> > +       }
> > +
> > +       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.365.g02bc693789-goog
> >

[PATCH dwarves v2 2/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 5b91d3a..6e06a58 100644
--- a/libbtf.c
+++ b/libbtf.c
@@ -741,9 +741,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);
@@ -752,7 +771,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;
 		}
 	}
 
@@ -761,6 +783,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) {
@@ -791,6 +819,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);
@@ -810,12 +839,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) {
@@ -827,6 +859,8 @@ static int btf_elf__write(const char *filename, struct btf *btf)
 	btf_shdr->sh_flags = SHF_ALLOC;
 	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",
@@ -834,6 +868,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.365.g02bc693789-goog

Re: [PATCH dwarves v2 2/4] btf_encoder: Manually lay out updated ELF sections

[Andrii Nakryiko]

On Mon, Feb 1, 2021 at 9:26 AM Giuliano Procida <gprocida@google.com> wrote:
>
> 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

It's not clear to me what's wrong with libelf handling all the layout.
Even if libelf will move program segments around, what's the harm?
Does it break anything if we just let libelf do this?

> ".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(-)
>

[...]

Re: [PATCH dwarves v2 2/4] btf_encoder: Manually lay out updated ELF sections

[Giuliano Procida]

Hi.

On Thu, 4 Feb 2021 at 04:13, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
>
> On Mon, Feb 1, 2021 at 9:26 AM Giuliano Procida <gprocida@google.com> wrote:
> >
> > 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
>
> It's not clear to me what's wrong with libelf handling all the layout.
> Even if libelf will move program segments around, what's the harm?
> Does it break anything if we just let libelf do this?
>

It doesn't hurt the userspace case I care about. I've no idea what it
means in terms of vmlinux.

However, I wrote that text before I discovered that pahole -J isn't
actually used to modify kernel images.

One thing I haven't tried is to try to make .BTF loadable but leave
placement to libelf.

> > ".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(-)
> >
>
> [...]

Re: [PATCH dwarves v2 2/4] btf_encoder: Manually lay out updated ELF sections

[Andrii Nakryiko]

On Thu, Feb 4, 2021 at 10:34 AM Giuliano Procida <gprocida@google.com> wrote:
>
> Hi.
>
> On Thu, 4 Feb 2021 at 04:13, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
> >
> > On Mon, Feb 1, 2021 at 9:26 AM Giuliano Procida <gprocida@google.com> wrote:
> > >
> > > 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
> >
> > It's not clear to me what's wrong with libelf handling all the layout.
> > Even if libelf will move program segments around, what's the harm?
> > Does it break anything if we just let libelf do this?
> >
>
> It doesn't hurt the userspace case I care about. I've no idea what it
> means in terms of vmlinux.
>
> However, I wrote that text before I discovered that pahole -J isn't
> actually used to modify kernel images.
>
> One thing I haven't tried is to try to make .BTF loadable but leave
> placement to libelf.

I'd concentrate on getting rid of llvm-objcopy dependency first.
Making .BTF loadable is even riskier change and there is no use case
that relies on that today, so definitely worth to split that out.


>
> > > ".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(-)
> > >
> >
> > [...]

[PATCH dwarves v2 3/4] btf_encoder: Add .BTF as a loadable segment

[Giuliano Procida]

In addition to adding .BTF to the Section Header Table, we also need
to add it to the Program Header Table and rewrite the PHT's
description of itself.

The segment as loadbale, at address 0 and read-only.

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

diff --git a/libbtf.c b/libbtf.c
index 6e06a58..048a873 100644
--- a/libbtf.c
+++ b/libbtf.c
@@ -699,6 +699,7 @@ static int btf_elf__write(const char *filename, struct btf *btf)
 	int fd, err = -1;
 	size_t strndx;
 	void *str_table = NULL;
+	GElf_Phdr *pht = NULL;
 
 	fd = open(filename, O_RDWR);
 	if (fd < 0) {
@@ -900,6 +901,47 @@ static int btf_elf__write(const char *filename, struct btf *btf)
 		goto out;
 	}
 
+	size_t phnum = 0;
+	if (!elf_getphdrnum(elf, &phnum)) {
+		pht = malloc((phnum + 1) * sizeof(GElf_Phdr));
+		if (!pht) {
+			fprintf(stderr, "%s: malloc (PHT) failed\n", __func__);
+			goto out;
+		}
+		for (size_t ix = 0; ix < phnum; ++ix) {
+			if (!gelf_getphdr(elf, ix, &pht[ix])) {
+				fprintf(stderr,
+					"%s: gelf_getphdr(%zu) failed: %s\n",
+					__func__, ix, elf_errmsg(elf_errno()));
+				goto out;
+			}
+			if (pht[ix].p_type == PT_PHDR) {
+				size_t fsize = gelf_fsize(elf, ELF_T_PHDR,
+							  phnum+1, EV_CURRENT);
+				pht[ix].p_memsz = pht[ix].p_filesz = fsize;
+			}
+		}
+		pht[phnum].p_type = PT_LOAD;
+		pht[phnum].p_offset = btf_shdr->sh_offset;
+		pht[phnum].p_memsz = pht[phnum].p_filesz = btf_shdr->sh_size;
+		pht[phnum].p_vaddr = pht[phnum].p_paddr = 0;
+		pht[phnum].p_flags = PF_R;
+		void *phdr = gelf_newphdr(elf, phnum+1);
+		if (!phdr) {
+			fprintf(stderr, "%s: gelf_newphdr failed: %s\n",
+				__func__, elf_errmsg(elf_errno()));
+			goto out;
+		}
+		for (size_t ix = 0; ix < phnum+1; ++ix) {
+			if (!gelf_update_phdr(elf, ix, &pht[ix])) {
+				fprintf(stderr,
+					"%s: gelf_update_phdr(%zu) failed: %s\n",
+					__func__, ix, elf_errmsg(elf_errno()));
+				goto out;
+			}
+		}
+	}
+
 	if (elf_update(elf, ELF_C_WRITE) < 0) {
 		fprintf(stderr, "%s: elf_update (write) failed: %s\n",
 			__func__, elf_errmsg(elf_errno()));
@@ -908,6 +950,8 @@ static int btf_elf__write(const char *filename, struct btf *btf)
 	err = 0;
 
 out:
+	if (pht)
+		free(pht);
 	if (str_table)
 		free(str_table);
 	if (fd != -1)
-- 
2.30.0.365.g02bc693789-goog

Re: [PATCH dwarves v2 3/4] btf_encoder: Add .BTF as a loadable segment

[Giuliano Procida]

Two issues here.

On Mon, 1 Feb 2021 at 17:26, Giuliano Procida <gprocida@google.com> wrote:
>
> In addition to adding .BTF to the Section Header Table, we also need
> to add it to the Program Header Table and rewrite the PHT's
> description of itself.
>
> The segment as loadbale, at address 0 and read-only.
>

Typos.

> Signed-off-by: Giuliano Procida <gprocida@google.com>
> ---
>  libbtf.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 44 insertions(+)
>
> diff --git a/libbtf.c b/libbtf.c
> index 6e06a58..048a873 100644
> --- a/libbtf.c
> +++ b/libbtf.c
> @@ -699,6 +699,7 @@ static int btf_elf__write(const char *filename, struct btf *btf)
>         int fd, err = -1;
>         size_t strndx;
>         void *str_table = NULL;
> +       GElf_Phdr *pht = NULL;
>
>         fd = open(filename, O_RDWR);
>         if (fd < 0) {
> @@ -900,6 +901,47 @@ static int btf_elf__write(const char *filename, struct btf *btf)
>                 goto out;
>         }
>
> +       size_t phnum = 0;
> +       if (!elf_getphdrnum(elf, &phnum)) {
> +               pht = malloc((phnum + 1) * sizeof(GElf_Phdr));

Adding a segment unconditionally is incorrect.
It should behave differently if .BTF already has its own segment or is
part of an existing segment containing other sections.
The ELF surgery needed in the general case may be considerable.

> +               if (!pht) {
> +                       fprintf(stderr, "%s: malloc (PHT) failed\n", __func__);
> +                       goto out;
> +               }
> +               for (size_t ix = 0; ix < phnum; ++ix) {
> +                       if (!gelf_getphdr(elf, ix, &pht[ix])) {
> +                               fprintf(stderr,
> +                                       "%s: gelf_getphdr(%zu) failed: %s\n",
> +                                       __func__, ix, elf_errmsg(elf_errno()));
> +                               goto out;
> +                       }
> +                       if (pht[ix].p_type == PT_PHDR) {
> +                               size_t fsize = gelf_fsize(elf, ELF_T_PHDR,
> +                                                         phnum+1, EV_CURRENT);
> +                               pht[ix].p_memsz = pht[ix].p_filesz = fsize;
> +                       }
> +               }
> +               pht[phnum].p_type = PT_LOAD;
> +               pht[phnum].p_offset = btf_shdr->sh_offset;
> +               pht[phnum].p_memsz = pht[phnum].p_filesz = btf_shdr->sh_size;
> +               pht[phnum].p_vaddr = pht[phnum].p_paddr = 0;
> +               pht[phnum].p_flags = PF_R;
> +               void *phdr = gelf_newphdr(elf, phnum+1);
> +               if (!phdr) {
> +                       fprintf(stderr, "%s: gelf_newphdr failed: %s\n",
> +                               __func__, elf_errmsg(elf_errno()));
> +                       goto out;
> +               }
> +               for (size_t ix = 0; ix < phnum+1; ++ix) {
> +                       if (!gelf_update_phdr(elf, ix, &pht[ix])) {
> +                               fprintf(stderr,
> +                                       "%s: gelf_update_phdr(%zu) failed: %s\n",
> +                                       __func__, ix, elf_errmsg(elf_errno()));
> +                               goto out;
> +                       }
> +               }
> +       }
> +
>         if (elf_update(elf, ELF_C_WRITE) < 0) {
>                 fprintf(stderr, "%s: elf_update (write) failed: %s\n",
>                         __func__, elf_errmsg(elf_errno()));
> @@ -908,6 +950,8 @@ static int btf_elf__write(const char *filename, struct btf *btf)
>         err = 0;
>
>  out:
> +       if (pht)
> +               free(pht);
>         if (str_table)
>                 free(str_table);
>         if (fd != -1)
> --
> 2.30.0.365.g02bc693789-goog
>

[PATCH dwarves v2 4/4] btf_encoder: Align .BTF section/segment 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 | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/libbtf.c b/libbtf.c
index 048a873..ae99a93 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
@@ -847,8 +853,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) {
@@ -856,6 +862,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 = SHF_ALLOC;
 	if (dot_btf_offset)
@@ -926,6 +933,7 @@ static int btf_elf__write(const char *filename, struct btf *btf)
 		pht[phnum].p_memsz = pht[phnum].p_filesz = btf_shdr->sh_size;
 		pht[phnum].p_vaddr = pht[phnum].p_paddr = 0;
 		pht[phnum].p_flags = PF_R;
+		pht[phnum].p_align = btf_alignment;
 		void *phdr = gelf_newphdr(elf, phnum+1);
 		if (!phdr) {
 			fprintf(stderr, "%s: gelf_newphdr failed: %s\n",
-- 
2.30.0.365.g02bc693789-goog

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

[Andrii Nakryiko]

On Mon, Feb 1, 2021 at 9:26 AM Giuliano Procida <gprocida@google.com> wrote:
>
> 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 | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/libbtf.c b/libbtf.c
> index 048a873..ae99a93 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
> @@ -847,8 +853,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) {
> @@ -856,6 +862,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;

if we set just this and let libelf do the layout, would libelf ensure
8-byte alignment of .BTF section inside the ELF file?

>         btf_shdr->sh_entsize = 0;
>         btf_shdr->sh_flags = SHF_ALLOC;
>         if (dot_btf_offset)
> @@ -926,6 +933,7 @@ static int btf_elf__write(const char *filename, struct btf *btf)
>                 pht[phnum].p_memsz = pht[phnum].p_filesz = btf_shdr->sh_size;
>                 pht[phnum].p_vaddr = pht[phnum].p_paddr = 0;
>                 pht[phnum].p_flags = PF_R;
> +               pht[phnum].p_align = btf_alignment;
>                 void *phdr = gelf_newphdr(elf, phnum+1);
>                 if (!phdr) {
>                         fprintf(stderr, "%s: gelf_newphdr failed: %s\n",
> --
> 2.30.0.365.g02bc693789-goog
>

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

[Giuliano Procida]

Hi.

On Thu, 4 Feb 2021 at 04:10, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
>
> On Mon, Feb 1, 2021 at 9:26 AM Giuliano Procida <gprocida@google.com> wrote:
> >
> > 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 | 12 ++++++++++--
> >  1 file changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/libbtf.c b/libbtf.c
> > index 048a873..ae99a93 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
> > @@ -847,8 +853,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) {
> > @@ -856,6 +862,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;
>
> if we set just this and let libelf do the layout, would libelf ensure
> 8-byte alignment of .BTF section inside the ELF file?
>

Yes. I'll follow up with a trivial patch that does that.




> >         btf_shdr->sh_entsize = 0;
> >         btf_shdr->sh_flags = SHF_ALLOC;
> >         if (dot_btf_offset)
> > @@ -926,6 +933,7 @@ static int btf_elf__write(const char *filename, struct btf *btf)
> >                 pht[phnum].p_memsz = pht[phnum].p_filesz = btf_shdr->sh_size;
> >                 pht[phnum].p_vaddr = pht[phnum].p_paddr = 0;
> >                 pht[phnum].p_flags = PF_R;
> > +               pht[phnum].p_align = btf_alignment;
> >                 void *phdr = gelf_newphdr(elf, phnum+1);
> >                 if (!phdr) {
> >                         fprintf(stderr, "%s: gelf_newphdr failed: %s\n",
> > --
> > 2.30.0.365.g02bc693789-goog
> >

[PATCH] 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 | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/libbtf.c b/libbtf.c
index 5b91d3a..9974747 100644
--- a/libbtf.c
+++ b/libbtf.c
@@ -740,6 +740,14 @@ static int btf_elf__write(const char *filename, struct btf *btf)
 		goto out;
 	}
 
+	/*
+	 * 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 check if there is already a .BTF section present.
 	 */
@@ -823,6 +831,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 = SHF_ALLOC;
 	if (dot_btf_offset)
-- 
2.30.0.365.g02bc693789-goog

[PATCH dwarves v3 0/5] ELF writing changes

[Giuliano Procida]

These v3 patches address comments by Andrii and Arnaldo.

1. This uses a variadic macro and relies on a GNU extension. It can
probably be reworked to not use this, with some effort.

2. I was following the original code that had aliases structs and
pointers for the same data. I adjusted my code not to do this (later
commits) and changed the existing occurrences in the same function
(this commit). The net effect is clearer lifetime and ownership.

3. In a similar vein, the variable scn really only needed to be a
loop-local iterator.

4. This removes the dependency on llvm-objcopy.

5. And we set the alignment on the seciton.

I'm not posting the later patches that perform explicit ELF section
layout and which (attempt) to also do things with ELF segments.

Giuliano Procida (5):
  btf_encoder: Funnel ELF error reporting through a macro
  btf_encoder: Do not use both structs and pointers for the same data
  btf_encoder: Traverse sections using a for-loop
  btf_encoder: Add .BTF section using libelf
  btf_encoder: Align .BTF section to 8 bytes

 libbtf.c | 183 ++++++++++++++++++++++++++++++++++---------------------
 1 file changed, 114 insertions(+), 69 deletions(-)

-- 
2.30.0.478.g8a0d178c01-goog

[PATCH dwarves v3 1/5] btf_encoder: Funnel ELF error reporting through a macro

[Giuliano Procida]

This adds elf_error which prepends error messages with the function
and appends a readable ELF error status.

Also capitalise ELF consistently in error messages.

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

diff --git a/libbtf.c b/libbtf.c
index 9f76283..7bc49ba 100644
--- a/libbtf.c
+++ b/libbtf.c
@@ -27,6 +27,16 @@
 #include "dwarves.h"
 #include "elf_symtab.h"
 
+/*
+ * This depends on the GNU extension to eliminate the stray comma in the zero
+ * arguments case.
+ *
+ * The difference between elf_errmsg(-1) and elf_errmsg(elf_errno()) is that the
+ * latter clears the current error.
+ */
+#define elf_error(fmt, ...) \
+	fprintf(stderr, "%s: " fmt ": %s.\n", __func__, ##__VA_ARGS__, elf_errmsg(-1))
+
 struct btf *base_btf;
 uint8_t btf_elf__verbose;
 uint8_t btf_elf__force;
@@ -103,15 +113,13 @@ try_as_raw_btf:
 			goto errout;
 
 		if (elf_version(EV_CURRENT) == EV_NONE) {
-			fprintf(stderr, "%s: cannot set libelf version.\n",
-				__func__);
+			elf_error("cannot set libelf version");
 			goto errout;
 		}
 
 		btfe->elf = elf_begin(btfe->in_fd, ELF_C_READ_MMAP, NULL);
 		if (!btfe->elf) {
-			fprintf(stderr, "%s: cannot read %s ELF file: %s.\n",
-				__func__, filename, elf_errmsg(elf_errno()));
+			elf_error("cannot read %s ELF file", filename);
 			goto errout;
 		}
 	}
@@ -127,7 +135,7 @@ try_as_raw_btf:
 			goto try_as_raw_btf;
 		}
 		if (btf_elf__verbose)
-			fprintf(stderr, "%s: cannot get elf header.\n", __func__);
+			elf_error("cannot get ELF header");
 		goto errout;
 	}
 
@@ -141,7 +149,7 @@ try_as_raw_btf:
 		btf__set_endianness(btfe->btf, BTF_BIG_ENDIAN);
 		break;
 	default:
-		fprintf(stderr, "%s: unknown elf endianness.\n", __func__);
+		fprintf(stderr, "%s: unknown ELF endianness.\n", __func__);
 		goto errout;
 	}
 
@@ -707,15 +715,13 @@ static int btf_elf__write(const char *filename, struct btf *btf)
 	}
 
 	if (elf_version(EV_CURRENT) == EV_NONE) {
-		fprintf(stderr, "Cannot set libelf version: %s.\n",
-			elf_errmsg(elf_errno()));
+		elf_error("Cannot set libelf version");
 		goto out;
 	}
 
 	elf = elf_begin(fd, ELF_C_RDWR, NULL);
 	if (elf == NULL) {
-		fprintf(stderr, "Cannot update ELF file: %s.\n",
-			elf_errmsg(elf_errno()));
+		elf_error("Cannot update ELF file");
 		goto out;
 	}
 
@@ -723,8 +729,7 @@ 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: %s.\n", __func__,
-			elf_errmsg(elf_errno()));
+		elf_error("elf_getehdr failed");
 		goto out;
 	}
 
@@ -736,7 +741,7 @@ static int btf_elf__write(const char *filename, struct btf *btf)
 		btf__set_endianness(btf, BTF_BIG_ENDIAN);
 		break;
 	default:
-		fprintf(stderr, "%s: unknown elf endianness.\n", __func__);
+		fprintf(stderr, "%s: unknown ELF endianness.\n", __func__);
 		goto out;
 	}
 
@@ -768,8 +773,7 @@ static int btf_elf__write(const char *filename, struct btf *btf)
 		    elf_update(elf, ELF_C_WRITE) >= 0)
 			err = 0;
 		else
-			fprintf(stderr, "%s: elf_update failed: %s.\n",
-				__func__, elf_errmsg(elf_errno()));
+			elf_error("elf_update failed");
 	} else {
 		const char *llvm_objcopy;
 		char tmp_fn[PATH_MAX];
-- 
2.30.0.478.g8a0d178c01-goog

Re: [PATCH dwarves v3 1/5] btf_encoder: Funnel ELF error reporting through a macro

[Andrii Nakryiko]

On Fri, Feb 5, 2021 at 5:42 AM Giuliano Procida <gprocida@google.com> wrote:
>
> This adds elf_error which prepends error messages with the function
> and appends a readable ELF error status.
>
> Also capitalise ELF consistently in error messages.
>
> Signed-off-by: Giuliano Procida <gprocida@google.com>
> ---

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

>  libbtf.c | 34 +++++++++++++++++++---------------
>  1 file changed, 19 insertions(+), 15 deletions(-)
>
> diff --git a/libbtf.c b/libbtf.c
> index 9f76283..7bc49ba 100644
> --- a/libbtf.c
> +++ b/libbtf.c
> @@ -27,6 +27,16 @@
>  #include "dwarves.h"
>  #include "elf_symtab.h"
>

[...]

[PATCH dwarves v3 2/5] btf_encoder: Do not use both structs and pointers for the same data

[Giuliano Procida]

Many operations in the libelf API return a pointer to a user-provided
struct (on success) or NULL (on failure).

There are a couple of places in btf_elf__write where both structs and
pointers to the same structs are used. Holding on to the pointers
raises ownership and lifetime issues unncessarily and the code is
cleaner with only a single access path for these data.

The code now treats the returned pointers as booleans.

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

diff --git a/libbtf.c b/libbtf.c
index 7bc49ba..ace8896 100644
--- a/libbtf.c
+++ b/libbtf.c
@@ -698,8 +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)
 {
-	GElf_Shdr shdr_mem, *shdr;
-	GElf_Ehdr ehdr_mem, *ehdr;
+	GElf_Ehdr ehdr;
 	Elf_Data *btf_data = NULL;
 	Elf_Scn *scn = NULL;
 	Elf *elf = NULL;
@@ -727,13 +726,12 @@ static int btf_elf__write(const char *filename, struct btf *btf)
 
 	elf_flagelf(elf, ELF_C_SET, ELF_F_DIRTY);
 
-	ehdr = gelf_getehdr(elf, &ehdr_mem);
-	if (ehdr == NULL) {
+	if (!gelf_getehdr(elf, &ehdr)) {
 		elf_error("elf_getehdr failed");
 		goto out;
 	}
 
-	switch (ehdr_mem.e_ident[EI_DATA]) {
+	switch (ehdr.e_ident[EI_DATA]) {
 	case ELFDATA2LSB:
 		btf__set_endianness(btf, BTF_LITTLE_ENDIAN);
 		break;
@@ -751,10 +749,10 @@ static int btf_elf__write(const char *filename, struct btf *btf)
 
 	elf_getshdrstrndx(elf, &strndx);
 	while ((scn = elf_nextscn(elf, scn)) != NULL) {
-		shdr = gelf_getshdr(scn, &shdr_mem);
-		if (shdr == NULL)
+		GElf_Shdr shdr;
+		if (!gelf_getshdr(scn, &shdr))
 			continue;
-		char *secname = elf_strptr(elf, strndx, shdr->sh_name);
+		char *secname = elf_strptr(elf, strndx, shdr.sh_name);
 		if (strcmp(secname, ".BTF") == 0) {
 			btf_data = elf_getdata(scn, btf_data);
 			break;
-- 
2.30.0.478.g8a0d178c01-goog

Re: [PATCH dwarves v3 2/5] btf_encoder: Do not use both structs and pointers for the same data

[Andrii Nakryiko]

On Fri, Feb 5, 2021 at 5:42 AM Giuliano Procida <gprocida@google.com> wrote:
>
> Many operations in the libelf API return a pointer to a user-provided
> struct (on success) or NULL (on failure).
>
> There are a couple of places in btf_elf__write where both structs and
> pointers to the same structs are used. Holding on to the pointers
> raises ownership and lifetime issues unncessarily and the code is

typo: unnecessarily

> cleaner with only a single access path for these data.
>
> The code now treats the returned pointers as booleans.
>
> Signed-off-by: Giuliano Procida <gprocida@google.com>
> ---

styling nits, but otherwise LGTM

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

>  libbtf.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/libbtf.c b/libbtf.c
> index 7bc49ba..ace8896 100644
> --- a/libbtf.c
> +++ b/libbtf.c
> @@ -698,8 +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)
>  {
> -       GElf_Shdr shdr_mem, *shdr;
> -       GElf_Ehdr ehdr_mem, *ehdr;
> +       GElf_Ehdr ehdr;
>         Elf_Data *btf_data = NULL;
>         Elf_Scn *scn = NULL;
>         Elf *elf = NULL;
> @@ -727,13 +726,12 @@ static int btf_elf__write(const char *filename, struct btf *btf)
>
>         elf_flagelf(elf, ELF_C_SET, ELF_F_DIRTY);
>
> -       ehdr = gelf_getehdr(elf, &ehdr_mem);
> -       if (ehdr == NULL) {
> +       if (!gelf_getehdr(elf, &ehdr)) {
>                 elf_error("elf_getehdr failed");
>                 goto out;
>         }
>
> -       switch (ehdr_mem.e_ident[EI_DATA]) {
> +       switch (ehdr.e_ident[EI_DATA]) {
>         case ELFDATA2LSB:
>                 btf__set_endianness(btf, BTF_LITTLE_ENDIAN);
>                 break;
> @@ -751,10 +749,10 @@ static int btf_elf__write(const char *filename, struct btf *btf)
>
>         elf_getshdrstrndx(elf, &strndx);
>         while ((scn = elf_nextscn(elf, scn)) != NULL) {
> -               shdr = gelf_getshdr(scn, &shdr_mem);
> -               if (shdr == NULL)
> +               GElf_Shdr shdr;

it's a good style to have an empty line between variable declaration
block and subsequent instructions


> +               if (!gelf_getshdr(scn, &shdr))
>                         continue;
> -               char *secname = elf_strptr(elf, strndx, shdr->sh_name);
> +               char *secname = elf_strptr(elf, strndx, shdr.sh_name);
>                 if (strcmp(secname, ".BTF") == 0) {
>                         btf_data = elf_getdata(scn, btf_data);
>                         break;
> --
> 2.30.0.478.g8a0d178c01-goog
>

Re: [PATCH dwarves v3 2/5] btf_encoder: Do not use both structs and pointers for the same data

[Giuliano Procida]

Hi.

On Mon, 8 Feb 2021 at 22:23, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
>
> On Fri, Feb 5, 2021 at 5:42 AM Giuliano Procida <gprocida@google.com> wrote:
> >
> > Many operations in the libelf API return a pointer to a user-provided
> > struct (on success) or NULL (on failure).
> >
> > There are a couple of places in btf_elf__write where both structs and
> > pointers to the same structs are used. Holding on to the pointers
> > raises ownership and lifetime issues unncessarily and the code is
>
> typo: unnecessarily
>

Thanks. Fixed.

> > cleaner with only a single access path for these data.
> >
> > The code now treats the returned pointers as booleans.
> >
> > Signed-off-by: Giuliano Procida <gprocida@google.com>
> > ---
>
> styling nits, but otherwise LGTM
>
> Acked-by: Andrii Nakryiko <andrii@kernel.org>
>
> >  libbtf.c | 14 ++++++--------
> >  1 file changed, 6 insertions(+), 8 deletions(-)
> >
> > diff --git a/libbtf.c b/libbtf.c
> > index 7bc49ba..ace8896 100644
> > --- a/libbtf.c
> > +++ b/libbtf.c
> > @@ -698,8 +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)
> >  {
> > -       GElf_Shdr shdr_mem, *shdr;
> > -       GElf_Ehdr ehdr_mem, *ehdr;
> > +       GElf_Ehdr ehdr;
> >         Elf_Data *btf_data = NULL;
> >         Elf_Scn *scn = NULL;
> >         Elf *elf = NULL;
> > @@ -727,13 +726,12 @@ static int btf_elf__write(const char *filename, struct btf *btf)
> >
> >         elf_flagelf(elf, ELF_C_SET, ELF_F_DIRTY);
> >
> > -       ehdr = gelf_getehdr(elf, &ehdr_mem);
> > -       if (ehdr == NULL) {
> > +       if (!gelf_getehdr(elf, &ehdr)) {
> >                 elf_error("elf_getehdr failed");
> >                 goto out;
> >         }
> >
> > -       switch (ehdr_mem.e_ident[EI_DATA]) {
> > +       switch (ehdr.e_ident[EI_DATA]) {
> >         case ELFDATA2LSB:
> >                 btf__set_endianness(btf, BTF_LITTLE_ENDIAN);
> >                 break;
> > @@ -751,10 +749,10 @@ static int btf_elf__write(const char *filename, struct btf *btf)
> >
> >         elf_getshdrstrndx(elf, &strndx);
> >         while ((scn = elf_nextscn(elf, scn)) != NULL) {
> > -               shdr = gelf_getshdr(scn, &shdr_mem);
> > -               if (shdr == NULL)
> > +               GElf_Shdr shdr;
>
> it's a good style to have an empty line between variable declaration
> block and subsequent instructions
>

The variable in this question is effectively initialised by the
statement on the next line, breaking them apart looks odd.
Also, this is not a variable that needs end-of-scope clean-up. Its
position at the top of the scope is coincidental.
Later commits in the series also place declaration and initialisation
as close together as possible.
The only variables I would intentionally place at the top of a given
scope *and* far from their natural points of initialisation are those
corresponding to resources that need to be released at the end of the
scope, with the labelled exit idiom.
I feel this gives a better balance between readability (keeping things
local) and keeping track of resources (memory, fds, other handles) in
a scope.

However, if that's contrary to the house style, it's easy enough to
pull all the declarations out and move them to the top and separate
them; the compiler should be clever enough to share stack slots in any
case.

Let me know.

Regards,
Giuliano.

>
> > +               if (!gelf_getshdr(scn, &shdr))
> >                         continue;
> > -               char *secname = elf_strptr(elf, strndx, shdr->sh_name);
> > +               char *secname = elf_strptr(elf, strndx, shdr.sh_name);
> >                 if (strcmp(secname, ".BTF") == 0) {
> >                         btf_data = elf_getdata(scn, btf_data);
> >                         break;
> > --
> > 2.30.0.478.g8a0d178c01-goog
> >

[PATCH dwarves v3 3/5] btf_encoder: Traverse sections using a for-loop

[Giuliano Procida]

The pointer (iterator) scn can be made local to the loop and a more
general while-loop is not needed.

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

diff --git a/libbtf.c b/libbtf.c
index ace8896..4ae7150 100644
--- a/libbtf.c
+++ b/libbtf.c
@@ -700,7 +700,6 @@ static int btf_elf__write(const char *filename, struct btf *btf)
 {
 	GElf_Ehdr ehdr;
 	Elf_Data *btf_data = NULL;
-	Elf_Scn *scn = NULL;
 	Elf *elf = NULL;
 	const void *raw_btf_data;
 	uint32_t raw_btf_size;
@@ -748,7 +747,7 @@ static int btf_elf__write(const char *filename, struct btf *btf)
 	 */
 
 	elf_getshdrstrndx(elf, &strndx);
-	while ((scn = elf_nextscn(elf, 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;
-- 
2.30.0.478.g8a0d178c01-goog

Re: [PATCH dwarves v3 3/5] btf_encoder: Traverse sections using a for-loop

[Andrii Nakryiko]

On Fri, Feb 5, 2021 at 5:42 AM Giuliano Procida <gprocida@google.com> wrote:
>
> The pointer (iterator) scn can be made local to the loop and a more
> general while-loop is not needed.
>
> Signed-off-by: Giuliano Procida <gprocida@google.com>
> ---
>  libbtf.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/libbtf.c b/libbtf.c
> index ace8896..4ae7150 100644
> --- a/libbtf.c
> +++ b/libbtf.c
> @@ -700,7 +700,6 @@ static int btf_elf__write(const char *filename, struct btf *btf)
>  {
>         GElf_Ehdr ehdr;
>         Elf_Data *btf_data = NULL;
> -       Elf_Scn *scn = NULL;
>         Elf *elf = NULL;
>         const void *raw_btf_data;
>         uint32_t raw_btf_size;
> @@ -748,7 +747,7 @@ static int btf_elf__write(const char *filename, struct btf *btf)
>          */
>
>         elf_getshdrstrndx(elf, &strndx);
> -       while ((scn = elf_nextscn(elf, scn)) != NULL) {

this is pretty "canonical" as far as libelf usage goes, I wouldn't
touch this code, but it's up to Arnaldo


> +       for (Elf_Scn *scn = elf_nextscn(elf, NULL); scn; scn = elf_nextscn(elf, scn)) {
>                 GElf_Shdr shdr;
>                 if (!gelf_getshdr(scn, &shdr))
>                         continue;
> --
> 2.30.0.478.g8a0d178c01-goog
>

Re: [PATCH dwarves v3 3/5] btf_encoder: Traverse sections using a for-loop

[Giuliano Procida]

Hi.

On Mon, 8 Feb 2021 at 22:24, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
>
> On Fri, Feb 5, 2021 at 5:42 AM Giuliano Procida <gprocida@google.com> wrote:
> >
> > The pointer (iterator) scn can be made local to the loop and a more
> > general while-loop is not needed.
> >
> > Signed-off-by: Giuliano Procida <gprocida@google.com>
> > ---
> >  libbtf.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/libbtf.c b/libbtf.c
> > index ace8896..4ae7150 100644
> > --- a/libbtf.c
> > +++ b/libbtf.c
> > @@ -700,7 +700,6 @@ static int btf_elf__write(const char *filename, struct btf *btf)
> >  {
> >         GElf_Ehdr ehdr;
> >         Elf_Data *btf_data = NULL;
> > -       Elf_Scn *scn = NULL;
> >         Elf *elf = NULL;
> >         const void *raw_btf_data;
> >         uint32_t raw_btf_size;
> > @@ -748,7 +747,7 @@ static int btf_elf__write(const char *filename, struct btf *btf)
> >          */
> >
> >         elf_getshdrstrndx(elf, &strndx);
> > -       while ((scn = elf_nextscn(elf, scn)) != NULL) {
>
> this is pretty "canonical" as far as libelf usage goes, I wouldn't
> touch this code, but it's up to Arnaldo
>

Ack.
In an intermediate version of the code, I got bitten when I used scn
by mistake instead of another pointer.
This wouldn't have compiled if scn had been scoped to the loop.

Giuliano.

>
> > +       for (Elf_Scn *scn = elf_nextscn(elf, NULL); scn; scn = elf_nextscn(elf, scn)) {
> >                 GElf_Shdr shdr;
> >                 if (!gelf_getshdr(scn, &shdr))
> >                         continue;
> > --
> > 2.30.0.478.g8a0d178c01-goog
> >

[PATCH dwarves v3 4/5] 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.

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(-)

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)
-- 
2.30.0.478.g8a0d178c01-goog

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

[Andrii Nakryiko]

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) {

[...]

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

[Giuliano Procida]

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) {
>
> [...]

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

[Giuliano Procida]

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

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

diff --git a/libbtf.c b/libbtf.c
index 9f4abb3..6754a17 100644
--- a/libbtf.c
+++ b/libbtf.c
@@ -744,6 +744,14 @@ static int btf_elf__write(const char *filename, struct btf *btf)
 		goto out;
 	}
 
+	/*
+	 * 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 check if there is already a .BTF section present.
 	 */
@@ -821,6 +829,7 @@ static int btf_elf__write(const char *filename, struct btf *btf)
 		elf_error("elf_getshdr(btf_scn) failed");
 		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.478.g8a0d178c01-goog

Re: [PATCH dwarves v3 5/5] btf_encoder: Align .BTF section to 8 bytes

[Andrii Nakryiko]

On Fri, Feb 5, 2021 at 5:42 AM Giuliano Procida <gprocida@google.com> wrote:
>
> This is to avoid misaligned access to BTF type structs when
> memory-mapping ELF objects.
>
> Signed-off-by: Giuliano Procida <gprocida@google.com>
> ---

I trust you did verify that it actually works in cases where
previously .BTF was mis-aligned?

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

>  libbtf.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/libbtf.c b/libbtf.c
> index 9f4abb3..6754a17 100644
> --- a/libbtf.c
> +++ b/libbtf.c
> @@ -744,6 +744,14 @@ static int btf_elf__write(const char *filename, struct btf *btf)
>                 goto out;
>         }
>
> +       /*
> +        * 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 check if there is already a .BTF section present.
>          */
> @@ -821,6 +829,7 @@ static int btf_elf__write(const char *filename, struct btf *btf)
>                 elf_error("elf_getshdr(btf_scn) failed");
>                 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.478.g8a0d178c01-goog
>

Re: [PATCH dwarves v3 5/5] btf_encoder: Align .BTF section to 8 bytes

[Giuliano Procida]

On Mon, 8 Feb 2021 at 22:30, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
>
> On Fri, Feb 5, 2021 at 5:42 AM Giuliano Procida <gprocida@google.com> wrote:
> >
> > This is to avoid misaligned access to BTF type structs when
> > memory-mapping ELF objects.
> >
> > Signed-off-by: Giuliano Procida <gprocida@google.com>
> > ---
>
> I trust you did verify that it actually works in cases where
> previously .BTF was mis-aligned?
>

Yes. :-)

> Acked-by: Andrii Nakryiko <andrii@kernel.org>
>
> >  libbtf.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> >
> > diff --git a/libbtf.c b/libbtf.c
> > index 9f4abb3..6754a17 100644
> > --- a/libbtf.c
> > +++ b/libbtf.c
> > @@ -744,6 +744,14 @@ static int btf_elf__write(const char *filename, struct btf *btf)
> >                 goto out;
> >         }
> >
> > +       /*
> > +        * 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 check if there is already a .BTF section present.
> >          */
> > @@ -821,6 +829,7 @@ static int btf_elf__write(const char *filename, struct btf *btf)
> >                 elf_error("elf_getshdr(btf_scn) failed");
> >                 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.478.g8a0d178c01-goog
> >
>
> --
> To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com.
>

[PATCH dwarves v4 0/5] ELF writing changes

[Giuliano Procida]

Arnaldo.

Versus v3 I've addressed Andrii's review comments, but some there may
be some style consistency issues remaining.

The first three changes are intended to improve code maintenance.
The fourth remoevs the llvm-objcopy dependency.
THe fifth aligns the .BTF section.

Giuliano.

Giuliano Procida (5):
  btf_encoder: Funnel ELF error reporting through a macro
  btf_encoder: Do not use both structs and pointers for the same data
  btf_encoder: Traverse sections using a for-loop
  btf_encoder: Add .BTF section using libelf
  btf_encoder: Align .BTF section to 8 bytes

 libbtf.c | 183 ++++++++++++++++++++++++++++++++++---------------------
 1 file changed, 114 insertions(+), 69 deletions(-)

-- 
2.30.0.478.g8a0d178c01-goog

[PATCH dwarves v4 1/5] btf_encoder: Funnel ELF error reporting through a macro

[Giuliano Procida]

This adds elf_error which prepends error messages with the function
and appends a readable ELF error status.

Also capitalise ELF consistently in error messages.

Acked-by: Andrii Nakryiko <andrii@kernel.org>
Signed-off-by: Giuliano Procida <gprocida@google.com>
---
 libbtf.c | 34 +++++++++++++++++++---------------
 1 file changed, 19 insertions(+), 15 deletions(-)

diff --git a/libbtf.c b/libbtf.c
index 9f76283..7bc49ba 100644
--- a/libbtf.c
+++ b/libbtf.c
@@ -27,6 +27,16 @@
 #include "dwarves.h"
 #include "elf_symtab.h"
 
+/*
+ * This depends on the GNU extension to eliminate the stray comma in the zero
+ * arguments case.
+ *
+ * The difference between elf_errmsg(-1) and elf_errmsg(elf_errno()) is that the
+ * latter clears the current error.
+ */
+#define elf_error(fmt, ...) \
+	fprintf(stderr, "%s: " fmt ": %s.\n", __func__, ##__VA_ARGS__, elf_errmsg(-1))
+
 struct btf *base_btf;
 uint8_t btf_elf__verbose;
 uint8_t btf_elf__force;
@@ -103,15 +113,13 @@ try_as_raw_btf:
 			goto errout;
 
 		if (elf_version(EV_CURRENT) == EV_NONE) {
-			fprintf(stderr, "%s: cannot set libelf version.\n",
-				__func__);
+			elf_error("cannot set libelf version");
 			goto errout;
 		}
 
 		btfe->elf = elf_begin(btfe->in_fd, ELF_C_READ_MMAP, NULL);
 		if (!btfe->elf) {
-			fprintf(stderr, "%s: cannot read %s ELF file: %s.\n",
-				__func__, filename, elf_errmsg(elf_errno()));
+			elf_error("cannot read %s ELF file", filename);
 			goto errout;
 		}
 	}
@@ -127,7 +135,7 @@ try_as_raw_btf:
 			goto try_as_raw_btf;
 		}
 		if (btf_elf__verbose)
-			fprintf(stderr, "%s: cannot get elf header.\n", __func__);
+			elf_error("cannot get ELF header");
 		goto errout;
 	}
 
@@ -141,7 +149,7 @@ try_as_raw_btf:
 		btf__set_endianness(btfe->btf, BTF_BIG_ENDIAN);
 		break;
 	default:
-		fprintf(stderr, "%s: unknown elf endianness.\n", __func__);
+		fprintf(stderr, "%s: unknown ELF endianness.\n", __func__);
 		goto errout;
 	}
 
@@ -707,15 +715,13 @@ static int btf_elf__write(const char *filename, struct btf *btf)
 	}
 
 	if (elf_version(EV_CURRENT) == EV_NONE) {
-		fprintf(stderr, "Cannot set libelf version: %s.\n",
-			elf_errmsg(elf_errno()));
+		elf_error("Cannot set libelf version");
 		goto out;
 	}
 
 	elf = elf_begin(fd, ELF_C_RDWR, NULL);
 	if (elf == NULL) {
-		fprintf(stderr, "Cannot update ELF file: %s.\n",
-			elf_errmsg(elf_errno()));
+		elf_error("Cannot update ELF file");
 		goto out;
 	}
 
@@ -723,8 +729,7 @@ 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: %s.\n", __func__,
-			elf_errmsg(elf_errno()));
+		elf_error("elf_getehdr failed");
 		goto out;
 	}
 
@@ -736,7 +741,7 @@ static int btf_elf__write(const char *filename, struct btf *btf)
 		btf__set_endianness(btf, BTF_BIG_ENDIAN);
 		break;
 	default:
-		fprintf(stderr, "%s: unknown elf endianness.\n", __func__);
+		fprintf(stderr, "%s: unknown ELF endianness.\n", __func__);
 		goto out;
 	}
 
@@ -768,8 +773,7 @@ static int btf_elf__write(const char *filename, struct btf *btf)
 		    elf_update(elf, ELF_C_WRITE) >= 0)
 			err = 0;
 		else
-			fprintf(stderr, "%s: elf_update failed: %s.\n",
-				__func__, elf_errmsg(elf_errno()));
+			elf_error("elf_update failed");
 	} else {
 		const char *llvm_objcopy;
 		char tmp_fn[PATH_MAX];
-- 
2.30.0.478.g8a0d178c01-goog

[PATCH dwarves v4 2/5] btf_encoder: Do not use both structs and pointers for the same data

[Giuliano Procida]

Many operations in the libelf API return a pointer to a user-provided
struct (on success) or NULL (on failure).

There are a couple of places in btf_elf__write where both structs and
pointers to the same structs are used. Holding on to the pointers
raises ownership and lifetime issues unnecessarily and the code is
cleaner with only a single access path for these data.

The code now treats the returned pointers as booleans.

Acked-by: Andrii Nakryiko <andrii@kernel.org>
Signed-off-by: Giuliano Procida <gprocida@google.com>
---
 libbtf.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/libbtf.c b/libbtf.c
index 7bc49ba..ace8896 100644
--- a/libbtf.c
+++ b/libbtf.c
@@ -698,8 +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)
 {
-	GElf_Shdr shdr_mem, *shdr;
-	GElf_Ehdr ehdr_mem, *ehdr;
+	GElf_Ehdr ehdr;
 	Elf_Data *btf_data = NULL;
 	Elf_Scn *scn = NULL;
 	Elf *elf = NULL;
@@ -727,13 +726,12 @@ static int btf_elf__write(const char *filename, struct btf *btf)
 
 	elf_flagelf(elf, ELF_C_SET, ELF_F_DIRTY);
 
-	ehdr = gelf_getehdr(elf, &ehdr_mem);
-	if (ehdr == NULL) {
+	if (!gelf_getehdr(elf, &ehdr)) {
 		elf_error("elf_getehdr failed");
 		goto out;
 	}
 
-	switch (ehdr_mem.e_ident[EI_DATA]) {
+	switch (ehdr.e_ident[EI_DATA]) {
 	case ELFDATA2LSB:
 		btf__set_endianness(btf, BTF_LITTLE_ENDIAN);
 		break;
@@ -751,10 +749,10 @@ static int btf_elf__write(const char *filename, struct btf *btf)
 
 	elf_getshdrstrndx(elf, &strndx);
 	while ((scn = elf_nextscn(elf, scn)) != NULL) {
-		shdr = gelf_getshdr(scn, &shdr_mem);
-		if (shdr == NULL)
+		GElf_Shdr shdr;
+		if (!gelf_getshdr(scn, &shdr))
 			continue;
-		char *secname = elf_strptr(elf, strndx, shdr->sh_name);
+		char *secname = elf_strptr(elf, strndx, shdr.sh_name);
 		if (strcmp(secname, ".BTF") == 0) {
 			btf_data = elf_getdata(scn, btf_data);
 			break;
-- 
2.30.0.478.g8a0d178c01-goog

[PATCH dwarves v4 3/5] btf_encoder: Traverse sections using a for-loop

[Giuliano Procida]

The pointer (iterator) scn can be made local to the loop and a more
general while-loop is not needed.

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

diff --git a/libbtf.c b/libbtf.c
index ace8896..4ae7150 100644
--- a/libbtf.c
+++ b/libbtf.c
@@ -700,7 +700,6 @@ static int btf_elf__write(const char *filename, struct btf *btf)
 {
 	GElf_Ehdr ehdr;
 	Elf_Data *btf_data = NULL;
-	Elf_Scn *scn = NULL;
 	Elf *elf = NULL;
 	const void *raw_btf_data;
 	uint32_t raw_btf_size;
@@ -748,7 +747,7 @@ static int btf_elf__write(const char *filename, struct btf *btf)
 	 */
 
 	elf_getshdrstrndx(elf, &strndx);
-	while ((scn = elf_nextscn(elf, 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;
-- 
2.30.0.478.g8a0d178c01-goog

[PATCH dwarves v4 4/5] 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.

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

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

diff --git a/libbtf.c b/libbtf.c
index 4ae7150..9ff03ca 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 BTF_SEC_NAME[] = ".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, BTF_SEC_NAME) == 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(BTF_SEC_NAME);
+		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, BTF_SEC_NAME, sizeof(BTF_SEC_NAME));
+		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)
-- 
2.30.0.478.g8a0d178c01-goog

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

[Giuliano Procida]

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

Acked-by: Andrii Nakryiko <andrii@kernel.org>
Signed-off-by: Giuliano Procida <gprocida@google.com>
---
 libbtf.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/libbtf.c b/libbtf.c
index 9ff03ca..ee677fa 100644
--- a/libbtf.c
+++ b/libbtf.c
@@ -744,6 +744,14 @@ static int btf_elf__write(const char *filename, struct btf *btf)
 		goto out;
 	}
 
+	/*
+	 * 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 check if there is already a .BTF section present.
 	 */
@@ -821,6 +829,7 @@ static int btf_elf__write(const char *filename, struct btf *btf)
 		elf_error("elf_getshdr(btf_scn) failed");
 		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.478.g8a0d178c01-goog