Re: [PATCH] gdb: fix edge case assertion from get_selected_frame

[Andrew Burgess <aburgess@redhat.com>]

Kevin Buettner <kevinb@redhat.com> writes:

> Hi Andrew,
>
> On Sat,  7 Mar 2026 14:10:19 +0000
> Andrew Burgess <aburgess@redhat.com> wrote:
>
>> diff --git a/gdb/frame.c b/gdb/frame.c
>> index 8cb1d0a5c42..8644e029e61 100644
>> --- a/gdb/frame.c
>> +++ b/gdb/frame.c
>> @@ -1874,21 +1874,30 @@ lookup_selected_frame (struct frame_id
>> a_frame_id, int frame_level) return;
>>      }
>>  
>> -  /* Nothing else to do, the frame layout really changed.  Select the
>> -     innermost stack frame.  */
>> -  select_frame (get_current_frame ());
>> +  /* We are unable to restore the required frame, so instead we'll
>> +     select the current (innermost) frame.  Do this before actually
>> +     setting the frame as print_stack_frame can make calls into
>> +     extension language hooks, which could invalidate the frame cache,
>> +     which will clear the selected frame.
>
> I found this comment puzzling.  I initially interpreted "Do this before..."
> as refering to the processs of selecting a frame as mentioned in the
> previous sentence.  But I now think that "this" is actually referring to the
> code below the comment.  Perhaps this construction is less confusing?...
>
>   /* We are unable to restore the required frame, so instead we'll
>      select the current (innermost) frame. We print the warning first
>      as print_stack_frame can make calls into extension language hooks,
>      which could invalidate the frame cache and clear the selected frame.
>      ...
>
>>  
>> -  /* Warn the user.  */
>> +     We only warn the user if we're trying to select something other
>> +     than frame #0 though, as the fallback is to just select the
>> +     current frame #0, even if it's different to the frame #0 we tried
>> +     to find (e.g. the frame-id changed).  */
>>    if (frame_level > 0 && !current_uiout->is_mi_like_p ())
>>      {
>> -      warning (_("Couldn't restore frame #%d in "
>> -		 "current thread.  Bottom (innermost) frame selected:"),
>> +      warning (_("Couldn't restore frame #%d in current thread.  "
>> +		 "Innermost frame selected:"),
>>  	       frame_level);
>>        /* For MI, we should probably have a notification about current
>>  	 frame change.  But this error is not very likely, so don't
>>  	 bother for now.  */
>> -      print_stack_frame (get_selected_frame (NULL), 1, SRC_AND_LOC, 1);
>> +      print_stack_frame (get_current_frame (), 1, SRC_AND_LOC, 1);
>>      }
>> +
>> +  /* We couldn't find the frame we were looking for, so just restore
>> +     the innermost frame instead.  */
>> +  select_frame (get_current_frame ());
>>  }
>
> You might also consider adding a comment regarding this observation
> from your commit message:
>
>     ... There is just one theoretical bug that I haven't tried to
>     fix; if the pretty printer changed the machine state then it is
>     possible that the current frame that's selected is not the current
>     frame that was printed.  This could be confusing, but not I think
>     fatal.  Trying to fix this felt like taking things too far, so
>     I've left this for now.
>
> Perhaps something like this?:
>
> /* Note: get_current_frame() is called twice - once for printing and
>    again for selection.  In theory, an extension language hook could
>    change machine state between these calls, making the printed and
>    selected frames different.  This could be confusing but is likely
>    not fatal.  */
>
> Aside from those documentation nits, the change and the test look fine
> to me.
>
> Approved-by: Kevin Buettner <kevinb@redhat.com>

I improved these comments as you suggested, and pushed this patch.

Thanks,
Andrew