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

Andrew Burgess
2026-04-09T08:55:03Z

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