Re: [PATCH 1/2] gdb: use get_current_frame consistently in print_stop_location

[Andrew Burgess <aburgess@redhat.com>]

Tom de Vries <tdevries@suse.de> writes:

> On 4/5/26 12:12 PM, Andrew Burgess wrote:
>> In print_stop_location, in the PRINT_UNKNOWN case we currently use a
>> strange mix of get_current_frame and get_selected_frame.  This works
>> fine because at the point print_stop_location is called the selected
>> frame will always be the current frame, but calling these two
>> different functions is confusing, at least for me.
>> 
>> As we are stopping, and deciding whether to print information about
>> the frame, it makes sense, I think, to make the choice based on the
>> current frame, and so let's call get_current_frame once, and then use
>> that result throughout the decision making process.
>> 
>> There should be no user visible changes after this commit.
>
> Hi Andrew,
>
> thanks for looking into this, for me using these two different functions 
> is also confusing.
>
> While reviewing this patch I wondered why this line at the end of the 
> function still uses get_selected_frame:
> ...
>      print_stack_frame (get_selected_frame (nullptr), 0, source_flag, 1);
> ...
> so I tried to use get_current_frame () there as well, and ran into 
> trouble in these ada test-cases:
> ...
>        1 gdb.ada/catch_assert_if.exp:
>        6 gdb.ada/catch_ex.exp:
>        6 gdb.ada/excep_handle.exp:
>        1 gdb.ada/mi_catch_assert.exp:
>        6 gdb.ada/mi_catch_ex.exp:
>        3 gdb.ada/mi_catch_ex_hand.exp:
>        1 gdb.ada/mi_ex_cond.exp:
> ...
>
> I investigated this, and found that bpstat_print may change the selected 
> frame, specifically ada_catchpoint::print_it which calls 
> ada_find_printable_frame.  As it happens, ada_catchpoint::print_it 
> returns PRINT_SRC_AND_LOC, so it doesn't interact with the PRINT_UNKNOWN 
> case.
>
> But if I consider the scenario where ada_catchpoint::print_it would 
> return PRINT_UNKNOWN, it seems more logical to me to use the selected 
> frame there as well.
>
> That is, if we're stepping in some function foo, and run into an ada 
> catchpoint, and ada_catchpoint::print_it selects the frame in foo, and 
> ada_catchpoint::print_it returns PRINT_UNKNOWN, you want to use the 
> selected frame in the PRINT_UNKNOWN case to select SRC_LINE, which is 
> appropriate because as far as the user can see, we haven't left foo.
>
> So I'm proposing a change to this patch (attached below, doesn't apply 
> to the first but to the second patch) that:
> - introduces a variable print_frame
> - assigns get_selected_frame (nullptr) to print_frame
> - adds a comment explaining how print_frame relates to the stop frame
> - uses print_frame everywhere in the function
>
> I tested the series in combination with the attached patch on 
> x86_64-linux, and found no regressions.

Thanks for the great analysis, and explanation, I wasn't aware of this
aspect of bpstat_print.  Given this new information, I think your patch
is the right solution.

Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks,
Andrew



>
> Thanks,
> - Tom
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index 344f13df4fa..687390f24dd 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -9349,18 +9349,22 @@ print_stop_location (const target_waitstatus &ws)
>    struct thread_info *tp = inferior_thread ();
>  
>    bpstat_ret = bpstat_print (tp->control.stop_bpstat, ws.kind ());
> +  /* Function bpstat_print selects the frame to print.  Typically, that is the
> +     stop frame, in other words get_current_frame ().  But bpstat_print may
> +     select a different frame, see for instance ada_catchpoint::print_it.  */
> +  frame_info_ptr print_frame = get_selected_frame (nullptr);
> +
>    switch (bpstat_ret)
>      {
>      case PRINT_UNKNOWN:
>        /* FIXME: cagney/2002-12-01: Given that a frame ID does (or
>  	 should) carry around the function and does (or should) use
>  	 that when doing a frame comparison.  */
> -      if (frame_info_ptr frame = get_current_frame ();
> -	  tp->control.stop_step
> -	  && (tp->control.step_frame_id == get_frame_id (frame))
> -	  && tp->control.in_step_start_function (frame))
> +      if (tp->control.stop_step
> +	  && (tp->control.step_frame_id == get_frame_id (print_frame))
> +	  && tp->control.in_step_start_function (print_frame))
>  	{
> -	  symtab_and_line sal = find_frame_sal (frame);
> +	  symtab_and_line sal = find_frame_sal (print_frame);
>  	  if (sal.symtab != tp->current_symtab)
>  	    {
>  	      /* Finished step in same frame but into different file, print
> @@ -9403,7 +9407,7 @@ print_stop_location (const target_waitstatus &ws)
>       LOCATION: Print only location
>       SRC_AND_LOC: Print location and source line.  */
>    if (do_frame_printing)
> -    print_stack_frame (get_selected_frame (nullptr), 0, source_flag, 1);
> +    print_stack_frame (print_frame, 0, source_flag, 1);
>  }
>  
>  /* See `print_stop_event` in infrun.h.  */