[PATCHv3,0/2] Handle empty ranges for inline subroutines

  In v3:

  - Based on Linaro CI test failures: tests have been updated to
    handle targets which compile as PIE by default.

In v2:

  - Rebased, resolved minor merge conflict.

--

This patch draws inspiration from the work submitted here:

  https://inbox.sourceware.org/gdb-patches/AS1PR01MB946510286FBF2497A6F03E83E4922@AS1PR01MB9465.eurprd01.prod.exchangelabs.com

Patches #2 and #3 from this original series deal with issues
interpreting the DWARF around the end address of inline functions.
There have been previous attempts to fix this problem:

  https://inbox.sourceware.org/gdb-patches/AM6PR03MB51709D8514A8796DB84D006DE4ED0@AM6PR03MB5170.eurprd03.prod.outlook.com
  https://inbox.sourceware.org/gdb-patches/AM0PR0602MB341029C1241395C969437ED3E4D80@AM0PR0602MB3410.eurprd06.prod.outlook.com
  https://inbox.sourceware.org/gdb-patches/HE1PR0602MB3417B82A6028274A529E6168E4D50@HE1PR0602MB3417.eurprd06.prod.outlook.com
  https://inbox.sourceware.org/gdb-patches/AM6PR03MB51707894C46730F4D2DD88A8E41E0@AM6PR03MB5170.eurprd03.prod.outlook.com
  https://inbox.sourceware.org/gdb-patches/AM6PR03MB51707FDDD28177A2B85DBE3BE4C40@AM6PR03MB5170.eurprd03.prod.outlook.com

There have been a number of different approaches proposed over the
years.

From a user perspective, the problem being addressed by these patches
is that GDB will sometimes fail to realise that the inferior is inside
an inlined function when the inferior is positioned at the end of an
inlined function.  This can impact placing breakpoints within the
inline function, and stepping through, and out of, inline functions.

The problem is caused by the interaction of two DWARF elements, the
inline functions address ranges, as defined within the DIE (including
with DW_AT_ranges), and the line table.

What is often observed is that the address range of an inline function
doesn't include a particular address, however, within the line table,
this same address is marked as is-stmt for lines within the inline
function.  This confuses GDB during stepping as from a block
perspective, GDB will believe it has stepped out of the block, but
from a line table perspective, GDB will report the line within the
inline function.

The most recent proposal creates a new line table attribute at the
address where an inline functions end.  This attribute is then used in
various places within GDB to influence the block selection algorithm.
The result is that GDB can, when stopped at an address, return a block
which does not appear to contain the current address.

There are then some follow on changes needed to handle the fact that
the inferior can be considered inside a block, despite the inferior
being stopped at an address outside the blocks address ranges.

The results achieved by this patch are indeed impressive.  The changes
proposed in this patch actually handle two slightly different, but
related cases.

In some cases, for small inline functions, the compiler ends up
reporting the inline function as having an empty address range.  This
case is particularly bad as with an empty address range GDB completely
discards the block, and its associated symbols, which means the inline
function appears to disappear completely, the user cannot place
breakpoints on the function, nor will GDB notice when the inferior
stops at the location of the inline function.

The second case is a little better, but still has some serious issues.
In this case the address range of the inline function appears
truncated.  The instruction, which is often tagged as is-stmt in the
line table, corresponding to the 'return' line for the inline
function, is not part of the inline function's address range.  In this
case GDB is able to place breakpoints on the inline function, but when
stepping out of the inline function GDB will be confused by the block
and line-table inconsistencies.

However, despite the impressive results of the original patch, I worry
that changing GDB's block lookup to return blocks which don't contain
a particular address, is too great of a risk.

I believe that the issues being addressed here can, and should, be
addressed within GDB's DWARF parser, and that we should spot the
problem cases and adjust the data to present a view of the world which
works with GDB's existing data structures.

For the empty address range issue I would suggest that a cleaner
solution is to spot the problem case and make the address range of the
block non-empty.  After this GDB's existing algorithms will "just work".

