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

[Tom de Vries <tdevries@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 2598 bytes --]

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

[-- Attachment #2: tmp.patch --]
[-- Type: text/x-patch, Size: 1782 bytes --]

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