From: Will Deacon <will.deacon@arm.com>
To: Hekuang <hekuang@huawei.com>
Cc: peterz@infradead.org, mingo@redhat.com, acme@kernel.org,
alexander.shishkin@linux.intel.com, mhiramat@kernel.org,
jolsa@redhat.com, wangnan0@huawei.com, bintian.wang@huawei.com,
linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v2 1/3] perf tools: Use offset instead of dwarfnum in register table.
Date: Mon, 6 Feb 2017 13:02:29 +0000 [thread overview]
Message-ID: <20170206130229.GI19939@arm.com> (raw)
In-Reply-To: <589598D8.8050304@huawei.com>
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 +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 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
next prev parent reply other threads:[~2017-02-06 13:02 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-02-03 11:06 [PATCH v2 1/3] perf tools: Use offset instead of dwarfnum in register table He Kuang
2017-02-03 11:06 ` [PATCH v2 2/3] perf tools: Enable bpf prologue for arm64 He Kuang
2017-02-03 11:06 ` [PATCH v2 3/3] perf tools: Add missing newline in debug messages He Kuang
2017-02-03 13:00 ` [PATCH v2 1/3] perf tools: Use offset instead of dwarfnum in register table Will Deacon
2017-02-04 9:03 ` [PATCH v3] perf tools arm64: Add support for generating bpf prologue He Kuang
2017-02-04 9:03 ` [PATCH v2 1/3] perf tools: Use offset instead of dwarfnum in register table Hekuang
2017-02-06 13:02 ` Will Deacon [this message]
2017-02-07 0:54 ` Masami Hiramatsu
2017-02-07 9:20 ` Will Deacon
2017-02-07 12:17 ` Masami Hiramatsu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20170206130229.GI19939@arm.com \
--to=will.deacon@arm.com \
--cc=acme@kernel.org \
--cc=alexander.shishkin@linux.intel.com \
--cc=bintian.wang@huawei.com \
--cc=hekuang@huawei.com \
--cc=jolsa@redhat.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mhiramat@kernel.org \
--cc=mingo@redhat.com \
--cc=peterz@infradead.org \
--cc=wangnan0@huawei.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).