Re: [PATCH] perf symbol: Fail to read phdr workaround

[Arnaldo Carvalho de Melo <acme@kernel.org>]

Em Sun, Jul 31, 2022 at 11:19:15PM -0700, Ian Rogers escreveu:
> On Sun, Jul 31, 2022, 6:53 PM Leo Yan <leo.yan@linaro.org> wrote:
> 
> > On Sun, Jul 31, 2022 at 09:49:23AM -0700, Ian Rogers wrote:
> > > The perf jvmti agent doesn't create program headers, in this case
> > > fallback on section headers as happened previously.
> > >
> > > Fixes: 882528d2e776 ("perf symbol: Skip symbols if SHF_ALLOC flag is not
> > set")
> >
> > It's good to change fix tag as:
> > Fixes: 2d86612aacb7 ("perf symbol: Correct address for bss symbols")
> >
> 
> Doh! I was rushing this morning. Thanks for catching and reviewing!

I made the adjustments and added a note with the repro, to help in the
future when trying to test this area.

I also think we could have something like a 'perf test' mode where, when
asked to, it would enable tests that involve downloading such files to
perform tests, such as this dacapo benchmark, and then would test if the
output matches expectations.

- Arnaldo

commit 6d518ac7be6223811ab947897273b1bbef846180
Author: Ian Rogers <irogers@google.com>
Date:   Sun Jul 31 09:49:23 2022 -0700

    perf symbol: Fail to read phdr workaround
    
    The perf jvmti agent doesn't create program headers, in this case
    fallback on section headers as happened previously.
    
    Committer notes:
    
    To test this, from a public post by Ian:
    
    1) download a Java workload dacapo-9.12-MR1-bach.jar from
    https://sourceforge.net/projects/dacapobench/
    
    2) build perf such as "make -C tools/perf O=/tmp/perf NO_LIBBFD=1" it
    should detect Java and create /tmp/perf/libperf-jvmti.so
    
    3) run perf with the jvmti agent:
    
      perf record -k 1 java -agentpath:/tmp/perf/libperf-jvmti.so -jar dacapo-9.12-MR1-bach.jar -n 10 fop
    
    4) run perf inject:
    
      perf inject -i perf.data -o perf-injected.data -j
    
    5) run perf report
    
      perf report -i perf-injected.data | grep org.apache.fop
    
    With this patch reverted I see lots of symbols like:
    
         0.00%  java             jitted-388040-4656.so  [.] org.apache.fop.fo.FObj.bind(org.apache.fop.fo.PropertyList)
    
    With the patch (2d86612aacb7805f ("perf symbol: Correct address for bss
    symbols")) I see lots of:
    
      dso__load_sym_internal: failed to find program header for symbol:
      Lorg/apache/fop/fo/FObj;bind(Lorg/apache/fop/fo/PropertyList;)V
      st_value: 0x40
    
    Fixes: 2d86612aacb7805f ("perf symbol: Correct address for bss symbols")
    Reviewed-by: Leo Yan <leo.yan@linaro.org>
    Signed-off-by: Ian Rogers <irogers@google.com>
    Tested-by: Leo Yan <leo.yan@linaro.org>
    Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
    Cc: Jiri Olsa <jolsa@kernel.org>
    Cc: Leo Yan <leo.yan@linaro.org>
    Cc: Mark Rutland <mark.rutland@arm.com>
    Cc: Namhyung Kim <namhyung@kernel.org>
    Cc: Peter Zijlstra <peterz@infradead.org>
    Cc: Stephane Eranian <eranian@google.com>
    Link: http://lore.kernel.org/lkml/20220731164923.691193-1-irogers@google.com
    Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>

diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
index b3be5b1d9dbb00bc..75bec32d4f571319 100644
--- a/tools/perf/util/symbol-elf.c
+++ b/tools/perf/util/symbol-elf.c
@@ -1305,16 +1305,29 @@ dso__load_sym_internal(struct dso *dso, struct map *map, struct symsrc *syms_ss,
 
 			if (elf_read_program_header(syms_ss->elf,
 						    (u64)sym.st_value, &phdr)) {
-				pr_warning("%s: failed to find program header for "
+				pr_debug4("%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);
+				/*
+				 * Fail to find program header, let's rollback
+				 * to use shdr.sh_addr and shdr.sh_offset to
+				 * calibrate symbol's file address, though this
+				 * is not necessary for normal C ELF file, we
+				 * still need to handle java JIT symbols in this
+				 * case.
+				 */
+				sym.st_value -= shdr.sh_addr - shdr.sh_offset;
+			} else {
+				pr_debug4("%s: adjusting symbol: st_value: %#" PRIx64 " "
+					"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;
 			}
-			pr_debug4("%s: adjusting symbol: st_value: %#" PRIx64 " "
-				  "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);