[RFC PATCH v1] perf symbol: Correct address for bss symbols

[RFC PATCH v1] perf symbol: Correct address for bss symbols

[Leo Yan]

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

Re: [RFC PATCH v1] perf symbol: Correct address for bss symbols

[Ian Rogers]

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
>

Re: [RFC PATCH v1] perf symbol: Correct address for bss symbols

[Fangrui Song]

On 2022-07-11, Ian Rogers wrote:
>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
>>     ...

It seems unclear how 0x30a8 and 0x3068 are derived,

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

so I cannot judge this paragraph.

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

This is not necessarily true.

* In GNU ld's internal linker script, .data1 sits between .data and .bss.
* A linker script can add other sections between .data and .bss
* A linker script may place .data and .bss in two PT_LOAD program headers.

% readelf -WS aa
There are 13 section headers, starting at offset 0x10a8:

With a linker script like

% cat a/a.lds
SECTIONS {
   .text : { *(.text) }
   data1 : { *(data1) }
   data2 : { *(data2) }
   .bss : { *(.bss) }
}

I can get something like

Section Headers:
   [Nr] Name              Type            Address          Off    Size   ES Flg Lk Inf Al
   [ 0]                   NULL            0000000000000000 000000 000000 00      0   0  0
   [ 1] .text             PROGBITS        0000000000000000 001000 000001 00  AX  0   0  1
   [ 2] data1             PROGBITS        0000000000000001 001001 000001 00  WA  0   0  1
   [ 3] data2             PROGBITS        0000000000000002 001002 000001 00  WA  0   0  1
   [ 4] .data             PROGBITS        0000000000000003 001003 000000 00  WA  0   0  1
   [ 5] data3             PROGBITS        0000000000000003 001003 000001 00  WA  0   0  1
   [ 6] .bss              NOBITS          0000000000000020 001004 000001 00  WA  0   0 32

.bss's sh_offset does not need to be aligned per http://www.sco.com/developers/gabi/latest/ch4.sheader.html

     sh_offset
     This member's value gives the byte offset from the beginning of the file to the first byte in the section. One section type, SHT_NOBITS described below, occupies no space in the file, and its sh_offset member locates the conceptual placement in the file.

I don't have more context why the file offset is needed for a variable in the all-zero section.
If the file offset has to be used and we want to use a heuristic, a better one is to find the section index of .bss, say, i.

const uint64_t align = shdr[i].sh_addralign;
assert(i > 0);
if (shdr[i].offset % align == 0)
   return shdr[i].offset;
return (shdr[i-1].sh_offset + shdr[i-1].sh_size + align - 1) & -align;

Really, it is better to use the program header to derive the virtual address of a variable residing in .bss.

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

Re: [RFC PATCH v1] perf symbol: Correct address for bss symbols

[Leo Yan]

On Mon, Jul 11, 2022 at 10:27:06AM -0700, Fangrui Song wrote:

Thanks for review, Ian and Fangrui!

[...]

> > > 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
> > >     ...
> 
> It seems unclear how 0x30a8 and 0x3068 are derived,

In perf tool, 0x30a8 and 0x3068 are adjusted address, which are derived
from 'st_value', 'sh_addr' and 'sh_offset' with the formula:

  adjusted_address = st_value - sh_addr + sh_offset

So perf computes symbol address for buf1:

  adjusted_address = st_value - sh_addr + sh_offset
                   = 0x4080 - 0x4040 + 0x3028
                   = 0x3068

> > > 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.
> 
> so I cannot judge this paragraph.
> 
> > > 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.
> 
> This is not necessarily true.
> 
> * In GNU ld's internal linker script, .data1 sits between .data and .bss.
> * A linker script can add other sections between .data and .bss
> * A linker script may place .data and .bss in two PT_LOAD program headers.

Agreed the approach in this patch cannot handle all cases.

> % readelf -WS aa
> There are 13 section headers, starting at offset 0x10a8:
> 
> With a linker script like
> 
> % cat a/a.lds
> SECTIONS {
>   .text : { *(.text) }
>   data1 : { *(data1) }
>   data2 : { *(data2) }
>   .bss : { *(.bss) }
> }
> 
> I can get something like
> 
> Section Headers:
>   [Nr] Name              Type            Address          Off    Size   ES Flg Lk Inf Al
>   [ 0]                   NULL            0000000000000000 000000 000000 00      0   0  0
>   [ 1] .text             PROGBITS        0000000000000000 001000 000001 00  AX  0   0  1
>   [ 2] data1             PROGBITS        0000000000000001 001001 000001 00  WA  0   0  1
>   [ 3] data2             PROGBITS        0000000000000002 001002 000001 00  WA  0   0  1
>   [ 4] .data             PROGBITS        0000000000000003 001003 000000 00  WA  0   0  1
>   [ 5] data3             PROGBITS        0000000000000003 001003 000001 00  WA  0   0  1
>   [ 6] .bss              NOBITS          0000000000000020 001004 000001 00  WA  0   0 32
> 
> .bss's sh_offset does not need to be aligned per http://www.sco.com/developers/gabi/latest/ch4.sheader.html
> 
>     sh_offset
>     This member's value gives the byte offset from the beginning of the file to the first byte in the section. One section type, SHT_NOBITS described below, occupies no space in the file, and its sh_offset member locates the conceptual placement in the file.

