All of lore.kernel.org
 help / color / mirror / Atom feed
From: Stephen Brennan <stephen.s.brennan@oracle.com>
To: Arnaldo Carvalho de Melo <acme@kernel.org>,
	Namhyung Kim <namhyung@kernel.org>
Cc: Ingo Molnar <mingo@redhat.com>,
	Peter Zijlstra <peterz@infradead.org>,
	"Liang, Kan" <kan.liang@linux.intel.com>,
	Mark Rutland <mark.rutland@arm.com>,
	Athira Rajeev <atrajeev@linux.vnet.ibm.com>,
	Jiri Olsa <jolsa@kernel.org>,
	linux-kernel@vger.kernel.org,
	Alexander Shishkin <alexander.shishkin@linux.intel.com>,
	James Clark <james.clark@linaro.org>,
	Chaitanya S Prakash <chaitanyas.prakash@arm.com>,
	Ian Rogers <irogers@google.com>,
	linux-perf-users@vger.kernel.org,
	Adrian Hunter <adrian.hunter@intel.com>,
	Andi Kleen <ak@linux.intel.com>
Subject: Re: [PATCH v2 0/3] Support .gnu_debugdata for symbols in perf
Date: Fri, 07 Mar 2025 14:33:01 -0800	[thread overview]
Message-ID: <87a59wwjxe.fsf@oracle.com> (raw)
In-Reply-To: <Z8tbEgh7rZscggv6@x1>

Arnaldo Carvalho de Melo <acme@kernel.org> writes:
> On Fri, Mar 07, 2025 at 05:18:36PM -0300, Arnaldo Carvalho de Melo wrote:
[...]
> It has a way to use that section tho:
>
> ⬢ [acme@toolbox perf-tools-next]$ man eu-readelf | grep -A2 -- --elf-section
>                [--elf-section [section] ]
>                [-w|
>                 --debug-dump[=line,=decodedline,=info,=info+,=abbrev,=pubnames,=aranges,=macro,=frames,=str,=loc,=ranges,=gdb_index,=addr]]
> --
>        --elf-section [section]
>            Use the named SECTION (default .gnu_debugdata) as (compressed) ELF input data
>
> ⬢ [acme@toolbox perf-tools-next]$
>
> ⬢ [acme@toolbox perf-tools-next]$ eu-readelf --elf-section -s /bin/find | grep -w FUNC | grep -vw UNDEF | wc -l
> 339
> ⬢ [acme@toolbox perf-tools-next]$ eu-readelf --elf-section -s /bin/find | grep -w FUNC | grep -vw UNDEF | head
>     1: 00000000000056d0     35 FUNC    LOCAL  DEFAULT       17 entry_hashfunc
>     2: 0000000000005700     34 FUNC    LOCAL  DEFAULT       17 entry_comparator
>     3: 0000000000005920    121 FUNC    LOCAL  DEFAULT       17 subtree_has_side_effects
>     4: 00000000000059a0    992 FUNC    LOCAL  DEFAULT       17 worst_cost.part.0
>     5: 0000000000005d80    449 FUNC    LOCAL  DEFAULT       17 traverse_tree
>     6: 0000000000005f50     73 FUNC    LOCAL  DEFAULT       17 undangle_file_pointers
>     7: 0000000000005fa0     72 FUNC    LOCAL  DEFAULT       17 looks_like_expression
>     8: 0000000000006030    303 FUNC    LOCAL  DEFAULT       17 get_fts_info_name
>     9: 0000000000006190     35 FUNC    LOCAL  DEFAULT       17 inside_dir.part.0
>    10: 0000000000006330    451 FUNC    LOCAL  DEFAULT       17 pred_sanity_check
> ⬢ [acme@toolbox perf-tools-next]$

Wow, thank you for teaching me that!
I had been using:

  gdb /usr/bin/bash --batch -ex 'maint print msymbols'

Because I knew GDB had support for .gnu_debugdata. But the --elf-section
argument to eu-readelf is much more useful.

> So there we can find the new entries, such as the top one in the example
> profile session above:
>
> ⬢ [acme@toolbox perf-tools-next]$ eu-readelf --elf-section -s /bin/find | grep -w FUNC | grep -vw UNDEF | grep -w consider_visiting
>    48: 000000000000b460   2544 FUNC    LOCAL  DEFAULT       17 consider_visiting
> ⬢ [acme@toolbox perf-tools-next]$
>
> And trat address matches the resolution perf did with your patches:
>
> ⬢ [acme@toolbox perf-tools-next]$ perf report -v --stdio |& head
> build id event received for [vdso]: a2184b81fbbc08eff401d16259eca8ad5f9d8988 [20]
> build id event received for /usr/bin/find: 3faf3f04d1b31abc9e5ce8428110e424fd980a37 [20]
> build id event received for /usr/lib64/ld-linux-x86-64.so.2: 765f7ab0f3569ffe98de85864a0cedda9b686994 [20]
> build id event received for /usr/lib64/libc.so.6: c8c3fa52aaee3f5d73b6fd862e39e9d4c010b6ba [20]
> build id event received for [kernel.kallsyms]: c3fbb7df4dfb94762b1648bc65e4363e50f45585 [20]
> read_gnu_debugdata: using .gnu_debugdata of /usr/bin/find
> # To display the perf.data header info, please use --header/--header-only options.
> #
> #
> # Total Lost Samples: 0
> ⬢ [acme@toolbox perf-tools-next]$ perf report -v --stdio |& head -20
> build id event received for [vdso]: a2184b81fbbc08eff401d16259eca8ad5f9d8988 [20]
> build id event received for /usr/bin/find: 3faf3f04d1b31abc9e5ce8428110e424fd980a37 [20]
> build id event received for /usr/lib64/ld-linux-x86-64.so.2: 765f7ab0f3569ffe98de85864a0cedda9b686994 [20]
> build id event received for /usr/lib64/libc.so.6: c8c3fa52aaee3f5d73b6fd862e39e9d4c010b6ba [20]
> build id event received for [kernel.kallsyms]: c3fbb7df4dfb94762b1648bc65e4363e50f45585 [20]
> read_gnu_debugdata: using .gnu_debugdata of /usr/bin/find
> # To display the perf.data header info, please use --header/--header-only options.
> #
> #
> # Total Lost Samples: 0
> #
> # Samples: 163  of event 'cpu_core/cycles/Pu'
> # Event count (approx.): 68126524
> #
> # Overhead  Command  Shared Object                    Symbol
> # ........  .......  ...............................  .......................................................
> #
>      8.72%  find     /usr/bin/find                    0xb498
>      7.90%  find     /usr/lib64/libc.so.6             0xe51e0            B [.] __GI___readdir64
>      7.44%  find     /usr/lib64/libc.so.6             0xa77cd            B [.] _int_malloc
> ⬢ [acme@toolbox perf-tools-next]$
>
> The only strange thing was not having it resolved in the -v case, which
> I think its because you added a new type of DSO but didn't update the
> code that does the 'perf report -v' verbose case?
>
> I ran out of time, have to go AFK now, can you please take a look,
> Stephen?

