[PATCH] perf symbol: Fail to read phdr workaround

[PATCH] perf symbol: Fail to read phdr workaround

[Ian Rogers]

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")
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/symbol-elf.c | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
index b3be5b1d9dbb..75bec32d4f57 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);
-- 
2.37.1.455.g008518b4e5-goog

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

[Leo Yan]

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

I saw stable kernel maintainers have back ported patch "perf symbol:
Correct address for bss symbols" for stable kernel branches, the
suggested fix tag would allow this patch to be landed on stable kernels
as well.

I think I need to manually send the patch "perf symbol: Skip symbols
if SHF_ALLOC flag is not set" to stable kernel mailing list, this
patch missed fix tag.

> Signed-off-by: Ian Rogers <irogers@google.com>

With updating fix tag:

Reviewed-by: Leo Yan <leo.yan@linaro.org>
Tested-by: Leo Yan <leo.yan@linaro.org>

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

[Arnaldo Carvalho de Melo]

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

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

[Leo Yan]

On Mon, Aug 01, 2022 at 09:38:23AM -0300, Arnaldo Carvalho de Melo wrote:
> 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.

Thanks, Arnaldo.

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

I will add a testing based on the steps, alongside with the discussed
testing for data symbols.  Will share out after get ready.

Thanks,
Leo

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

[Arnaldo Carvalho de Melo]

Em Mon, Aug 01, 2022 at 09:25:11PM +0800, Leo Yan escreveu:
> On Mon, Aug 01, 2022 at 09:38:23AM -0300, Arnaldo Carvalho de Melo wrote:
> > 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.
> 
> Thanks, Arnaldo.
> 
> > 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.
> 
> I will add a testing based on the steps, alongside with the discussed
> testing for data symbols.  Will share out after get ready.

Great! Thanks in advance,

- Arnaldo

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

[Leo Yan]

Hi Arnaldo, Ian,

On Mon, Aug 01, 2022 at 09:38:23AM -0300, Arnaldo Carvalho de Melo wrote:

[...]

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

I am working on testing script for java symbols, one of the steps shared by
Ian is:

  # /tmp/perf/perf record -k 1 java -agentpath:/tmp/perf/libperf-jvmti.so \
    -jar dacapo-9.12-MR1-bach.jar -n 10 fop

The question is how we can specify the path for the lib
libperf-jvmti.so in the testing script?

If we can run the test case from the root folder of Linux kernel
source code, the lib libperf-jvmti.so can be found in the folder
$linux/tools/perf, but for the integration testing the lib should be
placed in an installed folder.  Any suggestion if we have exited
way to specify the path for libperf-jvmti.so, or need to introduce a
new shell envorinment variable for the lib path?

Thanks,
Leo

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

[Ian Rogers]

On Wed, Aug 3, 2022 at 8:25 AM Leo Yan <leo.yan@linaro.org> wrote:
>
> Hi Arnaldo, Ian,
>
> On Mon, Aug 01, 2022 at 09:38:23AM -0300, Arnaldo Carvalho de Melo wrote:
>
> [...]
>
> > 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.
>
> I am working on testing script for java symbols, one of the steps shared by
> Ian is:
>
>   # /tmp/perf/perf record -k 1 java -agentpath:/tmp/perf/libperf-jvmti.so \
>     -jar dacapo-9.12-MR1-bach.jar -n 10 fop
>
> The question is how we can specify the path for the lib
> libperf-jvmti.so in the testing script?
>
> If we can run the test case from the root folder of Linux kernel
> source code, the lib libperf-jvmti.so can be found in the folder
> $linux/tools/perf, but for the integration testing the lib should be
> placed in an installed folder.  Any suggestion if we have exited
> way to specify the path for libperf-jvmti.so, or need to introduce a
> new shell envorinment variable for the lib path?

There is a hack in 'perf test' where we assume a few paths to tests:
https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/tests/builtin-test.c?h=perf/core#n308
so in this case we could look in the same directory. There is also a
#define for PERF_EXEC_PATH.

I'd prefer it if the test could be self contained for example:

echo "int fib(int x) { return x > 1 ? fib(x - 2) + fib(x - 1) : 1; }
int q = 0; for(int i=0; i < 10; i++) q += fib(i);
System.out.println(q);" | /tmp/perf/perf record -k 1 jshell
-J-agentpath:/tmp/perf/libperf-jvmti.so

where jshell runs on the JVM and so we should get some jitted execution time.

Thanks,
Ian

> Thanks,
> Leo

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

[Leo Yan]

On Wed, Aug 03, 2022 at 05:26:39PM -0700, Ian Rogers wrote:

[...]

> > The question is how we can specify the path for the lib
> > libperf-jvmti.so in the testing script?
> >
> > If we can run the test case from the root folder of Linux kernel
> > source code, the lib libperf-jvmti.so can be found in the folder
> > $linux/tools/perf, but for the integration testing the lib should be
> > placed in an installed folder.  Any suggestion if we have exited
> > way to specify the path for libperf-jvmti.so, or need to introduce a
> > new shell envorinment variable for the lib path?
> 
> There is a hack in 'perf test' where we assume a few paths to tests:
> https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/tests/builtin-test.c?h=perf/core#n308
> so in this case we could look in the same directory. There is also a
> #define for PERF_EXEC_PATH.

Thanks for the info.  I will try to search paths like the buildin-test.c
file, and the lib libperf-jvmti.so is installed in the folder
"$HOME/lib64/", I will check if can reuse PERF_EXEC_PATH for this
case.

> I'd prefer it if the test could be self contained for example:
> 
> echo "int fib(int x) { return x > 1 ? fib(x - 2) + fib(x - 1) : 1; }
> int q = 0; for(int i=0; i < 10; i++) q += fib(i);
> System.out.println(q);" | /tmp/perf/perf record -k 1 jshell
> -J-agentpath:/tmp/perf/libperf-jvmti.so
> 
> where jshell runs on the JVM and so we should get some jitted execution time.

Will do.  Appreciate for sharing this!

Leo