subject:"Re\: \[PATCH v2 1\/3\] perf tools\: Use offset instead of dwarfnum in register table."

Re: [PATCH v2 1/3] perf tools: Use offset instead of dwarfnum in register table.

2017-02-07 Thread Masami Hiramatsu

On Tue, 7 Feb 2017 09:20:03 +
Will Deacon  wrote:

> On Tue, Feb 07, 2017 at 09:54:51AM +0900, Masami Hiramatsu wrote:
> > On Mon, 6 Feb 2017 13:02:29 +
> > Will Deacon  wrote:
> > 
> > > On Sat, Feb 04, 2017 at 05:03:20PM +0800, Hekuang wrote:
> > > > hi
> > > > 
> > > > 在 2017/2/3 21:00, Will Deacon 写道:
> > > > >On Fri, Feb 03, 2017 at 11:06:05AM +, 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 
> > > > >>Signed-off-by: He Kuang 
> > > > >>---
> > > > >>  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 
> > > > >>+#include  /* for struct user_pt_regs */
> > > > >>  #include 
> > > > >>-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 think it's undefined from the C standard perspective.
> > 
> > Actually, this '%sp' is used for kprobes/uprobes dynamic events, which only 
> > depend on how regs_query_register_offset()@arch/arm64/kernel/ptrace.c is 
> > implemented.
> > And also, since perf-probe uses debuginfo, it can find out the base 
> > register.
> > 
> > So we don't need to care the C standard in this file :)
> 
> Up to the perf tool maintainers really, but I expect it will irritate
> anybody running UBSAN and it's really not difficult to fix.

Hmm, sorry, I'm not sure, what the point UBSAN user be irriteted?
And what is that related to this story?

Thank you,

-- 
Masami Hiramatsu

Re: [PATCH v2 1/3] perf tools: Use offset instead of dwarfnum in register table.

2017-02-07 Thread Masami Hiramatsu

On Tue, 7 Feb 2017 09:20:03 +
Will Deacon  wrote:

> On Tue, Feb 07, 2017 at 09:54:51AM +0900, Masami Hiramatsu wrote:
> > On Mon, 6 Feb 2017 13:02:29 +
> > Will Deacon  wrote:
> > 
> > > On Sat, Feb 04, 2017 at 05:03:20PM +0800, Hekuang wrote:
> > > > hi
> > > > 
> > > > 在 2017/2/3 21:00, Will Deacon 写道:
> > > > >On Fri, Feb 03, 2017 at 11:06:05AM +, 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 
> > > > >>Signed-off-by: He Kuang 
> > > > >>---
> > > > >>  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 
> > > > >>+#include  /* for struct user_pt_regs */
> > > > >>  #include 
> > > > >>-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 think it's undefined from the C standard perspective.
> > 
> > Actually, this '%sp' is used for kprobes/uprobes dynamic events, which only 
> > depend on how regs_query_register_offset()@arch/arm64/kernel/ptrace.c is 
> > implemented.
> > And also, since perf-probe uses debuginfo, it can find out the base 
> > register.
> > 
> > So we don't need to care the C standard in this file :)
> 
> Up to the perf tool maintainers really, but I expect it will irritate
> anybody running UBSAN and it's really not difficult to fix.

Hmm, sorry, I'm not sure, what the point UBSAN user be irriteted?
And what is that related to this story?

Thank you,

-- 
Masami Hiramatsu

Re: [PATCH v2 1/3] perf tools: Use offset instead of dwarfnum in register table.

2017-02-07 Thread Will Deacon

On Tue, Feb 07, 2017 at 09:54:51AM +0900, Masami Hiramatsu wrote:
> On Mon, 6 Feb 2017 13:02:29 +
> Will Deacon  wrote:
> 
> > On Sat, Feb 04, 2017 at 05:03:20PM +0800, Hekuang wrote:
> > > hi
> > > 
> > > 在 2017/2/3 21:00, Will Deacon 写道:
> > > >On Fri, Feb 03, 2017 at 11:06:05AM +, 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 
> > > >>Signed-off-by: He Kuang 
> > > >>---
> > > >>  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 
> > > >>+#include  /* for struct user_pt_regs */
> > > >>  #include 
> > > >>-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 think it's undefined from the C standard perspective.
> 
> Actually, this '%sp' is used for kprobes/uprobes dynamic events, which only 
> depend on how regs_query_register_offset()@arch/arm64/kernel/ptrace.c is 
> implemented.
> And also, since perf-probe uses debuginfo, it can find out the base register.
> 
> So we don't need to care the C standard in this file :)

