public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] perf jitdump: Add load_addr to build-ID generation
@ 2025-11-14  9:29 Namhyung Kim
  2025-11-14  9:29 ` [PATCH 2/2] perf test: Add python JIT dump test Namhyung Kim
  2025-11-14 17:33 ` [PATCH 1/2] perf jitdump: Add load_addr to build-ID generation Ian Rogers
  0 siblings, 2 replies; 11+ messages in thread
From: Namhyung Kim @ 2025-11-14  9:29 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo, Ian Rogers, James Clark
  Cc: Jiri Olsa, Adrian Hunter, Peter Zijlstra, Ingo Molnar, LKML,
	linux-perf-users, Eric Biggers, Pablo Galindo

It was reported that python backtrace with JIT dump was broken after the
change to built-in SHA-1 implementation.  It seems python generates the
same JIT code for each function.  They will become separate DSOs but the
contents are the same.  Only difference is in the symbol name.

But this caused a problem that every JIT'ed DSOs will have the same
build-ID which makes perf confused.  And it resulted in no python
symbols (from JIT) in the output.

Looking back at the original code before the conversion, it used the
load_addr as well as the code section to distinguish each DSO.  I think
we should do the same or use symbol table as an additional input for
SHA-1.

This patch is a quick-and-dirty fix just to add each byte of the
load_addr to the first 8 bytes of SHA-1 result.  Probably we need to add
sha1_update() or similar to update the existing hash value and use it
here.  I'd like something that can be backported to the stable trees
easily.

Fixes: e3f612c1d8f3945b ("perf genelf: Remove libcrypto dependency and use built-in sha1()")
Cc: Eric Biggers <ebiggers@kernel.org>
Cc: Pablo Galindo <pablogsal@gmail.com>
Link: https://github.com/python/cpython/issues/139544
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/genelf.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/tools/perf/util/genelf.c b/tools/perf/util/genelf.c
index 591548b10e34ef6a..a412e6faf70e37f3 100644
--- a/tools/perf/util/genelf.c
+++ b/tools/perf/util/genelf.c
@@ -395,6 +395,15 @@ jit_write_elf(int fd, uint64_t load_addr __maybe_unused, const char *sym,
 	 * build-id generation
 	 */
 	sha1(code, csize, bnote.build_id);
+	/* FIXME: update the SHA-1 hash using additional contents */
+	bnote.build_id[0] += (load_addr >> 0) & 0xff;
+	bnote.build_id[1] += (load_addr >> 8) & 0xff;
+	bnote.build_id[2] += (load_addr >> 16) & 0xff;
+	bnote.build_id[3] += (load_addr >> 24) & 0xff;
+	bnote.build_id[4] += (load_addr >> 32) & 0xff;
+	bnote.build_id[5] += (load_addr >> 40) & 0xff;
+	bnote.build_id[6] += (load_addr >> 48) & 0xff;
+	bnote.build_id[7] += (load_addr >> 56) & 0xff;
 	bnote.desc.namesz = sizeof(bnote.name); /* must include 0 termination */
 	bnote.desc.descsz = sizeof(bnote.build_id);
 	bnote.desc.type   = NT_GNU_BUILD_ID;
-- 
2.52.0.rc1.455.g30608eb744-goog


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 2/2] perf test: Add python JIT dump test
  2025-11-14  9:29 [PATCH 1/2] perf jitdump: Add load_addr to build-ID generation Namhyung Kim
@ 2025-11-14  9:29 ` Namhyung Kim
  2025-11-14 17:44   ` Ian Rogers
  2025-11-14 17:33 ` [PATCH 1/2] perf jitdump: Add load_addr to build-ID generation Ian Rogers
  1 sibling, 1 reply; 11+ messages in thread
From: Namhyung Kim @ 2025-11-14  9:29 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo, Ian Rogers, James Clark
  Cc: Jiri Olsa, Adrian Hunter, Peter Zijlstra, Ingo Molnar, LKML,
	linux-perf-users, Pablo Galindo

Add a test case for the python interpreter like below so that we can
make sure it won't break again.  To validate the effect of build-ID
generation, it adds and removes the JIT'ed DSOs to/from the build-ID
cache for the test.

  $ perf test -vv jitdump
   84: python profiling with jitdump:
  --- start ---
  test child forked, pid 214316
  Run python with -Xperf_jit
  [ perf record: Woken up 5 times to write data ]
  [ perf record: Captured and wrote 1.180 MB /tmp/__perf_test.perf.data.XbqZNm (140 samples) ]
  Generate JIT-ed DSOs using perf inject
  Add JIT-ed DSOs to the build-ID cache
  Check the symbol containing the script name
  Found 108 matching lines
  Remove JIT-ed DSOs from the build-ID cache
  ---- end(0) ----
   84: python profiling with jitdump                                   : Ok

Cc: Pablo Galindo <pablogsal@gmail.com>
Link: https://docs.python.org/3/howto/perf_profiling.html#how-to-work-without-frame-pointers
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/tests/shell/jitdump-python.sh | 41 ++++++++++++++++++++++++
 tools/perf/tests/shell/jitdump-test.py   | 14 ++++++++
 2 files changed, 55 insertions(+)
 create mode 100755 tools/perf/tests/shell/jitdump-python.sh
 create mode 100644 tools/perf/tests/shell/jitdump-test.py

diff --git a/tools/perf/tests/shell/jitdump-python.sh b/tools/perf/tests/shell/jitdump-python.sh
new file mode 100755
index 0000000000000000..101c15e65da1a0c0
--- /dev/null
+++ b/tools/perf/tests/shell/jitdump-python.sh
@@ -0,0 +1,41 @@
+#!/bin/bash
+# python profiling with jitdump
+# SPDX-License-Identifier: GPL-2.0
+
+if ! command -v python > /dev/null; then
+    echo "Skip: no python found"
+    exit 2
+fi
+
+SHELLDIR=$(dirname $0)
+PERF_DATA=$(mktemp /tmp/__perf_test.perf.data.XXXXXX)
+
+echo "Run python with -Xperf_jit"
+perf record -k 1 -g --call-graph dwarf -o "${PERF_DATA}" -- python -Xperf_jit ${SHELLDIR}/jitdump-test.py
+
+_PID=$(perf report -i "${PERF_DATA}" -F pid -q -g none | cut -d: -f1 -s)
+PID=$(echo -n $_PID)  # remove newlines
+
+echo "Generate JIT-ed DSOs using perf inject"
+perf inject -i "${PERF_DATA}" -j -o "${PERF_DATA}.jit"
+
+echo "Add JIT-ed DSOs to the build-ID cache"
+for F in /tmp/jitted-${PID}-*.so; do
+  perf buildid-cache -a "${F}"
+done
+
+echo "Check the symbol containing the script name"
+NUM=$(perf report -i "${PERF_DATA}.jit" -s sym | grep -c jitdump-test.py)
+
+echo "Found ${NUM} matching lines"
+
+echo "Remove JIT-ed DSOs from the build-ID cache"
+for F in /tmp/jitted-${PID}-*.so; do
+  perf buildid-cache -r "${F}"
+done
+
+rm -f ${PERF_DATA} ${PERF_DATA}.jit /tmp/jit-${PID}.dump /tmp/jitted-${PID}-*.so
+
+if [ "${NUM}" -eq 0 ]; then
+    exit 1
+fi
diff --git a/tools/perf/tests/shell/jitdump-test.py b/tools/perf/tests/shell/jitdump-test.py
new file mode 100644
index 0000000000000000..b427363ae4956db6
--- /dev/null
+++ b/tools/perf/tests/shell/jitdump-test.py
@@ -0,0 +1,14 @@
+def foo(n):
+    result = 0
+    for _ in range(n):
+        result += 1
+    return result
+
+def bar(n):
+    foo(n)
+
+def baz(n):
+    bar(n)
+
+if __name__ == "__main__":
+    baz(1000000)
-- 
2.52.0.rc1.455.g30608eb744-goog


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] perf jitdump: Add load_addr to build-ID generation
  2025-11-14  9:29 [PATCH 1/2] perf jitdump: Add load_addr to build-ID generation Namhyung Kim
  2025-11-14  9:29 ` [PATCH 2/2] perf test: Add python JIT dump test Namhyung Kim
