Hello,
I've found a couple of issues on the unwind code while I'm playing with
the JIT-dump code for the CPython. The code assumes normal DSOs mapped
from the beginning of the file and aligned to the page size. But it's
not true for the JIT-dumped DSOs which are generated for each function.
Depending on the JIT implementation, the code address and accompanied
ELF info (like ELF file headers and unwind info) can be overlapped to
adjacent (JIT-dumped) DSOs. So it should take more care when it
calculates the mapping address for the DSO.
It seems these changes need to go to the stable trees but they are
changed a lot since then so I'm not sure.
Thanks,
Namhyung
Namhyung Kim (3):
perf genelf: Set ELF program header addresses properly
perf unwind-libdw: Handle JIT-generated DSOs properly
perf unwind-libunwind: Fix base address for .eh_frame
tools/perf/util/genelf.c | 6 +++---
tools/perf/util/unwind-libdw.c | 21 +++++++++++++++++----
tools/perf/util/unwind-libunwind-local.c | 2 +-
3 files changed, 21 insertions(+), 8 deletions(-)
--
2.43.0.472.g3155946c3a-goog
The text section starts after the ELF headers so PHDR.p_vaddr and
others should have the correct addresses.
Fixes: babd04386b1d ("perf jit: Include program header in ELF files")
Signed-off-by: Namhyung Kim <[email protected]>
---
tools/perf/util/genelf.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/tools/perf/util/genelf.c b/tools/perf/util/genelf.c
index fefc72066c4e..ac17a3cb59dc 100644
--- a/tools/perf/util/genelf.c
+++ b/tools/perf/util/genelf.c
@@ -293,9 +293,9 @@ jit_write_elf(int fd, uint64_t load_addr, const char *sym,
*/
phdr = elf_newphdr(e, 1);
phdr[0].p_type = PT_LOAD;
- phdr[0].p_offset = 0;
- phdr[0].p_vaddr = 0;
- phdr[0].p_paddr = 0;
+ phdr[0].p_offset = GEN_ELF_TEXT_OFFSET;
+ phdr[0].p_vaddr = GEN_ELF_TEXT_OFFSET;
+ phdr[0].p_paddr = GEN_ELF_TEXT_OFFSET;
phdr[0].p_filesz = csize;
phdr[0].p_memsz = csize;
phdr[0].p_flags = PF_X | PF_R;
--
2.43.0.472.g3155946c3a-goog
Usually DSOs are mapped from the beginning of the file, so the base
address of the DSO can be calculated by map->start - map->pgoff.
However, JIT DSOs which are generated by `perf inject -j`, are mapped
only the code segment. This makes unwind-libdw code confusing and
rejects processing unwinds in the JIT DSOs. It should use the map
start address as base for them to fix the confusion.
Fixes: 1fe627da3033 ("perf unwind: Take pgoff into account when reporting elf to libdwfl")
Signed-off-by: Namhyung Kim <[email protected]>
---
tools/perf/util/unwind-libdw.c | 21 +++++++++++++++++----
1 file changed, 17 insertions(+), 4 deletions(-)
diff --git a/tools/perf/util/unwind-libdw.c b/tools/perf/util/unwind-libdw.c
index 8554db3fc0d7..6013335a8dae 100644
--- a/tools/perf/util/unwind-libdw.c
+++ b/tools/perf/util/unwind-libdw.c
@@ -46,6 +46,7 @@ static int __report_module(struct addr_location *al, u64 ip,
{
Dwfl_Module *mod;
struct dso *dso = NULL;
+ Dwarf_Addr base;
/*
* Some callers will use al->sym, so we can't just use the
* cheaper thread__find_map() here.
@@ -58,13 +59,25 @@ static int __report_module(struct addr_location *al, u64 ip,
if (!dso)
return 0;
+ /*
+ * The generated JIT DSO files only map the code segment without
+ * ELF headers. Since JIT codes used to be packed in a memory
+ * segment, calculating the base address using pgoff falls into
+ * a different code in another DSO. So just use the map->start
+ * directly to pick the correct one.
+ */
+ if (!strncmp(dso->long_name, "/tmp/jitted-", 12))
+ base = map__start(al->map);
+ else
+ base = map__start(al->map) - map__pgoff(al->map);
+
mod = dwfl_addrmodule(ui->dwfl, ip);
if (mod) {
Dwarf_Addr s;
dwfl_module_info(mod, NULL, &s, NULL, NULL, NULL, NULL, NULL);
- if (s != map__start(al->map) - map__pgoff(al->map))
- mod = 0;
+ if (s != base)
+ mod = NULL;
}
if (!mod) {
@@ -72,14 +85,14 @@ static int __report_module(struct addr_location *al, u64 ip,
__symbol__join_symfs(filename, sizeof(filename), dso->long_name);
mod = dwfl_report_elf(ui->dwfl, dso->short_name, filename, -1,
- map__start(al->map) - map__pgoff(al->map), false);
+ base, false);
}
if (!mod) {
char filename[PATH_MAX];
if (dso__build_id_filename(dso, filename, sizeof(filename), false))
mod = dwfl_report_elf(ui->dwfl, dso->short_name, filename, -1,
- map__start(al->map) - map__pgoff(al->map), false);
+ base, false);
}
if (mod) {
--
2.43.0.472.g3155946c3a-goog
On Mon, Dec 11, 2023 at 11:05 PM Namhyung Kim <[email protected]> wrote:
>
> Usually DSOs are mapped from the beginning of the file, so the base
> address of the DSO can be calculated by map->start - map->pgoff.
>
> However, JIT DSOs which are generated by `perf inject -j`, are mapped
> only the code segment. This makes unwind-libdw code confusing and
> rejects processing unwinds in the JIT DSOs. It should use the map
> start address as base for them to fix the confusion.
>
> Fixes: 1fe627da3033 ("perf unwind: Take pgoff into account when reporting elf to libdwfl")
> Signed-off-by: Namhyung Kim <[email protected]>
> ---
> tools/perf/util/unwind-libdw.c | 21 +++++++++++++++++----
> 1 file changed, 17 insertions(+), 4 deletions(-)
>
> diff --git a/tools/perf/util/unwind-libdw.c b/tools/perf/util/unwind-libdw.c
> index 8554db3fc0d7..6013335a8dae 100644
> --- a/tools/perf/util/unwind-libdw.c
> +++ b/tools/perf/util/unwind-libdw.c
> @@ -46,6 +46,7 @@ static int __report_module(struct addr_location *al, u64 ip,
> {
> Dwfl_Module *mod;
> struct dso *dso = NULL;
> + Dwarf_Addr base;
> /*
> * Some callers will use al->sym, so we can't just use the
> * cheaper thread__find_map() here.
> @@ -58,13 +59,25 @@ static int __report_module(struct addr_location *al, u64 ip,
> if (!dso)
> return 0;
>
> + /*
> + * The generated JIT DSO files only map the code segment without
> + * ELF headers. Since JIT codes used to be packed in a memory
> + * segment, calculating the base address using pgoff falls into
> + * a different code in another DSO. So just use the map->start
> + * directly to pick the correct one.
> + */
> + if (!strncmp(dso->long_name, "/tmp/jitted-", 12))
Perhaps it would be better to test:
dso->symtab_type == DSO_BINARY_TYPE__JAVA_JIT
Thanks,
Ian
> + base = map__start(al->map);
> + else
> + base = map__start(al->map) - map__pgoff(al->map);
> +
> mod = dwfl_addrmodule(ui->dwfl, ip);
> if (mod) {
> Dwarf_Addr s;
>
> dwfl_module_info(mod, NULL, &s, NULL, NULL, NULL, NULL, NULL);
> - if (s != map__start(al->map) - map__pgoff(al->map))
> - mod = 0;
> + if (s != base)
> + mod = NULL;
> }
>
> if (!mod) {
> @@ -72,14 +85,14 @@ static int __report_module(struct addr_location *al, u64 ip,
>
> __symbol__join_symfs(filename, sizeof(filename), dso->long_name);
> mod = dwfl_report_elf(ui->dwfl, dso->short_name, filename, -1,
> - map__start(al->map) - map__pgoff(al->map), false);
> + base, false);
> }
> if (!mod) {
> char filename[PATH_MAX];
>
> if (dso__build_id_filename(dso, filename, sizeof(filename), false))
> mod = dwfl_report_elf(ui->dwfl, dso->short_name, filename, -1,
> - map__start(al->map) - map__pgoff(al->map), false);
> + base, false);
> }
>
> if (mod) {
> --
> 2.43.0.472.g3155946c3a-goog
>
On Mon, Dec 11, 2023 at 11:05 PM Namhyung Kim <[email protected]> wrote:
>
> The text section starts after the ELF headers so PHDR.p_vaddr and
> others should have the correct addresses.
>
> Fixes: babd04386b1d ("perf jit: Include program header in ELF files")
> Signed-off-by: Namhyung Kim <[email protected]>
Reviewed-by: Ian Rogers <[email protected]>
Thanks,
Ian
> ---
> tools/perf/util/genelf.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/tools/perf/util/genelf.c b/tools/perf/util/genelf.c
> index fefc72066c4e..ac17a3cb59dc 100644
> --- a/tools/perf/util/genelf.c
> +++ b/tools/perf/util/genelf.c
> @@ -293,9 +293,9 @@ jit_write_elf(int fd, uint64_t load_addr, const char *sym,
> */
> phdr = elf_newphdr(e, 1);
> phdr[0].p_type = PT_LOAD;
> - phdr[0].p_offset = 0;
> - phdr[0].p_vaddr = 0;
> - phdr[0].p_paddr = 0;
> + phdr[0].p_offset = GEN_ELF_TEXT_OFFSET;
> + phdr[0].p_vaddr = GEN_ELF_TEXT_OFFSET;
> + phdr[0].p_paddr = GEN_ELF_TEXT_OFFSET;
> phdr[0].p_filesz = csize;
> phdr[0].p_memsz = csize;
> phdr[0].p_flags = PF_X | PF_R;
> --
> 2.43.0.472.g3155946c3a-goog
>
On Mon, Dec 11, 2023 at 11:05 PM Namhyung Kim <[email protected]> wrote:
>
> Hello,
>
> I've found a couple of issues on the unwind code while I'm playing with
> the JIT-dump code for the CPython. The code assumes normal DSOs mapped
> from the beginning of the file and aligned to the page size. But it's
> not true for the JIT-dumped DSOs which are generated for each function.
We have a JIT test in:
https://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
It'd be great if we could do similar for CPython.
Thanks,
Ian
> Depending on the JIT implementation, the code address and accompanied
> ELF info (like ELF file headers and unwind info) can be overlapped to
> adjacent (JIT-dumped) DSOs. So it should take more care when it
> calculates the mapping address for the DSO.
>
> It seems these changes need to go to the stable trees but they are
> changed a lot since then so I'm not sure.
>
> Thanks,
> Namhyung
>
>
> Namhyung Kim (3):
> perf genelf: Set ELF program header addresses properly
> perf unwind-libdw: Handle JIT-generated DSOs properly
> perf unwind-libunwind: Fix base address for .eh_frame
>
> tools/perf/util/genelf.c | 6 +++---
> tools/perf/util/unwind-libdw.c | 21 +++++++++++++++++----
> tools/perf/util/unwind-libunwind-local.c | 2 +-
> 3 files changed, 21 insertions(+), 8 deletions(-)
>
> --
> 2.43.0.472.g3155946c3a-goog
>
> It'd be great if we could do similar for CPython.
We (the CPython team) plan to release the perf jitdump support for
Python 3.13 (around next October) so you will need to build from
source or wait until is officially released for that test.
Pablo
On Tue, Dec 12, 2023 at 4:54 PM Pablo Galindo Salgado
<[email protected]> wrote:
>
> > It'd be great if we could do similar for CPython.
>
> We (the CPython team) plan to release the perf jitdump support for
> Python 3.13 (around next October) so you will need to build from
> source or wait until is officially released for that test.
Okay, I hope we can have all the fixes asap and most distros ship
the latest version by then.
Thanks,
Namhyung
On Tue, Dec 12, 2023 at 10:07 AM Ian Rogers <[email protected]> wrote:
>
> On Mon, Dec 11, 2023 at 11:05 PM Namhyung Kim <[email protected]> wrote:
> >
> > Usually DSOs are mapped from the beginning of the file, so the base
> > address of the DSO can be calculated by map->start - map->pgoff.
> >
> > However, JIT DSOs which are generated by `perf inject -j`, are mapped
> > only the code segment. This makes unwind-libdw code confusing and
> > rejects processing unwinds in the JIT DSOs. It should use the map
> > start address as base for them to fix the confusion.
> >
> > Fixes: 1fe627da3033 ("perf unwind: Take pgoff into account when reporting elf to libdwfl")
> > Signed-off-by: Namhyung Kim <[email protected]>
> > ---
> > tools/perf/util/unwind-libdw.c | 21 +++++++++++++++++----
> > 1 file changed, 17 insertions(+), 4 deletions(-)
> >
> > diff --git a/tools/perf/util/unwind-libdw.c b/tools/perf/util/unwind-libdw.c
> > index 8554db3fc0d7..6013335a8dae 100644
> > --- a/tools/perf/util/unwind-libdw.c
> > +++ b/tools/perf/util/unwind-libdw.c
> > @@ -46,6 +46,7 @@ static int __report_module(struct addr_location *al, u64 ip,
> > {
> > Dwfl_Module *mod;
> > struct dso *dso = NULL;
> > + Dwarf_Addr base;
> > /*
> > * Some callers will use al->sym, so we can't just use the
> > * cheaper thread__find_map() here.
> > @@ -58,13 +59,25 @@ static int __report_module(struct addr_location *al, u64 ip,
> > if (!dso)
> > return 0;
> >
> > + /*
> > + * The generated JIT DSO files only map the code segment without
> > + * ELF headers. Since JIT codes used to be packed in a memory
> > + * segment, calculating the base address using pgoff falls into
> > + * a different code in another DSO. So just use the map->start
> > + * directly to pick the correct one.
> > + */
> > + if (!strncmp(dso->long_name, "/tmp/jitted-", 12))
>
> Perhaps it would be better to test:
> dso->symtab_type == DSO_BINARY_TYPE__JAVA_JIT
Well.. it's a little different. The JAVA_JIT type files have
"/tmp/perf-" prefix and it's a plain text file (for symbols).
While this is an ELF file generated by `perf inject -j`.
Thanks,
Namhyung
Em Mon, Dec 11, 2023 at 11:05:43PM -0800, Namhyung Kim escreveu:
> Hello,
>
> I've found a couple of issues on the unwind code while I'm playing with
> the JIT-dump code for the CPython. The code assumes normal DSOs mapped
> from the beginning of the file and aligned to the page size. But it's
> not true for the JIT-dumped DSOs which are generated for each function.
>
> Depending on the JIT implementation, the code address and accompanied
> ELF info (like ELF file headers and unwind info) can be overlapped to
> adjacent (JIT-dumped) DSOs. So it should take more care when it
> calculates the mapping address for the DSO.
>
> It seems these changes need to go to the stable trees but they are
> changed a lot since then so I'm not sure.
>
Thanks, applied to perf-tools-next.
- Arnaldo
>
> Namhyung Kim (3):
> perf genelf: Set ELF program header addresses properly
> perf unwind-libdw: Handle JIT-generated DSOs properly
> perf unwind-libunwind: Fix base address for .eh_frame
>
> tools/perf/util/genelf.c | 6 +++---
> tools/perf/util/unwind-libdw.c | 21 +++++++++++++++++----
> tools/perf/util/unwind-libunwind-local.c | 2 +-
> 3 files changed, 21 insertions(+), 8 deletions(-)
>
> --
> 2.43.0.472.g3155946c3a-goog
>
--
- Arnaldo