Up to the perf tool maintainers really, but I expect it will irritate
anybody running UBSAN and it's really not difficult to fix.

Will

Re: [PATCH v2 1/3] perf tools: Use offset instead of dwarfnum in register table.

2017-02-07 Thread Will Deacon

On Tue, Feb 07, 2017 at 09:54:51AM +0900, Masami Hiramatsu wrote:
> On Mon, 6 Feb 2017 13:02:29 +
> Will Deacon  wrote:
> 
> > On Sat, Feb 04, 2017 at 05:03:20PM +0800, Hekuang wrote:
> > > hi
> > > 
> > > 在 2017/2/3 21:00, Will Deacon 写道:
> > > >On Fri, Feb 03, 2017 at 11:06:05AM +, 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 
> > > >>Signed-off-by: He Kuang 
> > > >>---
> > > >>  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 
> > > >>+#include  /* for struct user_pt_regs */
> > > >>  #include 
> > > >>-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 think it's undefined from the C standard perspective.
> 
> Actually, this '%sp' is used for kprobes/uprobes dynamic events, which only 
> depend on how regs_query_register_offset()@arch/arm64/kernel/ptrace.c is 
> implemented.
> And also, since perf-probe uses debuginfo, it can find out the base register.
> 
> So we don't need to care the C standard in this file :)

Up to the perf tool maintainers really, but I expect it will irritate
anybody running UBSAN and it's really not difficult to fix.

Will

Re: [PATCH v2 1/3] perf tools: Use offset instead of dwarfnum in register table.

2017-02-06 Thread Masami Hiramatsu

On Mon, 6 Feb 2017 13:02:29 +
Will Deacon  wrote:

> On Sat, Feb 04, 2017 at 05:03:20PM +0800, Hekuang wrote:
> > hi
> > 
> > 在 2017/2/3 21:00, Will Deacon 写道:
> > >On Fri, Feb 03, 2017 at 11:06:05AM +, 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 
> > >>Signed-off-by: He Kuang 
> > >>---
> > >>  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 
> > >>+#include  /* for struct user_pt_regs */
> > >>  #include 
> > >>-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 think it's undefined from the C standard perspective.

Actually, this '%sp' is used for kprobes/uprobes dynamic events, which only 
depend on how regs_query_register_offset()@arch/arm64/kernel/ptrace.c is 
implemented.
And also, since perf-probe uses debuginfo, it can find out the base register.

So we don't need to care the C standard in this file :)

Thank you,

-- 
Masami Hiramatsu

Re: [PATCH v2 1/3] perf tools: Use offset instead of dwarfnum in register table.

2017-02-06 Thread Masami Hiramatsu

On Mon, 6 Feb 2017 13:02:29 +
Will Deacon  wrote:

> On Sat, Feb 04, 2017 at 05:03:20PM +0800, Hekuang wrote:
> > hi
> > 
> > 在 2017/2/3 21:00, Will Deacon 写道:
> > >On Fri, Feb 03, 2017 at 11:06:05AM +, 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 
> > >>Signed-off-by: He Kuang 
> > >>---
> > >>  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 
> > >>+#include  /* for struct user_pt_regs */
> > >>  #include 
> > >>-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 think it's undefined from the C standard perspective.

Actually, this '%sp' is used for kprobes/uprobes dynamic events, which only 
depend on how regs_query_register_offset()@arch/arm64/kernel/ptrace.c is 
implemented.
And also, since perf-probe uses debuginfo, it can find out the base register.

So we don't need to care the C standard in this file :)

Thank you,

-- 
Masami Hiramatsu

Re: [PATCH v2 1/3] perf tools: Use offset instead of dwarfnum in register table.

2017-02-06 Thread Will Deacon

