Re: [PATCH v2] gdb/arm: Stop unwinding on error, but do not assert

[Pedro Alves <pedro@palves.net>]

On 2022-10-13 1:24 p.m., Torbjorn SVENSSON wrote:
> Hi Pedro,
> 
> On 2022-10-13 13:21, Pedro Alves wrote:
>> On 2022-10-13 10:17 a.m., Torbjörn SVENSSON via Gdb-patches wrote:
>>
>>> +  /* Unwind of this frame is not possible.  Return outer_frame_id to stop the
>>> +     unwinding.  */
>>> +  if (cache == NULL)
>>> +    {
>>> +      *this_id = outer_frame_id;
>>> +      return;
>>> +    }
>>
>> Please let's not add more uses of outer_frame_id if we can avoid it.  We're getting
>> close to eliminating it.  Can a cache object still be returned, and then a frame id
>> be successfully computed?
> 
> The problem is that it's not always possible to know what registers that was written on the stack or if there was padding between 2 frames on the stack.
> If a cache object is returned, wouldn't that imply that the content of this frame is supposed to be valid?

But that info is useful to unwind the prev frame from this_frame.  The contents of registers for this_frame are
unwound from this_frame->next.  The info needed to compute a frame id should be available.

On 2022-10-13 1:24 p.m., Torbjorn SVENSSON wrote:

>> Alternatively, you can implement a frame_unwind::stop_reason callback and return
>> UNWIND_OUTERMOST, which is already done in arm-tdep.c in other scenarios too.
> 
> Is it guaranteed that the prev_register method won't be called for a cache object that have the UNWIND_OUTERMOST stop reason?

That's how outer_frame_id manages to stop unwinding, because of the default implementation, here:

 /* The default frame unwinder stop_reason callback.  */
 
 enum unwind_stop_reason
 default_frame_unwind_stop_reason (frame_info_ptr this_frame,
 				  void **this_cache)
 {
   struct frame_id this_id = get_frame_id (this_frame);
 
   if (this_id == outer_frame_id)
     return UNWIND_OUTERMOST;
   else
     return UNWIND_NO_REASON;
 }

You hit this in frame.c:

  /* Check that this frame is unwindable.  If it isn't, don't try to
     unwind to the prev frame.  */
  this_frame->stop_reason
    = this_frame->unwind->stop_reason (this_frame,
				       &this_frame->prologue_cache);

  if (this_frame->stop_reason != UNWIND_NO_REASON)
    {
      frame_debug_printf
	("  -> nullptr // %s",
	 frame_stop_reason_symbol_string (this_frame->stop_reason));
      return NULL;
    }

> If so, I suppose the struct arm_prologue_cache could be extended with another member that indicates if the frame was
> successfully unwinded or if there were some problem and the UNWIND_OUTERMOST should be returned. Would this be okay?

Yes.