@ 2025-11-14 17:33 ` Ian Rogers
  2025-11-14 18:57   ` Namhyung Kim
  1 sibling, 1 reply; 11+ messages in thread
From: Ian Rogers @ 2025-11-14 17:33 UTC (permalink / raw)
  To: Namhyung Kim
  Cc: Arnaldo Carvalho de Melo, James Clark, Jiri Olsa, Adrian Hunter,
	Peter Zijlstra, Ingo Molnar, LKML, linux-perf-users, Eric Biggers,
	Pablo Galindo

On Fri, Nov 14, 2025 at 1:29 AM Namhyung Kim <namhyung@kernel.org> wrote:
>
> It was reported that python backtrace with JIT dump was broken after the
> change to built-in SHA-1 implementation.  It seems python generates the
> same JIT code for each function.  They will become separate DSOs but the
> contents are the same.  Only difference is in the symbol name.
>
> But this caused a problem that every JIT'ed DSOs will have the same
> build-ID which makes perf confused.  And it resulted in no python
> symbols (from JIT) in the output.

The lookup of a DSO involves the build ID and the filename. I'm
confused as to why things weren't deduplicated and why no symbols
rather than repeatedly the same symbol?

> Looking back at the original code before the conversion, it used the
> load_addr as well as the code section to distinguish each DSO.  I think
> we should do the same or use symbol table as an additional input for
> SHA-1.

Hmm.. the build ID for the contents of the code should be a constant.
As the build ID is a note for the entire ELF file then something is
wrong with the filename handling it seems.

Thanks,
Ian

> This patch is a quick-and-dirty fix just to add each byte of the
> load_addr to the first 8 bytes of SHA-1 result.  Probably we need to add
> sha1_update() or similar to update the existing hash value and use it
> here.  I'd like something that can be backported to the stable trees
> easily.
>
> Fixes: e3f612c1d8f3945b ("perf genelf: Remove libcrypto dependency and use built-in sha1()")
> Cc: Eric Biggers <ebiggers@kernel.org>
> Cc: Pablo Galindo <pablogsal@gmail.com>
> Link: https://github.com/python/cpython/issues/139544
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/util/genelf.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/tools/perf/util/genelf.c b/tools/perf/util/genelf.c
> index 591548b10e34ef6a..a412e6faf70e37f3 100644
> --- a/tools/perf/util/genelf.c
> +++ b/tools/perf/util/genelf.c
> @@ -395,6 +395,15 @@ jit_write_elf(int fd, uint64_t load_addr __maybe_unused, const char *sym,
>          * build-id generation
>          */
>         sha1(code, csize, bnote.build_id);
> +       /* FIXME: update the SHA-1 hash using additional contents */
> +       bnote.build_id[0] += (load_addr >> 0) & 0xff;
> +       bnote.build_id[1] += (load_addr >> 8) & 0xff;
> +       bnote.build_id[2] += (load_addr >> 16) & 0xff;
> +       bnote.build_id[3] += (load_addr >> 24) & 0xff;
> +       bnote.build_id[4] += (load_addr >> 32) & 0xff;
> +       bnote.build_id[5] += (load_addr >> 40) & 0xff;
> +       bnote.build_id[6] += (load_addr >> 48) & 0xff;
> +       bnote.build_id[7] += (load_addr >> 56) & 0xff;
>         bnote.desc.namesz = sizeof(bnote.name); /* must include 0 termination */
>         bnote.desc.descsz = sizeof(bnote.build_id);
>         bnote.desc.type   = NT_GNU_BUILD_ID;
> --
> 2.52.0.rc1.455.g30608eb744-goog
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] perf test: Add python JIT dump test
  2025-11-14  9:29 ` [PATCH 2/2] perf test: Add python JIT dump test Namhyung Kim
@ 2025-11-14 17:44   ` Ian Rogers
  2025-11-14 19:03     ` Namhyung Kim
  0 siblings, 1 reply; 11+ messages in thread
From: Ian Rogers @ 2025-11-14 17:44 UTC (permalink / raw)
  To: Namhyung Kim
  Cc: Arnaldo Carvalho de Melo, James Clark, Jiri Olsa, Adrian Hunter,
	Peter Zijlstra, Ingo Molnar, LKML, linux-perf-users,
	Pablo Galindo

On Fri, Nov 14, 2025 at 1:29 AM Namhyung Kim <namhyung@kernel.org> wrote:
>
> Add a test case for the python interpreter like below so that we can
> make sure it won't break again.  To validate the effect of build-ID
> generation, it adds and removes the JIT'ed DSOs to/from the build-ID
> cache for the test.
>
>   $ perf test -vv jitdump
>    84: python profiling with jitdump:
>   --- start ---
>   test child forked, pid 214316
>   Run python with -Xperf_jit
>   [ perf record: Woken up 5 times to write data ]
>   [ perf record: Captured and wrote 1.180 MB /tmp/__perf_test.perf.data.XbqZNm (140 samples) ]
>   Generate JIT-ed DSOs using perf inject
>   Add JIT-ed DSOs to the build-ID cache
>   Check the symbol containing the script name
>   Found 108 matching lines
>   Remove JIT-ed DSOs from the build-ID cache
>   ---- end(0) ----
>    84: python profiling with jitdump                                   : Ok
>
> Cc: Pablo Galindo <pablogsal@gmail.com>
> Link: https://docs.python.org/3/howto/perf_profiling.html#how-to-work-without-frame-pointers
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/tests/shell/jitdump-python.sh | 41 ++++++++++++++++++++++++
>  tools/perf/tests/shell/jitdump-test.py   | 14 ++++++++
>  2 files changed, 55 insertions(+)
>  create mode 100755 tools/perf/tests/shell/jitdump-python.sh
>  create mode 100644 tools/perf/tests/shell/jitdump-test.py
>
> diff --git a/tools/perf/tests/shell/jitdump-python.sh b/tools/perf/tests/shell/jitdump-python.sh
> new file mode 100755
> index 0000000000000000..101c15e65da1a0c0
> --- /dev/null
> +++ b/tools/perf/tests/shell/jitdump-python.sh
> @@ -0,0 +1,41 @@
> +#!/bin/bash
> +# python profiling with jitdump
> +# SPDX-License-Identifier: GPL-2.0
> +
> +if ! command -v python > /dev/null; then
> +    echo "Skip: no python found"
> +    exit 2
> +fi

Can this just reuse the python set up logic in:
https://web.git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/tests/shell/lib/setup_python.sh?h=perf-tools-next

> +
> +SHELLDIR=$(dirname $0)
> +PERF_DATA=$(mktemp /tmp/__perf_test.perf.data.XXXXXX)
> +
> +echo "Run python with -Xperf_jit"
> +perf record -k 1 -g --call-graph dwarf -o "${PERF_DATA}" -- python -Xperf_jit ${SHELLDIR}/jitdump-test.py

We can avoid an additional file by using the REPL, this is why the
java test is using jshell:
https://web.git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/tests/shell/test_java_symbol.sh?h=perf-tools-next#n41

> +
> +_PID=$(perf report -i "${PERF_DATA}" -F pid -q -g none | cut -d: -f1 -s)
> +PID=$(echo -n $_PID)  # remove newlines
> +
> +echo "Generate JIT-ed DSOs using perf inject"
> +perf inject -i "${PERF_DATA}" -j -o "${PERF_DATA}.jit"

Thomas Richter did a workaround in the Java version to avoid debuginfod queries:
https://web.git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/tests/shell/test_java_symbol.sh?h=perf-tools-next#n59

> +
> +echo "Add JIT-ed DSOs to the build-ID cache"
> +for F in /tmp/jitted-${PID}-*.so; do
> +  perf buildid-cache -a "${F}"
> +done
> +
> +echo "Check the symbol containing the script name"
> +NUM=$(perf report -i "${PERF_DATA}.jit" -s sym | grep -c jitdump-test.py)
> +
> +echo "Found ${NUM} matching lines"
> +
> +echo "Remove JIT-ed DSOs from the build-ID cache"
> +for F in /tmp/jitted-${PID}-*.so; do
> +  perf buildid-cache -r "${F}"
> +done
> +
> +rm -f ${PERF_DATA} ${PERF_DATA}.jit /tmp/jit-${PID}.dump /tmp/jitted-${PID}-*.so

It'd be nice to have this in a function that also gets run if there is
a trap. It looks like the Java test is missing a call to cleanup_files
on the success path.

Thanks,
Ian

> +
> +if [ "${NUM}" -eq 0 ]; then
> +    exit 1
> +fi
> diff --git a/tools/perf/tests/shell/jitdump-test.py b/tools/perf/tests/shell/jitdump-test.py
> new file mode 100644
> index 0000000000000000..b427363ae4956db6
> --- /dev/null
> +++ b/tools/perf/tests/shell/jitdump-test.py
> @@ -0,0 +1,14 @@
> +def foo(n):
> +    result = 0
> +    for _ in range(n):
> +        result += 1
> +    return result
> +
> +def bar(n):
> +    foo(n)
> +
> +def baz(n):
> +    bar(n)
> +
> +if __name__ == "__main__":
> +    baz(1000000)
> --
> 2.52.0.rc1.455.g30608eb744-goog
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] perf jitdump: Add load_addr to build-ID generation
  2025-11-14 17:33 ` [PATCH 1/2] perf jitdump: Add load_addr to build-ID generation Ian Rogers
