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.
Signed-off-by: elisabeth <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Arnaldo Carvalho de Melo <[email protected]>
Cc: Mark Rutland <[email protected]>
Cc: Alexander Shishkin <[email protected]>
Cc: Jiri Olsa <[email protected]>
Cc: Namhyung Kim <[email protected]>
---
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
Hello,
On Wed, May 31, 2023 at 7:35 AM elisabeth <[email protected]> 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
>
> Signed-off-by: elisabeth <[email protected]>
> Cc: Peter Zijlstra <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: Arnaldo Carvalho de Melo <[email protected]>
> Cc: Mark Rutland <[email protected]>
> Cc: Alexander Shishkin <[email protected]>
> Cc: Jiri Olsa <[email protected]>
> Cc: Namhyung Kim <[email protected]>
> ---
> 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
>
On Thu, 1 Jun 2023 at 18:29, Namhyung Kim <[email protected]> wrote:
>
> Hello,
>
> On Thu, Jun 1, 2023 at 5:01 AM Elisabeth Panholzer <[email protected]> wrote:
> >
> > Hello again,
> >
> > On Wed, 31 May 2023 at 22:50, Namhyung Kim <[email protected]> wrote:
> > >
> > > Hello,
> > >
> > > On Wed, May 31, 2023 at 7:35 AM elisabeth <[email protected]> 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 <[email protected]>
On Thu, 1 Jun 2023 at 18:29, Namhyung Kim <[email protected]> wrote:
>
> Hello,
>
> On Thu, Jun 1, 2023 at 5:01 AM Elisabeth Panholzer <[email protected]> wrote:
> >
> > Hello again,
> >
> > On Wed, 31 May 2023 at 22:50, Namhyung Kim <[email protected]> wrote:
> > >
> > > Hello,
> > >
> > > On Wed, May 31, 2023 at 7:35 AM elisabeth <[email protected]> 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 <[email protected]>
> > > > Cc: Peter Zijlstra <[email protected]>
> > > > Cc: Ingo Molnar <[email protected]>
> > > > Cc: Arnaldo Carvalho de Melo <[email protected]>
> > > > Cc: Mark Rutland <[email protected]>
> > > > Cc: Alexander Shishkin <[email protected]>
> > > > Cc: Jiri Olsa <[email protected]>
> > > > Cc: Namhyung Kim <[email protected]>
> > > > ---
> > > > 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
> > > >
On Wed, May 31, 2023 at 7:35 AM elisabeth <[email protected]> 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.
>
> Signed-off-by: elisabeth <[email protected]>
> Cc: Peter Zijlstra <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: Arnaldo Carvalho de Melo <[email protected]>
> Cc: Mark Rutland <[email protected]>
> Cc: Alexander Shishkin <[email protected]>
> Cc: Jiri Olsa <[email protected]>
> Cc: Namhyung Kim <[email protected]>
> ---
> 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'};
Can you please mention that it's from the jitdump format
either by renaming or adding a comment?
> 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) {
I think you can just use strcmp().
Thanks,
Namhyung
> emit_lne_define_filename(be, ent->name);
> cur_filename = ent->name;
> emit_set_file(be, ++cur_file_idx);
> --
> 2.34.1
>