On Sat, Feb 04, 2017 at 05:03:20PM +0800, Hekuang wrote:
> hi
> 
> 在 2017/2/3 21:00, Will Deacon 写道:
> >On Fri, Feb 03, 2017 at 11:06:05AM +, 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 
> >>Signed-off-by: He Kuang 
> >>---
> >>  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 
> >>+#include  /* for struct user_pt_regs */
> >>  #include 
> >>-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 think it's undefined from the C standard perspective.

> >
> >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];
> };

We won't reorder a uapi structure because that would binary compatibility.

Just send a patch using the dwarfnum as the index for the offset calculation,
like I've been saying since you posted the first version.

Will

Re: [PATCH v2 1/3] perf tools: Use offset instead of dwarfnum in register table.

2017-02-06 Thread Will Deacon

On Sat, Feb 04, 2017 at 05:03:20PM +0800, Hekuang wrote:
> hi
> 
> 在 2017/2/3 21:00, Will Deacon 写道:
> >On Fri, Feb 03, 2017 at 11:06:05AM +, 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 
> >>Signed-off-by: He Kuang 
> >>---
> >>  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 
> >>+#include  /* for struct user_pt_regs */
> >>  #include 
> >>-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 think it's undefined from the C standard perspective.

> >
> >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];
> };

We won't reorder a uapi structure because that would binary compatibility.

Just send a patch using the dwarfnum as the index for the offset calculation,
like I've been saying since you posted the first version.

Will

Re: [PATCH v2 1/3] perf tools: Use offset instead of dwarfnum in register table.

2017-02-04 Thread Hekuang


hi

在 2017/2/3 21:00, Will Deacon 写道:

On Fri, Feb 03, 2017 at 11:06:05AM +, 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 
Signed-off-by: He Kuang 
---
  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 

+#include  /* for struct user_pt_regs */
  #include 
  
-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.

Re: [PATCH v2 1/3] perf tools: Use offset instead of dwarfnum in register table.

2017-02-04 Thread Hekuang


hi

在 2017/2/3 21:00, Will Deacon 写道:

On Fri, Feb 03, 2017 at 11:06:05AM +, 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 
Signed-off-by: He Kuang 
---
  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 

+#include  /* for struct user_pt_regs */
  #include 
  
-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.

Re: [PATCH v2 1/3] perf tools: Use offset instead of dwarfnum in register table.

2017-02-03 Thread Will Deacon

On Fri, Feb 03, 2017 at 11:06:05AM +, 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 
> Signed-off-by: He Kuang 
> ---
>  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 
> +#include  /* for struct user_pt_regs */
>  #include 
>  
> -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.

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

Re: [PATCH v2 1/3] perf tools: Use offset instead of dwarfnum in register table.

2017-02-03 Thread Will Deacon

On Fri, Feb 03, 2017 at 11:06:05AM +, 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 
> Signed-off-by: He Kuang 
> ---
>  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 
> +#include  /* for struct user_pt_regs */
>  #include 
>  
> -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.

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

Re: [PATCH v2 1/3] perf tools: Use offset instead of dwarfnum in register table.

Re: [PATCH v2 1/3] perf tools: Use offset instead of dwarfnum in register table.

Re: [PATCH v2 1/3] perf tools: Use offset instead of dwarfnum in register table.

Re: [PATCH v2 1/3] perf tools: Use offset instead of dwarfnum in register table.

Re: [PATCH v2 1/3] perf tools: Use offset instead of dwarfnum in register table.

Re: [PATCH v2 1/3] perf tools: Use offset instead of dwarfnum in register table.

Re: [PATCH v2 1/3] perf tools: Use offset instead of dwarfnum in register table.

Re: [PATCH v2 1/3] perf tools: Use offset instead of dwarfnum in register table.

Re: [PATCH v2 1/3] perf tools: Use offset instead of dwarfnum in register table.

Re: [PATCH v2 1/3] perf tools: Use offset instead of dwarfnum in register table.

Re: [PATCH v2 1/3] perf tools: Use offset instead of dwarfnum in register table.

Re: [PATCH v2 1/3] perf tools: Use offset instead of dwarfnum in register table.

12 matches

Site Navigation

Mail list logo

Footer information