@ 2025-11-14 18:57   ` Namhyung Kim
  2025-11-14 19:32     ` Ian Rogers
  0 siblings, 1 reply; 11+ messages in thread
From: Namhyung Kim @ 2025-11-14 18:57 UTC (permalink / raw)
  To: Ian Rogers
  Cc: Arnaldo Carvalho de Melo, James Clark, Jiri Olsa, Adrian Hunter,
	Peter Zijlstra, Ingo Molnar, LKML, linux-perf-users, Eric Biggers,
	Pablo Galindo

On Fri, Nov 14, 2025 at 09:33:29AM -0800, Ian Rogers wrote:
> On Fri, Nov 14, 2025 at 1:29 AM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > It was reported that python backtrace with JIT dump was broken after the
> > change to built-in SHA-1 implementation.  It seems python generates the
> > same JIT code for each function.  They will become separate DSOs but the
> > contents are the same.  Only difference is in the symbol name.
> >
> > But this caused a problem that every JIT'ed DSOs will have the same
> > build-ID which makes perf confused.  And it resulted in no python
> > symbols (from JIT) in the output.
> 
> The lookup of a DSO involves the build ID and the filename. I'm
> confused as to why things weren't deduplicated and why no symbols
> rather than repeatedly the same symbol?

I don't know, but that's the symptom in the original bug report in the
python github (see Links: below).  I guess the behavior is
non-deterministic.

> 
> > Looking back at the original code before the conversion, it used the
> > load_addr as well as the code section to distinguish each DSO.  I think
> > we should do the same or use symbol table as an additional input for
> > SHA-1.
> 
> Hmm.. the build ID for the contents of the code should be a constant.
> As the build ID is a note for the entire ELF file then something is
> wrong with the filename handling it seems.

When it tries to load symbols from a DSO, it prefer reading from the
build-ID cache than the file system since it trusts build-IDs more than
the path name.  See dso__load() and binary_type_symtab[].

So having multiple DSO's with the same build-ID can be a problem if they
are in the build-ID cache.  Normally `perf inject -j` won't add the new
JIT-ed DSOs to the build-ID cache but it's still possible.

Thanks,
Namhyung

> 
> > This patch is a quick-and-dirty fix just to add each byte of the
> > load_addr to the first 8 bytes of SHA-1 result.  Probably we need to add
> > sha1_update() or similar to update the existing hash value and use it
> > here.  I'd like something that can be backported to the stable trees
> > easily.
> >
> > Fixes: e3f612c1d8f3945b ("perf genelf: Remove libcrypto dependency and use built-in sha1()")
> > Cc: Eric Biggers <ebiggers@kernel.org>
> > Cc: Pablo Galindo <pablogsal@gmail.com>
> > Link: https://github.com/python/cpython/issues/139544
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > ---
> >  tools/perf/util/genelf.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> >
> > diff --git a/tools/perf/util/genelf.c b/tools/perf/util/genelf.c
> > index 591548b10e34ef6a..a412e6faf70e37f3 100644
> > --- a/tools/perf/util/genelf.c
> > +++ b/tools/perf/util/genelf.c
> > @@ -395,6 +395,15 @@ jit_write_elf(int fd, uint64_t load_addr __maybe_unused, const char *sym,
> >          * build-id generation
> >          */
> >         sha1(code, csize, bnote.build_id);
> > +       /* FIXME: update the SHA-1 hash using additional contents */
> > +       bnote.build_id[0] += (load_addr >> 0) & 0xff;
> > +       bnote.build_id[1] += (load_addr >> 8) & 0xff;
> > +       bnote.build_id[2] += (load_addr >> 16) & 0xff;
> > +       bnote.build_id[3] += (load_addr >> 24) & 0xff;
> > +       bnote.build_id[4] += (load_addr >> 32) & 0xff;
> > +       bnote.build_id[5] += (load_addr >> 40) & 0xff;
> > +       bnote.build_id[6] += (load_addr >> 48) & 0xff;
> > +       bnote.build_id[7] += (load_addr >> 56) & 0xff;
> >         bnote.desc.namesz = sizeof(bnote.name); /* must include 0 termination */
> >         bnote.desc.descsz = sizeof(bnote.build_id);
> >         bnote.desc.type   = NT_GNU_BUILD_ID;
> > --
> > 2.52.0.rc1.455.g30608eb744-goog
> >

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] perf test: Add python JIT dump test
  2025-11-14 17:44   ` Ian Rogers
@ 2025-11-14 19:03     ` Namhyung Kim
  0 siblings, 0 replies; 11+ messages in thread
