Re: [PATCH] Relax ARM prologue unwinder assumption

[Luis Machado <lgustavo at codesourcery dot com>]

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

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,