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

  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.
---
 gdb/infrun.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
  

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,
- Tom
  

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.  */
  

On 4/9/26 10:54 AM, Andrew Burgess wrote:
>> 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>

Hi Andrew,

my initial idea here was that the patch I posted could be trivially 
merged with the first patch, but upon attempting this I ended up 
re-editing the series.  I'm currently testing this, and will submit if 
that goes ok.

The content of the patch series should be what you posted plus 
aforementioned patch, the changes I made are merely refactoring.

Thanks,
- Tom
  

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

> On 4/9/26 10:54 AM, Andrew Burgess wrote:
>>> 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>
>
> Hi Andrew,
>
> my initial idea here was that the patch I posted could be trivially 
> merged with the first patch, but upon attempting this I ended up 
> re-editing the series.  I'm currently testing this, and will submit if 
> that goes ok.
>
> The content of the patch series should be what you posted plus 
> aforementioned patch, the changes I made are merely refactoring.

The patch you posted applies on top of my patch #1, but I think it would
be best if you just merged those two patches together.   As you point
out, my patch moves in the wrong direction.  You're patch basically
reverts mine and does something different, which is the right thing to
do.

Thanks,
Andrew
  

On 4/10/26 10:57 AM, Andrew Burgess wrote:
> Tom de Vries <tdevries@suse.de> writes:
> 
>> On 4/9/26 10:54 AM, Andrew Burgess wrote:
>>>> 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>
>>
>> Hi Andrew,
>>
>> my initial idea here was that the patch I posted could be trivially
>> merged with the first patch, but upon attempting this I ended up
>> re-editing the series.  I'm currently testing this, and will submit if
>> that goes ok.
>>
>> The content of the patch series should be what you posted plus
>> aforementioned patch, the changes I made are merely refactoring.
> 
> The patch you posted applies on top of my patch #1, but I think it would
> be best if you just merged those two patches together.   As you point
> out, my patch moves in the wrong direction.  You're patch basically
> reverts mine and does something different, which is the right thing to
> do.

The problem with doing so is that it makes the first patch nonsensical, 
in the sense that it results in using the selected frame in one part of 
the condition, and the stop pc (corresponding to the current frame or 
stop frame) in another part of the condition.  So I ended up moving the 
patch to the end of the series.

Submitted here ( 
https://sourceware.org/pipermail/gdb-patches/2026-April/226422.html ).

Thanks,
- Tom