From: Namhyung Kim @ 2025-11-14 19:03 UTC (permalink / raw)
  To: Ian Rogers
  Cc: Arnaldo Carvalho de Melo, James Clark, Jiri Olsa, Adrian Hunter,
	Peter Zijlstra, Ingo Molnar, LKML, linux-perf-users,
	Pablo Galindo

On Fri, Nov 14, 2025 at 09:44:21AM -0800, Ian Rogers wrote:
> On Fri, Nov 14, 2025 at 1:29 AM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > Add a test case for the python interpreter like below so that we can
> > make sure it won't break again.  To validate the effect of build-ID
> > generation, it adds and removes the JIT'ed DSOs to/from the build-ID
> > cache for the test.
> >
> >   $ perf test -vv jitdump
> >    84: python profiling with jitdump:
> >   --- start ---
> >   test child forked, pid 214316
> >   Run python with -Xperf_jit
> >   [ perf record: Woken up 5 times to write data ]
> >   [ perf record: Captured and wrote 1.180 MB /tmp/__perf_test.perf.data.XbqZNm (140 samples) ]
> >   Generate JIT-ed DSOs using perf inject
> >   Add JIT-ed DSOs to the build-ID cache
> >   Check the symbol containing the script name
> >   Found 108 matching lines
> >   Remove JIT-ed DSOs from the build-ID cache
> >   ---- end(0) ----
> >    84: python profiling with jitdump                                   : Ok
> >
> > Cc: Pablo Galindo <pablogsal@gmail.com>
> > Link: https://docs.python.org/3/howto/perf_profiling.html#how-to-work-without-frame-pointers
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > ---
> >  tools/perf/tests/shell/jitdump-python.sh | 41 ++++++++++++++++++++++++
> >  tools/perf/tests/shell/jitdump-test.py   | 14 ++++++++
> >  2 files changed, 55 insertions(+)
> >  create mode 100755 tools/perf/tests/shell/jitdump-python.sh
> >  create mode 100644 tools/perf/tests/shell/jitdump-test.py
> >
> > diff --git a/tools/perf/tests/shell/jitdump-python.sh b/tools/perf/tests/shell/jitdump-python.sh
> > new file mode 100755
> > index 0000000000000000..101c15e65da1a0c0
> > --- /dev/null
> > +++ b/tools/perf/tests/shell/jitdump-python.sh
> > @@ -0,0 +1,41 @@
> > +#!/bin/bash
> > +# python profiling with jitdump
> > +# SPDX-License-Identifier: GPL-2.0
> > +
> > +if ! command -v python > /dev/null; then
> > +    echo "Skip: no python found"
> > +    exit 2
> > +fi
> 
> Can this just reuse the python set up logic in:
> https://web.git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/tests/shell/lib/setup_python.sh?h=perf-tools-next

Will do!

> 
> > +
> > +SHELLDIR=$(dirname $0)
> > +PERF_DATA=$(mktemp /tmp/__perf_test.perf.data.XXXXXX)
> > +
> > +echo "Run python with -Xperf_jit"
> > +perf record -k 1 -g --call-graph dwarf -o "${PERF_DATA}" -- python -Xperf_jit ${SHELLDIR}/jitdump-test.py
> 
> We can avoid an additional file by using the REPL, this is why the
> java test is using jshell:
> https://web.git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/tests/shell/test_java_symbol.sh?h=perf-tools-next#n41

I thought that too, but I wanted a filename to check the result.
Hmm.. probably I can use a different pattern without it.

> 
> > +
> > +_PID=$(perf report -i "${PERF_DATA}" -F pid -q -g none | cut -d: -f1 -s)
> > +PID=$(echo -n $_PID)  # remove newlines
> > +
> > +echo "Generate JIT-ed DSOs using perf inject"
> > +perf inject -i "${PERF_DATA}" -j -o "${PERF_DATA}.jit"
> 
> Thomas Richter did a workaround in the Java version to avoid debuginfod queries:
> https://web.git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/tests/shell/test_java_symbol.sh?h=perf-tools-next#n59

Yep, will add.

> 
> > +
> > +echo "Add JIT-ed DSOs to the build-ID cache"
> > +for F in /tmp/jitted-${PID}-*.so; do
> > +  perf buildid-cache -a "${F}"
> > +done
> > +
> > +echo "Check the symbol containing the script name"
> > +NUM=$(perf report -i "${PERF_DATA}.jit" -s sym | grep -c jitdump-test.py)
> > +
> > +echo "Found ${NUM} matching lines"
> > +
> > +echo "Remove JIT-ed DSOs from the build-ID cache"
> > +for F in /tmp/jitted-${PID}-*.so; do
> > +  perf buildid-cache -r "${F}"
> > +done
> > +
> > +rm -f ${PERF_DATA} ${PERF_DATA}.jit /tmp/jit-${PID}.dump /tmp/jitted-${PID}-*.so
> 
> It'd be nice to have this in a function that also gets run if there is
> a trap. It looks like the Java test is missing a call to cleanup_files
> on the success path.

I thought that it will go the end of the script regardless of any
command failures but we should protect it from Ctrl-C as well.. ok.

Thanks for your review!
Namhyung

> 
> > +
> > +if [ "${NUM}" -eq 0 ]; then
> > +    exit 1
> > +fi
> > diff --git a/tools/perf/tests/shell/jitdump-test.py b/tools/perf/tests/shell/jitdump-test.py
> > new file mode 100644
> > index 0000000000000000..b427363ae4956db6
> > --- /dev/null
> > +++ b/tools/perf/tests/shell/jitdump-test.py
> > @@ -0,0 +1,14 @@
> > +def foo(n):
> > +    result = 0
> > +    for _ in range(n):
> > +        result += 1
> > +    return result
> > +
> > +def bar(n):
> > +    foo(n)
> > +
> > +def baz(n):
> > +    bar(n)
> > +
> > +if __name__ == "__main__":
> > +    baz(1000000)
> > --
> > 2.52.0.rc1.455.g30608eb744-goog
> >

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] perf jitdump: Add load_addr to build-ID generation
  2025-11-14 18:57   ` Namhyung Kim
@ 2025-11-14 19:32     ` Ian Rogers
  2025-11-14 23:24       ` Namhyung Kim
  2025-11-16  7:22       ` Fangrui Song
  0 siblings, 2 replies; 11+ messages in thread
