Re: [PATCHv8 3/4] gdb: allow 'until' to work in outermost frame

Hi Simon,

Sorry it has taken me a while to get back to this series.  I have some
feedback inline below.

Simon Marchi <[email protected]> writes:

> On 7/20/26 5:52 AM, Andrew Burgess wrote:
>> The 'until' command with an argument, e.g. 'until *ADDRESS', is
>> implemented by until_break_command in breakpoint.c.
>> 
>> The most important thing this function does is convert the location
>> argument, '*ADDRESS' in my example, into a vector of symtab_and_line
>> objects.  Each of these symtab_and_line objects is then used to create
>> a temporary bp_until breakpoint.
>> 
>> The other thing that until_break_command does is create a breakpoint
>> in the caller frame.  This breakpoint is there to ensure the inferior
>> stops upon exiting the frame in which the 'until' command was issued,
>> if the until breakpoint was not hit.
>> 
>> When compiling an assembler file into a static test program, if I use
>> 'starti' to stop the inferior within the outermost frame, and then try
>> to use 'until *ADDRESS' I see the following error:
>> 
>>   (gdb) starti
>>   Starting program: /tmp/hello
>> 
>>   Program stopped.
>>   0x0000000000401000 in _start ()
>>   (gdb) bt
>>   #0  0x0000000000401000 in _start ()
>>   (gdb) until *0x0000000000401015
>>   Warning:
>>   Cannot insert breakpoint 0.
>>   Cannot access memory at address 0x1
>> 
>>   Command aborted.
>>   (gdb)
>> 
>> The problem here is the breakpoint that 'until' tries to create in the
>> caller frame.  Though the 'bt' in the above example indicates that
>> there is only a single frame, the outermost frame, this is only
>> because GDB has specific code in get_prev_frame to stop the backtrace
>> at the outermost frame.  If we turn this off and try 'bt' again:
>> 
>>   (gdb) set backtrace past-entry on
>>   (gdb) bt
>>   #0  0x0000000000401000 in _start ()
>>   #1  0x0000000000000001 in ?? ()
>>   #2  0x00007fffffffac73 in ?? ()
>>   #3  0x0000000000000000 in ?? ()
>>   (gdb)
>> 
>> What we are seeing here is the garbage values that happen to be in the
>> registers tricking GDB into thinking there are frames before _start.
>> 
>> GDB's code to handle this is in get_prev_frame, where we call
>> inside_entry_func.  This checks if a frame is one of the two possible
>> entry frames, the inferior entry frame or the executable entry frame.
>> See the previous commit for more details.  The important thing is that
>> the inferior entry frame is the absolute outer frame, the very first
>> frame that the inferior executed when starting, while the executable
>> entry frame is just the first frame within the main executable.
>> 
>> There are a number of user configurable filters in get_prev_frame, the
>> backtrace past-main filter, the backtrace frame limit filter, and the
>> backtrace past-entry filter that we are discussing here.
>> 
>> When creating the caller frame breakpoint, the 'until' command doesn't
>> use get_prev_frame, it uses frame_unwind_caller_frame, which calls
>> get_prev_frame_always.  This is so that the user configurable filters
>> don't prevent the caller frame breakpoint from being created.  But
>> this means that when creating the breakpoint, we skip the entry frame
>> check.  As a result, the 'until' command will try to place a
>> breakpoint in the bogus frame #1 shown above.  As the frame is at
>> address 0x1, which is non-writable, we see an error when trying to
>> insert the breakpoint.
>> 
>> In this commit I propose that we add an entry frame check into
>> frame_unwind_caller_frame, similar to the one found in
>> get_prev_frame.  However, while get_prev_frame checks for both the
>> inferior and executable entry frames (see previous commit for details
>> on the differences), in frame_unwind_caller_frame I think we only need
>> to check for the inferior entry frame.
>> 
>> My reasoning here is that, in most cases, any frames before the
>> inferior entry frame are likely to be invalid, and any attempt to
>> write to them will trigger an error.
>> 
>> In contrast, frames before the executable entry frame sit between
>> standard libraries entry code and the executable's entry function,
>> these frames, if GDB can find them, are likely to be valid.
>> 
>> An earlier version of this patch tried placing this check into
>> get_prev_frame_always, however, if get_prev_frame_always is unable to
>> unwind a frame then it must set the stop reason on the last frame,
>> this can be seen in `get_frame_unwind_stop_reason` where the assert
>> `gdb_assert (frame->prev_p);` will trigger if the previous frame is
>> not setup correctly.
>> 
>> However, setting the stop reason means the decision about whether
>> there's a previous frame or not is permanent (at least until the next
>> frame cache flush), but we need commands like 'bt -past-entry' to
>> work, which means the choice for whether there's a previous frame or
>> not needs to remain dynamic.
>> 
>> Placing the check in frame_unwind_caller_frame seemed like the best
>> solution.  This is the function that is called by other parts of GDB
>> when they need the caller frame.
>> 
>> The test for this fix ran into an issue where the frame-id for the
>> outermost frame would change between the first instruction and later
>> instructions in the frame.  I created bug PR gdb/34245 for this issue.
>> 
>> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=34245
>
> I don't know if I'm not testing correctly, but I'm trying with a simple
> binary, compiled with "gcc test.c -O0 -o test" (no debug info, if that
> matters).
>
>     $ ./gdb -nx -q --data-directory=data-directory ./test
>     Reading symbols from ./test...
>
>     This GDB supports auto-downloading debuginfo from the following URLs:
>       <https://debuginfod.archlinux.org>
>     Enable debuginfod for this session? (y or [n]) n
>     Debuginfod has been disabled.
>     To make this setting permanent, add 'set debuginfod enabled off' to .gdbinit.
>     (No debugging symbols found in ./test)
>     (gdb) b _start
>     Breakpoint 1 at 0x1020
>     (gdb) run
>     Starting program: /home/smarchi/build/binutils-gdb/gdb/test
>     [Thread debugging using libthread_db enabled]
>     Using host libthread_db library "/usr/lib/../lib/libthread_db.so.1".
>
>     Breakpoint 1, 0x0000555555555020 in _start ()
>     (gdb) disas
>     Dump of assembler code for function _start:
>     => 0x0000555555555020 <+0>:     endbr64
>        0x0000555555555024 <+4>:     xor    %ebp,%ebp
>        0x0000555555555026 <+6>:     mov    %rdx,%r9
>        0x0000555555555029 <+9>:     pop    %rsi
>        0x000055555555502a <+10>:    mov    %rsp,%rdx
>        0x000055555555502d <+13>:    and    $0xfffffffffffffff0,%rsp
>        0x0000555555555031 <+17>:    push   %rax
>        0x0000555555555032 <+18>:    push   %rsp
>        0x0000555555555033 <+19>:    xor    %r8d,%r8d
>        0x0000555555555036 <+22>:    xor    %ecx,%ecx
>        0x0000555555555038 <+24>:    lea    0xda(%rip),%rdi        # 0x555555555119 <main>
>        0x000055555555503f <+31>:    call   *0x2f7b(%rip)        # 0x555555557fc0
>        0x0000555555555045 <+37>:    hlt
>     End of assembler dump.
>     (gdb) bt
>     #0  0x0000555555555020 in _start ()
>     (gdb) bt -past-entry
>     #0  0x0000555555555020 in _start ()
>     #1  0x0000000000000001 in ?? ()
>     #2  0x00007fffffffdfc4 in ?? ()
>     #3  0x0000000000000000 in ?? ()
>     (gdb) until *0x0000555555555024
>     ❌️ Warning:
>     Cannot insert breakpoint 0.
>     Cannot access memory at address 0x1
>
>     ❌️ Command aborted.
>
> I can see that "inside_entry_func" returns false while I'm in _start,
> which is surprising given that the "bt" command seems to understand that
> it is the entry frame.

