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

Author: brobecke

gdb/ChangeLog

@@ -1,3 +1,8 @@
+2015-11-23  Joel Brobecker  <[email protected]>
+
+	* 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  <[email protected]>
 
 	* dwarf2read.c (read_structure_type): Set the type's length

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;