From: Ian Rogers @ 2025-11-14 19:32 UTC (permalink / raw)
  To: Namhyung Kim, maskray
  Cc: Arnaldo Carvalho de Melo, James Clark, Jiri Olsa, Adrian Hunter,
	Peter Zijlstra, Ingo Molnar, LKML, linux-perf-users, Eric Biggers,
	Pablo Galindo

On Fri, Nov 14, 2025 at 10:57 AM Namhyung Kim <namhyung@kernel.org> wrote:
>
> On Fri, Nov 14, 2025 at 09:33:29AM -0800, Ian Rogers wrote:
> > On Fri, Nov 14, 2025 at 1:29 AM Namhyung Kim <namhyung@kernel.org> wrote:
> > >
> > > It was reported that python backtrace with JIT dump was broken after the
> > > change to built-in SHA-1 implementation.  It seems python generates the
> > > same JIT code for each function.  They will become separate DSOs but the
> > > contents are the same.  Only difference is in the symbol name.
> > >
> > > But this caused a problem that every JIT'ed DSOs will have the same
> > > build-ID which makes perf confused.  And it resulted in no python
> > > symbols (from JIT) in the output.
> >
> > The lookup of a DSO involves the build ID and the filename. I'm
> > confused as to why things weren't deduplicated and why no symbols
> > rather than repeatedly the same symbol?
>
> I don't know, but that's the symptom in the original bug report in the
> python github (see Links: below).  I guess the behavior is
> non-deterministic.
>
> >
> > > Looking back at the original code before the conversion, it used the
> > > load_addr as well as the code section to distinguish each DSO.  I think
> > > we should do the same or use symbol table as an additional input for
> > > SHA-1.
> >
> > Hmm.. the build ID for the contents of the code should be a constant.
> > As the build ID is a note for the entire ELF file then something is
> > wrong with the filename handling it seems.
>
> When it tries to load symbols from a DSO, it prefer reading from the
> build-ID cache than the file system since it trusts build-IDs more than
> the path name.  See dso__load() and binary_type_symtab[].
>
> So having multiple DSO's with the same build-ID can be a problem if they
> are in the build-ID cache.  Normally `perf inject -j` won't add the new
> JIT-ed DSOs to the build-ID cache but it's still possible.

+Fangrui

I'm surprised that build IDs don't include symbol names but:
```
$ cat a.s
.text
.global main
.global foo
main:
foo:
       ret
$ cat b.s
.text
.global main
.global bar
main:
bar:
       ret
$ gcc -Wl,--build-id a.s -o a.out
$ gcc -Wl,--build-id b.s -o b.out
$ readelf -n a.out
...
   Build ID: 9dd0371b953db5d72929af5d98552e4ee1043616
...
$ readelf -n b.out
...
   Build ID: 9dd0371b953db5d72929af5d98552e4ee1043616
...
```
so ugh. Perhaps we need to have jitdump make a single object file (and
so 1 build ID) but with multiple unique symbols.

Thanks,
Ian




> Thanks,
> Namhyung
>
> >
> > > This patch is a quick-and-dirty fix just to add each byte of the
> > > load_addr to the first 8 bytes of SHA-1 result.  Probably we need to add
> > > sha1_update() or similar to update the existing hash value and use it
> > > here.  I'd like something that can be backported to the stable trees
> > > easily.
> > >
> > > Fixes: e3f612c1d8f3945b ("perf genelf: Remove libcrypto dependency and use built-in sha1()")
> > > Cc: Eric Biggers <ebiggers@kernel.org>
> > > Cc: Pablo Galindo <pablogsal@gmail.com>
> > > Link: https://github.com/python/cpython/issues/139544
> > > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > > ---
> > >  tools/perf/util/genelf.c | 9 +++++++++
> > >  1 file changed, 9 insertions(+)
> > >
> > > diff --git a/tools/perf/util/genelf.c b/tools/perf/util/genelf.c
> > > index 591548b10e34ef6a..a412e6faf70e37f3 100644
> > > --- a/tools/perf/util/genelf.c
> > > +++ b/tools/perf/util/genelf.c
> > > @@ -395,6 +395,15 @@ jit_write_elf(int fd, uint64_t load_addr __maybe_unused, const char *sym,
> > >          * build-id generation
> > >          */
> > >         sha1(code, csize, bnote.build_id);
> > > +       /* FIXME: update the SHA-1 hash using additional contents */
> > > +       bnote.build_id[0] += (load_addr >> 0) & 0xff;
> > > +       bnote.build_id[1] += (load_addr >> 8) & 0xff;
> > > +       bnote.build_id[2] += (load_addr >> 16) & 0xff;
> > > +       bnote.build_id[3] += (load_addr >> 24) & 0xff;
> > > +       bnote.build_id[4] += (load_addr >> 32) & 0xff;
> > > +       bnote.build_id[5] += (load_addr >> 40) & 0xff;
> > > +       bnote.build_id[6] += (load_addr >> 48) & 0xff;
> > > +       bnote.build_id[7] += (load_addr >> 56) & 0xff;
> > >         bnote.desc.namesz = sizeof(bnote.name); /* must include 0 termination */
> > >         bnote.desc.descsz = sizeof(bnote.build_id);
> > >         bnote.desc.type   = NT_GNU_BUILD_ID;
> > > --
> > > 2.52.0.rc1.455.g30608eb744-goog
> > >

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] perf jitdump: Add load_addr to build-ID generation
  2025-11-14 19:32     ` Ian Rogers
@ 2025-11-14 23:24       ` Namhyung Kim
  2025-11-14 23:58         ` Ian Rogers
  2025-11-16  7:22       ` Fangrui Song
  1 sibling, 1 reply; 11+ messages in thread
From: Namhyung Kim @ 2025-11-14 23:24 UTC (permalink / raw)
  To: Ian Rogers
  Cc: maskray, Arnaldo Carvalho de Melo, James Clark, Jiri Olsa,
	Adrian Hunter, Peter Zijlstra, Ingo Molnar, LKML,
	linux-perf-users, Eric Biggers, Pablo Galindo