See conversation below...

>
>> diff --git a/gdb/frame.c b/gdb/frame.c
>> index e61bfd8d1f7..caa208c6d42 100644
>> --- a/gdb/frame.c
>> +++ b/gdb/frame.c
>> @@ -734,6 +734,52 @@ get_stack_frame_id (const frame_info_ptr &next_frame)
>>    return get_frame_id (skip_artificial_frames (next_frame));
>>  }
>>  
>> +/* When checking if a frame is an entry frame, there are two different
>> +   entry frames to consider.  This enum is used to select which entry
>> +   frame(s) we wish to consider.  See the inside_entry_func function.  */
>> +
>> +enum class entry_address_type_flag
>> +{
>> +  /* The executable's entry frame.  This is the first frame within the main
>> +     executable.  */
>> +  executable = (1 << 0),
>> +
>> +  /* The inferior's entry frame.  This is the first frame within the
>> +     inferior, this can be outside the main executable, e.g. for a
>> +     dynamically linked executable, this will be the first frame in the
>> +     dynamic linker.  */
>> +  inferior = (1 << 1),
>> +};
>> +DEF_ENUM_FLAGS_TYPE (enum entry_address_type_flag, entry_address_type_flags);
>> +
>> +/* Return true if THIS_FRAME is inside the either of the possible entry
>> +   frames based on ENTRY_ADDR_TYPES, otherwise return false.  */
>> +
>> +static bool
>> +inside_entry_func (const frame_info_ptr &this_frame,
>> +		   entry_address_type_flags entry_addr_types)
>> +{
>> +  /* It doesn't make sense to call this with no flag bits set.  */
>> +  gdb_assert (entry_addr_types != (entry_address_type_flags) 0);
>> +
>> +  CORE_ADDR frame_func_addr;
>> +  if (!get_frame_func_if_available (this_frame, &frame_func_addr))
>> +    return false;
>> +
>> +  const entry_point_info &ep_info
>> +    = current_program_space->get_entry_point_info ();
>> +
>> +  /* For each flag set in ENTRY_ADDR_TYPES if FRAME_FUNC_ADDR matches the
>> +     corresponding entry address from EP_INFO then THIS_FRAME is an entry
>> +     frame.  */
>> +  return (((entry_addr_types & entry_address_type_flag::executable)
>> +	   == entry_address_type_flag::executable
>> +	   && ep_info.exec_entry_address () == frame_func_addr)
>> +	  || ((entry_addr_types & entry_address_type_flag::inferior)
>> +	      == entry_address_type_flag::inferior
>> +	      && ep_info.inferior_entry_address () == frame_func_addr));
>
> For readability, I really wouldn't mind if you split that in two.  Also,
> you can compare the result of the bitwise and to 0, it's less verbose (I
> think it would be as, if not more readable, without the explicit
> comparison, but I guess that it you want to strictly follow the GNU
> style you need it).
>
>   if ((entry_addr_types & entry_address_type_flag::executable) != 0
>       && ep_info.exec_entry_address () == frame_func_addr)
>     return true;
>
>   if ((entry_addr_types & entry_address_type_flag::inferior) != 0
>       && ep_info.inferior_entry_address () == frame_func_addr)
>     return true;
>
>   return false;

