Re: [PATCH] Relax ARM prologue unwinder assumption

[Luis Machado <lgustavo@codesourcery.com>]

[-- Attachment #1: Type: text/plain, Size: 2722 bytes --]

On 02/06/2015 01:25 PM, Pedro Alves wrote:
> On 02/05/2015 07:06 PM, Luis Machado wrote:
>> Just recently i ran into a problem with a bare metal ARM target where
>> GDB would not allow some registers to be changed after the PC was
>> manually set to some other value.
>>
>> In reality the target had just started and one of its cores, out of
>> four, was running the program, but the other ones were in some random state.
>>
>> The PC of one of the other 3 cores was then adjusted to point to a known
>> function address.
>>
>> GDB's reaction to this change is to invalidate the regcache and frame
>> and build a brand new chain and cache, while trying to retain the
>> previously selected frame - #0 in this case.
>>
>> What i noticed is that GDB was selecting frame #1 instead of #0 due to
>> unfortunate coincidences with both frames' SP's being 0. And we can't
>> modify some registers on non-innermost frames for obvious reasons.
>>
>> Here's a brief log of what happens:
>>
>>
>> [Switching to thread 2 (Thread 2)]
>> #0  0x0093ff10 in ?? ()
>> (gdb) set $pc=functioncore2
>> (gdb) bt
>> #0  functioncore2 () at test.c:32
>> #1  0x0000fc44 in ?? ()
>
> So in this case, frame #0's unwinder must now be the dwarf
> unwinder, right?
>

Correct.

>> The attached patch removes this restriction and does not cause any
>> regressions for ARM bare metal, but i'd like feedback.
>
> Nowadays we have the frame_unwind_stop_reason hook to make it possible
> to have different outermost frame ids, but still have the frame claim
> that its the outermost, instead of forcing all outermost frame ids
> use the outer_frame_id id.  See i386_frame_unwind_stop_reason.
>
> Sounds like ARM could add an arm_frame_unwind_stop_reason that
> returns UNWIND_OUTERMOST when prev_sp is 0.
>
> And it looks like this bit here:
>
>    /* This is meant to halt the backtrace at "_start".  */
>    pc = get_frame_pc (this_frame);
>    if (pc <= gdbarch_tdep (get_frame_arch (this_frame))->lowest_pc)
>      return;
>
> can well cause a fail in the same way.
>

Thanks. That mechanism does sound a bit more flexible and it indeed 
solves the problem i'm seeing.

How does the updated patch look? I have arm_prologue_unwind_stop_reason 
implemented now.

> (It may also make sense to think through the cases where we are
> trying to restore the selected frame, and change that code.  It may be
> that it never makes sense to go from frame #0 to any other frame
> number, for instance.)

Trying to come up with the best match for a certain frame that 
potentially does not exist anymore can be troublesome, but it's been 
working well so far. I'd go for always picking up frame 0 as well. It is 
easier, cleaner and less prone to failures.

Luis

[-- Attachment #2: arm_unwind.diff --]
[-- Type: text/x-patch, Size: 2318 bytes --]

2015-02-09  Luis Machado  <lgustavo@codesourcery.com>

	* arm-tdep.c (arm_prologue_unwind_stop_reason): New function.
	(arm_prologue_this_id): Move PC and SP limit checks to
	arm_prologue_unwind_stop_reason.
	(arm_prologue_unwind) <stop_reason> : Set to
	arm_prologue_unwind_stop_reason.

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index 8e9552a..f3a6325 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -2021,6 +2021,31 @@ arm_make_prologue_cache (struct frame_info *this_frame)
   return cache;
 }
 
+/* Implementation of the stop_reason hook for arm_prologue frames.  */
+
+static enum unwind_stop_reason
+arm_prologue_unwind_stop_reason (struct frame_info *this_frame,
+				 void **this_cache)
+{
+  struct arm_prologue_cache *cache;
+  CORE_ADDR pc;
+
+  if (*this_cache == NULL)
+    *this_cache = arm_make_prologue_cache (this_frame);
+  cache = *this_cache;
+
+  /* This is meant to halt the backtrace at "_start".  */
+  pc = get_frame_pc (this_frame);
+  if (pc <= gdbarch_tdep (get_frame_arch (this_frame))->lowest_pc)
+    return UNWIND_OUTERMOST;
+
+  /* If we've hit a wall, stop.  */
+  if (cache->prev_sp == 0)
+    return UNWIND_OUTERMOST;
+
+  return UNWIND_NO_REASON;
+}
+
 /* Our frame ID for a normal frame is the current function's starting PC
    and the caller's SP when we were called.  */
 
@@ -2037,18 +2062,10 @@ arm_prologue_this_id (struct frame_info *this_frame,
     *this_cache = arm_make_prologue_cache (this_frame);
   cache = *this_cache;
 
-  /* This is meant to halt the backtrace at "_start".  */
-  pc = get_frame_pc (this_frame);
-  if (pc <= gdbarch_tdep (get_frame_arch (this_frame))->lowest_pc)
-    return;
-
-  /* If we've hit a wall, stop.  */
-  if (cache->prev_sp == 0)
-    return;
-
   /* Use function start address as part of the frame ID.  If we cannot
      identify the start address (due to missing symbol information),
      fall back to just using the current PC.  */
+  pc = get_frame_pc (this_frame);
   func = get_frame_func (this_frame);
   if (!func)
     func = pc;
@@ -2117,7 +2134,7 @@ arm_prologue_prev_register (struct frame_info *this_frame,
 
 struct frame_unwind arm_prologue_unwind = {
   NORMAL_FRAME,
-  default_frame_unwind_stop_reason,
+  arm_prologue_unwind_stop_reason,
   arm_prologue_this_id,
   arm_prologue_prev_register,
   NULL,