dwarf: Push correct CFA onto stack for dwarf expression evaluation. (#93)

dwarf: Push correct CFA onto stack for dwarf expression evaluation.

This change fixes a bug where stale CFAs were pushed onto the dwarf
expression stack before expression evaluation. Some optimising compilers
emit CFI which relies on this being correct.

8 files changed, 190 insertions, 12 deletions

diff --git a/.gitignore b/.gitignore
index 7b7905f0..af68f0e6 100644
--- a/.gitignore
+++ b/.gitignore
@@ -75,5 +75,6 @@ tests/[GL]ia64-test-readonly
 tests/[GL]ia64-test-stack
 tests/ia64-test-dyn1
 tests/ia64-test-sig
+tests/[GL]x64-test-dwarf-expressions
 tests/*.log
 tests/*.trs
diff --git a/include/dwarf.h b/include/dwarf.h
index fab93c61..764f6f20 100644
--- a/include/dwarf.h
+++ b/include/dwarf.h
@@ -419,7 +419,7 @@ extern int dwarf_find_unwind_table (struct elf_dyn_info *edi, unw_addr_space_t a
                                     unw_word_t ip);
 extern void dwarf_put_unwind_info (unw_addr_space_t as,
                                    unw_proc_info_t *pi, void *arg);
-extern int dwarf_eval_expr (struct dwarf_cursor *c, unw_word_t *addr,
+extern int dwarf_eval_expr (struct dwarf_cursor *c, unw_word_t stack_val, unw_word_t *addr,
                             unw_word_t len, unw_word_t *valp,
                             int *is_register);
 extern int
diff --git a/src/dwarf/Gexpr.c b/src/dwarf/Gexpr.c
index 709c0c8f..2af45433 100644
--- a/src/dwarf/Gexpr.c
+++ b/src/dwarf/Gexpr.c
@@ -237,8 +237,8 @@ dwarf_stack_aligned(struct dwarf_cursor *c, unw_word_t cfa_addr,
 }
 
 HIDDEN int
-dwarf_eval_expr (struct dwarf_cursor *c, unw_word_t *addr, unw_word_t len,
-                 unw_word_t *valp, int *is_register)
+dwarf_eval_expr (struct dwarf_cursor *c, unw_word_t stack_val, unw_word_t *addr,
+                 unw_word_t len, unw_word_t *valp, int *is_register)
 {
   unw_word_t operand1 = 0, operand2 = 0, tmp1, tmp2 = 0, tmp3, end_addr;
   uint8_t opcode, operands_signature, u8;
@@ -287,10 +287,14 @@ do {                                            \
   end_addr = *addr + len;
   *is_register = 0;
 
-  Debug (14, "len=%lu, pushing cfa=0x%lx\n",
-         (unsigned long) len, (unsigned long) c->cfa);
+  Debug (14, "len=%lu, pushing initial value=0x%lx\n",
+         (unsigned long) len, (unsigned long) stack_val);
 
-  push (c->cfa);        /* push current CFA as required by DWARF spec */
+  /* The DWARF standard requires the current CFA to be pushed onto the stack */
+  /* before evaluating DW_CFA_expression and DW_CFA_val_expression programs. */
+  /* DW_CFA_def_cfa_expressions do not take an initial value, but we push on */
+  /* a dummy value to keep this logic consistent. */
+  push (stack_val);
 
   while (*addr < end_addr)
     {
diff --git a/src/dwarf/Gparser.c b/src/dwarf/Gparser.c
index 1ce862c2..fe7c5817 100644
--- a/src/dwarf/Gparser.c
+++ b/src/dwarf/Gparser.c
@@ -734,7 +734,7 @@ create_state_record_for (struct dwarf_cursor *c, dwarf_state_record_t *sr,
 }
 
 static inline int
-eval_location_expr (struct dwarf_cursor *c, unw_addr_space_t as,
+eval_location_expr (struct dwarf_cursor *c, unw_word_t stack_val, unw_addr_space_t as,
                     unw_accessors_t *a, unw_word_t addr,
                     dwarf_loc_t *locp, void *arg)
 {
@@ -746,7 +746,7 @@ eval_location_expr (struct dwarf_cursor *c, unw_addr_space_t as,
     return ret;
 
   /* evaluate the expression: */
-  if ((ret = dwarf_eval_expr (c, &addr, len, &val, &is_register)) < 0)
+  if ((ret = dwarf_eval_expr (c, stack_val, &addr, len, &val, &is_register)) < 0)
     return ret;
 
   if (is_register)
@@ -804,7 +804,10 @@ apply_reg_state (struct dwarf_cursor *c, struct dwarf_reg_state *rs)
       assert (rs->reg.where[DWARF_CFA_REG_COLUMN] == DWARF_WHERE_EXPR);
 
       addr = rs->reg.val[DWARF_CFA_REG_COLUMN];
-      if ((ret = eval_location_expr (c, as, a, addr, &cfa_loc, arg)) < 0)
+      /* The dwarf standard doesn't specify an initial value to be pushed on */
+      /* the stack before DW_CFA_def_cfa_expression evaluation. We push on a */
+      /* dummy value (0) to keep the eval_location_expr function consistent. */
+      if ((ret = eval_location_expr (c, 0, as, a, addr, &cfa_loc, arg)) < 0)
         return ret;
       /* the returned location better be a memory location... */
       if (DWARF_IS_REG_LOC (cfa_loc))
@@ -844,13 +847,17 @@ apply_reg_state (struct dwarf_cursor *c, struct dwarf_reg_state *rs)
 
         case DWARF_WHERE_EXPR:
           addr = rs->reg.val[i];
-          if ((ret = eval_location_expr (c, as, a, addr, new_loc + i, arg)) < 0)
+          /* The dwarf standard requires the current CFA to be pushed on the */
+          /* stack before DW_CFA_expression evaluation. */
+          if ((ret = eval_location_expr (c, cfa, as, a, addr, new_loc + i, arg)) < 0)
             return ret;
           break;
 
         case DWARF_WHERE_VAL_EXPR:
           addr = rs->reg.val[i];
-          if ((ret = eval_location_expr (c, as, a, addr, new_loc + i, arg)) < 0)
+          /* The dwarf standard requires the current CFA to be pushed on the */
+          /* stack before DW_CFA_val_expression evaluation. */
+          if ((ret = eval_location_expr (c, cfa, as, a, addr, new_loc + i, arg)) < 0)
             return ret;
           new_loc[i] = DWARF_VAL_LOC (c, DWARF_GET_LOC (new_loc[i]));
           break;
diff --git a/tests/Gx64-test-dwarf-expressions.c b/tests/Gx64-test-dwarf-expressions.c
new file mode 100644
index 00000000..209f8713
--- /dev/null
+++ b/tests/Gx64-test-dwarf-expressions.c
@@ -0,0 +1,68 @@
+#include <stdio.h>
+#include <stdint.h>
+#include <stdlib.h>
+
+#include <libunwind.h>
+
+static int verbose;
+static int nerrors;
+
+#define panic(args...)							  \
+	do { printf (args); ++nerrors; } while (0)
+
+// Assembly routine which sets up the stack for the test then calls another one
+// which clobbers the stack, and which in turn calls recover_register below
+extern int64_t DW_CFA_expression_testcase(int64_t regnum, int64_t height);
+
+// recover_register is called by the assembly routines. It returns the value of
+// a register at a specified height from the inner-most frame. The return value
+// is propagated back through the assembly routines to the testcase.
+extern int64_t recover_register(int64_t regnum, int64_t height)
+{
+  // Initialize cursor to current frame
+  int rc, i;
+  unw_cursor_t cursor;
+  unw_context_t context;
+  unw_getcontext(&context);
+  unw_init_local(&cursor, &context);
+  // Unwind frames until required height from inner-most frame (i.e. this one)
+  for (i = 0; i < height; ++i)
+    {
+      rc = unw_step(&cursor);
+      if (rc < 0)
+        panic("%s: unw_step failed on step %d with return code %d", __FUNCTION__, i, rc);
+      else if (rc == 0)
+        panic("%s: unw_step failed to reach the end of the stack", __FUNCTION__);
+      unw_word_t pc;
+      rc = unw_get_reg(&cursor, UNW_REG_IP, &pc);
+      if (rc < 0 || pc == 0)
+        panic("%s: unw_get_reg failed to locate the program counter", __FUNCTION__);
+    }
+  // We're now at the required height, extract register
+  uint64_t value;
+  if ((rc = unw_get_reg(&cursor, (unw_regnum_t) regnum, &value)) != 0)
+    panic("%s: unw_get_reg failed to retrieve register %lu", __FUNCTION__, regnum);
+  return value;
+}
+
+int
+main (int argc, char **argv)
+{
+  if (argc > 1)
+    verbose = 1;
+
+  if (DW_CFA_expression_testcase(12, 1) != 0)
+    panic("r12 should be clobbered at height 1 (DW_CFA_expression_inner)");
+  if (DW_CFA_expression_testcase(12, 2) != 111222333)
+    panic("r12 should be restored at height 2 (DW_CFA_expression_testcase)");
+
+  if (nerrors > 0)
+    {
+      fprintf (stderr, "FAILURE: detected %d errors\n", nerrors);
+      exit (-1);
+    }
+
+  if (verbose)
+    printf ("SUCCESS.\n");
+  return 0;
+}
diff --git a/tests/Lx64-test-dwarf-expressions.c b/tests/Lx64-test-dwarf-expressions.c
new file mode 100644
index 00000000..07e916e6
--- /dev/null
+++ b/tests/Lx64-test-dwarf-expressions.c
@@ -0,0 +1,5 @@
+#define UNW_LOCAL_ONLY
+#include <libunwind.h>
+#if !defined(UNW_REMOTE_ONLY)
+#include "Gx64-test-dwarf-expressions.c"
+#endif
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 4b0b9db7..fee8a70b 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -37,7 +37,11 @@ if ARCH_PPC64
 if USE_ALTIVEC
  noinst_PROGRAMS_arch += ppc64-test-altivec
 endif #USE_ALTIVEC
-endif #ARCH_PPC64
+else  #!ARCH_PPC64
+if ARCH_X86_64
+ check_PROGRAMS_arch +=	Gx64-test-dwarf-expressions Lx64-test-dwarf-expressions
+endif #ARCH X86_64
+endif #!ARCH_PPC64
 endif #!ARCH_IA64
  check_PROGRAMS_cdep +=	Gtest-bt Ltest-bt Gtest-exc Ltest-exc		 \
 			Gtest-init Ltest-init				 \
@@ -139,6 +143,14 @@ Gia64_test_nat_SOURCES = Gia64-test-nat.c ia64-test-nat-asm.S
 ia64_test_dyn1_SOURCES = ia64-test-dyn1.c ia64-dyn-asm.S flush-cache.S \
 				flush-cache.h
 ppc64_test_altivec_SOURCES = ppc64-test-altivec.c  ppc64-test-altivec-utils.c
+
+
+Gx64_test_dwarf_expressions_SOURCES =  Gx64-test-dwarf-expressions.c \
+																			 x64-test-dwarf-expressions.S
+Lx64_test_dwarf_expressions_SOURCES =  Lx64-test-dwarf-expressions.c \
+																			 x64-test-dwarf-expressions.S
+
+
 Gtest_init_SOURCES = Gtest-init.cxx
 Ltest_init_SOURCES = Ltest-init.cxx
 Ltest_cxx_exceptions_SOURCES = Ltest-cxx-exceptions.cxx
@@ -232,3 +244,6 @@ Lia64_test_readonly_LDADD = $(LIBUNWIND_local)
 ia64_test_dyn1_LDADD = $(LIBUNWIND)
 ia64_test_sig_LDADD = $(LIBUNWIND)
 ppc64_test_altivec_LDADD = $(LIBUNWIND)
+
+Gx64_test_dwarf_expressions_LDADD = $(LIBUNWIND) $(LIBUNWIND_local)
+Lx64_test_dwarf_expressions_LDADD = $(LIBUNWIND_local)
diff --git a/tests/x64-test-dwarf-expressions.S b/tests/x64-test-dwarf-expressions.S
new file mode 100644
index 00000000..f275625d
--- /dev/null
+++ b/tests/x64-test-dwarf-expressions.S
@@ -0,0 +1,78 @@
+.global DW_CFA_expression_testcase
+
+.extern recover_register
+
+.text
+
+# CFI expressions were added in DWARF v3 to allow compilers to specify memory
+# locations or register values using DWARF programs. These programs are simple
+# stack-based operations which allow the compiler to encode integer mathematics
+# and other complex logic. CFI expressions are therefore more powerful than the
+# conventional register + offset schemes.
+#
+# These tests capture a bug we have fixed in libunwind. CFI expression programs
+# always start with the current CFA pushed onto the stack. This file contains a
+# pair of routines which test CFI expression parsing. Specifically they test
+# DW_CFA_expression logic, which uses DWARF expressions to compute the address
+# where a non-volatile register was stored.
+#
+# Main calls DW_CFA_expression_testcase, which sets up known state in a
+# non-volatile (caller-saved) register. We use r12 for this purpose. After this
+# DW_CFA_expression_testcase then calls DW_CFA_expression_inner, which clobbers
+# r12 after stashing its value on the stack. This routine contains a DWARF3 CFI
+# expression to restore the value of r12 on unwind which should allow libunwind
+# to recover clobbered state. DW_CFA_expression_inner calls recover_register to
+# retrieve the cached register value. This function recovers the register value
+# by using libunwind to unwind the stack through DW_CFA_expression_inner and up
+# to the call site in DW_CFA_expression_testcase. If our expression is correct,
+# libunwind will be able to restore r12 from the stack.
+#
+# BE CAREFUL WITH rdi, rsi, rax HERE! The arguments to recover_register are
+# passed in via rdi, rsi and I just let them flow through unchanged. Similarly
+# RAX flows back unchanged. Adding any function calls to the below may clobber
+# these registers and cause this test to fail mysteriously.
+
+
+########################################################
+# Test: Restoring a register using a DW_CFA_expression #
+# which uses implicit CFA pushed onto stack.           #
+########################################################
+
+.type DW_CFA_expression_testcase STT_FUNC
+DW_CFA_expression_testcase:
+  .cfi_startproc
+  push %r12
+  .cfi_adjust_cfa_offset 8
+  # Move our sentinel (known) value into non-volatile (Callee-saved) r12
+  mov $111222333, %r12
+  .cfi_rel_offset %r12, 0
+  call DW_CFA_expression_inner
+  pop %r12
+  .cfi_restore %r12
+  .cfi_adjust_cfa_offset -8
+  ret
+  .cfi_endproc
+.size DW_CFA_expression_testcase,.-DW_CFA_expression_testcase
+
+.type DW_CFA_expression_inner STT_FUNC
+DW_CFA_expression_inner:
+  .cfi_startproc
+  push %r12
+  .cfi_adjust_cfa_offset 8
+  # !! IMPORTANT BIT !! The test is all about how we parse the following bytes.
+  # Now we use an expression to describe where our sentinel value is stored:
+  # DW_CFA_expression(0x10), r12(0x0c), Length(0x02),        (preamble)
+  # DW_OP_lit16(0x40), DW_OP_minus(0x1c)                     (instructions)
+  # Parsing starts with the CFA on the stack, then pushes 16, then does a minus
+  # which is eqivalent to a=pop(), b=pop(), push(b-a), leaving us with a value
+  # of cfa-16 (cfa points at old rsp, cfa-8 is our rip, so we stored r12 at
+  # cfa-16).
+  xor %r12, %r12                             # Trash r12
+  .cfi_escape 0x10, 0x0c, 0x2, 0x40, 0x1c   # DW_CFA_expression for recovery
+  call recover_register
+  pop %r12
+  .cfi_restore %r12
+  .cfi_adjust_cfa_offset -8
+  ret
+  .cfi_endproc
+.size DW_CFA_expression_inner,.-DW_CFA_expression_inner