lkml.org 
[lkml]   [2017]   [Feb]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v2 1/3] perf tools: Use offset instead of dwarfnum in register table.
From
Date
hi

在 2017/2/3 21:00, Will Deacon 写道:
> On Fri, Feb 03, 2017 at 11:06:05AM +0000, He Kuang wrote:
>> This patch changes the 'dwarfnum' to 'offset' in register table, so
>> the index of array becomes the dwarfnum (the index of each register
>> defined by DWARF) and the "offset" member means the byte-offset of the
>> register in (user_)pt_regs. This change makes the code consistent with
>> x86.
>>
>> Acked-by: Masami Hiramatsu <mhiramat@kernel.org>
>> Signed-off-by: He Kuang <hekuang@huawei.com>
>> ---
>> tools/perf/arch/arm64/util/dwarf-regs.c | 107 ++++++++++++++++----------------
>> 1 file changed, 52 insertions(+), 55 deletions(-)
> Thanks for splitting this up. Comment below.
>
>> diff --git a/tools/perf/arch/arm64/util/dwarf-regs.c b/tools/perf/arch/arm64/util/dwarf-regs.c
>> index d49efeb..090f36b 100644
>> --- a/tools/perf/arch/arm64/util/dwarf-regs.c
>> +++ b/tools/perf/arch/arm64/util/dwarf-regs.c
>> @@ -9,72 +9,69 @@
>> */
>>
>> #include <stddef.h>
>> +#include <linux/ptrace.h> /* for struct user_pt_regs */
>> #include <dwarf-regs.h>
>>
>> -struct pt_regs_dwarfnum {
>> +struct pt_regs_offset {
>> const char *name;
>> - unsigned int dwarfnum;
>> + int offset;
>> };
>>
>> -#define STR(s) #s
>> -#define REG_DWARFNUM_NAME(r, num) {.name = r, .dwarfnum = num}
>> -#define GPR_DWARFNUM_NAME(num) \
>> - {.name = STR(%x##num), .dwarfnum = num}
>> -#define REG_DWARFNUM_END {.name = NULL, .dwarfnum = 0}
>> -
>> /*
>> * Reference:
>> * http://infocenter.arm.com/help/topic/com.arm.doc.ihi0057b/IHI0057B_aadwarf64.pdf
>> */
>> -static const struct pt_regs_dwarfnum regdwarfnum_table[] = {
>> - GPR_DWARFNUM_NAME(0),
>> - GPR_DWARFNUM_NAME(1),
>> - GPR_DWARFNUM_NAME(2),
>> - GPR_DWARFNUM_NAME(3),
>> - GPR_DWARFNUM_NAME(4),
>> - GPR_DWARFNUM_NAME(5),
>> - GPR_DWARFNUM_NAME(6),
>> - GPR_DWARFNUM_NAME(7),
>> - GPR_DWARFNUM_NAME(8),
>> - GPR_DWARFNUM_NAME(9),
>> - GPR_DWARFNUM_NAME(10),
>> - GPR_DWARFNUM_NAME(11),
>> - GPR_DWARFNUM_NAME(12),
>> - GPR_DWARFNUM_NAME(13),
>> - GPR_DWARFNUM_NAME(14),
>> - GPR_DWARFNUM_NAME(15),
>> - GPR_DWARFNUM_NAME(16),
>> - GPR_DWARFNUM_NAME(17),
>> - GPR_DWARFNUM_NAME(18),
>> - GPR_DWARFNUM_NAME(19),
>> - GPR_DWARFNUM_NAME(20),
>> - GPR_DWARFNUM_NAME(21),
>> - GPR_DWARFNUM_NAME(22),
>> - GPR_DWARFNUM_NAME(23),
>> - GPR_DWARFNUM_NAME(24),
>> - GPR_DWARFNUM_NAME(25),
>> - GPR_DWARFNUM_NAME(26),
>> - GPR_DWARFNUM_NAME(27),
>> - GPR_DWARFNUM_NAME(28),
>> - GPR_DWARFNUM_NAME(29),
>> - REG_DWARFNUM_NAME("%lr", 30),
>> - REG_DWARFNUM_NAME("%sp", 31),
>> - REG_DWARFNUM_END,
>> -};
>> +#define REG_OFFSET_NAME(r, num) {.name = "%" #r, \
>> + .offset = offsetof(struct user_pt_regs, regs[num])}
> Whilst this works in practice, this is undefined behaviour for "sp", since
> you'll go off the end of the regs array.

It's not undefined behaviour here,
struct user_pt_regs {
__u64 regs[31];
__u64 sp;
__u64 pc;
__u64 pstate;
};
user_pt_regs->regs[31] is user_pt_regs->sp and the offset value is correct.
>
> I still think you're better off sticking with the dwarfnum, then just having
> a dwarfnum2offset macro that multiplies by the size of a register.
>
> Will
I think add a ptregs_offset field is more suitable and makes the code
indepent
to struct user_pt_regs layout, for example if the structure changed to this:

struct user_pt_regs {
__u64 sp;
__u64 pc;
__u64 pstate;
__u64 regs[31];
};

The multiply result will be incorrect.
Patch updated and the change is similar to commit "4679bccaa308"
(perf tools powerpc: Add support for generating bpf prologue)

Please review, thanks.


\
 
 \ /
  Last update: 2017-02-04 10:05    [W:0.128 / U:0.368 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site