> I don't have more context why the file offset is needed for a variable in the all-zero section.

We need to create symbol info for not only .text section but also for
.data section and .bss sectionṡ.  So based on the data address, we can
know what's the symbol for the data access.

But I need to correct the description for "st_value" [1]: In
executable and shared object files, st_value holds a virtual address.
To make these files' symbols more useful for the dynamic linker, the
section offset (file interpretation) gives way to a virtual address
(memory interpretation) for which the section number is irrelevant.

So perf tool uses the formula "st_value - sh_addr + sh_offset" to
convert from the memory address to file address.  But it calculates
the wrong file address because "sh_offset" doesn't respect the
alignment.

[1] http://www.sco.com/developers/gabi/latest/ch4.symtab.html#symbol_value

> If the file offset has to be used and we want to use a heuristic, a better one is to find the section index of .bss, say, i.
> 
> const uint64_t align = shdr[i].sh_addralign;
> assert(i > 0);
> if (shdr[i].offset % align == 0)
>   return shdr[i].offset;
> return (shdr[i-1].sh_offset + shdr[i-1].sh_size + align - 1) & -align;
> 
> Really, it is better to use the program header to derive the virtual address of a variable residing in .bss.

I think we can simplify the code.  Because:

  shdr[i].sh_offset = shdr[i-1].sh_offset + shdr[i-1].sh_size

... thus we can simply fixup "sh_offset":

  const uint64_t align = shdr[i].sh_addralign;
  aligned_sh_offset = (shdr[i].sh_offset + align - 1) & ~(align - 1);

So:

  symbol_addr = st_value - sh_addr + aligned_sh_offset

If you still see any issue, please let me know.

Thanks a lot for suggestions.

Leo

Re: [RFC PATCH v1] perf symbol: Correct address for bss symbols

[Fangrui Song]

On Mon, Jul 11, 2022 at 9:05 PM Leo Yan <leo.yan@linaro.org> wrote:
>
> On Mon, Jul 11, 2022 at 10:27:06AM -0700, Fangrui Song wrote:
>
> Thanks for review, Ian and Fangrui!

You are welcome:)

> [...]
>
> > > > 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
> > > >     ...
> >
> > It seems unclear how 0x30a8 and 0x3068 are derived,
>
> In perf tool, 0x30a8 and 0x3068 are adjusted address, which are derived
> from 'st_value', 'sh_addr' and 'sh_offset' with the formula:
>
>   adjusted_address = st_value - sh_addr + sh_offset
>
> So perf computes symbol address for buf1:
>
>   adjusted_address = st_value - sh_addr + sh_offset
>                    = 0x4080 - 0x4040 + 0x3028
>                    = 0x3068

Thanks.