The second problem, that of block range truncation, is slightly harder
to handle, but I think this can still be resolved by cross referencing
the block address range with the line table, and, where appropriate,
extending the address range of the block.  Again, if this is done
correctly then GDB's existing algorithms will "just work".

This series sets out to only address the first problem, that of empty
address ranges.  This is the far easier case to handle, but also, has
the largest impact.  Inline functions with an empty address range
disappear completely from GDB's world view, so fixing these
immediately means that GDB will know of these functions, and can break
on them.

If this patch is accepted then I will follow up addressing the second
issue, I have a very rough prototype of the fix for this case which
appears to work, but it is a bigger task, so I wanted to see how this
change is received first.

However, if my idea of addressing the second issue doesn't
materialise, the original patch series does still apply on top of this
patch, I have done this merge and pushed it to sourceware in the
users/aburgess/gdb-opt-code-debug branch.

The second patch in this series was already posted as a stand-alone
patch here:

  https://inbox.sourceware.org/gdb-patches/2fa8d328be94dff72ec7be0609ec799765894594.1722007578.git.aburgess@redhat.com

There was some feedback on the tests given, which I believe I have
addressed.  There was also a concern that the patch would conflict
with the original optimised code series, which the sourceware branch
referenced above shows is not an issue.

I look forward to peoples thoughts on this series,

Thanks,
Andrew

--

Andrew Burgess (2):
  gdb: handle empty ranges for inline subroutines
  gdb: improve line number lookup around inline functions

 gdb/dwarf2/read.c                             |  71 ++++-
 gdb/symtab.c                                  |  25 +-
 gdb/testsuite/gdb.cp/step-and-next-inline.exp | 145 +++++-----
 .../gdb.dwarf2/dw2-empty-inline-low-high.c    |  39 +++
 .../gdb.dwarf2/dw2-empty-inline-low-high.exp  | 128 +++++++++
 .../gdb.dwarf2/dw2-empty-inline-ranges.c      |  54 ++++
 .../gdb.dwarf2/dw2-empty-inline-ranges.exp    | 261 ++++++++++++++++++
 gdb/testsuite/gdb.dwarf2/dw2-inline-bt.c      |  79 ++++++
 gdb/testsuite/gdb.dwarf2/dw2-inline-bt.exp    | 227 +++++++++++++++
 .../gdb.dwarf2/dw2-unexpected-entry-pc.exp    |  67 +++--
 gdb/testsuite/gdb.opt/empty-inline-cxx.cc     |  65 +++++
 gdb/testsuite/gdb.opt/empty-inline-cxx.exp    |  95 +++++++
 gdb/testsuite/gdb.opt/empty-inline.c          |  40 +++
 gdb/testsuite/gdb.opt/empty-inline.exp        | 115 ++++++++
 gdb/testsuite/gdb.opt/inline-bt.c             |  28 ++
 gdb/testsuite/gdb.opt/inline-bt.exp           | 127 ++++++---
 16 files changed, 1435 insertions(+), 131 deletions(-)
 create mode 100644 gdb/testsuite/gdb.dwarf2/dw2-empty-inline-low-high.c
 create mode 100644 gdb/testsuite/gdb.dwarf2/dw2-empty-inline-low-high.exp
 create mode 100644 gdb/testsuite/gdb.dwarf2/dw2-empty-inline-ranges.c
 create mode 100644 gdb/testsuite/gdb.dwarf2/dw2-empty-inline-ranges.exp
 create mode 100644 gdb/testsuite/gdb.dwarf2/dw2-inline-bt.c
 create mode 100644 gdb/testsuite/gdb.dwarf2/dw2-inline-bt.exp
 create mode 100644 gdb/testsuite/gdb.opt/empty-inline-cxx.cc
 create mode 100644 gdb/testsuite/gdb.opt/empty-inline-cxx.exp
 create mode 100644 gdb/testsuite/gdb.opt/empty-inline.c
 create mode 100644 gdb/testsuite/gdb.opt/empty-inline.exp


base-commit: 127f733f88717bdb52f03c12c0c2d240bbc892e3
  

Andrew Burgess <aburgess@redhat.com> writes:

