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

2017-04-27 Thread Mark Wielaard 
On Thu, Apr 27, 2017 at 10:31:55AM +0200, Ulf Hermann wrote:
> > Maybe something like the attached patch?
> 
> Well that's actually the original patch (as opposed to V2) with relaxed test
> conditions. You can write that a bit nicer by setting the new PC directly
> after retrieving LR and returning early if it doesn't work. See "[PATCH 2/3]
> Add frame pointer unwinding as fallback on arm" from February 16th. That's
> the original algorithm; for aarch64 I just added a few defines and included
> arm_unwind.c.

I think the simplier implementation with relaxed test is better.
I'll adjust the patch to look more like your original.

> It's in fact a bit annoying for my use case as the non-CFI stack sections
> are mostly in between CFI-enabled stack sections here. However, I can accept
> this.

Does every fp-only frame gets duplicated after a DWARF CFI frame?
I'll look if I can better understand why that is.

Cheers,

Mark

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

2017-04-26 Thread Ulf Hermann 
On 04/26/2017 04:33 PM, Mark Wielaard wrote:
> 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.

In fact, with the new binaries there is no difference. I was confused, sorry.

However, if you strip .eh_frame and .eh_frame_hdr from the exe (thus triggering 
the fp unwinding on the first frame), you will see that it skips sigusr2. At 
the same time it invents another frame 0x403f40 on the main thread. Apparently 
pthread_join creates two stack frames. As it correctly unwinds the rest, the 
latter seemed harmless to me.

With .eh_frame and .eh_frame_hdr:

ulf@zebra:~/dev/build-elfutils/tests$ ./backtrace 
--core=backtrace.aarch64.fp.core -e backtrace.aarch64.fp.exec
0x400x4a3000/home/ulf/backtrace.aarch64.fp.exec
0x7fb6380x7fb6381000linux-vdso.so.1
TID 350:
# 0 0x40583craise
# 1 0x401aac - 1sigusr2
# 2 0x401ba8 - 1stdarg
# 3 0x401c04 - 1backtracegen
# 4 0x401c10 - 1start
# 5 0x402f44 - 1start_thread
# 6 0x41dc70 - 1__clone
TID 349:
# 0 0x403fccpthread_join
# 1 0x401810 - 1main
# 2 0x406544 - 1__libc_start_main
# 3 0x401918 - 1$x
./backtrace: dwfl_thread_getframes: address out of range

Without .eh_frame and .eh_frame_hdr, code from PATCH V2:

ulf@zebra:~/dev/build-elfutils/tests$ ./backtrace 
--core=backtrace.aarch64.fp.core -e backtrace.aarch64.fp.stripped 
0x400x4a3000/home/ulf/backtrace.aarch64.fp.exec
0x7fb6380x7fb6381000linux-vdso.so.1
TID 350:
# 0 0x40583c(null)
# 1 0x401aac - 1(null)
# 2 0x401ba8 - 1(null)
# 3 0x401c04 - 1(null)
# 4 0x401c10 - 1(null)
# 5 0x402f44 - 1(null)
# 6 0x41dc70 - 1(null)
./backtrace: dwfl_thread_getframes: address out of range
TID 349:
# 0 0x403fcc(null)
# 1 0x403f40 - 1(null)
# 2 0x401810 - 1(null)
# 3 0x406544 - 1(null)
# 4 0x401918 - 1(null)
./backtrace: dwfl_thread_getframes: address out of range

Without .eh_frame and .eh_frame_hdr, without initial frame adjustment:

ulf@zebra:~/dev/build-elfutils/tests$ ./backtrace 
--core=backtrace.aarch64.fp.core -e backtrace.aarch64.fp.stripped 
0x400x4a3000/home/ulf/backtrace.aarch64.fp.exec
0x7fb6380x7fb6381000linux-vdso.so.1
TID 350:
# 0 0x40583c(null)
# 1 0x401ba8 - 1(null)
# 2 0x401c04 - 1(null)
# 3 0x401c10 - 1(null)
# 4 0x402f44 - 1(null)
# 5 0x41dc70 - 1(null)
./backtrace: dwfl_thread_getframes: address out of range
TID 349:
# 0 0x403fcc(null)
# 1 0x401810 - 1(null)
# 2 0x406544 - 1(null)
# 3 0x401918 - 1(null)
./backtrace: dwfl_thread_getframes: address out of range

You have to drop all the asserts from backtrace.c to actually test this:

diff --git a/tests/backtrace.c b/tests/backtrace.c
index 1ff6353..a910a77 100644
--- a/tests/backtrace.c
+++ b/tests/backtrace.c
@@ -71,14 +71,14 @@ static void
 callback_verify (pid_t tid, unsigned frameno, Dwarf_Addr pc,
 const char *symname, Dwfl *dwfl)
 {
-  static bool seen_main = false;
+//  static bool seen_main = false;
   if (symname && *symname == '.')
 symname++;
-  if (symname && strcmp (symname, "main") == 0)
-seen_main = true;
+//  if (symname && strcmp (symname, "main") == 0)
+//seen_main = true;
   if (pc == 0)
 {
-  assert (seen_main);
+//  assert (seen_main);
   return;
 }
   if (check_tid == 0)
@@ -103,11 +103,11 @@ callback_verify (pid_t tid, unsigned frameno, Dwarf_Addr 
pc,
   && (strcmp (symname, "__kernel_vsyscall") == 0
   || strcmp (symname, "__libc_do_syscall") == 0))
reduce_frameno = true;
-  else
-   assert (symname && strcmp (symname, "raise") == 0);
+//  else
+// assert (symname && strcmp (symname, "raise") == 0);
   break;
 case 1:
-  assert (symname != NULL && strcmp (symname, "sigusr2") == 0);
+//  assert (symname != NULL && strcmp (symname, "sigusr2") == 0);
   break;
 case 2: // x86_64 only
   /* __restore_rt - glibc maybe does not have to have this symbol.  */
@@ -125,11 +125,11 @@ callback_verify (pid_t tid, unsigned frameno, Dwarf_Addr 
pc,
}
   /* FALLTHRU */
 case 4:
-  assert (symname != NULL && strcmp (symname, "stdarg") == 0);
+//  assert (symname != NULL && strcmp (symname, "stdarg") == 0);
   break;
 case 5:

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

2017-04-26 Thread Mark Wielaard 
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, , 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, , 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  0x0040583c raise - /home/ulf/backtrace.aarch64.fp.exec
../nptl/sysdeps/unix/sysv/linux/pt-raise.c:37
#1  0x00401aac - 1 sigusr2 - /home/ulf/backtrace.aarch64.fp.exec
#2  0x00401ba8 - 1 stdarg - /home/ulf/backtrace.aarch64.fp.exec
#3  0x00401c04 - 1 backtracegen - /home/ulf/backtrace.aarch64.fp.exec
#4  0x00401c10 - 1 start - /home/ulf/backtrace.aarch64.fp.exec
#5  0x00402f44 - 1 start_thread - /home/ulf/backtrace.aarch64.fp.exec
/build/glibc-MsMi75/glibc-2.19/nptl/pthread_create.c:311
#6  0x0041dc70 - 1 __clone - /home/ulf/backtrace.aarch64.fp.exec
TID 349:
#0  0x00403fcc pthread_join - /home/ulf/backtrace.aarch64.fp.exec
/build/glibc-MsMi75/glibc-2.19/nptl/pthread_join.c:92
#1  0x00401810 - 1 main - /home/ulf/backtrace.aarch64.fp.exec
#2  0x00406544 - 1 __libc_start_main - 
/home/ulf/backtrace.aarch64.fp.exec
#3  0x00401918 - 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

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

2017-04-25 Thread Mark Wielaard 
From: Ulf Hermann 

If we don't find any debug information for a given frame, we usually
cannot unwind any further. However, the binary in question might have
been compiled with frame pointers, in which case we can look up the
well known frame pointer locations in the stack snapshot and use them
to bridge the frames without debug information.

Signed-off-by: Ulf Hermann 
---
 backends/ChangeLog |   6 +++
 backends/Makefile.am   |   2 +-
 backends/aarch64_init.c|   1 +
 backends/aarch64_unwind.c  |  96 +
 tests/ChangeLog|   7 +++
 tests/Makefile.am  |   3 ++
 tests/backtrace.aarch64.fp.core.bz2| Bin 0 -> 8437 bytes
 tests/backtrace.aarch64.fp.exec.bz2| Bin 0 -> 394972 bytes
 tests/run-backtrace-fp-core-aarch64.sh |  28 ++
 9 files changed, 142 insertions(+), 1 deletion(-)
 create mode 100644 backends/aarch64_unwind.c
 create mode 100644 tests/backtrace.aarch64.fp.core.bz2
 create mode 100644 tests/backtrace.aarch64.fp.exec.bz2
 create mode 100755 tests/run-backtrace-fp-core-aarch64.sh

diff --git a/backends/ChangeLog b/backends/ChangeLog
index 733e72c..d742bca 100644
--- a/backends/ChangeLog
+++ b/backends/ChangeLog
@@ -6,6 +6,12 @@
 
 2017-02-09  Ulf Hermann  
 
+   * aarch64_unwind.c: New file
+   * Makefile.am (aarch64_SRCS): Add aarch64_unwind.c
+   * aarch64_init.c (aarch64_init): Hook aarch64_unwind
+
+2017-02-09  Ulf Hermann  
+
* x86_64_unwind.c: New file
* Makefile.am (x86_64_SRCS): Add x86_64_unwind.c
* x86_64_init.c (x86_64_init): Hook x86_64_unwind
diff --git a/backends/Makefile.am b/backends/Makefile.am
index 22eb6ac..ff80a82 100644
--- a/backends/Makefile.am
+++ b/backends/Makefile.am
@@ -80,7 +80,7 @@ am_libebl_arm_pic_a_OBJECTS = $(arm_SRCS:.c=.os)
 
 aarch64_SRCS = aarch64_init.c aarch64_regs.c aarch64_symbol.c  \
   aarch64_corenote.c aarch64_retval.c aarch64_cfi.c \
-  aarch64_initreg.c
+  aarch64_initreg.c aarch64_unwind.c
 libebl_aarch64_pic_a_SOURCES = $(aarch64_SRCS)
 am_libebl_aarch64_pic_a_OBJECTS = $(aarch64_SRCS:.c=.os)
 
diff --git a/backends/aarch64_init.c b/backends/aarch64_init.c
index 6395f11..0866494 100644
--- a/backends/aarch64_init.c
+++ b/backends/aarch64_init.c
@@ -63,6 +63,7 @@ aarch64_init (Elf *elf __attribute__ ((unused)),
  + ALT_FRAME_RETURN_COLUMN (used when LR isn't used) = 97 DWARF regs. */
   eh->frame_nregs = 97;
   HOOK (eh, set_initial_registers_tid);
+  HOOK (eh, unwind);
 
   return MODVERSION;
 }
diff --git a/backends/aarch64_unwind.c b/backends/aarch64_unwind.c
new file mode 100644
index 000..cac4ebd
--- /dev/null
+++ b/backends/aarch64_unwind.c
@@ -0,0 +1,96 @@
+/* Get previous frame state for an existing frame state.
+   Copyright (C) 2016 The Qt Company Ltd.
+   This file is part of elfutils.
+
+   This file is free software; you can redistribute it and/or modify
+   it under the terms of either
+
+ * the GNU Lesser General Public License as published by the Free
+   Software Foundation; either version 3 of the License, or (at
+   your option) any later version
+
+   or
+
+ * the GNU General Public License as published by the Free
+   Software Foundation; either version 2 of the License, or (at
+   your option) any later version
+
+   or both in parallel, as here.
+
+   elfutils is distributed in the hope that it will be useful, but
+   WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   General Public License for more details.
+
+   You should have received copies of the GNU General Public License and
+   the GNU Lesser General Public License along with this program.  If
+   not, see .  */
+
+#ifdef HAVE_CONFIG_H
+# include 
+#endif
+
+#define BACKEND aarch64_
+#define FP_REG 29
+#define LR_REG 30
+#define SP_REG 31
+#define FP_OFFSET 0
+#define LR_OFFSET 8
+#define SP_OFFSET 16
+
+#include "libebl_CPU.h"
+
+/* There was no CFI. Maybe we happen to have a frame pointer and can unwind 
from that?  */
+
+bool
+EBLHOOK(unwind) (Ebl *ebl __attribute__ ((unused)), Dwarf_Addr pc 
__attribute__ ((unused)),
+ ebl_tid_registers_t *setfunc, ebl_tid_registers_get_t 
*getfunc,
+ ebl_pid_memory_read_t *readfunc, void *arg,
+ bool *signal_framep __attribute__ ((unused)))
+{
+  Dwarf_Word fp, lr, sp;
+
+  if (!getfunc(LR_REG, 1, , arg))
+return false;
+
+  if (!getfunc(FP_REG, 1, , arg))
+fp = 0;
+
+  if (!getfunc(SP_REG, 1, , arg))
+sp = 0;
+
+  Dwarf_Word newPc, newLr, newFp, newSp;
+
+  // 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)
+