Thanks for the catch. I double checked all the places where
DSO_BINARY_TYPE constants are enumerated, and it turns out I missed
adding an entry to

char dso__symtab_origin(const struct dso *dso) ...

I assume that the array defaulted to '\0' which terminated the string
too early for this line. Oops!

Most of the letters I would associate with ".gnu_debugdata" are
taken (namely, g/G for GNU, m/M for MiniDebugInfo, d/D for
debugdata...). So 'n', for the second letter of GNU, is my selection
unless you feel differently. With that change, the table is fixed for
"perf report -v". Here it is running against my test data focusing on a
symbol only found in .gnu_debugdata of bash:

$ ./perf report -v --stdio -i ~/repos/UEK6/perf.data 2>&1 | egrep yy_readline_get\|gnu_debugdata
read_gnu_debugdata: using .gnu_debugdata of /usr/bin/bash
unwind: yy_readline_get:ip = 0x55fdc4509dbe (0x33dbe)
unwind: yy_readline_get:ip = 0x55fdc4509dbe (0x33dbe)
unwind: yy_readline_get:ip = 0x55fdc4509dbe (0x33dbe)
unwind: yy_readline_get:ip = 0x55fdc4509dbe (0x33dbe)
unwind: yy_readline_get:ip = 0x55fdc4509dbe (0x33dbe)
unwind: yy_readline_get:ip = 0x55fdc4509dbe (0x33dbe)
read_gnu_debugdata: using .gnu_debugdata of /usr/bin/sed
read_gnu_debugdata: using .gnu_debugdata of /usr/bin/date
read_gnu_debugdata: using .gnu_debugdata of /usr/bin/sqlite3
read_gnu_debugdata: using .gnu_debugdata of /usr/bin/sleep
     0.20%     0.00%  bash     /usr/bin/bash                    0x55fdc4509dbe     n [.] yy_readline_get

I'll update the patch accordingly.

> DSO_BINARY_TYPE__GNU_DEBUGDATA should be handled at...
>
> int dso__read_binary_type_filename(const struct dso *dso,
>                                    enum dso_binary_type type,
>                                    char *root_dir, char *filename, size_t size)
>
> But you have it there, ok, I'll try to continue later.
>
> Other than that the patch looks great and makes use of this new mini
> symtab, excellent!

And thank you for your testing!

> - Arnaldo

  reply	other threads:[~2025-03-07 22:33 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-02-20 18:55 [PATCH v2 0/3] Support .gnu_debugdata for symbols in perf Stephen Brennan
2025-02-20 18:55 ` [PATCH v2 1/3] tools: perf: add dummy functions for !HAVE_LZMA_SUPPORT Stephen Brennan
2025-02-20 18:55 ` [PATCH v2 2/3] tools: perf: add LZMA decompression from FILE Stephen Brennan
2025-02-20 18:55 ` [PATCH v2 3/3] tools: perf: support .gnu_debugdata for symbols Stephen Brennan
2025-02-26 22:06 ` [PATCH v2 0/3] Support .gnu_debugdata for symbols in perf Namhyung Kim
2025-03-07 20:10   ` Arnaldo Carvalho de Melo
2025-03-07 20:18     ` Arnaldo Carvalho de Melo
2025-03-07 20:46       ` Arnaldo Carvalho de Melo
2025-03-07 22:33         ` Stephen Brennan [this message]
2025-03-10 15:21           ` Arnaldo Carvalho de Melo

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87a59wwjxe.fsf@oracle.com \
    --to=stephen.s.brennan@oracle.com \
    --cc=acme@kernel.org \
    --cc=adrian.hunter@intel.com \
    --cc=ak@linux.intel.com \
    --cc=alexander.shishkin@linux.intel.com \
    --cc=atrajeev@linux.vnet.ibm.com \
    --cc=chaitanyas.prakash@arm.com \
    --cc=irogers@google.com \
    --cc=james.clark@linaro.org \
    --cc=jolsa@kernel.org \
    --cc=kan.liang@linux.intel.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-perf-users@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=mingo@redhat.com \
    --cc=namhyung@kernel.org \
    --cc=peterz@infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.