Re: [PATCH] Subject: perf jit: Fix incorrect file name in DWARF line table

[Elisabeth Panholzer]

On Thu, 1 Jun 2023 at 18:29, Namhyung Kim <namhyung@xxxxxxxxxx> wrote:
>
> Hello,
>
> On Thu, Jun 1, 2023 at 5:01 AM Elisabeth Panholzer <paniii94@xxxxxxxxx> wrote:
> >
> > Hello again,
> >
> > On Wed, 31 May 2023 at 22:50, Namhyung Kim <namhyung@xxxxxxxxxx> wrote:
> > >
> > > Hello,
> > >
> > > On Wed, May 31, 2023 at 7:35 AM elisabeth <paniii94@xxxxxxxxx> wrote:
> > > >
> > > > Fix an issue where an incorrect file name was added in the DWARF line table
> > > > due to not checking the filename against the repeated name marker (/xff/0).
> > > > This can be seen with `objdump --dwarf=line` on the ELF file after perf inject.
> > >
> > > I'm not aware of the marker.  Could you please provide a link or something?
> > >
> > > Thanks,
> > > Namhyung
> > >
> > >
> >
> > The marker is mentioned in the tools/perf/util/jitdump.h header, this
> > file describes the jitdump binary format. According to a comment in
> > the ‘debug_entry’ struct in the ‘jitdump.h’ file, a filename set to
> > ‘\xff\0’ indicates that the name is the same as the one from the
> > previous entry. When profiling applications in the browser together
> > with v8, I noticed ‘perf inject --jit’ inserts a bogus file name in
> > the DWARF line table. This happens because in the perf source code in
> > the file tools/perf/util/genelf-debug.c, in the function
> > emit_lineno_info(), the debug entry file gets compared to the previous
> > debug entry file. If they are not the same, a new filename is added to
> > the DWARF line table. However, there is no check for ‘\xff\0’, which
> > indicates that it's the same file. As a result, ‘\xff\0’ is inserted
> > as the filename into the DWARF section. This not only makes ‘objdump
> > --dwarf=line’ print bogus as the filename, but it also makes no source
> > code information show up in perf annotate.
>
> Thanks for your detailed answer.  It would be great if you could send it
> to the mailing list so that others can see it as well.
>

I added the mailing lists to the cc again.

> So it's from the jitdump format.  I thought it was a part of DWARF or
> some other specification.

It is part of the jitdump format, yes.
The data from the jitdump, generated by for example v8, is just used
to generate the ELF images and the DWARF data with 'perf inject
--jit'.

>
> >
> > Hope this makes it more clear,
> > Elisabeth
>
> Sure, it really helped, thanks.
> Namhyung
>

You're welcome.

>
> >
> > > >
> > > > Signed-off-by: Elisabeth Panholzer <elisabeth@xxxxxxxxxxxxxxx>


On Thu, 1 Jun 2023 at 18:29, Namhyung Kim <namhyung@xxxxxxxxxx> wrote:
>
> Hello,
>
> On Thu, Jun 1, 2023 at 5:01 AM Elisabeth Panholzer <paniii94@xxxxxxxxx> wrote:
> >
> > Hello again,
> >
> > On Wed, 31 May 2023 at 22:50, Namhyung Kim <namhyung@xxxxxxxxxx> wrote:
> > >
> > > Hello,
> > >
> > > On Wed, May 31, 2023 at 7:35 AM elisabeth <paniii94@xxxxxxxxx> wrote:
> > > >
> > > > Fix an issue where an incorrect file name was added in the DWARF line table
> > > > due to not checking the filename against the repeated name marker (/xff/0).
> > > > This can be seen with `objdump --dwarf=line` on the ELF file after perf inject.
> > >
> > > I'm not aware of the marker.  Could you please provide a link or something?
> > >
> > > Thanks,
> > > Namhyung
> > >
> > >
> >
> > The marker is mentioned in the tools/perf/util/jitdump.h header, this
> > file describes the jitdump binary format. According to a comment in
> > the ‘debug_entry’ struct in the ‘jitdump.h’ file, a filename set to
> > ‘\xff\0’ indicates that the name is the same as the one from the
> > previous entry. When profiling applications in the browser together
> > with v8, I noticed ‘perf inject --jit’ inserts a bogus file name in
> > the DWARF line table. This happens because in the perf source code in
> > the file tools/perf/util/genelf-debug.c, in the function
> > emit_lineno_info(), the debug entry file gets compared to the previous
> > debug entry file. If they are not the same, a new filename is added to
> > the DWARF line table. However, there is no check for ‘\xff\0’, which
> > indicates that it's the same file. As a result, ‘\xff\0’ is inserted
> > as the filename into the DWARF section. This not only makes ‘objdump
> > --dwarf=line’ print bogus as the filename, but it also makes no source
> > code information show up in perf annotate.
>
> Thanks for your detailed answer.  It would be great if you could send it
> to the mailing list so that others can see it as well.
>
> So it's from the jitdump format.  I thought it was a part of DWARF or
> some other specification.
>
> >
> > Hope this makes it more clear,
> > Elisabeth
>
> Sure, it really helped, thanks.
> Namhyung
>
>
> >
> > > >
> > > > Signed-off-by: elisabeth <paniii94@xxxxxxxxx>
> > > > Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> > > > Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> > > > Cc: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>
> > > > Cc: Mark Rutland <mark.rutland@xxxxxxx>
> > > > Cc: Alexander Shishkin <alexander.shishkin@xxxxxxxxxxxxxxx>
> > > > Cc: Jiri Olsa <jolsa@xxxxxxxxxx>
> > > > Cc: Namhyung Kim <namhyung@xxxxxxxxxx>
> > > > ---
> > > >  tools/perf/util/genelf_debug.c | 4 +++-
> > > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/tools/perf/util/genelf_debug.c b/tools/perf/util/genelf_debug.c
> > > > index aa5dcc56b2ac..2928b59bb9a5 100644
> > > > --- a/tools/perf/util/genelf_debug.c
> > > > +++ b/tools/perf/util/genelf_debug.c
> > > > @@ -349,6 +349,7 @@ static void emit_lineno_info(struct buffer_ext *be,
> > > >          */
> > > >
> > > >         /* start state of the state machine we take care of */
> > > > +       char const repeated_name_marker[] = {'\xff', '\0'};
> > > >         unsigned long last_vma = 0;
> > > >         char const  *cur_filename = NULL;
> > > >         unsigned long cur_file_idx = 0;
> > > > @@ -363,7 +364,8 @@ static void emit_lineno_info(struct buffer_ext *be,
> > > >                 /*
> > > >                  * check if filename changed, if so add it
> > > >                  */
> > > > -               if (!cur_filename || strcmp(cur_filename, ent->name)) {
> > > > +               if ((!cur_filename || strcmp(cur_filename, ent->name)) &&
> > > > +                       memcmp(repeated_name_marker, ent->name, sizeof(repeated_name_marker)) != 0) {
> > > >                         emit_lne_define_filename(be, ent->name);
> > > >                         cur_filename = ent->name;
> > > >                         emit_set_file(be, ++cur_file_idx);
> > > > --
> > > > 2.34.1
> > > >