Re: [PATCH 5/5] Add frame pointer unwinding for aarch64

[Mark Wielaard <mark@klomp.org>]

On Tue, 2017-04-25 at 15:38 +0200, Ulf Hermann wrote:
> > My question is about this "initial frame". In our testcase we don't have
> > this case since the backtrace starts in a function that has some CFI.
> > But I assume you have some tests that rely on this behavior.
> 
> Actually the test I provided does exercise this code. The initial
> __libc_do_syscall() frame does not have CFI. Only raise() has. You can
> check that by dropping the code for pc & 0x1.

Maybe I am using the wrong binaries (exec and core), but for me there is
no difference.

With or with commenting out the adjustments:

diff --git a/backends/aarch64_unwind.c b/backends/aarch64_unwind.c
index cac4ebd..36cd0e1 100644
--- a/backends/aarch64_unwind.c
+++ b/backends/aarch64_unwind.c
@@ -63,6 +63,7 @@ EBLHOOK(unwind) (Ebl *ebl __attribute__ ((unused)), Dwarf_Addr pc __attribute__
 
   // The initial frame is special. We are expected to return lr directly in this case, and we'll
   // come back to the same frame again in the next round.
+/*
   if ((pc & 0x1) == 0)
     {
       newLr = lr;
@@ -70,6 +71,7 @@ EBLHOOK(unwind) (Ebl *ebl __attribute__ ((unused)), Dwarf_Addr pc __attribute__
       newSp = sp;
     }
   else
+*/
     {
       if (!readfunc(fp + LR_OFFSET, &newLr, arg))
         newLr = 0;
@@ -80,7 +82,7 @@ EBLHOOK(unwind) (Ebl *ebl __attribute__ ((unused)), Dwarf_Addr pc __attribute__
       newSp = fp + SP_OFFSET;
     }
 
-  newPc = newLr & (~0x1);
+  newPc = newLr /* & (~0x1) */;
   if (!setfunc(-1, 1, &newPc, arg))
     return false;
 
@@ -92,5 +94,5 @@ EBLHOOK(unwind) (Ebl *ebl __attribute__ ((unused)), Dwarf_Addr pc __attribute__
   // If the fp is invalid, we might still have a valid lr.
   // But if the fp is valid, then the stack should be moving in the right direction.
   // Except, if this is the initial frame. Then the stack doesn't move.
-  return newPc != 0 && (fp == 0 || newSp > sp || (pc & 0x1) == 0);
+  return newPc != 0 && (fp == 0 || newSp > sp /* || (pc & 0x1) == 0 */);
 }

The testcase (run-backtrace-fp-core-aarch64.sh) PASSes and produces the
same output for:

LD_LIBRARY_PATH=backends:libelf:libdw src/stack -v --exec
backtrace.aarch64.fp.exec --core backtrace.aarch64.fp.core

PID 349 - core
TID 350:
#0  0x000000000040583c     raise - /home/ulf/backtrace.aarch64.fp.exec
    ../nptl/sysdeps/unix/sysv/linux/pt-raise.c:37
#1  0x0000000000401aac - 1 sigusr2 - /home/ulf/backtrace.aarch64.fp.exec
#2  0x0000000000401ba8 - 1 stdarg - /home/ulf/backtrace.aarch64.fp.exec
#3  0x0000000000401c04 - 1 backtracegen - /home/ulf/backtrace.aarch64.fp.exec
#4  0x0000000000401c10 - 1 start - /home/ulf/backtrace.aarch64.fp.exec
#5  0x0000000000402f44 - 1 start_thread - /home/ulf/backtrace.aarch64.fp.exec
    /build/glibc-MsMi75/glibc-2.19/nptl/pthread_create.c:311
#6  0x000000000041dc70 - 1 __clone - /home/ulf/backtrace.aarch64.fp.exec
TID 349:
#0  0x0000000000403fcc     pthread_join - /home/ulf/backtrace.aarch64.fp.exec
    /build/glibc-MsMi75/glibc-2.19/nptl/pthread_join.c:92
#1  0x0000000000401810 - 1 main - /home/ulf/backtrace.aarch64.fp.exec
#2  0x0000000000406544 - 1 __libc_start_main - /home/ulf/backtrace.aarch64.fp.exec
#3  0x0000000000401918 - 1 $x - /home/ulf/backtrace.aarch64.fp.exec
src/stack: dwfl_thread_getframes tid 349 at 0x401917 in /home/ulf/backtrace.aarch64.fp.exec: address out of range

Since I cannot find the __libc_do_syscall I assume I am not using the
right executable & core? Could you double check them on the
mjw/fp-unwind branch?

> > The first question is how/why the (pc & 0x1) == 0 check works?
> > Why is that the correct check?
> > 
> > Secondly, if it really is the initial (or signal frame) we are after,
> > should we pass in into bool *signal_framep argument. Currently we don't
> 
> We have this piece of code in __libdwfl_frame_unwind, in frame_unwind.c:
> 
>   if (! state->initial_frame && ! state->signal_frame)
>       pc--;
> 
> AArch64 has a fixed instruction width of 32bit. So, normally the pc is
> aligned to 4 bytes. Except if we decrement it, then we are guaranteed
> to have an odd number, which we can then test to see if the frame in
> question is the initial or a signal frame.

Aha, OK. I forgot we explicitly decrement the pc for the frame before
doing the actual unwind. That makes sense.

> Of course it would be nicer to pass this information directly, but the
> signal_frame parameter is supposed to be an output parameter. After
> all we do the following after calling ebl_unwind():
> 
>   state->unwound->signal_frame = signal_frame;

Right, but that doesn't mean we couldn't also provide it as input if we
know that it is a signal or initial frame already. It just means that
unwinders would have to explicitly set it to false if cannot determine
it for the unwound frame (which is for all of them except the s390x
unwinder). It would really be just one line change in the call to and in
the unwinder functions. This isn't a public API, so we can change it to
be smarter.

Cheers,

Mark