I think this needs reworking anyway, see conversation below...

>
>> +}
>> +
>>  /* Helper for the various frame_unwind_caller_* functions.  Unwind
>>     INITIAL_NEXT_FRAME at least one frame, but skip any artificial frames,
>>     that is inline or tailcall frames.
>> @@ -744,6 +790,15 @@ get_stack_frame_id (const frame_info_ptr &next_frame)
>>  static frame_info_ptr
>>  frame_unwind_caller_frame (const frame_info_ptr &initial_next_frame)
>>  {
>> +  /* Avoid returning a frame which is possibly before the inferior's entry
>> +     frame, any such frame is likely invalid.  However, if the user has
>> +     turned on 'backtrace past-entry' then we assume they know what they
>> +     are doing and allow these frames.  */
>> +  if (!user_set_backtrace_options.backtrace_past_entry
>> +      && inside_entry_func (initial_next_frame,
>> +			    entry_address_type_flag::inferior))
>> +    return nullptr;
>
> I'm trying to put words on why we would only check for
> entry_address_type_flag::inferior here, and not
> entry_address_type_flag::executable, but I don't really understand.
> Perhaps you can explain it?  Now that I say it, that could explain why
> the example above didn't work, _start is the exec entry point, not the
> inferior entry point.

Indeed, I think this is exactly what's going on.

