Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220710012204.2390293-1-leo.yan@linaro.org>
In-Reply-To: <20220710012204.2390293-1-leo.yan@linaro.org>
From:   Ian Rogers <irogers@google.com>
Date:   Mon, 11 Jul 2022 09:09:37 -0700
Message-ID: <CAP-5=fX_fT7e9tqDBKXh_1CQ8w80iOXCGz2kJT_nHpY6wYWqmQ@mail.gmail.com>
Subject: Re: [RFC PATCH v1] perf symbol: Correct address for bss symbols
To:     Leo Yan <leo.yan@linaro.org>, Fangrui Song <maskray@google.com>
Cc:     Arnaldo Carvalho de Melo <acme@kernel.org>,
        Peter Zijlstra <peterz@infradead.org>,
        Namhyung Kim <namhyung@kernel.org>,
        Ingo Molnar <mingo@redhat.com>,
        Mark Rutland <mark.rutland@arm.com>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Jiri Olsa <jolsa@kernel.org>, linux-perf-users@vger.kernel.org,
        linux-kernel@vger.kernel.org, Chang Rui <changruinj@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Sat, Jul 9, 2022 at 6:22 PM Leo Yan <leo.yan@linaro.org> wrote:
>
> When using 'perf mem' and 'perf c2c', an issue is observed that tool
> reports the wrong offset for global data symbols.  This is a common
> issue on both x86 and Arm64 platforms.
>
> Let's see an example, for a test program, below is the disassembly for
> its .bss section which is dumped with objdump:
>
>   ...
>
>   Disassembly of section .data:
>
>   0000000000004000 <__data_start>:
>         ...
>
>   0000000000004008 <__dso_handle>:
>       4008:     08 40 00                or     %al,0x0(%rax)
>       400b:     00 00                   add    %al,(%rax)
>       400d:     00 00                   add    %al,(%rax)
>         ...
>
>   0000000000004010 <wait_to_begin>:
>       4010:     01 00                   add    %eax,(%rax)
>       4012:     00 00                   add    %al,(%rax)
>       4014:     00 00                   add    %al,(%rax)
>         ...
>
>   0000000000004018 <lock_thd_name>:
>       4018:     08 20                   or     %ah,(%rax)
>       401a:     00 00                   add    %al,(%rax)
>       401c:     00 00                   add    %al,(%rax)
>         ...
>
>   0000000000004020 <reader_thd_name>:
>       4020:     10 20                   adc    %ah,(%rax)
>       4022:     00 00                   add    %al,(%rax)
>       4024:     00 00                   add    %al,(%rax)
>         ...
>
>   Disassembly of section .bss:
>
>   0000000000004040 <completed.0>:
>         ...
>
>   0000000000004080 <buf1>:
>         ...
>
>   00000000000040c0 <buf2>:
>         ...
>
>   0000000000004100 <thread>:
>         ...
>
> First we used 'perf mem record' to run the test program and then used
> 'perf --debug verbose=4 mem report' to observe what's the symbol info
> for 'buf1' and 'buf2' structures.
>
>   # ./perf mem record -e ldlat-loads,ldlat-stores -- false_sharing.exe 8
>   # ./perf --debug verbose=4 mem report
>     ...
>     dso__load_sym_internal: adjusting symbol: st_value: 0x40c0 sh_addr: 0x4040 sh_offset: 0x3028
>     symbol__new: buf2 0x30a8-0x30e8
>     ...
>     dso__load_sym_internal: adjusting symbol: st_value: 0x4080 sh_addr: 0x4040 sh_offset: 0x3028
>     symbol__new: buf1 0x3068-0x30a8
>     ...
>
> Perf tool relies on libelf to parse symbols, here 'st_value' is the
> address from executable file, 'sh_addr' is the belonged section's linked
> start address, and 'sh_offset' is the dynamic loaded address for this
> section, then perf tool uses below formula to adjust symbol address:
>
>   adjusted_address = st_value - sh_addr + sh_offset
>
> So we can see the final adjusted address ranges for buf1 and buf2 are
> [0x30a8-0x30e8) and [0x3068-0x30a8) respectively, apparently this is
> incorrect, in the code, the structure for 'buf1' and 'buf2' specifies
> compiler attribute with 64-byte alignment.
>
> The problem happens for 'sh_offset', libelf returns it as 0x3028 which
> is not 64-byte aligned, on the other hand, we can see both 'st_value'
> and 'sh_addr' are 64-byte aligned.  Combining with disassembly, it's
> likely libelf uses the .data section end address as .bss section
> start address, therefore, it doesn't respect the alignment attribute for
> structures in .bss section.
>
> Since .data and .bss sections are in the continuous virtual address
> space, and .data section info returned by libelf is reliable, to fix
> this issue, if detects it's a bss symbol, it rolls back to use .data
> section info to adjust symbol's virtual address.
>
> Essentially, we need to fix libelf to return correct offsets for
> sections, on the other hand, we live commonly with existed versions of
> libelf.  So we also need this change in perf tool.
>
> Fixes: f17e04afaff8 ("perf report: Fix ELF symbol parsing")
> Reported-by: Chang Rui <changruinj@gmail.com>
> Signed-off-by: Leo Yan <leo.yan@linaro.org>

