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

[Kevin Buettner <kevinb@redhat.com>]

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>