> > > > 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.
> >
> > so I cannot judge this paragraph.
> >
> > > > 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.
> >
> > This is not necessarily true.
> >
> > * In GNU ld's internal linker script, .data1 sits between .data and .bss.
> > * A linker script can add other sections between .data and .bss
> > * A linker script may place .data and .bss in two PT_LOAD program headers.
>
> Agreed the approach in this patch cannot handle all cases.
>
> > % readelf -WS aa
> > There are 13 section headers, starting at offset 0x10a8:
> >
> > With a linker script like
> >
> > % cat a/a.lds
> > SECTIONS {
> >   .text : { *(.text) }
> >   data1 : { *(data1) }
> >   data2 : { *(data2) }
> >   .bss : { *(.bss) }
> > }
> >
> > I can get something like
> >
> > Section Headers:
> >   [Nr] Name              Type            Address          Off    Size   ES Flg Lk Inf Al
> >   [ 0]                   NULL            0000000000000000 000000 000000 00      0   0  0
> >   [ 1] .text             PROGBITS        0000000000000000 001000 000001 00  AX  0   0  1
> >   [ 2] data1             PROGBITS        0000000000000001 001001 000001 00  WA  0   0  1
> >   [ 3] data2             PROGBITS        0000000000000002 001002 000001 00  WA  0   0  1
> >   [ 4] .data             PROGBITS        0000000000000003 001003 000000 00  WA  0   0  1
> >   [ 5] data3             PROGBITS        0000000000000003 001003 000001 00  WA  0   0  1
> >   [ 6] .bss              NOBITS          0000000000000020 001004 000001 00  WA  0   0 32
> >
> > .bss's sh_offset does not need to be aligned per http://www.sco.com/developers/gabi/latest/ch4.sheader.html
> >
> >     sh_offset
> >     This member's value gives the byte offset from the beginning of the file to the first byte in the section. One section type, SHT_NOBITS described below, occupies no space in the file, and its sh_offset member locates the conceptual placement in the file.
>
> > I don't have more context why the file offset is needed for a variable in the all-zero section.
>
> We need to create symbol info for not only .text section but also for
> .data section and .bss sectionṡ.  So based on the data address, we can
> know what's the symbol for the data access.
>
> But I need to correct the description for "st_value" [1]: In
> executable and shared object files, st_value holds a virtual address.
> To make these files' symbols more useful for the dynamic linker, the
> section offset (file interpretation) gives way to a virtual address
> (memory interpretation) for which the section number is irrelevant.
>
> So perf tool uses the formula "st_value - sh_addr + sh_offset" to
> convert from the memory address to file address.  But it calculates
> the wrong file address because "sh_offset" doesn't respect the
> alignment.

Thanks for the explanation. I think st_value - p_vaddr + p_offset  may
be a better formula where p_vaddr/p_offset is from the PT_LOAD program
header.

For a SHT_NOBITS section, sh_offset may not be accurate, but PT_LOAD
has precise information.

> [1] http://www.sco.com/developers/gabi/latest/ch4.symtab.html#symbol_value
>
> > If the file offset has to be used and we want to use a heuristic, a better one is to find the section index of .bss, say, i.
> >
> > const uint64_t align = shdr[i].sh_addralign;
> > assert(i > 0);
> > if (shdr[i].offset % align == 0)
> >   return shdr[i].offset;
> > return (shdr[i-1].sh_offset + shdr[i-1].sh_size + align - 1) & -align;
> >
> > Really, it is better to use the program header to derive the virtual address of a variable residing in .bss.
>
> I think we can simplify the code.  Because:
>
>   shdr[i].sh_offset = shdr[i-1].sh_offset + shdr[i-1].sh_size
>
> ... thus we can simply fixup "sh_offset":
>
>   const uint64_t align = shdr[i].sh_addralign;
>   aligned_sh_offset = (shdr[i].sh_offset + align - 1) & ~(align - 1);
>
> So:
>
>   symbol_addr = st_value - sh_addr + aligned_sh_offset
>
> If you still see any issue, please let me know.
>
> Thanks a lot for suggestions.
>
> Leo

Re: [RFC PATCH v1] perf symbol: Correct address for bss symbols

[Leo Yan]

Hi Fangrui,

On Tue, Jul 12, 2022 at 08:29:52PM -0700, Fangrui Song wrote:

[...]

> > We need to create symbol info for not only .text section but also for
> > .data section and .bss sectionṡ.  So based on the data address, we can
> > know what's the symbol for the data access.
> >
> > But I need to correct the description for "st_value" [1]: In
> > executable and shared object files, st_value holds a virtual address.
> > To make these files' symbols more useful for the dynamic linker, the
> > section offset (file interpretation) gives way to a virtual address
> > (memory interpretation) for which the section number is irrelevant.
> >
> > So perf tool uses the formula "st_value - sh_addr + sh_offset" to
> > convert from the memory address to file address.  But it calculates
> > the wrong file address because "sh_offset" doesn't respect the
> > alignment.
> 
> Thanks for the explanation. I think st_value - p_vaddr + p_offset  may
> be a better formula where p_vaddr/p_offset is from the PT_LOAD program
> header.
> 
> For a SHT_NOBITS section, sh_offset may not be accurate, but PT_LOAD
> has precise information.

Thanks a lot for suggestion, it's very helpful and reasonable for me!

I struggled a bit for considering two things.  One is how to refactor
kernel symbol parsing with PT_LOAD program headers, because the kernel
symbol parsing is relative complex for both kernel symbols and module
symbols, this is why I didn't move furthermore for refactoring kernel
symbol parsing.

The second thing is I observe there have some spurious symbols with
'st_value' are zeros.  So there have an extra fixing for this case.

Welcome comments or suggestions for the new patch set:
https://lore.kernel.org/lkml/20220724022857.2621520-1-leo.yan@linaro.org/T/#t

Thanks,
Leo