From: Nick Desaulniers <ndesaulniers@google.com>
To: Nathan Huckleberry <nhuck15@gmail.com>
Cc: Russell King <linux@armlinux.org.uk>,
Andrew Morton <akpm@linux-foundation.org>,
Chunyan Zhang <zhang.lyra@gmail.com>,
clang-built-linux <clang-built-linux@googlegroups.com>,
Dmitry Safonov <0x7f454c46@gmail.com>,
Linux ARM <linux-arm-kernel@lists.infradead.org>,
LKML <linux-kernel@vger.kernel.org>,
linux-mediatek@lists.infradead.org,
Lvqiang Huang <lvqiang.huang@unisoc.com>,
Matthias Brugger <matthias.bgg@gmail.com>,
Miles Chen <miles.chen@mediatek.com>,
Nathan Huckleberry <nhuck@google.com>
Subject: Re: [PATCH 3/4] ARM: backtrace-clang: give labels more descriptive names
Date: Mon, 10 Aug 2020 15:32:32 -0700 [thread overview]
Message-ID: <CAKwvOdn+MGgYf8k9uAUT55vBL+ERTjv+jx+t8SD9HO98-h2c0w@mail.gmail.com> (raw)
In-Reply-To: <CAN=-RxstJBjJUcOf9iuAxEcxYUhJTdF9JhPVWwQuefnE+3s52w@mail.gmail.com>
On Thu, Aug 6, 2020 at 3:39 PM Nathan Huckleberry <nhuck15@gmail.com> wrote:
>
> The style cleanup looks great. I just have one extra thing that
> can probably be thrown into this patch.
>
> On Thu, Jul 30, 2020 at 3:51 PM Nick Desaulniers
> <ndesaulniers@google.com> wrote:
> >
> > Removes the 1004 label; it was neither a control flow target, nor an
> > instruction we expect to produce a fault.
> >
> > Gives the labels slightly more readable names. The `b` suffixes are
> > handy to disambiguate between labels of the same identifier when there's
> > more than one. Since these labels are unique, let's just give them
> > names.
> >
> > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> > ---
> > arch/arm/lib/backtrace-clang.S | 22 ++++++++++------------
> > 1 file changed, 10 insertions(+), 12 deletions(-)
> >
> > diff --git a/arch/arm/lib/backtrace-clang.S b/arch/arm/lib/backtrace-clang.S
> > index 40eb2215eaf4..7dad2a6843a5 100644
> > --- a/arch/arm/lib/backtrace-clang.S
> > +++ b/arch/arm/lib/backtrace-clang.S
> > @@ -121,8 +121,8 @@ for_each_frame: tst frame, mask @ Check for address exceptions
> > * start. This value gets updated to be the function start later if it is
> > * possible.
> > */
> > -1001: ldr sv_pc, [frame, #4] @ get saved 'pc'
> > -1002: ldr sv_fp, [frame, #0] @ get saved fp
> > +load_pc: ldr sv_pc, [frame, #4] @ get saved 'pc'
> > +load_fp: ldr sv_fp, [frame, #0] @ get saved fp
> >
> > teq sv_fp, mask @ make sure next frame exists
> > beq no_frame
> > @@ -142,7 +142,7 @@ for_each_frame: tst frame, mask @ Check for address exceptions
> > * registers for the current function, but the stacktrace is still printed
> > * properly.
> > */
> > -1003: ldr sv_lr, [sv_fp, #4] @ get saved lr from next frame
> > +load_lr: ldr sv_lr, [sv_fp, #4] @ get saved lr from next frame
> >
> > tst sv_lr, #0 @ If there's no previous lr,
> > beq finished_setup @ we're done.
> > @@ -166,8 +166,7 @@ finished_setup:
> > /*
> > * Print the function (sv_pc) and where it was called from (sv_lr).
> > */
> > -1004: mov r0, sv_pc
> > -
> > + mov r0, sv_pc
> > mov r1, sv_lr
> > mov r2, frame
> > bic r1, r1, mask @ mask PC/LR for the mode
> > @@ -182,7 +181,7 @@ finished_setup:
> > * pointer the comparison will fail and no registers will print. Unwinding will
> > * continue as if there had been no registers stored in this frame.
> > */
> > -1005: ldr r1, [sv_pc, #0] @ if stmfd sp!, {..., fp, lr}
> > +load_stmfd: ldr r1, [sv_pc, #0] @ if stmfd sp!, {..., fp, lr}
> > ldr r3, .Lopcode @ instruction exists,
> > teq r3, r1, lsr #11
> > ldr r0, [frame] @ locals are stored in
> > @@ -201,7 +200,7 @@ finished_setup:
> > mov frame, sv_fp @ above the current frame
> > bhi for_each_frame
> >
> > -1006: adr r0, .Lbad
> > +bad_frame: adr r0, .Lbad
> > mov r1, loglvl
> > mov r2, frame
> > bl printk
> > @@ -216,11 +215,10 @@ bad_lr: mov sv_fp, #0
> > ENDPROC(c_backtrace)
> > .pushsection __ex_table,"a"
> > .align 3
> > - .long 1001b, 1006b
> > - .long 1002b, 1006b
> > - .long 1003b, 1006b
> > - .long 1004b, 1006b
> > - .long 1005b, 1006b
> > + .long load_pc, bad_frame
> > + .long load_fp, bad_frame
> > + .long load_lr, bad_frame
> > + .long load_stmfd, bad_frame
>
> Load_stmfd should get its own fixup
> handler since it should emit errors about a bad
> pc, not a bad frame pointer.
Yeah, I can add that. It's a little orthogonal though to this patch
that renames labels. I'd consider more so a pre-existing bug. Let me
add a patch to the series that gives it a new fixup handler, separate
from the label renaming, in a v2 of the series.
>
> > .long prev_call, bad_lr
> > .popsection
> >
> > --
> > 2.28.0.163.g6104cc2f0b6-goog
> >
--
Thanks,
~Nick Desaulniers
WARNING: multiple messages have this Message-ID (diff)
From: Nick Desaulniers <ndesaulniers@google.com>
To: Nathan Huckleberry <nhuck15@gmail.com>
Cc: Chunyan Zhang <zhang.lyra@gmail.com>,
Dmitry Safonov <0x7f454c46@gmail.com>,
Russell King <linux@armlinux.org.uk>,
LKML <linux-kernel@vger.kernel.org>,
clang-built-linux <clang-built-linux@googlegroups.com>,
Miles Chen <miles.chen@mediatek.com>,
linux-mediatek@lists.infradead.org,
Matthias Brugger <matthias.bgg@gmail.com>,
Andrew Morton <akpm@linux-foundation.org>,
Nathan Huckleberry <nhuck@google.com>,
Lvqiang Huang <lvqiang.huang@unisoc.com>,
Linux ARM <linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH 3/4] ARM: backtrace-clang: give labels more descriptive names
Date: Mon, 10 Aug 2020 15:32:32 -0700 [thread overview]
Message-ID: <CAKwvOdn+MGgYf8k9uAUT55vBL+ERTjv+jx+t8SD9HO98-h2c0w@mail.gmail.com> (raw)
In-Reply-To: <CAN=-RxstJBjJUcOf9iuAxEcxYUhJTdF9JhPVWwQuefnE+3s52w@mail.gmail.com>
On Thu, Aug 6, 2020 at 3:39 PM Nathan Huckleberry <nhuck15@gmail.com> wrote:
>
> The style cleanup looks great. I just have one extra thing that
> can probably be thrown into this patch.
>
> On Thu, Jul 30, 2020 at 3:51 PM Nick Desaulniers
> <ndesaulniers@google.com> wrote:
> >
> > Removes the 1004 label; it was neither a control flow target, nor an
> > instruction we expect to produce a fault.
> >
> > Gives the labels slightly more readable names. The `b` suffixes are
> > handy to disambiguate between labels of the same identifier when there's
> > more than one. Since these labels are unique, let's just give them
> > names.
> >
> > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> > ---
> > arch/arm/lib/backtrace-clang.S | 22 ++++++++++------------
> > 1 file changed, 10 insertions(+), 12 deletions(-)
> >
> > diff --git a/arch/arm/lib/backtrace-clang.S b/arch/arm/lib/backtrace-clang.S
> > index 40eb2215eaf4..7dad2a6843a5 100644
> > --- a/arch/arm/lib/backtrace-clang.S
> > +++ b/arch/arm/lib/backtrace-clang.S
> > @@ -121,8 +121,8 @@ for_each_frame: tst frame, mask @ Check for address exceptions
> > * start. This value gets updated to be the function start later if it is
> > * possible.
> > */
> > -1001: ldr sv_pc, [frame, #4] @ get saved 'pc'
> > -1002: ldr sv_fp, [frame, #0] @ get saved fp
> > +load_pc: ldr sv_pc, [frame, #4] @ get saved 'pc'
> > +load_fp: ldr sv_fp, [frame, #0] @ get saved fp
> >
> > teq sv_fp, mask @ make sure next frame exists
> > beq no_frame
> > @@ -142,7 +142,7 @@ for_each_frame: tst frame, mask @ Check for address exceptions
> > * registers for the current function, but the stacktrace is still printed
> > * properly.
> > */
> > -1003: ldr sv_lr, [sv_fp, #4] @ get saved lr from next frame
> > +load_lr: ldr sv_lr, [sv_fp, #4] @ get saved lr from next frame
> >
> > tst sv_lr, #0 @ If there's no previous lr,
> > beq finished_setup @ we're done.
> > @@ -166,8 +166,7 @@ finished_setup:
> > /*
> > * Print the function (sv_pc) and where it was called from (sv_lr).
> > */
> > -1004: mov r0, sv_pc
> > -
> > + mov r0, sv_pc
> > mov r1, sv_lr
> > mov r2, frame
> > bic r1, r1, mask @ mask PC/LR for the mode
> > @@ -182,7 +181,7 @@ finished_setup:
> > * pointer the comparison will fail and no registers will print. Unwinding will
> > * continue as if there had been no registers stored in this frame.
> > */
> > -1005: ldr r1, [sv_pc, #0] @ if stmfd sp!, {..., fp, lr}
> > +load_stmfd: ldr r1, [sv_pc, #0] @ if stmfd sp!, {..., fp, lr}
> > ldr r3, .Lopcode @ instruction exists,
> > teq r3, r1, lsr #11
> > ldr r0, [frame] @ locals are stored in
> > @@ -201,7 +200,7 @@ finished_setup:
> > mov frame, sv_fp @ above the current frame
> > bhi for_each_frame
> >
> > -1006: adr r0, .Lbad
> > +bad_frame: adr r0, .Lbad
> > mov r1, loglvl
> > mov r2, frame
> > bl printk
> > @@ -216,11 +215,10 @@ bad_lr: mov sv_fp, #0
> > ENDPROC(c_backtrace)
> > .pushsection __ex_table,"a"
> > .align 3
> > - .long 1001b, 1006b
> > - .long 1002b, 1006b
> > - .long 1003b, 1006b
> > - .long 1004b, 1006b
> > - .long 1005b, 1006b
> > + .long load_pc, bad_frame
> > + .long load_fp, bad_frame
> > + .long load_lr, bad_frame
> > + .long load_stmfd, bad_frame
>
> Load_stmfd should get its own fixup
> handler since it should emit errors about a bad
> pc, not a bad frame pointer.
Yeah, I can add that. It's a little orthogonal though to this patch
that renames labels. I'd consider more so a pre-existing bug. Let me
add a patch to the series that gives it a new fixup handler, separate
from the label renaming, in a v2 of the series.
>
> > .long prev_call, bad_lr
> > .popsection
> >
> > --
> > 2.28.0.163.g6104cc2f0b6-goog
> >
--
Thanks,
~Nick Desaulniers
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek
WARNING: multiple messages have this Message-ID (diff)
From: Nick Desaulniers <ndesaulniers@google.com>
To: Nathan Huckleberry <nhuck15@gmail.com>
Cc: Chunyan Zhang <zhang.lyra@gmail.com>,
Dmitry Safonov <0x7f454c46@gmail.com>,
Russell King <linux@armlinux.org.uk>,
LKML <linux-kernel@vger.kernel.org>,
clang-built-linux <clang-built-linux@googlegroups.com>,
Miles Chen <miles.chen@mediatek.com>,
linux-mediatek@lists.infradead.org,
Matthias Brugger <matthias.bgg@gmail.com>,
Andrew Morton <akpm@linux-foundation.org>,
Nathan Huckleberry <nhuck@google.com>,
Lvqiang Huang <lvqiang.huang@unisoc.com>,
Linux ARM <linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH 3/4] ARM: backtrace-clang: give labels more descriptive names
Date: Mon, 10 Aug 2020 15:32:32 -0700 [thread overview]
Message-ID: <CAKwvOdn+MGgYf8k9uAUT55vBL+ERTjv+jx+t8SD9HO98-h2c0w@mail.gmail.com> (raw)
In-Reply-To: <CAN=-RxstJBjJUcOf9iuAxEcxYUhJTdF9JhPVWwQuefnE+3s52w@mail.gmail.com>
On Thu, Aug 6, 2020 at 3:39 PM Nathan Huckleberry <nhuck15@gmail.com> wrote:
>
> The style cleanup looks great. I just have one extra thing that
> can probably be thrown into this patch.
>
> On Thu, Jul 30, 2020 at 3:51 PM Nick Desaulniers
> <ndesaulniers@google.com> wrote:
> >
> > Removes the 1004 label; it was neither a control flow target, nor an
> > instruction we expect to produce a fault.
> >
> > Gives the labels slightly more readable names. The `b` suffixes are
> > handy to disambiguate between labels of the same identifier when there's
> > more than one. Since these labels are unique, let's just give them
> > names.
> >
> > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> > ---
> > arch/arm/lib/backtrace-clang.S | 22 ++++++++++------------
> > 1 file changed, 10 insertions(+), 12 deletions(-)
> >
> > diff --git a/arch/arm/lib/backtrace-clang.S b/arch/arm/lib/backtrace-clang.S
> > index 40eb2215eaf4..7dad2a6843a5 100644
> > --- a/arch/arm/lib/backtrace-clang.S
> > +++ b/arch/arm/lib/backtrace-clang.S
> > @@ -121,8 +121,8 @@ for_each_frame: tst frame, mask @ Check for address exceptions
> > * start. This value gets updated to be the function start later if it is
> > * possible.
> > */
> > -1001: ldr sv_pc, [frame, #4] @ get saved 'pc'
> > -1002: ldr sv_fp, [frame, #0] @ get saved fp
> > +load_pc: ldr sv_pc, [frame, #4] @ get saved 'pc'
> > +load_fp: ldr sv_fp, [frame, #0] @ get saved fp
> >
> > teq sv_fp, mask @ make sure next frame exists
> > beq no_frame
> > @@ -142,7 +142,7 @@ for_each_frame: tst frame, mask @ Check for address exceptions
> > * registers for the current function, but the stacktrace is still printed
> > * properly.
> > */
> > -1003: ldr sv_lr, [sv_fp, #4] @ get saved lr from next frame
> > +load_lr: ldr sv_lr, [sv_fp, #4] @ get saved lr from next frame
> >
> > tst sv_lr, #0 @ If there's no previous lr,
> > beq finished_setup @ we're done.
> > @@ -166,8 +166,7 @@ finished_setup:
> > /*
> > * Print the function (sv_pc) and where it was called from (sv_lr).
> > */
> > -1004: mov r0, sv_pc
> > -
> > + mov r0, sv_pc
> > mov r1, sv_lr
> > mov r2, frame
> > bic r1, r1, mask @ mask PC/LR for the mode
> > @@ -182,7 +181,7 @@ finished_setup:
> > * pointer the comparison will fail and no registers will print. Unwinding will
> > * continue as if there had been no registers stored in this frame.
> > */
> > -1005: ldr r1, [sv_pc, #0] @ if stmfd sp!, {..., fp, lr}
> > +load_stmfd: ldr r1, [sv_pc, #0] @ if stmfd sp!, {..., fp, lr}
> > ldr r3, .Lopcode @ instruction exists,
> > teq r3, r1, lsr #11
> > ldr r0, [frame] @ locals are stored in
> > @@ -201,7 +200,7 @@ finished_setup:
> > mov frame, sv_fp @ above the current frame
> > bhi for_each_frame
> >
> > -1006: adr r0, .Lbad
> > +bad_frame: adr r0, .Lbad
> > mov r1, loglvl
> > mov r2, frame
> > bl printk
> > @@ -216,11 +215,10 @@ bad_lr: mov sv_fp, #0
> > ENDPROC(c_backtrace)
> > .pushsection __ex_table,"a"
> > .align 3
> > - .long 1001b, 1006b
> > - .long 1002b, 1006b
> > - .long 1003b, 1006b
> > - .long 1004b, 1006b
> > - .long 1005b, 1006b
> > + .long load_pc, bad_frame
> > + .long load_fp, bad_frame
> > + .long load_lr, bad_frame
> > + .long load_stmfd, bad_frame
>
> Load_stmfd should get its own fixup
> handler since it should emit errors about a bad
> pc, not a bad frame pointer.
Yeah, I can add that. It's a little orthogonal though to this patch
that renames labels. I'd consider more so a pre-existing bug. Let me
add a patch to the series that gives it a new fixup handler, separate
from the label renaming, in a v2 of the series.
>
> > .long prev_call, bad_lr
> > .popsection
> >
> > --
> > 2.28.0.163.g6104cc2f0b6-goog
> >
--
Thanks,
~Nick Desaulniers
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2020-08-10 22:32 UTC|newest]
Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-30 20:51 [PATCH 0/4] CONFIG_UNWINDER_FRAME_POINTER fixes+cleanups Nick Desaulniers
2020-07-30 20:51 ` Nick Desaulniers
2020-07-30 20:51 ` Nick Desaulniers
2020-07-30 20:51 ` [PATCH 1/4] ARM: backtrace-clang: check for NULL lr Nick Desaulniers
2020-07-30 20:51 ` Nick Desaulniers
2020-07-30 20:51 ` Nick Desaulniers
2020-08-07 18:07 ` Nathan Huckleberry
2020-08-07 18:07 ` Nathan Huckleberry
2020-08-07 18:07 ` Nathan Huckleberry
2020-07-30 20:51 ` [PATCH 2/4] ARM: backtrace-clang: add fixup for lr dereference Nick Desaulniers
2020-07-30 20:51 ` Nick Desaulniers
2020-07-30 20:51 ` Nick Desaulniers
2020-08-01 23:18 ` Sasha Levin
2020-08-03 18:13 ` Nick Desaulniers
2020-08-04 6:27 ` Greg KH
2020-08-06 1:24 ` Sasha Levin
2020-08-06 22:38 ` Nathan Huckleberry
2020-08-06 22:38 ` Nathan Huckleberry
2020-08-06 22:38 ` Nathan Huckleberry
2020-08-10 22:33 ` Nick Desaulniers
2020-08-10 22:33 ` Nick Desaulniers
2020-08-10 22:33 ` Nick Desaulniers
2020-08-20 0:13 ` Nick Desaulniers
2020-08-20 0:13 ` Nick Desaulniers
2020-08-20 0:13 ` Nick Desaulniers
2020-08-13 16:25 ` Sasha Levin
2020-08-19 23:56 ` Sasha Levin
2020-07-30 20:51 ` [PATCH 3/4] ARM: backtrace-clang: give labels more descriptive names Nick Desaulniers
2020-07-30 20:51 ` Nick Desaulniers
2020-07-30 20:51 ` Nick Desaulniers
2020-08-06 22:39 ` Nathan Huckleberry
2020-08-06 22:39 ` Nathan Huckleberry
2020-08-06 22:39 ` Nathan Huckleberry
2020-08-10 22:32 ` Nick Desaulniers [this message]
2020-08-10 22:32 ` Nick Desaulniers
2020-08-10 22:32 ` Nick Desaulniers
2020-07-30 20:51 ` [PATCH 4/4] ARM: backtrace: use more descriptive labels Nick Desaulniers
2020-07-30 20:51 ` Nick Desaulniers
2020-07-30 20:51 ` Nick Desaulniers
2020-08-06 1:24 ` [PATCH 0/4] CONFIG_UNWINDER_FRAME_POINTER fixes+cleanups Sasha Levin
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=CAKwvOdn+MGgYf8k9uAUT55vBL+ERTjv+jx+t8SD9HO98-h2c0w@mail.gmail.com \
--to=ndesaulniers@google.com \
--cc=0x7f454c46@gmail.com \
--cc=akpm@linux-foundation.org \
--cc=clang-built-linux@googlegroups.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mediatek@lists.infradead.org \
--cc=linux@armlinux.org.uk \
--cc=lvqiang.huang@unisoc.com \
--cc=matthias.bgg@gmail.com \
--cc=miles.chen@mediatek.com \
--cc=nhuck15@gmail.com \
--cc=nhuck@google.com \
--cc=zhang.lyra@gmail.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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.