Relax ARM prologue unwinder assumption
Commit Message
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
Comments
On 02/09/2015 02:51 PM, Luis Machado wrote:
> 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.
That looks good to me.
Thanks,
Pedro Alves
On 02/10/2015 07:37 AM, Pedro Alves wrote:
> On 02/09/2015 02:51 PM, Luis Machado wrote:
>
>> 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.
>
> That looks good to me.
>
> Thanks,
> Pedro Alves
>
I've checked this in now.
Thanks,
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.
@@ -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,