From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: Giuliano Procida <gprocida@google.com>
Cc: dwarves@vger.kernel.org,
"Arnaldo Carvalho de Melo" <acme@kernel.org>,
"Andrii Nakryiko" <andrii@kernel.org>,
"Alexei Starovoitov" <ast@kernel.org>,
"Matthias Männich" <maennich@google.com>,
kernel-team@android.com, "Kernel Team" <kernel-team@fb.com>,
bpf <bpf@vger.kernel.org>
Subject: Re: [PATCH dwarves v2 1/4] btf_encoder: Add .BTF section using libelf
Date: Wed, 3 Feb 2021 20:10:13 -0800 [thread overview]
Message-ID: <CAEf4BzYxfO72ozDtjjXynewfQv_ZLvVEFWrEHwro7J1uwMy-Kw@mail.gmail.com> (raw)
In-Reply-To: <20210201172530.1141087-2-gprocida@google.com>
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
>
next prev parent reply other threads:[~2021-02-04 4:11 UTC|newest]
Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <87a83353155506cc02141e6e4108d89aa4e7d284>
2021-02-01 17:25 ` [PATCH dwarves v2 0/4] BTF ELF writing changes Giuliano Procida
2021-02-01 17:25 ` [PATCH dwarves v2 1/4] btf_encoder: Add .BTF section using libelf Giuliano Procida
2021-02-04 4:10 ` Andrii Nakryiko [this message]
2021-02-04 18:29 ` Giuliano Procida
2021-02-01 17:25 ` [PATCH dwarves v2 2/4] btf_encoder: Manually lay out updated ELF sections Giuliano Procida
2021-02-04 4:13 ` Andrii Nakryiko
2021-02-04 18:34 ` Giuliano Procida
2021-02-04 23:06 ` Andrii Nakryiko
2021-02-01 17:25 ` [PATCH dwarves v2 3/4] btf_encoder: Add .BTF as a loadable segment Giuliano Procida
2021-02-02 10:54 ` Giuliano Procida
2021-02-01 17:25 ` [PATCH dwarves v2 4/4] btf_encoder: Align .BTF section/segment to 8 bytes Giuliano Procida
2021-02-04 4:10 ` Andrii Nakryiko
2021-02-04 15:11 ` Giuliano Procida
2021-02-04 15:11 ` [PATCH] btf_encoder: Align .BTF section " Giuliano Procida
2021-02-05 13:42 ` [PATCH dwarves v3 0/5] ELF writing changes Giuliano Procida
2021-02-05 13:42 ` [PATCH dwarves v3 1/5] btf_encoder: Funnel ELF error reporting through a macro Giuliano Procida
2021-02-08 22:20 ` Andrii Nakryiko
2021-02-05 13:42 ` [PATCH dwarves v3 2/5] btf_encoder: Do not use both structs and pointers for the same data Giuliano Procida
2021-02-08 22:23 ` Andrii Nakryiko
2021-02-09 14:52 ` Giuliano Procida
2021-02-05 13:42 ` [PATCH dwarves v3 3/5] btf_encoder: Traverse sections using a for-loop Giuliano Procida
2021-02-08 22:24 ` Andrii Nakryiko
2021-02-09 14:59 ` Giuliano Procida
2021-02-05 13:42 ` [PATCH dwarves v3 4/5] btf_encoder: Add .BTF section using libelf Giuliano Procida
2021-02-08 22:29 ` Andrii Nakryiko
2021-02-09 15:04 ` Giuliano Procida
2021-02-05 13:42 ` [PATCH dwarves v3 5/5] btf_encoder: Align .BTF section to 8 bytes Giuliano Procida
2021-02-08 22:29 ` Andrii Nakryiko
2021-02-09 15:05 ` Giuliano Procida
2021-02-17 11:07 ` [PATCH dwarves v4 0/5] ELF writing changes Giuliano Procida
2021-02-17 11:08 ` [PATCH dwarves v4 1/5] btf_encoder: Funnel ELF error reporting through a macro Giuliano Procida
2021-02-17 11:08 ` [PATCH dwarves v4 2/5] btf_encoder: Do not use both structs and pointers for the same data Giuliano Procida
2021-02-17 11:08 ` [PATCH dwarves v4 3/5] btf_encoder: Traverse sections using a for-loop Giuliano Procida
2021-02-17 11:08 ` [PATCH dwarves v4 4/5] btf_encoder: Add .BTF section using libelf Giuliano Procida
2021-02-17 11:08 ` [PATCH dwarves v4 5/5] btf_encoder: Align .BTF section to 8 bytes Giuliano Procida
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAEf4BzYxfO72ozDtjjXynewfQv_ZLvVEFWrEHwro7J1uwMy-Kw@mail.gmail.com \
--to=andrii.nakryiko@gmail.com \
--cc=acme@kernel.org \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=dwarves@vger.kernel.org \
--cc=gprocida@google.com \
--cc=kernel-team@android.com \
--cc=kernel-team@fb.com \
--cc=maennich@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).