mbox series

[0/2] Fix missing print frame when stepping out of function

Message ID cover.1775383137.git.aburgess@redhat.com
Headers
Series Fix missing print frame when stepping out of function |

Message

Andrew Burgess April 5, 2026, 10:12 a.m. UTC 
  Hi Tom,

I looked into the regression you saw with gdb.opt/inline-cmds.exp, and
read your notes.  As you predicted, the issue is that we need to
better take into account the inline frame state when setting up, and
checking during, the step/next process.

Luckily, we already have a mechanism in GDB to do this,
get_frame_function.  This returns the function symbol for a frame,
taking into account which frames are inlined but being skipped.

Assuming a call stack like: 'non-inline function -> inline function'
the original code was always returning the non-inline function.  My
initial proposal was always returning the inline function, and the
reality is neither of these is always correct.

The get_frame_function returns the function that represents the given
frame, which is exactly what we want.

In fact, I did consider the idea that we should move away from
tracking via the function symbol, and instead just hold a separate
frame-id for the "original frame when stepping started".  I think this
would probably work just fine, but currently, if the frame-id changes,
but the function symbol stays the same then GDB would work a
particular way, switching to using a frame-id would change this.  I
doubt this is actually a real concern, but I didn't want to change too
much in one go, so I'm sticking with function symbol tracking.

The gdb.opt/inline-cmds.exp regression you ran into was only visible
from part of the test that runs in MI mode, but the bug itself would
manifest in CLI and MI mode, we just didn't spot the bug in CLI mode.
So I extended the test to reveal the bug in CLI mode too.  I expect
that the bug likely was present in other tests too, but the test
patterns are too lax, and so didn't trigger for the regression, which
is a bit of a shame, but I don't have the time right now to track down
tests that _should_ have failed and fix them, at least we have one
test that we know checks this stuff now.

Your new additions to gdb.dwarf2/dw2-extend-inline-block.exp all pass,
and I've done a full local test run and don't see any other
regressions.

Your original RFC patch #1 was no longer needed for this series, but
you might want to repost that separately as it did have one use
outside this series, so maybe it's worth having anyway?

I folded RFC patch #2 into what is the second patch in this series.  I
ended up changing the API anyway, so it seemed to make sense to do all
the changes in a single patch.

Take a look through, and let me know what you think.

Thanks,
Andrew

---

Andrew Burgess (1):
  gdb: use get_current_frame consistently in print_stop_location

Tom de Vries (1):
  gdb: fix missing print frame when stepping out of function

 gdb/gdbthread.h                               | 36 ++++++++++++++-
 gdb/infcmd.c                                  |  3 +-
 gdb/infrun.c                                  | 20 ++++-----
 .../gdb.dwarf2/dw2-extend-inline-block.exp    | 45 +++++++++++++++++--
 gdb/testsuite/gdb.opt/inline-cmds.exp         | 33 +++++++++-----
 5 files changed, 108 insertions(+), 29 deletions(-)


base-commit: 9d3cf9efd51ebae3f45bb49e3544cb7eeb63a138

Comments

Tom de Vries April 10, 2026, 10:48 a.m. UTC | #1 
On 4/5/26 12:12 PM, Andrew Burgess wrote:
> Hi Tom,
> 
> I looked into the regression you saw with gdb.opt/inline-cmds.exp, and
> read your notes. 

Hi Andrew,

a massive thanks for helping out with this.

> As you predicted, the issue is that we need to
> better take into account the inline frame state when setting up, and
> checking during, the step/next process.
> 
> Luckily, we already have a mechanism in GDB to do this,
> get_frame_function.  This returns the function symbol for a frame,
> taking into account which frames are inlined but being skipped.
> 
> Assuming a call stack like: 'non-inline function -> inline function'
> the original code was always returning the non-inline function.  My
> initial proposal was always returning the inline function, and the
> reality is neither of these is always correct.
> 
> The get_frame_function returns the function that represents the given
> frame, which is exactly what we want.
> 
> In fact, I did consider the idea that we should move away from
> tracking via the function symbol, and instead just hold a separate
> frame-id for the "original frame when stepping started". 

Yes, I actually had already started working on that approach when I saw 
this series.

> I think this
> would probably work just fine, but currently, if the frame-id changes,
> but the function symbol stays the same then GDB would work a
> particular way, switching to using a frame-id would change this.  I
> doubt this is actually a real concern, but I didn't want to change too
> much in one go, so I'm sticking with function symbol tracking.

I see.

> The gdb.opt/inline-cmds.exp regression you ran into was only visible
> from part of the test that runs in MI mode, but the bug itself would
> manifest in CLI and MI mode, we just didn't spot the bug in CLI mode.
> So I extended the test to reveal the bug in CLI mode too. 

That's a great idea, thanks.

> I expect
> that the bug likely was present in other tests too, but the test
> patterns are too lax, and so didn't trigger for the regression, which
> is a bit of a shame, but I don't have the time right now to track down
> tests that _should_ have failed and fix them, at least we have one
> test that we know checks this stuff now.
I didn't think of that, good point.

> Your new additions to gdb.dwarf2/dw2-extend-inline-block.exp all pass,
> and I've done a full local test run and don't see any other
> regressions.
> 
> Your original RFC patch #1 was no longer needed for this series, but
> you might want to repost that separately as it did have one use
> outside this series, so maybe it's worth having anyway?
> 

OK, I'll do that.

> I folded RFC patch #2 into what is the second patch in this series.  I
> ended up changing the API anyway, so it seemed to make sense to do all
> the changes in a single patch.
> 

I ended up reverting that in v4, I think the fix is more readable with 
infrastructure changes factored out.

> Take a look through, and let me know what you think.

I think v4 is ok to commit.

Thanks,
- Tom