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

[Giuliano Procida <gprocida@google.com>]

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
>