On Tue, May 17, 2022 at 11:20:03AM +0100, James Clark wrote:
> Architectures can detect availability of extra registers at
> runtime so use this more complete set for unwinding. This
> will include the VG register on arm64 in a later commit.
>
> If the function isn't implemented then PERF_REGS_MASK is
> returned and there is no change.
>
> Signed-off-by: James Clark <[email protected]>
This patch looks good to me:
Reviewed-by: Leo Yan <[email protected]>
Just curious, do you think should update the test (e.g.
arch/arm64/tests/dwarf-unwind.c) to use arch__user_reg_mask()?
Thanks,
Leo
> ---
> tools/perf/util/evsel.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index 5fd7924f8eb3..787bbcbcd2ae 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -896,7 +896,7 @@ static void __evsel__config_callchain(struct evsel *evsel, struct record_opts *o
> "specifying a subset with --user-regs may render DWARF unwinding unreliable, "
> "so the minimal registers set (IP, SP) is explicitly forced.\n");
> } else {
> - attr->sample_regs_user |= PERF_REGS_MASK;
> + attr->sample_regs_user |= arch__user_reg_mask();
> }
> attr->sample_stack_user = param->dump_size;
> attr->exclude_callchain_user = 1;
> --
> 2.28.0
>
On 17/05/2022 12:03, Leo Yan wrote:
> On Tue, May 17, 2022 at 11:20:03AM +0100, James Clark wrote:
>> Architectures can detect availability of extra registers at
>> runtime so use this more complete set for unwinding. This
>> will include the VG register on arm64 in a later commit.
>>
>> If the function isn't implemented then PERF_REGS_MASK is
>> returned and there is no change.
>>
>> Signed-off-by: James Clark <[email protected]>
>
> This patch looks good to me:
> Reviewed-by: Leo Yan <[email protected]>
>
> Just curious, do you think should update the test (e.g.
> arch/arm64/tests/dwarf-unwind.c) to use arch__user_reg_mask()?
I don't think so because the normal set of registers is manually
loaded in tools/perf/arch/arm64/tests/regs_load.S so it wouldn't
include this pseudo register. Also there is no SVE in the call
chain of the test so it would never have an effect.
I could add a new test for SVE, but it depends on getting the
libunwind changes through first so will have to come later.
Thanks,
James
>
> Thanks,
> Leo
>
>> ---
>> tools/perf/util/evsel.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
>> index 5fd7924f8eb3..787bbcbcd2ae 100644
>> --- a/tools/perf/util/evsel.c
>> +++ b/tools/perf/util/evsel.c
>> @@ -896,7 +896,7 @@ static void __evsel__config_callchain(struct evsel *evsel, struct record_opts *o
>> "specifying a subset with --user-regs may render DWARF unwinding unreliable, "
>> "so the minimal registers set (IP, SP) is explicitly forced.\n");
>> } else {
>> - attr->sample_regs_user |= PERF_REGS_MASK;
>> + attr->sample_regs_user |= arch__user_reg_mask();
>> }
>> attr->sample_stack_user = param->dump_size;
>> attr->exclude_callchain_user = 1;
>> --
>> 2.28.0
>>
On Wed, May 18, 2022 at 02:25:38PM +0100, James Clark wrote:
>
>
> On 17/05/2022 12:03, Leo Yan wrote:
> > On Tue, May 17, 2022 at 11:20:03AM +0100, James Clark wrote:
> >> Architectures can detect availability of extra registers at
> >> runtime so use this more complete set for unwinding. This
> >> will include the VG register on arm64 in a later commit.
> >>
> >> If the function isn't implemented then PERF_REGS_MASK is
> >> returned and there is no change.
> >>
> >> Signed-off-by: James Clark <[email protected]>
> >
> > This patch looks good to me:
> > Reviewed-by: Leo Yan <[email protected]>
> >
> > Just curious, do you think should update the test (e.g.
> > arch/arm64/tests/dwarf-unwind.c) to use arch__user_reg_mask()?
>
> I don't think so because the normal set of registers is manually
> loaded in tools/perf/arch/arm64/tests/regs_load.S so it wouldn't
> include this pseudo register. Also there is no SVE in the call
> chain of the test so it would never have an effect.
>
> I could add a new test for SVE, but it depends on getting the
> libunwind changes through first so will have to come later.
It makes sense to add new SVE test later, thanks for clarification.
So this patch series looks good for me.
Leo