This looks good to me, I'm happy to add my:
Acked-by: Ian Rogers <irogers@google.com>
I've added Fangrui Song who is more knowledge-able on ELF, libelf,
etc. than me and may have additional thoughts.

Thanks,
Ian

> ---
>  tools/perf/util/dso.h        |  1 +
>  tools/perf/util/symbol-elf.c | 26 ++++++++++++++++++++++++++
>  2 files changed, 27 insertions(+)
>
> diff --git a/tools/perf/util/dso.h b/tools/perf/util/dso.h
> index 3a9fd4d389b5..00f57f4ac6bc 100644
> --- a/tools/perf/util/dso.h
> +++ b/tools/perf/util/dso.h
> @@ -180,6 +180,7 @@ struct dso {
>         u8               rel;
>         struct build_id  bid;
>         u64              text_offset;
> +       int              data_sec_index;
>         const char       *short_name;
>         const char       *long_name;
>         u16              long_name_len;
> diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
> index ecd377938eea..ed65dd26d58e 100644
> --- a/tools/perf/util/symbol-elf.c
> +++ b/tools/perf/util/symbol-elf.c
> @@ -1095,6 +1095,7 @@ dso__load_sym_internal(struct dso *dso, struct map *map, struct symsrc *syms_ss,
>         Elf *elf;
>         int nr = 0;
>         bool remap_kernel = false, adjust_kernel_syms = false;
> +       size_t sec_index;
>
>         if (kmap && !kmaps)
>                 return -1;
> @@ -1113,6 +1114,10 @@ dso__load_sym_internal(struct dso *dso, struct map *map, struct symsrc *syms_ss,
>                                 ".text", NULL))
>                 dso->text_offset = tshdr.sh_addr - tshdr.sh_offset;
>
> +       if (elf_section_by_name(runtime_ss->elf, &runtime_ss->ehdr, &tshdr,
> +                               ".data", &sec_index))
> +               dso->data_sec_index = sec_index;
> +
>         if (runtime_ss->opdsec)
>                 opddata = elf_rawdata(runtime_ss->opdsec, NULL);
>
> @@ -1227,6 +1232,27 @@ dso__load_sym_internal(struct dso *dso, struct map *map, struct symsrc *syms_ss,
>
>                 gelf_getshdr(sec, &shdr);
>
> +               /*
> +                * When the first data structure in .bss section is attributed
> +                * with alignment (e.g. 64-byte aligned), libelf doesn't reflect
> +                * the alignment in the 'shdr.sh_offset' field, at the end the
> +                * field is filled with the end loading address of a prior
> +                * section rather than the aligned address of .bss section.
> +                * This leads to mess for later parsing .bss symbols.
> +                *
> +                * Since .data and .bss sections are in the continuous virtual
> +                * address space, and .data section's info is reliable.  So if
> +                * detects it's a bss symbol, we retrieve .data section info
> +                * for adjusting address.
> +                */
> +               if (!strcmp(elf_sec__name(&shdr, secstrs_sym), ".bss")) {
> +                       sec = elf_getscn(syms_ss->elf, dso->data_sec_index);
> +                       if (!sec)
> +                               goto out_elf_end;
> +
> +                       gelf_getshdr(sec, &shdr);
> +               }
> +
>                 secstrs = secstrs_sym;
>
>                 /*
> --
> 2.25.1
>