[PATCH v2 0/2] perf symbol: Minor fixing

[PATCH v2 0/2] perf symbol: Minor fixing

[Leo Yan]

This patch set contains two minor fixing for parsing symbols.

The first patch changes to use program header for parsing symbols of
user space executable and shared objects.  Since kernel's symbol parsing
is more complex than userspace for support both kernel symbols and module
symbols, this is why this patch set uses conservative way and doesn't
change kernel symbols parsing.

The second patch is to detect symbols from '.gnu.warning.*' sections,
these symbols are used for linker warning, skip to record them to
avoid spurious symbols.

Changes from v1:
- Changed to use program header / PT_LOAD segments to parse symbols for
  userspace executable and shared object files (Fangrui).


Leo Yan (2):
  perf symbol: Correct address for bss symbols
  perf symbol: Skip recording symbols in '.gnu.warning.*' sections

 tools/perf/util/symbol-elf.c | 53 +++++++++++++++++++++++++++++++++---
 1 file changed, 49 insertions(+), 4 deletions(-)

-- 
2.25.1

[PATCH v2 1/2] 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 .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, in executable and shared
object files, 'st_value' holds a virtual address; 'sh_addr' is the
address at which section's first byte should reside in memory, and
'sh_offset' is the byte offset from the beginning of the file to the
first byte in the section.  Perf tool uses below formula to convert
symbol's memory address to file address:

  file_address = st_value - sh_addr + sh_offset
                    ^
                    ` Memory address

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, combining with disassembly, it's likely libelf
doesn't respect the alignment for .bss section, therefore, it doesn't
return the aligned value for 'sh_offset'.

Suggested by Fangrui Song, elf file contains program header which
contains PT_LOAD segments, the fields p_vaddr and p_offset in PT_LOAD
segments contain the execution info.  A better choice for converting
memory address to file address is using the formula:

  file_address = st_value - p_vaddr + p_offset

This patch introduces function elf_read_program_header() which returns
out the program header based on the passed 'st_value', then it uses
above formula to calculate the symbol file address; and the debugging
log is updated respectively.

After applying the change:

  # ./perf --debug verbose=4 mem report
    ...
    dso__load_sym_internal: adjusting symbol: st_value: 0x40c0 p_vaddr: 0x3d28 p_offset: 0x2d28
    symbol__new: buf2 0x30c0-0x3100
    ...
    dso__load_sym_internal: adjusting symbol: st_value: 0x4080 p_vaddr: 0x3d28 p_offset: 0x2d28
    symbol__new: buf1 0x3080-0x30c0
    ...

Fixes: f17e04afaff8 ("perf report: Fix ELF symbol parsing")
Reported-by: Chang Rui <changruinj@gmail.com>
Suggested-by: Fangrui Song <maskray@google.com>
Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 tools/perf/util/symbol-elf.c | 45 ++++++++++++++++++++++++++++++++----
 1 file changed, 41 insertions(+), 4 deletions(-)

diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
index ecd377938eea..ef6ced5c5746 100644
--- a/tools/perf/util/symbol-elf.c
+++ b/tools/perf/util/symbol-elf.c
@@ -233,6 +233,33 @@ Elf_Scn *elf_section_by_name(Elf *elf, GElf_Ehdr *ep,
 	return NULL;
 }
 
+static int elf_read_program_header(Elf *elf, u64 vaddr, GElf_Phdr *phdr)
+{
+	size_t i, phdrnum;
+	u64 sz;
+
+	if (elf_getphdrnum(elf, &phdrnum))
+		return -1;
+
+	for (i = 0; i < phdrnum; i++) {
+		if (gelf_getphdr(elf, i, phdr) == NULL)
+			return -1;
+
+		if (phdr->p_type != PT_LOAD)
+			continue;
+
+		sz = max(phdr->p_memsz, phdr->p_filesz);
+		if (!sz)
+			continue;
+
+		if (vaddr >= phdr->p_vaddr && (vaddr < phdr->p_vaddr + sz))
+			return 0;
+	}
+
+	/* Not found any valid program header */
+	return -1;
+}
+
 static bool want_demangle(bool is_kernel_sym)
 {
 	return is_kernel_sym ? symbol_conf.demangle_kernel : symbol_conf.demangle;
@@ -1209,6 +1236,7 @@ dso__load_sym_internal(struct dso *dso, struct map *map, struct symsrc *syms_ss,
 					sym.st_value);
 			used_opd = true;
 		}
+
 		/*
 		 * When loading symbols in a data mapping, ABS symbols (which
 		 * has a value of SHN_ABS in its st_shndx) failed at
@@ -1262,11 +1290,20 @@ dso__load_sym_internal(struct dso *dso, struct map *map, struct symsrc *syms_ss,
 				goto out_elf_end;
 		} else if ((used_opd && runtime_ss->adjust_symbols) ||
 			   (!used_opd && syms_ss->adjust_symbols)) {
+			GElf_Phdr phdr;
+
+			if (elf_read_program_header(syms_ss->elf,
+						    (u64)sym.st_value, &phdr)) {
+				pr_warning("%s: failed to find program header for "
+					   "symbol: %s st_value: %#" PRIx64 "\n",
+					   __func__, elf_name, (u64)sym.st_value);
+				continue;
+			}
 			pr_debug4("%s: adjusting symbol: st_value: %#" PRIx64 " "
-				  "sh_addr: %#" PRIx64 " sh_offset: %#" PRIx64 "\n", __func__,
-				  (u64)sym.st_value, (u64)shdr.sh_addr,
-				  (u64)shdr.sh_offset);
-			sym.st_value -= shdr.sh_addr - shdr.sh_offset;
+				  "p_vaddr: %#" PRIx64 " p_offset: %#" PRIx64 "\n",
+				  __func__, (u64)sym.st_value, (u64)phdr.p_vaddr,
+				  (u64)phdr.p_offset);
+			sym.st_value -= phdr.p_vaddr - phdr.p_offset;
 		}
 
 		demangled = demangle_sym(dso, kmodule, elf_name);
-- 
2.25.1

[PATCH v2 2/2] perf symbol: Skip recording symbols in '.gnu.warning.*' sections

[Leo Yan]

Some symbols are observed their 'st_value' field are zeros.  E.g.
libc.so.6 in Ubuntu contains a symbol '__evoke_link_warning_getwd' which
resides in the '.gnu.warning.getwd' section, unlike normal symbols, this
kind of symbols are only used for linker warning.

This patch skips to record symbols from '.gnu.warning.*' sections by
detecting the sub string '.gnu.warning' is contained in section name.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 tools/perf/util/symbol-elf.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
index ef6ced5c5746..4b621e355c0e 100644
--- a/tools/perf/util/symbol-elf.c
+++ b/tools/perf/util/symbol-elf.c
@@ -1277,6 +1277,14 @@ dso__load_sym_internal(struct dso *dso, struct map *map, struct symsrc *syms_ss,
 
 		section_name = elf_sec__name(&shdr, secstrs);
 
+		/*
+		 * A symbol coming from ".gnu.warning.*" sections is used to
+		 * generate linker warnings, its 'sym.st_value' field usually
+		 * is zero, skip to record it.
+		 */
+		if (strstr(section_name, ".gnu.warning"))
+			continue;
+
 		/* On ARM, symbols for thumb functions have 1 added to
 		 * the symbol address as a flag - remove it */
 		if ((ehdr.e_machine == EM_ARM) &&
-- 
2.25.1

Re: [PATCH v2 2/2] perf symbol: Skip recording symbols in '.gnu.warning.*' sections

[Fangrui Song]

On Sat, Jul 23, 2022 at 7:29 PM Leo Yan <leo.yan@linaro.org> wrote:
>
> Some symbols are observed their 'st_value' field are zeros.  E.g.
> libc.so.6 in Ubuntu contains a symbol '__evoke_link_warning_getwd' which
> resides in the '.gnu.warning.getwd' section, unlike normal symbols, this
> kind of symbols are only used for linker warning.
>
> This patch skips to record symbols from '.gnu.warning.*' sections by
> detecting the sub string '.gnu.warning' is contained in section name.
>
> Signed-off-by: Leo Yan <leo.yan@linaro.org>

Presumably __evoke_link_warning_getwd is due to `clang -fuse-ld=lld
-static ...` on a file calling the deprecated getwd.
GNU ld and gold implement a .gnu.warning.* feature which removes the
section. ld.lld just ignores this section as the usefulness of the
functionality is unclear.

The section .gnu.warning.getwd does not have the SHF_ALLOC flag. Such
sections are not part of memory images and I think it is more generic
ignoring all symbols residing in a non-SHF_ALLOC section.

> ---
>  tools/perf/util/symbol-elf.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
> index ef6ced5c5746..4b621e355c0e 100644
> --- a/tools/perf/util/symbol-elf.c
> +++ b/tools/perf/util/symbol-elf.c
> @@ -1277,6 +1277,14 @@ dso__load_sym_internal(struct dso *dso, struct map *map, struct symsrc *syms_ss,
>
>                 section_name = elf_sec__name(&shdr, secstrs);
>
> +               /*
> +                * A symbol coming from ".gnu.warning.*" sections is used to
> +                * generate linker warnings, its 'sym.st_value' field usually
> +                * is zero, skip to record it.
> +                */
> +               if (strstr(section_name, ".gnu.warning"))
> +                       continue;
> +
>                 /* On ARM, symbols for thumb functions have 1 added to
>                  * the symbol address as a flag - remove it */
>                 if ((ehdr.e_machine == EM_ARM) &&
> --
> 2.25.1
>


-- 
宋方睿

Re: [PATCH v2 2/2] perf symbol: Skip recording symbols in '.gnu.warning.*' sections

[Leo Yan]

On Sat, Jul 23, 2022 at 08:42:20PM -0700, Fangrui Song wrote:
> On Sat, Jul 23, 2022 at 7:29 PM Leo Yan <leo.yan@linaro.org> wrote:
> >
> > Some symbols are observed their 'st_value' field are zeros.  E.g.
> > libc.so.6 in Ubuntu contains a symbol '__evoke_link_warning_getwd' which
> > resides in the '.gnu.warning.getwd' section, unlike normal symbols, this
> > kind of symbols are only used for linker warning.
> >
> > This patch skips to record symbols from '.gnu.warning.*' sections by
> > detecting the sub string '.gnu.warning' is contained in section name.
> >
> > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> 
> Presumably __evoke_link_warning_getwd is due to `clang -fuse-ld=lld
> -static ...` on a file calling the deprecated getwd.
> GNU ld and gold implement a .gnu.warning.* feature which removes the
> section. ld.lld just ignores this section as the usefulness of the
> functionality is unclear.
> 
> The section .gnu.warning.getwd does not have the SHF_ALLOC flag. Such
> sections are not part of memory images and I think it is more generic
> ignoring all symbols residing in a non-SHF_ALLOC section.

Good point!  Will refine the patch for this and send out soon.

Thanks a lot, Fangrui.