Message-Id: <20241012143003.eeb8d3eb43e45494193bbfd4@kernel.org>
Date: Sat, 12 Oct 2024 14:30:03 +0900
From: Masami Hiramatsu (Google) <mhiramat@...nel.org>
To: Leo Yan <leo.yan@....com>
Cc: Arnaldo Carvalho de Melo <acme@...nel.org>, Namhyung Kim
 <namhyung@...nel.org>, Mark Rutland <mark.rutland@....com>, Alexander
 Shishkin <alexander.shishkin@...ux.intel.com>, Jiri Olsa
 <jolsa@...nel.org>, Ian Rogers <irogers@...gle.com>, Adrian Hunter
 <adrian.hunter@...el.com>, "Liang, Kan" <kan.liang@...ux.intel.com>, Dima
 Kogan <dima@...retsauce.net>, james.clark@...aro.org,
 linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 3/3] perf probe: Generate hash event for long symbol

On Fri, 11 Oct 2024 09:41:39 +0100
Leo Yan <leo.yan@....com> wrote:

> On 10/11/2024 4:07 AM, Masami Hiramatsu (Google) wrote:
> 
> [...]
> 
> >>> OK, personally, I recommend you to specify event name instead of generating
> >>> long event name in this case. But I understand sometimes this kind of feature
> >>> is good for someone.
> >>
> >> Sometimes, users try to add probe for long symbol and returns error, but there
> >>   have no clue for proceeding.
> > 
> > OK, no warning messsage is not good.
> > It should warn them to recommend adding it with their own event name too.
> 
> Okay, will do this in next spin.
> 
> >> Either we automatically generate a hashed name, or a guidance in the failure
> >> log for setting event name would be helpful. If you have concern for hashed
> >> name, maybe we can refine the log to give info for setting event name?
> > 
> > Yeah, I think this long event name is not useful for user to type.
> 
> Agreed.
> 
> >>> BTW, I would like to confirm. Can't we demangle the symbol name and parse it?
> >>
> >> I did test for C++ demangle symbols with the command:
> >>
> >>    perf probe -x /mnt/test_cpp_mangle -F --demangle
> >>
> >> The command doesn't work as I cannot see it output correctly for demangled
> >> symbols. But I don't look into details why this does not work at my side.
> > 
> > Oops, that is another issue to be solved.
> 
> After install libiberty, then I can see the tool can show demangled symbols.
> However, I found another issue for probing demangle symbol, please see details
> in below patch and let me know if makes sense.

Yeah, I think that will fixes a mangled symbol problem.
But I have one comment below.

> 
> ---8<---
> 
> From 3b09a6f89c7e383c6b1d2b7e6bd80c6bfa658d5b Mon Sep 17 00:00:00 2001
> From: Leo Yan <leo.yan@....com>
> Date: Fri, 11 Oct 2024 07:58:08 +0000
> Subject: [PATCH] perf probe: Correct demangled symbols in C++ program
> 
> An issue can be observed when probe C++ demangled symbol with steps:
> 
>   # nm test_cpp_mangle | grep print_data
>     0000000000000c94 t _GLOBAL__sub_I__Z10print_datai
>     0000000000000afc T _Z10print_datai
>     0000000000000b38 T _Z10print_dataR5Point
> 
>   # ./perf probe -x /home/niayan01/test_cpp_mangle -F --demangle
>     ...
>     print_data(Point&)
>     print_data(int)
>     ...
> 
>   # ./perf --debug verbose=3 probe -x test_cpp_mangle --add "test=print_data(int)"
>     probe-definition(0): test=print_data(int)
>     symbol:print_data(int) file:(null) line:0 offset:0 return:0 lazy:(null)
>     0 arguments
>     Open Debuginfo file: /home/niayan01/test_cpp_mangle
>     Try to find probe point from debuginfo.
>     Symbol print_data(int) address found : afc
>     Matched function: print_data [2ccf]
>     Probe point found: print_data+0
>     Found 1 probe_trace_events.
>     Opening /sys/kernel/tracing//uprobe_events write=1
>     Opening /sys/kernel/tracing//README write=0
>     Writing event: p:probe_test_cpp_mangle/test /home/niayan01/test_cpp_mangle:0xb38
>     ...
> 
> When tried to probe symbol "print_data(int)", the log show:
> 
>     Symbol print_data(int) address found : afc
> 
> The found address is 0xafc - which is right if we connect with the
> output result from nm. Afterwards when write event, the command uses
> offset 0xb38 in the last log, which is a wrong address.
> 
> The dwarf_diename() gets a common function name, in above case, it
> returns string "print_data". As a result, the tool parses the offset
> based on the common name. This leads to probe at the wrong symbol
> "print_data(Point&)".
> 
> To fix the issue, use the die_get_linkage_name() function to retrieve
> the distinct linkage name - this is the mangled name for the C++ case.
> Based on this unique name, the tool can get a correct offset for
> probing. Based on DWARF doc, it is possible the linkage name is missed
> in the DIE, it rolls back to use dwarf_diename().

Can you add the result after applying this patch here?

Thank you,

> 
> Signed-off-by: Leo Yan <leo.yan@....com>
> ---
>  tools/perf/util/probe-finder.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/util/probe-finder.c b/tools/perf/util/probe-finder.c
> index 630e16c54ed5..28a14005fc1f 100644
> --- a/tools/perf/util/probe-finder.c
> +++ b/tools/perf/util/probe-finder.c
> @@ -1583,8 +1583,21 @@ int debuginfo__find_probe_point(struct debuginfo *dbg, u64 addr,
> 
>         /* Find a corresponding function (name, baseline and baseaddr) */
>         if (die_find_realfunc(&cudie, (Dwarf_Addr)addr, &spdie)) {
> -               /* Get function entry information */
> -               func = basefunc = dwarf_diename(&spdie);
> +               /*
> +                * Get function entry information.
> +                *
> +                * As described in the document DWARF Debugging Information
> +                * Format Version 5, section 2.22 Linkage Names, "mangled names,
> +                * are used in various ways, ... to distinguish multiple
> +                * entities that have the same name".
> +                *
> +                * Firstly try to get distinct linkage name, if fail then
> +                * rollback to get associated name in DIE.
> +                */
> +               func = basefunc = die_get_linkage_name(&spdie);
> +               if (!func)
> +                        func = basefunc = dwarf_diename(&spdie);
> +
>                 if (!func ||
>                     die_entrypc(&spdie, &baseaddr) != 0 ||
>                     dwarf_decl_line(&spdie, &baseline) != 0) {
> --
> 2.25.1
> 
> 


-- 
Masami Hiramatsu (Google) <mhiramat@...nel.org>

Powered by blists - more mailing lists