> In v3:
>
>   - Based on Linaro CI test failures: tests have been updated to
>     handle targets which compile as PIE by default.
>
> In v2:
>
>   - Rebased, resolved minor merge conflict.
>
> --
>

What's the plan for these? I've been running with the latest version
since you posted them (and the various revisions before that) and have
been happy with the results, FWIW.

> [...]

thanks,
sam
  

Sam James <sam@gentoo.org> writes:

> Andrew Burgess <aburgess@redhat.com> writes:
>
>> In v3:
>>
>>   - Based on Linaro CI test failures: tests have been updated to
>>     handle targets which compile as PIE by default.
>>
>> In v2:
>>
>>   - Rebased, resolved minor merge conflict.
>>
>> --
>>
>
> What's the plan for these? I've been running with the latest version
> since you posted them (and the various revisions before that) and have
> been happy with the results, FWIW.

Summary: It's not forgotten about.  It's waiting for some follow up work
to be posted before I push harder to get this merged.

Long story: This series is an alternative approach to a series that
Bernd originally posted[1].  But this series only handles part of the
problem that Bernd originally addressed, that is empty ranges from small
inline functions.

Bernd's patches also addressed problems caused by truncated ranges on
longer inline functions, which this series doesn't address.

I have a patch which also fixes the issue of these longer inline
functions, but I haven't posted it yet.  While it works, the initial
implementation was just too slow.

So, right now I need to see if I can make it faster.

I would love to merge this series, as the small empty inline function
problem is one I see being hit far more than the truncated ranges for
longer inline functions, so I feel this series has by far the greater
impact.

