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