I couldn't reproduce your exact session where the first '_start' that
you hit is the '_start' in the executable.  On all the machines I tried
the first '_start' that I hit is in the dynamic linker, but if I
continue to the second '_start' then this is the one in the executable
and I see the exact same thing that you do, which as you point out, is
because we only check for entry_address_type_flag::inferior here.

My reasoning at the time was that there's no reason why the backtrace at
the executable entry point _has_ to be broken like it is.  There could
be a valid unwind patch from the executable's _start symbol back to the
inferior's _start symbol.

But I think my position has now changed.

I think we should just treat both the inferior and executable entry
point the same; assume by default that there are no frames before the
entry frame.  This would fix your test case, and is clearly true for at
least a couple of GNU/Linux targets.  If a user really does want to
ensure that `until` places a guard in a previous frame they can always
use the 'set backtrace past-entry' setting (more on that below).

Once we start treating both entry frames the same then this allows a
whole bunch of things to be simplified, including the code you call out
as overly complex above, the whole entry_address_type_flag can be
removed and things get simpler.

> It seemed strange at first for "set backtrace past-entry" to be checked
> here, influencing the behavior of other commands like "until".  I was
> going to suggest that the decision of going past the entry frame should
> perhaps be done on a case by case basis *.  But perhaps that "set
> backtrace past-entry" should be understood as: "for everything you (GDB)
> are doing (not just the backtrace command), pretend that there exists
> stack frames under what you think is the entry frame".  If the user is
> telling this to GDB, then it makes sense for "until" to try and put a
> breakpoint in the calling frame, since the user told GDB to assume that
> there are more stack frames there.  So maybe it's just that the setting
> name is a bit too specific, not a big deal.

This is how I was choosing to interpret the flag: if the user tells me
that there are frames before the entry frame, then it's OK for "until"
to make use of them.

We could add a new flag, but I really wanted to avoid that for two
reasons, first, just GDB's general excessive flags problem, and second,
I don't think most users will even know that "until" is placing a
breakpoint in the caller frame, so then we need to explain GDB's
internal implementation of "until" so that they can make an informed
decision about whether to change the flag or not.

> * For the record, my idea was, instead of taking the decision here
>   whether or not to stop the unwind, perhaps we could mark the
>   frame_info as "this is the exec entry point", "this is the inferior
>   entry point", and even "this is the main entry point".  And then, each
>   command could decide what to do with it, whether to ignore frames past
>   the entry point/main or not.  But as noted above, it perhaps makes
>   sense after all to do it in a central spot.

We certainly could go this route, I don't think there are that many
commands that have this "breakpoint in the caller frame" functionality,
off the top of my head, "until", "finish", "watch", .... there's
probably a few more.

But beyond allowing for a different flag per command, can we think of
any way in which the behaviour would change?  I mean, we can have:

  set until use-frame-past-entry on|off
  set finish use-frame-past-entry on|off
  set watch use-frame-past-entry on|off

but they all do basically the same thing, and if you set one of these
flags you usually want to set all of them I would guess.  So maybe we
have:

  set until-finish-watch use-frame-past-entry on|off

But if you are setting that then you probably also want to set:

  set backtrace past-entry on|off

So I just tied everything to the existing backtrace flag.  In addition I
figure that we can always add a new flag in the future, if there's some
situation where we don't want to backtrace past-entry, but 'until' will
still work.

One case which I did think about is, could we "probe" for whether the
frame is usable or not?  If that was possible then we might want to do
that even when the 'backtrace past-entry' flag is off.  But I figure
that's fine.

Anyway, thanks for your great review.  I'm going to merge the two
entry-point detection cases, which should simplify the code you
highlighted above.  Then I'll try to add an extra test to cover the
problem case you showed at the start of this email.

I'll post an update hopefully next week.

Thanks,
Andrew