On Fri, Nov 14, 2025 at 11:32:52AM -0800, Ian Rogers wrote:
> On Fri, Nov 14, 2025 at 10:57 AM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > On Fri, Nov 14, 2025 at 09:33:29AM -0800, Ian Rogers wrote:
> > > On Fri, Nov 14, 2025 at 1:29 AM Namhyung Kim <namhyung@kernel.org> wrote:
> > > >
> > > > It was reported that python backtrace with JIT dump was broken after the
> > > > change to built-in SHA-1 implementation.  It seems python generates the
> > > > same JIT code for each function.  They will become separate DSOs but the
> > > > contents are the same.  Only difference is in the symbol name.
> > > >
> > > > But this caused a problem that every JIT'ed DSOs will have the same
> > > > build-ID which makes perf confused.  And it resulted in no python
> > > > symbols (from JIT) in the output.
> > >
> > > The lookup of a DSO involves the build ID and the filename. I'm
> > > confused as to why things weren't deduplicated and why no symbols
> > > rather than repeatedly the same symbol?
> >
> > I don't know, but that's the symptom in the original bug report in the
> > python github (see Links: below).  I guess the behavior is
> > non-deterministic.
> >
> > >
> > > > Looking back at the original code before the conversion, it used the
> > > > load_addr as well as the code section to distinguish each DSO.  I think
> > > > we should do the same or use symbol table as an additional input for
> > > > SHA-1.
> > >
> > > Hmm.. the build ID for the contents of the code should be a constant.
> > > As the build ID is a note for the entire ELF file then something is
> > > wrong with the filename handling it seems.
> >
> > When it tries to load symbols from a DSO, it prefer reading from the
> > build-ID cache than the file system since it trusts build-IDs more than
> > the path name.  See dso__load() and binary_type_symtab[].
> >
> > So having multiple DSO's with the same build-ID can be a problem if they
> > are in the build-ID cache.  Normally `perf inject -j` won't add the new
> > JIT-ed DSOs to the build-ID cache but it's still possible.
> 
> +Fangrui
> 
> I'm surprised that build IDs don't include symbol names but:
> ```
> $ cat a.s
> .text
> .global main
> .global foo
> main:
> foo:
>        ret
> $ cat b.s
> .text
> .global main
> .global bar
> main:
> bar:
>        ret
> $ gcc -Wl,--build-id a.s -o a.out
> $ gcc -Wl,--build-id b.s -o b.out
> $ readelf -n a.out
> ...
>    Build ID: 9dd0371b953db5d72929af5d98552e4ee1043616
> ...
> $ readelf -n b.out
> ...
>    Build ID: 9dd0371b953db5d72929af5d98552e4ee1043616
> ...
> ```
> so ugh. Perhaps we need to have jitdump make a single object file (and
> so 1 build ID) but with multiple unique symbols.

Right, that'd be better.  But I'm afraid some JIT code could spread to
many segments so it's not possible to create a map to cover all areas
due to conflicts with other libraries.

Thanks,
Namhyung


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] perf jitdump: Add load_addr to build-ID generation
  2025-11-14 23:24       ` Namhyung Kim
@ 2025-11-14 23:58         ` Ian Rogers
  0 siblings, 0 replies; 11+ messages in thread
From: Ian Rogers @ 2025-11-14 23:58 UTC (permalink / raw)
  To: Namhyung Kim
  Cc: maskray, Arnaldo Carvalho de Melo, James Clark, Jiri Olsa,
	Adrian Hunter, Peter Zijlstra, Ingo Molnar, LKML,
	linux-perf-users, Eric Biggers, Pablo Galindo

On Fri, Nov 14, 2025 at 3:24 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> On Fri, Nov 14, 2025 at 11:32:52AM -0800, Ian Rogers wrote:
> > On Fri, Nov 14, 2025 at 10:57 AM Namhyung Kim <namhyung@kernel.org> wrote:
> > >
> > > On Fri, Nov 14, 2025 at 09:33:29AM -0800, Ian Rogers wrote:
> > > > On Fri, Nov 14, 2025 at 1:29 AM Namhyung Kim <namhyung@kernel.org> wrote:
> > > > >
> > > > > It was reported that python backtrace with JIT dump was broken after the
> > > > > change to built-in SHA-1 implementation.  It seems python generates the
> > > > > same JIT code for each function.  They will become separate DSOs but the
> > > > > contents are the same.  Only difference is in the symbol name.
> > > > >
> > > > > But this caused a problem that every JIT'ed DSOs will have the same
> > > > > build-ID which makes perf confused.  And it resulted in no python
> > > > > symbols (from JIT) in the output.
> > > >
> > > > The lookup of a DSO involves the build ID and the filename. I'm
> > > > confused as to why things weren't deduplicated and why no symbols
> > > > rather than repeatedly the same symbol?
> > >
> > > I don't know, but that's the symptom in the original bug report in the
> > > python github (see Links: below).  I guess the behavior is
> > > non-deterministic.
> > >
> > > >
> > > > > Looking back at the original code before the conversion, it used the
> > > > > load_addr as well as the code section to distinguish each DSO.  I think
> > > > > we should do the same or use symbol table as an additional input for
> > > > > SHA-1.
> > > >
> > > > Hmm.. the build ID for the contents of the code should be a constant.
> > > > As the build ID is a note for the entire ELF file then something is
> > > > wrong with the filename handling it seems.
> > >
> > > When it tries to load symbols from a DSO, it prefer reading from the
> > > build-ID cache than the file system since it trusts build-IDs more than
> > > the path name.  See dso__load() and binary_type_symtab[].
> > >
> > > So having multiple DSO's with the same build-ID can be a problem if they
> > > are in the build-ID cache.  Normally `perf inject -j` won't add the new
> > > JIT-ed DSOs to the build-ID cache but it's still possible.
> >
> > +Fangrui
> >
> > I'm surprised that build IDs don't include symbol names but:
> > ```
> > $ cat a.s
> > .text
> > .global main
> > .global foo
> > main:
> > foo:
> >        ret
> > $ cat b.s
> > .text
> > .global main
> > .global bar
> > main:
> > bar:
> >        ret
> > $ gcc -Wl,--build-id a.s -o a.out
> > $ gcc -Wl,--build-id b.s -o b.out
> > $ readelf -n a.out
> > ...
> >    Build ID: 9dd0371b953db5d72929af5d98552e4ee1043616
> > ...
> > $ readelf -n b.out
> > ...
> >    Build ID: 9dd0371b953db5d72929af5d98552e4ee1043616
> > ...
> > ```
> > so ugh. Perhaps we need to have jitdump make a single object file (and
> > so 1 build ID) but with multiple unique symbols.
>
> Right, that'd be better.  But I'm afraid some JIT code could spread to
> many segments so it's not possible to create a map to cover all areas
> due to conflicts with other libraries.

I'm not familiar with any JITs doing this. JITs often have to run on
Windows where there is a reserve for the code cache and then commits
to actually use individual pages. On Linux you can do a large
PROT_NONE mmap and then mprotect pages within this code cache region
(we should have perf events on this). Apple has some optimizations
with the MAP_JIT mmap flag. SBCL has a code cache within the
executable itself. On x86 you could spread code around within +/-2GB,
on RISC it'd be a pain due to offset limitations.

Anyway, it sounds like something is off when we are writing the
executable and likely having too many of them. There is also something
off with dsos__find as it should only give a dso if the name and build
ID are matching. The sample should have an IP we turn into a map
within maps, the dso of the map should differ for each of the call
chain IPs (even though they are < page_size()) and the symbol lookup
should work.

If we're not going to maintain that jitdump build IDs behave like
build IDs then there is little point using a SHA-1 hash and patch
series like:
https://lore.kernel.org/lkml/20251016205126.2882625-1-irogers@google.com/
are even more tedious to land :-)

Thanks,
Ian

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] perf jitdump: Add load_addr to build-ID generation
  2025-11-14 19:32     ` Ian Rogers
  2025-11-14 23:24       ` Namhyung Kim
@ 2025-11-16  7:22       ` Fangrui Song
  2025-11-17 16:58         ` Ian Rogers
  1 sibling, 1 reply; 11+ messages in thread