But, I suspect (though don't know) that Bernd will object if I try to
push this series forwards without a concrete proposal that also fixes
the second problem.  He will take the (maybe not unreasonable) position
that his proposal fixes both issues, and that if we merge this, and then
I cannot fix the second problem, we might end up taking his solution
anyway, in which case these patches would be redundant.

I am actively working on the performance issues for the truncated ranges
problem, but once I missed GDB 16, I did take my foot off the gas a
little.

Once I have something posted that fixes the second problem, then I plan
to push harder to get this series merged.

Of course, if enough people thought this patch was good, then maybe we
could merge this sooner?  There's nothing stopping it being reverted
later if we decide that this was the wrong approach.

Thanks,
Andrew

[1] https://inbox.sourceware.org/gdb-patches/AS1PR01MB946510286FBF2497A6F03E83E4922@AS1PR01MB9465.eurprd01.prod.exchangelabs.com
  

Andrew Burgess <aburgess@redhat.com> writes:

> Sam James <sam@gentoo.org> writes:
>
>> Andrew Burgess <aburgess@redhat.com> writes:
>>
>>> In v3:
>>>
>>>   - Based on Linaro CI test failures: tests have been updated to
>>>     handle targets which compile as PIE by default.
>>>
>>> In v2:
>>>
>>>   - Rebased, resolved minor merge conflict.
>>>
>>> --
>>>
>>
>> What's the plan for these? I've been running with the latest version
>> since you posted them (and the various revisions before that) and have
>> been happy with the results, FWIW.
>
> Summary: It's not forgotten about.  It's waiting for some follow up work
> to be posted before I push harder to get this merged.
>
> Long story: This series is an alternative approach to a series that
> Bernd originally posted[1].  But this series only handles part of the
> problem that Bernd originally addressed, that is empty ranges from small
> inline functions.

Thank you for explaining & the background. I'm not at all experienced
yet in DWARF so all I can do is be a cheerleader and keep daily-driving
it.

Most of what I use gdb for is debugging miscompilations which are
inherently (almost) always optimised, so any little helps.

And thanks again for both of your efforts.
  

This series is now superseded by this series:

  https://inbox.sourceware.org/gdb-patches/cover.1753006060.git.aburgess@redhat.com/

These patches are (1) and (2) in the new series.

Thanks,
Andrew




Andrew Burgess <aburgess@redhat.com> writes:

> In v3:
>
>   - Based on Linaro CI test failures: tests have been updated to
>     handle targets which compile as PIE by default.
>
> In v2:
>
>   - Rebased, resolved minor merge conflict.
>
> --
>
> This patch draws inspiration from the work submitted here:
>
>   https://inbox.sourceware.org/gdb-patches/AS1PR01MB946510286FBF2497A6F03E83E4922@AS1PR01MB9465.eurprd01.prod.exchangelabs.com
>
> Patches #2 and #3 from this original series deal with issues
> interpreting the DWARF around the end address of inline functions.
> There have been previous attempts to fix this problem:
>
>   https://inbox.sourceware.org/gdb-patches/AM6PR03MB51709D8514A8796DB84D006DE4ED0@AM6PR03MB5170.eurprd03.prod.outlook.com
>   https://inbox.sourceware.org/gdb-patches/AM0PR0602MB341029C1241395C969437ED3E4D80@AM0PR0602MB3410.eurprd06.prod.outlook.com
>   https://inbox.sourceware.org/gdb-patches/HE1PR0602MB3417B82A6028274A529E6168E4D50@HE1PR0602MB3417.eurprd06.prod.outlook.com
>   https://inbox.sourceware.org/gdb-patches/AM6PR03MB51707894C46730F4D2DD88A8E41E0@AM6PR03MB5170.eurprd03.prod.outlook.com
>   https://inbox.sourceware.org/gdb-patches/AM6PR03MB51707FDDD28177A2B85DBE3BE4C40@AM6PR03MB5170.eurprd03.prod.outlook.com
>
> There have been a number of different approaches proposed over the
> years.
>
> From a user perspective, the problem being addressed by these patches
> is that GDB will sometimes fail to realise that the inferior is inside
> an inlined function when the inferior is positioned at the end of an
> inlined function.  This can impact placing breakpoints within the
> inline function, and stepping through, and out of, inline functions.
>
> The problem is caused by the interaction of two DWARF elements, the
> inline functions address ranges, as defined within the DIE (including
> with DW_AT_ranges), and the line table.
>
> What is often observed is that the address range of an inline function
> doesn't include a particular address, however, within the line table,
> this same address is marked as is-stmt for lines within the inline
> function.  This confuses GDB during stepping as from a block
> perspective, GDB will believe it has stepped out of the block, but
> from a line table perspective, GDB will report the line within the
> inline function.
>
> The most recent proposal creates a new line table attribute at the
> address where an inline functions end.  This attribute is then used in
> various places within GDB to influence the block selection algorithm.
> The result is that GDB can, when stopped at an address, return a block
> which does not appear to contain the current address.
>
> There are then some follow on changes needed to handle the fact that
> the inferior can be considered inside a block, despite the inferior
> being stopped at an address outside the blocks address ranges.
>
> The results achieved by this patch are indeed impressive.  The changes
> proposed in this patch actually handle two slightly different, but
> related cases.
>
> In some cases, for small inline functions, the compiler ends up
> reporting the inline function as having an empty address range.  This
> case is particularly bad as with an empty address range GDB completely
> discards the block, and its associated symbols, which means the inline
> function appears to disappear completely, the user cannot place
> breakpoints on the function, nor will GDB notice when the inferior
> stops at the location of the inline function.
>
> The second case is a little better, but still has some serious issues.
> In this case the address range of the inline function appears
> truncated.  The instruction, which is often tagged as is-stmt in the
> line table, corresponding to the 'return' line for the inline
> function, is not part of the inline function's address range.  In this
> case GDB is able to place breakpoints on the inline function, but when
> stepping out of the inline function GDB will be confused by the block
> and line-table inconsistencies.
>
> However, despite the impressive results of the original patch, I worry
> that changing GDB's block lookup to return blocks which don't contain
> a particular address, is too great of a risk.
>
> I believe that the issues being addressed here can, and should, be
> addressed within GDB's DWARF parser, and that we should spot the
> problem cases and adjust the data to present a view of the world which
> works with GDB's existing data structures.
>
> For the empty address range issue I would suggest that a cleaner
> solution is to spot the problem case and make the address range of the
> block non-empty.  After this GDB's existing algorithms will "just work".
>
> The second problem, that of block range truncation, is slightly harder
> to handle, but I think this can still be resolved by cross referencing
> the block address range with the line table, and, where appropriate,
> extending the address range of the block.  Again, if this is done
> correctly then GDB's existing algorithms will "just work".
>
> This series sets out to only address the first problem, that of empty
> address ranges.  This is the far easier case to handle, but also, has
> the largest impact.  Inline functions with an empty address range
> disappear completely from GDB's world view, so fixing these
> immediately means that GDB will know of these functions, and can break
> on them.
>
> If this patch is accepted then I will follow up addressing the second
> issue, I have a very rough prototype of the fix for this case which
> appears to work, but it is a bigger task, so I wanted to see how this
> change is received first.
>
> However, if my idea of addressing the second issue doesn't
> materialise, the original patch series does still apply on top of this
> patch, I have done this merge and pushed it to sourceware in the
> users/aburgess/gdb-opt-code-debug branch.
>
> The second patch in this series was already posted as a stand-alone
> patch here:
>
>   https://inbox.sourceware.org/gdb-patches/2fa8d328be94dff72ec7be0609ec799765894594.1722007578.git.aburgess@redhat.com
>
> There was some feedback on the tests given, which I believe I have
> addressed.  There was also a concern that the patch would conflict
> with the original optimised code series, which the sourceware branch
> referenced above shows is not an issue.
>
> I look forward to peoples thoughts on this series,
>
> Thanks,
> Andrew
>
> --
>
> Andrew Burgess (2):
>   gdb: handle empty ranges for inline subroutines
>   gdb: improve line number lookup around inline functions
>
>  gdb/dwarf2/read.c                             |  71 ++++-
>  gdb/symtab.c                                  |  25 +-
>  gdb/testsuite/gdb.cp/step-and-next-inline.exp | 145 +++++-----
>  .../gdb.dwarf2/dw2-empty-inline-low-high.c    |  39 +++
>  .../gdb.dwarf2/dw2-empty-inline-low-high.exp  | 128 +++++++++
>  .../gdb.dwarf2/dw2-empty-inline-ranges.c      |  54 ++++
>  .../gdb.dwarf2/dw2-empty-inline-ranges.exp    | 261 ++++++++++++++++++
>  gdb/testsuite/gdb.dwarf2/dw2-inline-bt.c      |  79 ++++++
>  gdb/testsuite/gdb.dwarf2/dw2-inline-bt.exp    | 227 +++++++++++++++
>  .../gdb.dwarf2/dw2-unexpected-entry-pc.exp    |  67 +++--
>  gdb/testsuite/gdb.opt/empty-inline-cxx.cc     |  65 +++++
>  gdb/testsuite/gdb.opt/empty-inline-cxx.exp    |  95 +++++++
>  gdb/testsuite/gdb.opt/empty-inline.c          |  40 +++
>  gdb/testsuite/gdb.opt/empty-inline.exp        | 115 ++++++++
>  gdb/testsuite/gdb.opt/inline-bt.c             |  28 ++
>  gdb/testsuite/gdb.opt/inline-bt.exp           | 127 ++++++---
>  16 files changed, 1435 insertions(+), 131 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.dwarf2/dw2-empty-inline-low-high.c
>  create mode 100644 gdb/testsuite/gdb.dwarf2/dw2-empty-inline-low-high.exp
>  create mode 100644 gdb/testsuite/gdb.dwarf2/dw2-empty-inline-ranges.c
>  create mode 100644 gdb/testsuite/gdb.dwarf2/dw2-empty-inline-ranges.exp
>  create mode 100644 gdb/testsuite/gdb.dwarf2/dw2-inline-bt.c
>  create mode 100644 gdb/testsuite/gdb.dwarf2/dw2-inline-bt.exp
>  create mode 100644 gdb/testsuite/gdb.opt/empty-inline-cxx.cc
>  create mode 100644 gdb/testsuite/gdb.opt/empty-inline-cxx.exp
>  create mode 100644 gdb/testsuite/gdb.opt/empty-inline.c
>  create mode 100644 gdb/testsuite/gdb.opt/empty-inline.exp
>
>
> base-commit: 127f733f88717bdb52f03c12c0c2d240bbc892e3
> -- 
> 2.47.1