lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  PHC 
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Thu, 30 May 2019 19:24:57 +0300
From:   Alexey Budankov <>
To:     Arnaldo Carvalho de Melo <>
Cc:     Jiri Olsa <>, Namhyung Kim <>,
        Alexander Shishkin <>,
        Peter Zijlstra <>,
        Ingo Molnar <>,
        Andi Kleen <>,
        linux-kernel <>
Subject: Re: [PATCH v4] perf record: collect user registers set jointly with
 dwarf stacks

On 30.05.2019 16:13, Arnaldo Carvalho de Melo wrote:
> Em Thu, May 30, 2019 at 11:24:49AM +0300, Alexey Budankov escreveu:
>> On 29.05.2019 22:25, Arnaldo Carvalho de Melo wrote:
>>> Em Wed, May 29, 2019 at 05:30:49PM +0300, Alexey Budankov escreveu:
>> <SNIP>
>>>> +++ b/tools/perf/util/evsel.c
>>>> +#define DWARF_REGS_MASK ((1ULL << PERF_REG_IP) | \
>>>> +			 (1ULL << PERF_REG_SP))
>>>> +
>>>>  static void __perf_evsel__config_callchain(struct perf_evsel *evsel,
>>>>  					   struct record_opts *opts,
>>>>  					   struct callchain_param *param)
>>>> @@ -702,7 +705,13 @@ static void __perf_evsel__config_callchain(struct perf_evsel *evsel,
>>>>  		if (!function) {
>>>>  			perf_evsel__set_sample_bit(evsel, REGS_USER);
>>>>  			perf_evsel__set_sample_bit(evsel, STACK_USER);
>>>> -			attr->sample_regs_user |= PERF_REGS_MASK;
>>>> +			if (opts->sample_user_regs) {
>>> Where are you checking that opts->sample_user_regs doesn't have either
>>> IP or SP?
>> Sure. The the intention was to avoid such a complication, merge two 
>> masks and provide explicit warning that the resulting mask is extended.
> s/is/may be/g
>> If you still see the checking and auto detection of the exact mask 
>> extension as essential it can be implemented.
> perf, tracing, systems internals, etc are super complicated, full of
> details, the more precise we can make the messages, the better.
>>> So, __perf_evsel__config_callchain its the routine that sets up the
>>> attr->sample_regs_user when callchains are asked for, and what was it
>>> doing? Asking for _all_ user regs, right?
>>> I.e. what you're saying is that when --callgraph-dwarf is asked for,
>>> then only IP and BP are needed, and we should stop doing that, so that
>>> would be a first patch, if that is the case. I.e. a patch that doesn't
>>> even mention opts->sample_user_regs.
>>> Then, a second patch would fix the opt->sample_user_regs request clash
>>> with --callgraph dwarf, i.e. it would do something like:
>>> 	      if ((opts->sample_regs_user & DWARF_REGS_MASK) != DWARF_REGS_MASK) {
>>> 	      		char * ip = (opts->sample_regs_user & (1ULL << PERF_REG_IP)) ? NULL : "IP",
>>> 	      		     * sp = (opts->sample_regs_user & (1ULL << PERF_REG_SP)) ? NULL : "SP",
>>> 			     * all = (!ip && !sp) ?  "s" : "";
>>> 			pr_warning("WARNING: specified --user-regs register set doesn't include register%s "
>>> 				   "needed by also specified --call-graph=dwarf, auto adding %s%s%s register%s.\n",
>>> 				   all, ip, all : ", " : "", sp, all);
>>> 		}
>>> This if and only if all the registers that are needed to do DWARF
>>> unwinding are just IP and BP, which doesn't look like its true, since
>>> when no --user_regs is set (i.e. opts->user_regs is not set) then we
>>> continue asking for PERF_REGS_MASK...
>>> Can you check where I'm missing something?
>> 1.  -g call-graph dwarf,K                         full_regs
>> 2.  --user-regs=user_regs                         user_regs
>> 3.  -g call-graph dwarf,K --user-regs=user_regs	  user_regs + dwarf_regs
>> The default behavior stays the same for cases 1, 2 above.
>> For case 3 register set becomes the one asked using --user_regs option.
>> If the option value misses IP or SP or the both then they are explicitly
>> added to the option value and a warning message mentioning the exact 
>> added registers is provided.
>>> Jiri DWARF unwind uses just IP and SP? Looking at
>>> tools/perf/util/unwind-libunwind-local.c's access_reg() I don't think
>>> so, right?
>> If you ask me, AFAIK, DWARF unwind rules sometimes can refer additional 
>> general purpose registers for frames boundaries calculation.
> :-) So that DWARF_REGS is misleading, should be something like
> DWARF_MINIMAL_REGS, as we may need other registers, so the original code
> was correct, right?

Right. Actually came to the same conclusion with the same naming for IP,SP mask :)

> After all if the user asks for both --call-graph dwarf and --user-regs,
> then probably we should require --force? I.e. the message then would be:
> "
> WARNING: The use of --call-graph=dwarf may require all the user
> registers, specifying a subset with --user-regs may render DWARF
> unwinding unreliable, please use --force if you're sure that the subset
> specified via --user-regs is enough for your specific use case.
> "
> And then plain refuse, if the user _really_ wants it, then we have
> --force/-f for those cases.
> Does this sound better?

If --user-regs is specified jointly with --call-graph dwarf option then
--user-regs already serves as the --force and, IMHO, a warning does the best.

The ideal solution, I could imagine, is to also dynamically calculate regs 
set extension and provide it in the warning, but it is only for two registers.

So, if --call-graph dwarf --user-regs=A,B,C are specified jointly then
WARNING: The use of --call-graph=dwarf may require all the user registers, 
specifying a subset with --user-regs may render DWARF unwinding unreliable,
so the minimal registers set (IP, SP) is explicitly forced.
The message is precise and it would fit the majority of use cases.
Final decision is up to you.


> - Arnaldo

Powered by blists - more mailing lists