From: Fangrui Song @ 2025-11-16  7:22 UTC (permalink / raw)
  To: Ian Rogers
  Cc: Namhyung Kim, maskray, Arnaldo Carvalho de Melo, James Clark,
	Jiri Olsa, Adrian Hunter, Peter Zijlstra, Ingo Molnar, LKML,
	linux-perf-users, Eric Biggers, Pablo Galindo

On Fri, Nov 14, 2025 at 11:33 AM Ian Rogers <irogers@google.com> wrote:
>
> On Fri, Nov 14, 2025 at 10:57 AM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > On Fri, Nov 14, 2025 at 09:33:29AM -0800, Ian Rogers wrote:
> > > On Fri, Nov 14, 2025 at 1:29 AM Namhyung Kim <namhyung@kernel.org> wrote:
> > > >
> > > > It was reported that python backtrace with JIT dump was broken after the
> > > > change to built-in SHA-1 implementation.  It seems python generates the
> > > > same JIT code for each function.  They will become separate DSOs but the
> > > > contents are the same.  Only difference is in the symbol name.
> > > >
> > > > But this caused a problem that every JIT'ed DSOs will have the same
> > > > build-ID which makes perf confused.  And it resulted in no python
> > > > symbols (from JIT) in the output.
> > >
> > > The lookup of a DSO involves the build ID and the filename. I'm
> > > confused as to why things weren't deduplicated and why no symbols
> > > rather than repeatedly the same symbol?
> >
> > I don't know, but that's the symptom in the original bug report in the
> > python github (see Links: below).  I guess the behavior is
> > non-deterministic.
> >
> > >
> > > > Looking back at the original code before the conversion, it used the
> > > > load_addr as well as the code section to distinguish each DSO.  I think
> > > > we should do the same or use symbol table as an additional input for
> > > > SHA-1.
> > >
> > > Hmm.. the build ID for the contents of the code should be a constant.
> > > As the build ID is a note for the entire ELF file then something is
> > > wrong with the filename handling it seems.
> >
> > When it tries to load symbols from a DSO, it prefer reading from the
> > build-ID cache than the file system since it trusts build-IDs more than
> > the path name.  See dso__load() and binary_type_symtab[].
> >
> > So having multiple DSO's with the same build-ID can be a problem if they
> > are in the build-ID cache.  Normally `perf inject -j` won't add the new
> > JIT-ed DSOs to the build-ID cache but it's still possible.
>
> +Fangrui
>
> I'm surprised that build IDs don't include symbol names but:
> ```
> $ cat a.s
> .text
> .global main
> .global foo
> main:
> foo:
>        ret
> $ cat b.s
> .text
> .global main
> .global bar
> main:
> bar:
>        ret
> $ gcc -Wl,--build-id a.s -o a.out
> $ gcc -Wl,--build-id b.s -o b.out
> $ readelf -n a.out
> ...
>    Build ID: 9dd0371b953db5d72929af5d98552e4ee1043616
> ...
> $ readelf -n b.out
> ...
>    Build ID: 9dd0371b953db5d72929af5d98552e4ee1043616
> ...
> ```
> so ugh. Perhaps we need to have jitdump make a single object file (and
> so 1 build ID) but with multiple unique symbols.
>
> Thanks,
> Ian

Looks like a GNU ld issue. Other linkers should give different build IDs.

 .symtab/.strtab/.shstrtab are special sections and BFD seem to skip
their content for the build ID.
Raised  https://sourceware.org/bugzilla/show_bug.cgi?id=33633 ("ld
--build-id does not use symtab/strtab content")

>
>
>
> > Thanks,
> > Namhyung
> >
> > >
> > > > This patch is a quick-and-dirty fix just to add each byte of the
> > > > load_addr to the first 8 bytes of SHA-1 result.  Probably we need to add
> > > > sha1_update() or similar to update the existing hash value and use it
> > > > here.  I'd like something that can be backported to the stable trees
> > > > easily.
> > > >
> > > > Fixes: e3f612c1d8f3945b ("perf genelf: Remove libcrypto dependency and use built-in sha1()")
> > > > Cc: Eric Biggers <ebiggers@kernel.org>
> > > > Cc: Pablo Galindo <pablogsal@gmail.com>
> > > > Link: https://github.com/python/cpython/issues/139544
> > > > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > > > ---
> > > >  tools/perf/util/genelf.c | 9 +++++++++
> > > >  1 file changed, 9 insertions(+)
> > > >
> > > > diff --git a/tools/perf/util/genelf.c b/tools/perf/util/genelf.c
> > > > index 591548b10e34ef6a..a412e6faf70e37f3 100644
> > > > --- a/tools/perf/util/genelf.c
> > > > +++ b/tools/perf/util/genelf.c
> > > > @@ -395,6 +395,15 @@ jit_write_elf(int fd, uint64_t load_addr __maybe_unused, const char *sym,
> > > >          * build-id generation
> > > >          */
> > > >         sha1(code, csize, bnote.build_id);
> > > > +       /* FIXME: update the SHA-1 hash using additional contents */
> > > > +       bnote.build_id[0] += (load_addr >> 0) & 0xff;
> > > > +       bnote.build_id[1] += (load_addr >> 8) & 0xff;
> > > > +       bnote.build_id[2] += (load_addr >> 16) & 0xff;
> > > > +       bnote.build_id[3] += (load_addr >> 24) & 0xff;
> > > > +       bnote.build_id[4] += (load_addr >> 32) & 0xff;
> > > > +       bnote.build_id[5] += (load_addr >> 40) & 0xff;
> > > > +       bnote.build_id[6] += (load_addr >> 48) & 0xff;
> > > > +       bnote.build_id[7] += (load_addr >> 56) & 0xff;
> > > >         bnote.desc.namesz = sizeof(bnote.name); /* must include 0 termination */
> > > >         bnote.desc.descsz = sizeof(bnote.build_id);
> > > >         bnote.desc.type   = NT_GNU_BUILD_ID;
> > > > --
> > > > 2.52.0.rc1.455.g30608eb744-goog
> > > >

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] perf jitdump: Add load_addr to build-ID generation
  2025-11-16  7:22       ` Fangrui Song
@ 2025-11-17 16:58         ` Ian Rogers
  0 siblings, 0 replies; 11+ messages in thread
From: Ian Rogers @ 2025-11-17 16:58 UTC (permalink / raw)
  To: Fangrui Song
  Cc: Namhyung Kim, Arnaldo Carvalho de Melo, James Clark, Jiri Olsa,
	Adrian Hunter, Peter Zijlstra, Ingo Molnar, LKML,
	linux-perf-users, Eric Biggers, Pablo Galindo

