[binutils-gdb] [ARM] "svc" insn check at irrelevant address in ARM unwind info sniffer

[Joel Brobecker <brobecke at sourceware dot org>]

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=416dc9c6e9acd57255015d255799ac031a262182

commit 416dc9c6e9acd57255015d255799ac031a262182
Author: Joel Brobecker <brobecker@adacore.com>
Date:   Mon Nov 23 09:50:55 2015 -0800

    [ARM] "svc" insn check at irrelevant address in ARM unwind info sniffer
    
    The following issue has been observed on arm-android, trying to step
    over the following line of code:
    
            Put_Line (">>> " & Integer'Image (Message (I)));
    
    Below is a copy of the GDB transcript:
    
        (gdb) cont
        Breakpoint 1, q.dump (message=...) at q.adb:11
        11               Put_Line (">>> " & Integer'Image (Message (I)));
        (gdb) next
        0x00016000 in system.concat_2.str_concat_2 ()
    
    The expected behavior for the "next" command is to step over
    the call to Put_Line and stop at line 12:
    
        (gdb) next
        12               I := I + 1;
    
    What happens during the next step is that the code for line 11
    above make a call to system.concat_2.str_concat_2 (to implement
    the '&' string concatenation operator) before making the call
    to Put_Line. While stepping, GDB stops eventually stops at the
    first instruction of that function, and fails to detect that
    it's a function call from where we were before, and so decides
    to stop stepping.
    
    And the reason why it fails to detect that we landed inside a function
    call is because it fails to unwind from that function:
    
        (gdb) bt
        #0  0x00016000 in system.concat_2.str_concat_2 ()
        #1  0x0001bc74 in ?? ()
    
    Debugging GDB, I found that GDB decides to use the ARM unwind info
    for that function, which contains the following data:
    
        0x16000 <system__concat_2__str_concat_2>: 0x80acb0b0
          Compact model index: 0
          0xac      pop {r4, r5, r6, r7, r8, r14}
          0xb0      finish
          0xb0      finish
    
    But, in fact, using that data is wrong, in this case, because
    it mentions a pop of 6 registers, and therefore hints at a frame
    size of 24 bytes. The problem is that, because we're at the first
    instruction of the function, the 6 registers haven't been pushed
    to the stack yet. In other words, using the ARM unwind entry above,
    GDB is tricked into thinking that the frame size is 24 bytes, and
    that the return address (r14) is available on the stack.
    
    One visible manifestation of this issue can been seen by looking
    at the value of the stack pointer, and the frame's base address:
    
        (gdb) p /x $sp
        $2 = 0xbee427b0
        (gdb) info frame
        Stack level 0, frame at 0xbee427c8:
                                ^^^^^^^^^^
                                ||||||||||
    
    The frame's base address should be equal to the value of the stack
    pointer at entry. And you eventually get the correct frame address,
    as well as the correct backtrace if you just single-step one additional
    instruction, past the push:
    
        (gdb) x /i $pc
        => 0x16000 <system__concat_2__str_concat_2>:
            push        {r4, r5, r6, r7, r8, lr}
        (gdb) stepi
        (gdb) bt
        #0  0x00016004 in system.concat_2.str_concat_2 ()
        #1  0x00012b6c in q.dump (message=...) at q.adb:11
        #2  0x00012c3c in q () at q.adb:19
    
    Digging further, I found that GDB tries to use the ARM unwind info
    only when sure that it is relevant, as explained in the following
    comment:
    
      /* The ARM exception table does not describe unwind information
         for arbitrary PC values, but is guaranteed to be correct only
         at call sites.  We have to decide here whether we want to use
         ARM exception table information for this frame, or fall back [...]
    
    There is one case where it decides that the info is relevant,
    described in the following comment:
    
          /* We also assume exception information is valid if we're currently
             blocked in a system call.  The system library is supposed to
             ensure this, so that e.g. pthread cancellation works.
    
    For that, it just parses the instruction at the address it believes
    to be the point of call, and matches it against an "svc" instruction.
    For instance, for a non-thumb instruction, it is at...
    
        get_frame_pc (this_frame) - 4
    
    ... and the code checking looks like the following.
    
                  if (safe_read_memory_integer (get_frame_pc (this_frame) - 4, 4,
                                                byte_order_for_code, &insn)
                      && (insn & 0x0f000000) == 0x0f000000 /* svc */)
                    exc_valid = 1;
    
    However, the reason why this doesn't work in our case is that
    because we are at the first instruction of a function in the innermost
    frame. That frame can't possibly be making a call, and therefore
    be stuck on a system call.
    
    What the code above ends up doing is checking the instruction
    just before the start of our function, which in our case is not
    even an actual instruction, but unlucky for us, happens to match
    the pattern it is looking for, thus leading GDB to improperly
    trust the ARM unwinding data.
    
    gdb/ChangeLog:
    
            * arm-tdep.c (arm_exidx_unwind_sniffer): Do not check for a frame
            stuck on a system call if the given frame is the innermost frame.

Diff:
---
 gdb/ChangeLog  |  5 +++++
 gdb/arm-tdep.c | 43 +++++++++++++++++++++++++++----------------
 2 files changed, 32 insertions(+), 16 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 437dbc2..5655ccb 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,10 @@
 2015-11-23  Joel Brobecker  <brobecker@adacore.com>
 
+	* arm-tdep.c (arm_exidx_unwind_sniffer): Do not check for a frame
+	stuck on a system call if the given frame is the innermost frame.
+
+2015-11-23  Joel Brobecker  <brobecker@adacore.com>
+
 	* dwarf2read.c (read_structure_type): Set the type's length
 	to zero if it has a DW_AT_byte_size attribute which is not
 	a constant.
diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index ef1a007..6ce6f09c 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -2814,26 +2814,37 @@ arm_exidx_unwind_sniffer (const struct frame_unwind *self,
 
       /* We also assume exception information is valid if we're currently
 	 blocked in a system call.  The system library is supposed to
-	 ensure this, so that e.g. pthread cancellation works.  */
-      if (arm_frame_is_thumb (this_frame))
-	{
-	  LONGEST insn;
+	 ensure this, so that e.g. pthread cancellation works.
 
-	  if (safe_read_memory_integer (get_frame_pc (this_frame) - 2, 2,
-					byte_order_for_code, &insn)
-	      && (insn & 0xff00) == 0xdf00 /* svc */)
-	    exc_valid = 1;
-	}
-      else
+	 But before verifying the instruction at the point of call, make
+	 sure this_frame is actually making a call (or, said differently,
+	 that it is not the innermost frame).  For that, we compare
+	 this_frame's PC vs this_frame's addr_in_block. If equal, it means
+	 there is no call (otherwise, the PC would be the return address,
+	 which is the instruction after the call).  */
+
+      if (get_frame_pc (this_frame) != addr_in_block)
 	{
-	  LONGEST insn;
+	  if (arm_frame_is_thumb (this_frame))
+	    {
+	      LONGEST insn;
 
-	  if (safe_read_memory_integer (get_frame_pc (this_frame) - 4, 4,
-					byte_order_for_code, &insn)
-	      && (insn & 0x0f000000) == 0x0f000000 /* svc */)
-	    exc_valid = 1;
+	      if (safe_read_memory_integer (get_frame_pc (this_frame) - 2, 2,
+					    byte_order_for_code, &insn)
+		  && (insn & 0xff00) == 0xdf00 /* svc */)
+		exc_valid = 1;
+	    }
+	  else
+	    {
+	      LONGEST insn;
+
+	      if (safe_read_memory_integer (get_frame_pc (this_frame) - 4, 4,
+					    byte_order_for_code, &insn)
+		  && (insn & 0x0f000000) == 0x0f000000 /* svc */)
+		exc_valid = 1;
+	    }
 	}
-	
+
       /* Bail out if we don't know that exception information is valid.  */
       if (!exc_valid)
 	return 0;