On Tue, 5 Oct 2021 at 20:46, Nick Desaulniers [off-list ref] wrote:

On Tue, Oct 5, 2021 at 5:22 AM Arnd Bergmann [off-list ref] wrote:

quoted

On Tue, Oct 5, 2021 at 9:15 AM Ard Biesheuvel [off-list ref] wrote:

quoted

Restructure the code and add the unwind annotations so that both the
frame pointer unwinder as well as the ELF unwind info based unwinder
will be able to follow the call stack through call_with_stack().

Note that the former cannot support GCC and Clang at the same time, as
they use a different idiom for the prologue/epilogue. So the code uses
the GCC idiom, adding full frame pointer based unwind support for GCC
while preserving the existing behavior of the Clang version, which
simply omits call_with_stack() from its call stack.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>

I would like Nick to take a look at this for the clang support, he spent
some time on getting the frame pointer unwinder working with clang,
so he may have additional comments about this.

quoted

---
 arch/arm/lib/call_with_stack.S | 44 +++++++++++++++++---
 1 file changed, 38 insertions(+), 6 deletions(-)

diff --git a/arch/arm/lib/call_with_stack.S b/arch/arm/lib/call_with_stack.S
index 28b0341ae786..133dffa2404a 100644
--- a/arch/arm/lib/call_with_stack.S
+++ b/arch/arm/lib/call_with_stack.S

@@ -8,25 +8,57 @@

 #include <linux/linkage.h>
 #include <asm/assembler.h>
+#include <asm/unwind.h>

 /*
  * void call_with_stack(void (*fn)(void *), void *arg, void *sp)
  *
  * Change the stack to that pointed at by sp, then invoke fn(arg) with
  * the new stack.
+ *
+ * The sequence below follows the APCS frame convention for frame pointer
+ * unwinding, and implements the unwinder annotations needed by the EABI
+ * unwinder.
+ */
+
+#if defined(CONFIG_THUMB2_KERNEL) || \
+    (defined(CONFIG_UNWINDER_FRAME_POINTER) && defined(CONFIG_CC_IS_CLANG))

Doesn't clang use r11 (fp) as the frame pointer in ARM mode?
https://godbolt.org/z/1x4x99M1x
Or is this what you meant by "So the best we can do here is not touch
the frame pointer at all"?

Yes. Both unwinders expect to see the exact prologue that the
respective compiler generates, and there is no way we can do both at
the same time.

So by using r7 instead of r11, the call_with_stack() frame simply
disappears from the backtrace, which is unfortunate but not the end of
the world.

If someone wants to create a Clang clone of call_with_stack.S to work
around this, I wouldn't obje

quoted

quoted

+/*
+ * Thumb-2 builds must use R7 as the frame pointer due to the way our unwind
+ * info based unwinder is constructed.
+ *
+ * The code below uses the GCC idiom for managing the frame pointer in the
+ * function prologue and epilogue, which Clang does not support. So the best we

IIRC, it's only slightly different; it's just that FP points to the
previous FP in clang, rather than LR; at a fixed offset. At least when
looking through Doug's notes and diagrams:

GCC stores {fp, sp, lr, pc}, and makes FP point at PC. The fact that
they are only slightly different doesn't help - we either emit one or
the other, but we cannot do both.


 https://lore.kernel.org/lkml/20210507135509.1.I5d969beafa0d7507f1e37fadaa6e4d88d428253d@changeid/ (local)

Though looking at the diagram, it looks like neither toolchain
implements APCS...did I understand that correctly?

There's also some documentation in
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/kernel/stacktrace.c#n11
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/lib/backtrace-clang.S#n31

I guess I'm more so curious about this code when built with clang,
both before and after this patch.  Was it broken for either unwinder
on ARM or THUMB2+UNWINDER_ARM?  Does it regress with this patch?
What's the best way to test/verify this?

Unwinding using unwind info as definitely broken, since there is no
way to track the SP change. Unwinding using frame pointers probably
worked, but call_switch_stack() itself would just not show up.
However, there are some heuristics in the frame pointer unwinder about
the next frame always being above the previous one, and so it also
depends on where the new stack lives in memory wrt to the old one.

The reason I am fixing this is so that the next patch doesn't result
in cases where we cannot tell from a backtrace where do_softirq() was
called from, which would be annoying.

quoted

quoted

+ * can do here is not touch the frame pointer at all: this will simply omit
+ * this frame when unwinding the call stack. So use R7 in this case as well,
+ * and leave R11 unmodified.
  */
+       fpreg   .req    r7

TIL about `.req`: https://sourceware.org/binutils/docs/as/ARM-Directives.html
This patch demonstrates the usage of quite a few of these!

quoted

quoted

+#else
+       fpreg   .req    fp
+#endif
+
 ENTRY(call_with_stack)
-       str     sp, [r2, #-4]!
-       str     lr, [r2, #-4]!
+UNWIND(        .fnstart                )
+UNWIND(        .movsp  ip              )
+       mov     ip, sp
+
+UNWIND(        .pad    #4              )
+UNWIND(        .save   {fpreg, ip, lr} )
+THUMB( sub     sp, #4          )
+       push    {fpreg, ip, lr ARM(, pc)}
+
+UNWIND(        .setfp  fpreg, ip, #-4  )
+       sub     fpreg, ip, #4

        mov     sp, r2
        mov     r2, r0
        mov     r0, r1

-       badr    lr, 1f
-       ret     r2
+       bl_r    r2

-1:     ldr     lr, [sp]
-       ldr     sp, [sp, #4]
+       ldmdb   fpreg, {fpreg, ip, lr}
+       mov     sp, ip
        ret     lr
+UNWIND(        .fnend                  )
 ENDPROC(call_with_stack)
--
2.30.2

--
Thanks,
~Nick Desaulniers

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel