Re: [PATCH v2] Add frame pointer unwinding for aarch64

2017-04-25 Thread Ulf Hermann 
On 04/24/2017 04:53 PM, Mark Wielaard wrote:
> I got these separately. I assume they are as in the email you sent on
> Mon, 10 Apr 2017 14:48:06 +0200 (which didn't hit the list because it
> had the binaries attached...)

Yes. Those are the right binaries.

> This description doesn't seem to match anymore. Should this be:
> 
> # The binary is generated by compiling with eh_frame CFI, but with frame
> # pointers.
> #
> # gcc -static -O2 -fno-omit-frame-pointer -fno-asynchronous-unwind-tables \
> # -D_GNU_SOURCE -pthread -o tests/backtrace.x86_64.fp.exec -I. -Ilib \
> # tests/backtrace-child.c

Almost right. It's backtrace.aarch64.fp.exec.

Thanks for taking care of this,
Ulf

Re: frame unwinding patches

2017-04-25 Thread Mark Wielaard 
On Thu, 2017-04-20 at 11:26 +0200, Ulf Hermann wrote:
> The x86_64 case already works with the test case I sent. Maybe we can
> accept that one before the others. The aarch64 case almost works, but
> seems to generally duplicate the first entry it unwinds by frame
> pointer after unwinding anything by CFI. That should be fixable. I
> will research it and post a follow up patch. The 32bit arm case is a
> horrible mess and we may indeed need to lower our expectations for
> that one. Or maybe I can find a raise() that follows the same frame
> conventions as the gcc I'm using ...

I tweaked the testcases a little to test more things. I reverted the
"Optionally allow unknown symbols in the backtrace tests" and added an
explicit check for a frame in the middle, the backtracegen function.
That showed the ppc32 core testfiles were generated without CFI for the
main executable, so I regenerated those.

Then I added your x86_64 frame pointer unwinder, testcases, my i386
frame pointer unwinder and your aarch64 frame pointer unwinder. I
dropped the arm32 frame pointer unwinder for now (maybe we need a less
demanding testcase for that or, more awesome, add code to translate the
exidx section for that).

I do have a question about the aarch64 frame pointer unwinder (and the
initial frame), but I'll reply to the patch email for that.

I'll post the patches as reply to this message (stripping the binaries)
and have pushed them all to the (rebased) mjw/fpunwind branch:
https://sourceware.org/git/?p=elfutils.git;a=shortlog;h=refs/heads/mjw/fp-unwind
Could you take a look and see if that looks good?

Thanks,

Mark

[PATCH 4/5] Add i386 frame pointer unwinder.

2017-04-25 Thread Mark Wielaard 
Add a simple i386_unwind.c frame pointer unwinder as fallback if DWARF/CFI
unwinding fails.

Signed-off-by: Mark Wielaard 
---
 backends/ChangeLog  |   6 +++
 backends/Makefile.am|   2 +-
 backends/i386_init.c|   3 +-
 backends/i386_unwind.c  |  84 
 tests/ChangeLog |   9 
 tests/Makefile.am   |   5 ++-
 tests/backtrace.i386.fp.core.bz2| Bin 0 -> 8532 bytes
 tests/backtrace.i386.fp.exec.bz2| Bin 0 -> 357436 bytes
 tests/run-backtrace-fp-core-i386.sh |  29 +
 9 files changed, 135 insertions(+), 3 deletions(-)
 create mode 100644 backends/i386_unwind.c
 create mode 100644 tests/backtrace.i386.fp.core.bz2
 create mode 100755 tests/backtrace.i386.fp.exec.bz2
 create mode 100755 tests/run-backtrace-fp-core-i386.sh

diff --git a/backends/ChangeLog b/backends/ChangeLog
index eec9923..733e72c 100644
--- a/backends/ChangeLog
+++ b/backends/ChangeLog
@@ -1,3 +1,9 @@
+2017-04-06  Mark Wielaard  
+
+   * i386_unwind.c: New file.
+   * i386_init.c: Hook i386_unwind.
+   * Makefile.am (i386_SRCS): Add i386_unwind.c
+
 2017-02-09  Ulf Hermann  
 
* x86_64_unwind.c: New file
diff --git a/backends/Makefile.am b/backends/Makefile.am
index 60917b9..22eb6ac 100644
--- a/backends/Makefile.am
+++ b/backends/Makefile.am
@@ -48,7 +48,7 @@ libdw = ../libdw/libdw.so
 
 i386_SRCS = i386_init.c i386_symbol.c i386_corenote.c i386_cfi.c \
i386_retval.c i386_regs.c i386_auxv.c i386_syscall.c \
-   i386_initreg.c
+   i386_initreg.c i386_unwind.c
 cpu_i386 = ../libcpu/libcpu_i386.a
 libebl_i386_pic_a_SOURCES = $(i386_SRCS)
 am_libebl_i386_pic_a_OBJECTS = $(i386_SRCS:.c=.os)
diff --git a/backends/i386_init.c b/backends/i386_init.c
index 515d5ac..fc1587a 100644
--- a/backends/i386_init.c
+++ b/backends/i386_init.c
@@ -1,5 +1,5 @@
 /* Initialization of i386 specific backend library.
-   Copyright (C) 2000-2009, 2013 Red Hat, Inc.
+   Copyright (C) 2000-2009, 2013, 2017 Red Hat, Inc.
This file is part of elfutils.
Written by Ulrich Drepper , 2000.
 
@@ -65,6 +65,7 @@ i386_init (Elf *elf __attribute__ ((unused)),
   /* gcc/config/ #define DWARF_FRAME_REGISTERS.  For i386 it is 17, why?  */
   eh->frame_nregs = 9;
   HOOK (eh, set_initial_registers_tid);
+  HOOK (eh, unwind);
 
   return MODVERSION;
 }
diff --git a/backends/i386_unwind.c b/backends/i386_unwind.c
new file mode 100644
index 000..5c9a5de
--- /dev/null
+++ b/backends/i386_unwind.c
@@ -0,0 +1,84 @@
+/* Get previous frame state for an existing frame state using frame pointers.
+   Copyright (C) 2017 Red Hat, Inc.
+   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
+
+#include 
+#include 
+
+#define BACKEND i386_
+#include "libebl_CPU.h"
+
+/* Register numbers for frame and stack pointers.  We take advantage of
+   them being next to each other when calling getfunc and setfunc.  */
+#define ESP 4
+#define EBP (ESP + 1)
+
+/* Most basic frame pointer chasing with EBP as frame pointer.
+   PC = *(FP + 4), SP = FP + 8, FP = *FP.  */
+bool
+i386_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)))
+{
+  /* sp = 0, fp = 1 */
+  Dwarf_Word regs[2];
+
+  /* Get current stack and frame pointers.  */
+  if (! getfunc (ESP, 2, regs, arg))
+return false;
+
+  Dwarf_Word sp = regs[0];
+  Dwarf_Word fp = regs[1];
+
+  /* Sanity check.  We only support traditional stack frames.  */
+  if (fp == 0 || sp == 0 || fp < sp)
+return false;
+
+  /* Get the return address from the stack, it is our new pc.  */
+  Dwarf_Word ret_addr;
+  if (! readfunc (fp + 4, &ret_addr, arg) || ret_addr == 0)
+return false;
+
+  /* Get new sp and fp.  Sanity check again.  */
+  sp = fp + 8;
+  if (! readfunc (f

[PATCH 1/5] Revert "Optionally allow unknown symbols in the backtrace tests"

2017-04-25 Thread Mark Wielaard 
This reverts commit f9971cb422df39adea7e8c7e22689b879e39c626.

Allowing no symbol resolving at all makes it too hard to see
whether the test actually tests anything.

But do keep "address out of range" as allowed error in check_err.
This can be interpreted as DWARF not available (if end of callstack
marker is missing, which it unfortunately often is missing even if CFI
is available.).

Signed-off-by: Mark Wielaard 
---
 tests/ChangeLog |  6 ++
 tests/backtrace-subr.sh | 12 +++-
 tests/backtrace.c   | 30 --
 3 files changed, 17 insertions(+), 31 deletions(-)

diff --git a/tests/ChangeLog b/tests/ChangeLog
index 5f7bcdd..a71db45 100644
--- a/tests/ChangeLog
+++ b/tests/ChangeLog
@@ -1,3 +1,9 @@
+2017-04-24  Mark Wielaard  
+
+   * backtrace.c: Remove option to allow unknown symbols in the trace.
+   * backtrace-substr.sh: Remove option to allow unknown symbols
+   to check_core() and allow failed symbol lookups in check_err().
+
 2017-04-05  Mark Wielaard  
 
* test-subr.sh (testrun_on_self_compressed): New function.
diff --git a/tests/backtrace-subr.sh b/tests/backtrace-subr.sh
index 5d3937c..e746dc1 100644
--- a/tests/backtrace-subr.sh
+++ b/tests/backtrace-subr.sh
@@ -44,9 +44,6 @@ check_gsignal()
 # In some cases we cannot reliably find out we got behind _start as some
 # operating system do not properly terminate CFI by undefined PC.
 # Ignore it here as it is a bug of OS, not a bug of elfutils.
-# If the CFI is not terminated correctly, we might find another frame by
-# checking for frame pointers. This is still not our problem, but only
-# gives an error message when trying to look up the function name.
 check_err()
 {
   if [ $(egrep -v <$1 'dwfl_thread_getframes: (No DWARF information found|no 
matching address range|address out of range)$' \
@@ -64,9 +61,7 @@ check_all()
   bt=$1
   err=$2
   testname=$3
-  if [ "x$4" != "x--allow-unknown" ]; then
-check_main $bt $testname
-  fi
+  check_main $bt $testname
   check_gsignal $bt $testname
   check_err $err $testname
 }
@@ -103,14 +98,13 @@ check_native_unsupported()
 check_core()
 {
   arch=$1
-  args=$2
   testfiles backtrace.$arch.{exec,core}
   tempfiles backtrace.$arch.{bt,err}
   echo ./backtrace ./backtrace.$arch.{exec,core}
-  testrun ${abs_builddir}/backtrace $args -e ./backtrace.$arch.exec 
--core=./backtrace.$arch.core 1>backtrace.$arch.bt 2>backtrace.$arch.err || true
+  testrun ${abs_builddir}/backtrace -e ./backtrace.$arch.exec 
--core=./backtrace.$arch.core 1>backtrace.$arch.bt 2>backtrace.$arch.err || true
   cat backtrace.$arch.{bt,err}
   check_unsupported backtrace.$arch.err backtrace.$arch.core
-  check_all backtrace.$arch.{bt,err} backtrace.$arch.core $args
+  check_all backtrace.$arch.{bt,err} backtrace.$arch.core
 }
 
 # Backtrace live process.
diff --git a/tests/backtrace.c b/tests/backtrace.c
index 34a2ab0..1ff6353 100644
--- a/tests/backtrace.c
+++ b/tests/backtrace.c
@@ -64,7 +64,6 @@ dump_modules (Dwfl_Module *mod, void **userdata __attribute__ 
((unused)),
   return DWARF_CB_OK;
 }
 
-static bool allow_unknown;
 static bool use_raise_jmp_patching;
 static pid_t check_tid;
 
@@ -79,8 +78,7 @@ callback_verify (pid_t tid, unsigned frameno, Dwarf_Addr pc,
 seen_main = true;
   if (pc == 0)
 {
-  if (!allow_unknown)
-assert (seen_main);
+  assert (seen_main);
   return;
 }
   if (check_tid == 0)
@@ -105,12 +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 if (!allow_unknown || symname)
+  else
assert (symname && strcmp (symname, "raise") == 0);
   break;
 case 1:
-  if (!allow_unknown || symname)
-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.  */
@@ -119,24 +116,20 @@ callback_verify (pid_t tid, unsigned frameno, Dwarf_Addr 
pc,
   if (use_raise_jmp_patching)
{
  /* Verify we trapped on the very first instruction of jmp.  */
-  if (!allow_unknown || symname)
-assert (symname != NULL && strcmp (symname, "jmp") == 0);
+ assert (symname != NULL && strcmp (symname, "jmp") == 0);
  mod = dwfl_addrmodule (dwfl, pc - 1);
  if (mod)
symname2 = dwfl_module_addrname (mod, pc - 1);
-  if (!allow_unknown || symname2)
-assert (symname2 == NULL || strcmp (symname2, "jmp") != 0);
+ assert (symname2 == NULL || strcmp (symname2, "jmp") != 0);
  break;
}
   /* FALLTHRU */
 case 4:
-  if (!allow_unknown || symname)
-assert (symname != NULL && strcmp (symname, "stdarg") == 0);
+  as

[PATCH 3/5] Add frame pointer unwinding as fallback on x86_64

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.

The "unwind" hook is the right place for this as it is so far only
used on s390 and called only after trying to unwind with debug
information.

Signed-off-by: Ulf Hermann 
---
 backends/ChangeLog|   6 +++
 backends/Makefile.am  |   2 +-
 backends/x86_64_init.c|   1 +
 backends/x86_64_unwind.c  |  86 ++
 tests/ChangeLog   |   7 +++
 tests/Makefile.am |   3 ++
 tests/backtrace.x86_64.fp.core.bz2| Bin 0 -> 11072 bytes
 tests/backtrace.x86_64.fp.exec.bz2| Bin 0 -> 434645 bytes
 tests/run-backtrace-fp-core-x86_64.sh |  29 
 9 files changed, 133 insertions(+), 1 deletion(-)
 create mode 100644 backends/x86_64_unwind.c
 create mode 100644 tests/backtrace.x86_64.fp.core.bz2
 create mode 100644 tests/backtrace.x86_64.fp.exec.bz2
 create mode 100755 tests/run-backtrace-fp-core-x86_64.sh

diff --git a/backends/ChangeLog b/backends/ChangeLog
index 39390cb..eec9923 100644
--- a/backends/ChangeLog
+++ b/backends/ChangeLog
@@ -1,3 +1,9 @@
+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
+
 2017-02-15  Mark Wielaard  
 
* ppc64_init.c (ppc64_init): Add check_object_attribute HOOK.
diff --git a/backends/Makefile.am b/backends/Makefile.am
index b553ec3..60917b9 100644
--- a/backends/Makefile.am
+++ b/backends/Makefile.am
@@ -59,7 +59,7 @@ am_libebl_sh_pic_a_OBJECTS = $(sh_SRCS:.c=.os)
 
 x86_64_SRCS = x86_64_init.c x86_64_symbol.c x86_64_corenote.c x86_64_cfi.c \
  x86_64_retval.c x86_64_regs.c i386_auxv.c x86_64_syscall.c \
- x86_64_initreg.c x32_corenote.c
+ x86_64_initreg.c x86_64_unwind.c x32_corenote.c
 cpu_x86_64 = ../libcpu/libcpu_x86_64.a
 libebl_x86_64_pic_a_SOURCES = $(x86_64_SRCS)
 am_libebl_x86_64_pic_a_OBJECTS = $(x86_64_SRCS:.c=.os)
diff --git a/backends/x86_64_init.c b/backends/x86_64_init.c
index cfd0158..adfa479 100644
--- a/backends/x86_64_init.c
+++ b/backends/x86_64_init.c
@@ -68,6 +68,7 @@ x86_64_init (Elf *elf __attribute__ ((unused)),
   /* gcc/config/ #define DWARF_FRAME_REGISTERS.  */
   eh->frame_nregs = 17;
   HOOK (eh, set_initial_registers_tid);
+  HOOK (eh, unwind);
 
   return MODVERSION;
 }
diff --git a/backends/x86_64_unwind.c b/backends/x86_64_unwind.c
new file mode 100644
index 000..ade64c0
--- /dev/null
+++ b/backends/x86_64_unwind.c
@@ -0,0 +1,86 @@
+/* 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
+
+#include 
+#include 
+
+#define BACKEND x86_64_
+#include "libebl_CPU.h"
+
+/* There was no CFI. Maybe we happen to have a frame pointer and can unwind 
from that?  */
+
+bool
+x86_64_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)))
+{
+  // Register 6 is supposed to be rbp, thus the conventional frame pointer
+  const int fpReg = 6;
+  const int spReg = 7;
+
+  Dwarf_Word fp;
+  if (!getfunc(fpReg, 1, &fp, arg) || fp == 0)
+return false;
+
+  // Try to read old sp, so that we can avoid infinite loops below
+  Dwarf_Word sp;
+  if (!getfunc(spReg, 1, &sp, arg))
+sp = 0;
+
+  Dwarf_Word prev_fp;
+  if (!readfunc(fp, &prev_fp, arg))
+prev_fp = 0;
+
+  Dwarf_Word ret;
+  if (!readfunc(fp + 8, &ret, arg))
+

[PATCH 2/5] tests: Add core backtracegen chec and regenerate ppc32 backtrace test files.

2017-04-25 Thread Mark Wielaard 
Add a check to check_core to make sure the backtracegen function is
found in the backtrace. This function is in the middle of the backtrace
in the main executable and if not found it means the backtrace was
incomplete or the frame was skipped (which could happen on a bad frame
pointer only unwind).

This showed that the ppc32 backtrace test files were missing DWARF CFI
for the main executable. Regenerated them to include full CFI.

Signed-off-by: Mark Wielaard 
---
 tests/ChangeLog |   7 +++
 tests/backtrace-subr.sh |  14 ++
 tests/backtrace.ppc.core.bz2| Bin 46357 -> 44482 bytes
 tests/backtrace.ppc.exec.bz2| Bin 352898 -> 352197 bytes
 tests/run-backtrace-core-ppc.sh |   9 +
 5 files changed, 30 insertions(+)

diff --git a/tests/ChangeLog b/tests/ChangeLog
index a71db45..6f5956b 100644
--- a/tests/ChangeLog
+++ b/tests/ChangeLog
@@ -1,3 +1,10 @@
+2017-04-25  Mark Wielaard  
+
+   * backtrace-subr.sh (check_backtracegen): New function.
+   (check_core): Add check_backtracegen call.
+   * backtrace.ppc.exec.bz2: Regenerated.
+   * backtrace.ppc.core.bz2: Likewise.
+
 2017-04-24  Mark Wielaard  
 
* backtrace.c: Remove option to allow unknown symbols in the trace.
diff --git a/tests/backtrace-subr.sh b/tests/backtrace-subr.sh
index e746dc1..a303e32 100644
--- a/tests/backtrace-subr.sh
+++ b/tests/backtrace-subr.sh
@@ -40,6 +40,19 @@ check_gsignal()
   false
 }
 
+
+# Makes sure we saw the function that initiated the backtrace
+# when the core was generated through the tests backtrace --gencore.
+# This might disappear when frame pointer chasing gone bad.
+check_backtracegen()
+{
+  if grep -w backtracegen $1; then
+return
+  fi
+  echo >&2 $2: no backtracegen
+  false
+}
+
 # Verify the STDERR output does not contain unexpected errors.
 # In some cases we cannot reliably find out we got behind _start as some
 # operating system do not properly terminate CFI by undefined PC.
@@ -105,6 +118,7 @@ check_core()
   cat backtrace.$arch.{bt,err}
   check_unsupported backtrace.$arch.err backtrace.$arch.core
   check_all backtrace.$arch.{bt,err} backtrace.$arch.core
+  check_backtracegen backtrace.$arch.bt backtrace.$arch.core
 }
 
 # Backtrace live process.

diff --git a/tests/run-backtrace-core-ppc.sh b/tests/run-backtrace-core-ppc.sh
index 65c9279..555ac35 100755
--- a/tests/run-backtrace-core-ppc.sh
+++ b/tests/run-backtrace-core-ppc.sh
@@ -17,4 +17,13 @@
 
 . $srcdir/backtrace-subr.sh
 
+# executable generated by:
+#
+# gcc -D_GNU_SOURCE -I. -I.. -I../lib -m32 -pthread -static -g \
+# -o backtrace.ppc.exec backtrace-child.c
+#
+# core generated by:
+#
+# ./backtrace.ppc.exec --gencore
+
 check_core ppc
-- 
1.8.3.1

Re: [PATCH 1/5] Revert "Optionally allow unknown symbols in the backtrace tests"

2017-04-25 Thread Ulf Hermann 
On 04/25/2017 02:49 PM, Mark Wielaard wrote:
> This reverts commit f9971cb422df39adea7e8c7e22689b879e39c626.
> 
> Allowing no symbol resolving at all makes it too hard to see
> whether the test actually tests anything.
> [...]

Looks good to me.

Ulf

Re: [PATCH 2/5] tests: Add core backtracegen chec and regenerate ppc32 backtrace test files.

2017-04-25 Thread Ulf Hermann 
On 04/25/2017 02:49 PM, Mark Wielaard wrote:
> Add a check to check_core to make sure the backtracegen function is
> found in the backtrace. This function is in the middle of the backtrace
> in the main executable and if not found it means the backtrace was
> incomplete or the frame was skipped (which could happen on a bad frame
> pointer only unwind).
> 
> This showed that the ppc32 backtrace test files were missing DWARF CFI
> for the main executable. Regenerated them to include full CFI.

Looks good to me.

Ulf

Re: [PATCH 3/5] Add frame pointer unwinding as fallback on x86_64

2017-04-25 Thread Ulf Hermann 
On 04/25/2017 02:49 PM, Mark Wielaard wrote:
> 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.
> 
> The "unwind" hook is the right place for this as it is so far only
> used on s390 and called only after trying to unwind with debug
> information.

Looks good to me.
Ulf

[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, &lr, arg))
+return false;
+
+  if (!getfunc(FP_REG, 1, &fp, arg))
+fp = 0;
+
+  if (!getfunc(SP_REG, 1, &sp, 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)
+{
+  newLr = lr;
+  newFp = fp;
+  newSp = sp;
+

Re: [PATCH 4/5] Add i386 frame pointer unwinder.

2017-04-25 Thread Ulf Hermann 
On 04/25/2017 02:49 PM, Mark Wielaard wrote:
> Add a simple i386_unwind.c frame pointer unwinder as fallback if DWARF/CFI
> unwinding fails.

Looks good to me. The logic could be relaxed a bit so that failure to e.g. 
write the new value for sp would not be fatal. Then we might be able to unwind 
even more frames. But refusing to unwind obviously broken frames is acceptable, 
too.

Ulf

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

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

Looks good to me.

> +# The binary is generated by compiling with eh_frame CFI, but with frame
> +# pointers.
> +#
> +# gcc -static -O2 -fno-omit-frame-pointer -fno-asynchronous-unwind-tables \
> +# -D_GNU_SOURCE -pthread -o tests/backtrace.aarch64.fp.exec -I. -Ilib \
> +# tests/backtrace-child.c#

Trailing '#', but that is insignificant.

Ulf

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

2017-04-25 Thread Mark Wielaard 
On Tue, 2017-04-25 at 14:49 +0200, Mark Wielaard wrote:
> +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, &lr, arg))
> +return false;
> +
> +  if (!getfunc(FP_REG, 1, &fp, arg))
> +fp = 0;
> +
> +  if (!getfunc(SP_REG, 1, &sp, 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)
> +{
> +  newLr = lr;
> +  newFp = fp;
> +  newSp = sp;
> +}
> +  else
> +{
> +  if (!readfunc(fp + LR_OFFSET, &newLr, arg))
> +newLr = 0;
> +
> +  if (!readfunc(fp + FP_OFFSET, &newFp, arg))
> +newFp = 0;
> +
> +  newSp = fp + SP_OFFSET;
> +}
> +
> +  newPc = newLr & (~0x1);
> +  if (!setfunc(-1, 1, &newPc, arg))
> +return false;

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.

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

Lastly could you instead of returning the frame itself with just the pc
adjusted do that directly?

  if ((pc & 0x1) == 0)
lr = lr & (~0x1);

And then use the code in the else clause always?

All the above might simply be me not understanding the significance of
the initial frame.

Cheers,

Mark

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

2017-04-25 Thread Ulf Hermann 
> 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.

> 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. 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;

> Lastly could you instead of returning the frame itself with just the pc
> adjusted do that directly?
> 
>   if ((pc & 0x1) == 0)
> lr = lr & (~0x1);

If I dig up the first frame after the initial one from the stack, then we drop 
whatever we initially had in LR. Apparently, on aarch64 PC is always one frame 
"ahead" of the other registers. To establish that, we have to set PC to the 
value of LR on the initial frame, without actually unwinding.

br,
Ulf

Re: [PATCH v2] Include sys/types.h before fts.h

2017-04-25 Thread Mark Wielaard 
On Thu, Apr 20, 2017 at 04:45:51PM +0200, Ulf Hermann wrote:
> The bad fts not only needs to be included before config.h, but also
> requires various special types without including sys/types.h.
> 
> Change-Id: I31ac8d2aadcf7ffb3efb63583b2745991bfd6f90
> Signed-off-by: Ulf Hermann 

Thanks pushed to master.
But I removed the Change-Id since I don't know how it is helpful.

Cheers,

Mark

Re: [PATCH] Clean up linux-specific system includes

2017-04-25 Thread Mark Wielaard 
On Thu, Apr 20, 2017 at 03:37:04PM +0200, Ulf Hermann wrote:
> We only include them where we actually need them and only on linux.

Looks correct. Quickly tested on Fedora and RHEL x86_64.
But I believe the other arches are correct too.

Pushed to master.

Thanks,

Mark

Re: [PATCH] Don't use comparison_fn_t

2017-04-25 Thread Mark Wielaard 
On Thu, Apr 20, 2017 at 03:40:46PM +0200, Ulf Hermann wrote:
> Not all search.h declare it, and it is not very helpful anyway.

Well, it is less typing and it kept the lines under 80 chars.
But because there were other longer lines already I just kept it.
Pushed to master.

Thanks,

Mark

Re: [PATCH] Avoid YESSTR and NOSTR

2017-04-25 Thread Mark Wielaard 
On Thu, Apr 20, 2017 at 03:47:49PM +0200, Ulf Hermann wrote:
> Those are deprecated and apparently some implementations of nl_langinfo
> return empty strings for them. The tests even tested for those empty
> strings even though the intention of the code was clearly to output
> "yes" or "no" there.

Urgh. Good find. YESSTR and NOSTR were once meant for matching user
input. Using them for output was probably always wrong.

Pushed to master.

Thanks,

Mark