On Sat, Nov 15, 2025 at 11:21 PM Fangrui Song <maskray@sourceware.org> wrote:
>
> On Fri, Nov 14, 2025 at 11:33 AM Ian Rogers <irogers@google.com> wrote:
> >
> > On Fri, Nov 14, 2025 at 10:57 AM Namhyung Kim <namhyung@kernel.org> wrote:
> > >
> > > On Fri, Nov 14, 2025 at 09:33:29AM -0800, Ian Rogers wrote:
> > > > On Fri, Nov 14, 2025 at 1:29 AM Namhyung Kim <namhyung@kernel.org> wrote:
> > > > >
> > > > > It was reported that python backtrace with JIT dump was broken after the
> > > > > change to built-in SHA-1 implementation.  It seems python generates the
> > > > > same JIT code for each function.  They will become separate DSOs but the
> > > > > contents are the same.  Only difference is in the symbol name.
> > > > >
> > > > > But this caused a problem that every JIT'ed DSOs will have the same
> > > > > build-ID which makes perf confused.  And it resulted in no python
> > > > > symbols (from JIT) in the output.
> > > >
> > > > The lookup of a DSO involves the build ID and the filename. I'm
> > > > confused as to why things weren't deduplicated and why no symbols
> > > > rather than repeatedly the same symbol?
> > >
> > > I don't know, but that's the symptom in the original bug report in the
> > > python github (see Links: below).  I guess the behavior is
> > > non-deterministic.
> > >
> > > >
> > > > > Looking back at the original code before the conversion, it used the
> > > > > load_addr as well as the code section to distinguish each DSO.  I think
> > > > > we should do the same or use symbol table as an additional input for
> > > > > SHA-1.
> > > >
> > > > Hmm.. the build ID for the contents of the code should be a constant.
> > > > As the build ID is a note for the entire ELF file then something is
> > > > wrong with the filename handling it seems.
> > >
> > > When it tries to load symbols from a DSO, it prefer reading from the
> > > build-ID cache than the file system since it trusts build-IDs more than
> > > the path name.  See dso__load() and binary_type_symtab[].
> > >
> > > So having multiple DSO's with the same build-ID can be a problem if they
> > > are in the build-ID cache.  Normally `perf inject -j` won't add the new
> > > JIT-ed DSOs to the build-ID cache but it's still possible.
> >
> > +Fangrui
> >
> > I'm surprised that build IDs don't include symbol names but:
> > ```
> > $ cat a.s
> > .text
> > .global main
> > .global foo
> > main:
> > foo:
> >        ret
> > $ cat b.s
> > .text
> > .global main
> > .global bar
> > main:
> > bar:
> >        ret
> > $ gcc -Wl,--build-id a.s -o a.out
> > $ gcc -Wl,--build-id b.s -o b.out
> > $ readelf -n a.out
> > ...
> >    Build ID: 9dd0371b953db5d72929af5d98552e4ee1043616
> > ...
> > $ readelf -n b.out
> > ...
> >    Build ID: 9dd0371b953db5d72929af5d98552e4ee1043616
> > ...
> > ```
> > so ugh. Perhaps we need to have jitdump make a single object file (and
> > so 1 build ID) but with multiple unique symbols.
> >
> > Thanks,
> > Ian
>
> Looks like a GNU ld issue. Other linkers should give different build IDs.
>
>  .symtab/.strtab/.shstrtab are special sections and BFD seem to skip
> their content for the build ID.
> Raised  https://sourceware.org/bugzilla/show_bug.cgi?id=33633 ("ld
> --build-id does not use symtab/strtab content")

Thanks Fangrui!

I repeated my test above and found for GNU ld and bfd the build IDs
didn't vary, but for gold and lld they do vary.

So it looks like in perf we need to update the sha1 calculation:
https://web.git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/genelf.c?h=perf-tools-next#n397
to include at least the symbol information.

I don't know if it is specified how the build ID should be calculated.
It seems like it should be a specified thing to stop tampering with
files, but I also see each linker produce a different value for my
test. I'm wondering if the sha1 calculation should really be including
other sections in the jitdump generated ELF files? For example, the
.eh_frame could be useful as that has unwinding implications.

Thanks,
Ian

> >
> >
> >
> > > Thanks,
> > > Namhyung
> > >
> > > >
> > > > > This patch is a quick-and-dirty fix just to add each byte of the
> > > > > load_addr to the first 8 bytes of SHA-1 result.  Probably we need to add
> > > > > sha1_update() or similar to update the existing hash value and use it
> > > > > here.  I'd like something that can be backported to the stable trees
> > > > > easily.
> > > > >
> > > > > Fixes: e3f612c1d8f3945b ("perf genelf: Remove libcrypto dependency and use built-in sha1()")
> > > > > Cc: Eric Biggers <ebiggers@kernel.org>
> > > > > Cc: Pablo Galindo <pablogsal@gmail.com>
> > > > > Link: https://github.com/python/cpython/issues/139544
> > > > > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > > > > ---
> > > > >  tools/perf/util/genelf.c | 9 +++++++++
> > > > >  1 file changed, 9 insertions(+)
> > > > >
> > > > > diff --git a/tools/perf/util/genelf.c b/tools/perf/util/genelf.c
> > > > > index 591548b10e34ef6a..a412e6faf70e37f3 100644
> > > > > --- a/tools/perf/util/genelf.c
> > > > > +++ b/tools/perf/util/genelf.c
> > > > > @@ -395,6 +395,15 @@ jit_write_elf(int fd, uint64_t load_addr __maybe_unused, const char *sym,
> > > > >          * build-id generation
> > > > >          */
> > > > >         sha1(code, csize, bnote.build_id);
> > > > > +       /* FIXME: update the SHA-1 hash using additional contents */
> > > > > +       bnote.build_id[0] += (load_addr >> 0) & 0xff;
> > > > > +       bnote.build_id[1] += (load_addr >> 8) & 0xff;
> > > > > +       bnote.build_id[2] += (load_addr >> 16) & 0xff;
> > > > > +       bnote.build_id[3] += (load_addr >> 24) & 0xff;
> > > > > +       bnote.build_id[4] += (load_addr >> 32) & 0xff;
> > > > > +       bnote.build_id[5] += (load_addr >> 40) & 0xff;
> > > > > +       bnote.build_id[6] += (load_addr >> 48) & 0xff;
> > > > > +       bnote.build_id[7] += (load_addr >> 56) & 0xff;
> > > > >         bnote.desc.namesz = sizeof(bnote.name); /* must include 0 termination */
> > > > >         bnote.desc.descsz = sizeof(bnote.build_id);
> > > > >         bnote.desc.type   = NT_GNU_BUILD_ID;
> > > > > --
> > > > > 2.52.0.rc1.455.g30608eb744-goog
> > > > >

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2025-11-17 16:58 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-11-14  9:29 [PATCH 1/2] perf jitdump: Add load_addr to build-ID generation Namhyung Kim
2025-11-14  9:29 ` [PATCH 2/2] perf test: Add python JIT dump test Namhyung Kim
2025-11-14 17:44   ` Ian Rogers
2025-11-14 19:03     ` Namhyung Kim
2025-11-14 17:33 ` [PATCH 1/2] perf jitdump: Add load_addr to build-ID generation Ian Rogers
2025-11-14 18:57   ` Namhyung Kim
2025-11-14 19:32     ` Ian Rogers
2025-11-14 23:24       ` Namhyung Kim
2025-11-14 23:58         ` Ian Rogers
2025-11-16  7:22       ` Fangrui Song
2025-11-17 16:58         ` Ian Rogers

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox