Re: [PATCH v1 2/5] perf parse-regs: Introduce functions arch__reg_{ip|sp}()

[James Clark]

On 22/05/2023 13:07, Leo Yan wrote:
> On Mon, May 22, 2023 at 09:57:25AM +0100, James Clark wrote:
>>
>>
>> On 20/05/2023 03:55, Leo Yan wrote:
>>> Ideally, we want util/perf_regs.c to be general enough and doesn't bind
>>> with specific architecture.
>>>
>>> But since util/perf_regs.c uses the macros PERF_REG_IP and PERF_REG_SP
>>> which are defined by architecture, thus util/perf_regs.c is dependent on
>>> architecture header (see util/perf_regs.h includes "<perf_regs.h>", here
>>> perf_regs.h is architecture specific header).
>>>
>>> As a step to generalize util/perf_regs.c, this commit introduces weak
>>> functions arch__reg_ip() and arch__reg_sp() and every architecture can
>>> define their own functions; thus, util/perf_regs.c doesn't need to use
>>> PERF_REG_IP and PERF_REG_SP anymore.
>>>
>>> This is a preparation to get rid of architecture specific header from
>>> util/perf_regs.h.
>>>
>>> Signed-off-by: Leo Yan <leo.yan@xxxxxxxxxx>
>>> ---
>> [...]
>>>  
>>> -#define DWARF_MINIMAL_REGS ((1ULL << PERF_REG_IP) | (1ULL << PERF_REG_SP))
>>> +#define DWARF_MINIMAL_REGS ((1ULL << arch__reg_ip()) | (1ULL << arch__reg_sp()))
>>>  
>>>  const char *perf_reg_name(int id, const char *arch);
>>>  int perf_reg_value(u64 *valp, struct regs_dump *regs, int id);
>>> diff --git a/tools/perf/util/unwind-libdw.c b/tools/perf/util/unwind-libdw.c
>>> index bdccfc511b7e..f308f2ea512b 100644
>>> --- a/tools/perf/util/unwind-libdw.c
>>> +++ b/tools/perf/util/unwind-libdw.c
>>> @@ -252,7 +252,7 @@ int unwind__get_entries(unwind_entry_cb_t cb, void *arg,
>>>  	if (!ui->dwfl)
>>>  		goto out;
>>>  
>>> -	err = perf_reg_value(&ip, &data->user_regs, PERF_REG_IP);
>>> +	err = perf_reg_value(&ip, &data->user_regs, arch__reg_ip());
>>
>> Shouldn't it be more like this, because the weak symbols are a compile
>> time thing and it's supposed to support cross arch unwinding at runtime
>> (assuming something containing the arch from the file is passed down,
>> like we did with perf_reg_name()):
>>
>>   char *arch = perf_env__arch(evsel__env(evsel));
>>   err = perf_reg_value(&ip, &data->user_regs, arch__reg_ip(arch));
> 
> Thanks for pointing out, James.
> 
> Agreed that we need to return the IP and SP register based on the
> arch.  I will look into more details and spin for a new patch set for
> this.
> 

You might be able to skip the extra work for now though, seeing as your
change is no worse than it was before and it fixes the duplicate
declaration issue.

>> Now I'm wondering how cross unwinding ever worked because I see
>> libunwind also has something hard coded too:
>>
>>   #define LIBUNWIND__ARCH_REG_SP PERF_REG_SP
> 
> Yeah, I also used arch__reg_sp() to replace PERF_REG_SP; but as you
> suggestion, we should fix this with passing 'arch' parameter for
> getting SP register based on arch.
> 
> Another important thing is to find a good test for cross unwinding.
> Maybe I can use tools/perf/tests/shell/record.sh, function
> test_register_capture() for testing registers, if you have any other
> suggesion, please let me know.

The only way I can think is to have pre-recorded perf.data files in the
repo, but they'd also need the binary from the recording too. I think
this is probably not a very good idea because it's going to make the
repo huge if we keep changing them (which we will).

Some kind of third party perf test suite hosted somewhere might work but
I don't think this exists.

> 
> Thanks,
> Leo