Re: [PATCHv2 1/2] gdb: remove sentinel frame check in frame_find_by_id

[Andrew Burgess <aburgess@redhat.com>]

Simon Marchi <simark@simark.ca> writes:

> On 6/22/26 5:33 PM, Andrew Burgess wrote:
>> While reviewing another patch Simon pointed out that the code in
>> frame_find_by_id for looking up the sentinel frame is no longer needed
>> since commit:
>> 
>>   commit 19f988359a62889b00d67fb59ef8c7d6d759fc98
>>   Date:   Mon Jan 30 15:02:49 2023 -0500
>> 
>>     gdb: give sentinel for user frames distinct IDs, register sentinel frames to the frame cache
>> 
>> Prior to this commit the sentinel_frame was not added to the frame
>> stash, so we needed a manual check to lookup the sentinel frame.
>> After this commit the sentinel frame is added to the stash so the
>> manual check, though perfectly valid, is just unnecessary complexity,
>> and can be removed.  A frame stash lookup is just a hash lookup, and
>> is relatively cheap, so switching to this isn't much extra cost.
>> 
>> While I was working in frame_find_by_id I made a couple of additional
>> changes:
>> 
>>   1. I moved the declarations of frame and prev_frame locals into the
>>      function body, closer to where they are first defined.
>> 
>>   2. Instead of treating a frame_info_ptr as a bool, I made use of the
>>      frame_info_ptr::is_null method.
>> 
>> There should be no user visible changes after this commit.
>> ---
>>  gdb/frame.c | 13 ++++---------
>>  1 file changed, 4 insertions(+), 9 deletions(-)
>> 
>> diff --git a/gdb/frame.c b/gdb/frame.c
>> index f64f5554f8c..c1a7c41dc4a 100644
>> --- a/gdb/frame.c
>> +++ b/gdb/frame.c
>> @@ -959,17 +959,11 @@ frame_id_inner (struct gdbarch *gdbarch, struct frame_id l, struct frame_id r)
>>  frame_info_ptr
>>  frame_find_by_id (struct frame_id id)
>>  {
>> -  frame_info_ptr frame, prev_frame;
>> -
>>    /* ZERO denotes the null frame, let the caller decide what to do
>>       about it.  Should it instead return get_current_frame()?  */
>>    if (!frame_id_p (id))
>>      return NULL;
>>  
>> -  /* Check for the sentinel frame.  */
>> -  if (id == frame_id_build_sentinel (0, 0))
>> -    return frame_info_ptr (sentinel_frame);
>
> From what you know, was this check even working today?  This compares
> the id we're looking for (which presumably has "real" addresses) with
> "code" and "special" addresses set to 0.

Yes it was working.  In get_current_frame, where we build the actual
stack based on the current register state, the sentinel frame is created
with addresses 0 and 0 for stack and code.

It's only for the case of user created frames, where the user provides a
stack and code address (in create_new_frame) that we create a sentinel
frame with actual, real addresses.

So in the first case the above check would find the sentinel frame, but
in the second we relied on the cache.

I don't see any reason why these two cases need to be handled
differently.  Maybe there's a tiny performance improvement from the hard
coded check, but unless someone complains I think just handling both
cases via the frame cache makes more sense.

Thanks,
Andrew

>
> In any case, LGTM.
>
> Approved-By: Simon Marchi <simon.marchi@efficios.com>
>
> Simon