Re: [PATCH] perf tools: Resolve symbols against debug file first

Hi both of Jiri,

On Wed, Jan 13, 2021 at 8:43 PM Jiri Slaby [off-list ref] wrote:

On 13. 01. 21, 11:46, Jiri Olsa wrote:

quoted

On Wed, Jan 13, 2021 at 09:01:28AM +0100, Jiri Slaby wrote:

quoted

With LTO, there are symbols like these:
/usr/lib/debug/usr/lib64/libantlr4-runtime.so.4.8-4.8-1.4.x86_64.debug
  10305: 0000000000955fa4     0 NOTYPE  LOCAL  DEFAULT   29 Predicate.cpp.2bc410e7

This comes from a runtime/debug split done by the standard way:
objcopy --only-keep-debug $runtime $debug
objcopy --add-gnu-debuglink=$debugfn -R .comment -R .GCC.command.line --strip-all $runtime

perf currently cannot resolve such symbols (relicts of LTO), as section
29 exists only in the debug file (29 is .debug_info). And perf resolves
symbols only against runtime file. This results in all symbols from such
a library being unresolved:
      0.38%  main2    libantlr4-runtime.so.4.8  [.] 0x00000000000671e0

So try resolving against the debug file first. And only if it fails (the
section has NOBITS set), try runtime file. We can do this, as "objcopy
--only-keep-debug" per documentation preserves all sections, but clears
data of some of them (the runtime ones) and marks them as NOBITS.

The correct result is now:
      0.38%  main2    libantlr4-runtime.so.4.8  [.] antlr4::IntStream::~IntStream

Note that these LTO symbols are properly skipped anyway as they belong
neither to *text* nor to *data* (is_label && !elf_sec__filter(&shdr,
secstrs) is true).

Signed-off-by: Jiri Slaby <redacted>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Jiri Olsa <redacted>
Cc: Namhyung Kim <namhyung@kernel.org>
---
  tools/perf/util/symbol-elf.c | 10 +++++++++-
  1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
index f3577f7d72fe..a31b716fa61c 100644
--- a/tools/perf/util/symbol-elf.c
+++ b/tools/perf/util/symbol-elf.c

@@ -1226,12 +1226,20 @@ int dso__load_sym(struct dso *dso, struct map *map, struct symsrc *syms_ss,
             if (sym.st_shndx == SHN_ABS)
                     continue;

-            sec = elf_getscn(runtime_ss->elf, sym.st_shndx);
+            sec = elf_getscn(syms_ss->elf, sym.st_shndx);
             if (!sec)
                     goto out_elf_end;

we iterate symbols from syms_ss, so the fix seems to be correct
to call elf_getscn on syms_ss, not on runtime_ss as we do now

I'd think this worked only when runtime_ss == syms_ss

No, because the headers are copied 1:1 from runtime_ss to syms_ss. And
runtime_ss is then stripped, so only .debug* sections are removed there.
(And syms_ss's are set as NOBITS.)

We iterated .debug* sections in syms_ss and used runtime_ss section
_headers_ only to adjust symbols (sometimes). That worked.

It seems PPC has an opd section only in the runtime_ss and that's why
we use it for section headers.

quoted

quoted

             gelf_getshdr(sec, &shdr);

+            if (shdr.sh_type == SHT_NOBITS) {
+                    sec = elf_getscn(runtime_ss->elf, sym.st_shndx);
+                    if (!sec)
+                            goto out_elf_end;
+
+                    gelf_getshdr(sec, &shdr);
+            }

is that fallback necessary? the symbol is from syms_ss

Provided the above, we don't need the section data here, only headers,
so the NOBITS test is superfluous and the fallback shouldn't be needed.
Let me test it.

We need to talk to PPC folks like I said.  Or maybe we can change the
default ss depending on the arch.

Thanks,
Namhyung