[PATCHv2] gdb: fix parsing of DIEs with both low/high pc AND ranges attributes

  In v2:

  - Added a DWARF assembler test.

Given this patch is fixing a regression (on older distributions) I do
plan to merge this fix later this week unless someone jumps in with
any concerns.  As always, I'm happy to address issues after I commit,
if nobody wants to review ahead of time.

Thanks,
Andrew

--

After the commit:

  commit b9de07a5ff74663ff39bf03632d1b2ea417bf8d5
  Date:   Thu Oct 10 11:37:34 2024 +0100

      gdb: fix handling of DW_AT_entry_pc of inlined subroutines

GDB's buildbot CI testing highlighted this assertion failure:

  (gdb) c
  Continuing.
  ../../binutils-gdb/gdb/block.h:203: internal-error: set_entry_pc: Assertion `start >= this->start () && start < this->end ()' failed.
  A problem internal to GDB has been detected,
  further debugging may prove unreliable.
  ----- Backtrace -----
  FAIL: gdb.base/break-probes.exp: run til our library loads (GDB internal error)

This assertion was in the new function set_entry_pc and is asserting
that the default_entry_pc() value is within the blocks start/end
range.

The default_entry_pc() is the value GDB will use as the entry-pc if
the DWARF doesn't specifically override the entry-pc.  This value is
calculated as:

  1. The start address of the first sub-range within the block, if the
  block has more than 1 range, or

  2. The low address (from DW_AT_low_pc) for the block.

If the block only has a single range then this means the block was
defined with low/high pc attributes (case #2 above).  These low/high
pc values are what block::start() and block::end() return.  This means
that by definition, if the block is continuous, the above assert
cannot trigger as 'start', the default_entry_pc() would be equivalent
to block::start().

This means that, for the assert to trigger, the block must have
multiple ranges, and the first address of the first range is not
within the blocks low/high address range.  This seems wrong.

I inspected the state at the time the assert triggered and discovered
the block's start() address.  Then I removed the assert and restarted
GDB.  I was now able to inspect the blocks at the offending address:

  (gdb) maintenance info blocks 0x7ffff7dddaa4
  Blocks at 0x7ffff7dddaa4:
    from objfile: [(objfile *) 0x44a37f0] /lib64/ld-linux-x86-64.so.2
 [(block *) 0x46b30c0] 0x7ffff7ddd5a0..0x7ffff7dde8a6
    entry pc: 0x7ffff7ddd5a0
    is global block
    symbol count: 4
    is contiguous
  [(block *) 0x46b3020] 0x7ffff7ddd5a0..0x7ffff7dde8a6
    entry pc: 0x7ffff7ddd5a0
    is static block
    symbol count: 9
    is contiguous
  [(block *) 0x46b2f70] 0x7ffff7ddda00..0x7ffff7dddac3
    entry pc: 0x7ffff7ddda00
    function: __GI__dl_find_dso_for_object
    symbol count: 4
    is contiguous
  [(block *) 0x46b2e10] 0x7ffff7dddaa4..0x7ffff7dddac3
    entry pc: 0x7ffff7dddaa4
    inline function: __GI__dl_find_dso_for_object
    symbol count: 5
    is contiguous
  [(block *) 0x46b2a40] 0x7ffff7dddaa4..0x7ffff7dddac3
    entry pc: 0x7ffff7dddaa4
    symbol count: 1
    is contiguous
  [(block *) 0x46b2970] 0x7ffff7dddaa4..0x7ffff7dddac3
    entry pc: 0x7ffff7dddaa4
    symbol count: 2
    address ranges:
      0x7ffff7ddda0e..0x7ffff7ddda77
      0x7ffff7ddda90..0x7ffff7ddda96

I've left everything in for context, but the only really interesting
bit is the very last block, it's low/high range is:

  0x7ffff7dddaa4..0x7ffff7dddac3

but it has separate ranges:

  0x7ffff7ddda0e..0x7ffff7ddda77
  0x7ffff7ddda90..0x7ffff7ddda96

which are all outside the low/high range.  This is what triggers the
assert.  But why does that block exist at all?

What I believe is happening is that we're running into a bug in older
versions of GCC.  The buildbot failure was with an 8.5 gcc, and Tom de
Vries also reported seeing failures when using version 7 and 8 gcc,
but not with gcc 9 and onward.

Looking at the DWARF I can see that the problematic block is created
from this DIE:

  <4><15efb>: Abbrev Number: 83 (DW_TAG_lexical_block)
     <15efc>   DW_AT_abstract_origin: <0x15e9f>
     <15efe>   DW_AT_low_pc      : 0x7ffff7dddaa4
     <15f06>   DW_AT_high_pc     : 31

which links via DW_AT_abstract_origin to:

  <2><15e9f>: Abbrev Number: 80 (DW_TAG_lexical_block)
     <15ea0>   DW_AT_ranges      : 0x38e0
     <15ea4>   DW_AT_sibling     : <0x15eca>

And so we can see that <15efb> has got both low/high pc attributes and
a ranges attribute.

If I widen my checking to parents of DIE <15efb> then I see that they
also have DW_AT_abstract_origin, however, there is something
interesting going on, the parent DIEs are linking to a different DIE
tree than <15efb>.

What I believe is happening is this, we have an abstract instance
tree, this is rooted at a DW_AT_subprogram, and contains all the
blocks, variables, parameters, etc, that you would expect.  As this is
an abstract instance, then there are no low/high pc attributes, and no
ranges attributes in this tree.  This makes sense.

Now elsewhere we have a DW_TAG_subprogram (not
DW_TAG_inlined_subroutine) which links via
DW_AT_abstract_origin to the abstract DW_AT_subprogram.  This case is
documented in the DWARF 5 spec in section 3.3.8.3, and describes an
Out-of-Line Instance of an Inlined Subroutine.  Within this out of
line instance many of the DIE correctly link back, using
DW_AT_abstract_origin to the abstract instance tree.  This tree also
includes the DIE <15e9f>, which is where our problem DIE references.

Now, to really confuse things, within this out-of-line instance we
have a DW_TAG_inlined_subroutine, which is another instance of the
same abstract instance tree!  This would seem to indicate a recursive
call to the inline function, and the compiler, for some reason, needed
to instantiate an out of line instance of this function.

And it is within this nested, inlined subroutine, that the problem DIE
exists.  The problem DIE is referencing the corresponding DIE within
the out of line instance tree, but I am convinced this must be a (long
fixed) GCC bug, and that the problem DIE should be referencing the DIE
within the abstract instance tree.

I'm aware that the above is pretty confusing.  The actual DWARF would
be a around 200 lines long, so I'd like to avoid dumping it in here.
But here's my attempt at representing what's going on in a minimal
example.  The numbers down the side represent the section offset, not
the nesting level, and I've removed any attributes that are not
relevant:

  <1> DW_TAG_subprogram
  <2>   DW_TAG_lexical_block
  <3> DW_TAG_subprogram
        DW_AT_abstract_origin <1>
  <4>   DW_TAG_lexical_block
          DW_AT_ranges ...
  <5>   DW_TAG_inlined_subroutine
          DW_AT_abstract_origin <1>
  <6>     DW_TAG_lexical_block
            DW_AT_abstract_origin <4>
            DW_AT_low_pc ...
            DW_AT_high_pc ...

The lexical block at <6> is linking to <4> when it should be linking
to <2>.

There is one additional thing that we might wonder about, which is,
when calculating the low/high pc range for a block, why does GDB not
make use of the range information and expand the range beyond the
defined low/high values?

The answer to this is in dwarf_get_pc_bounds_ranges_or_highlow_pc in
dwarf/read.c.  This is where the low/high bounds are calculated.  What
we see is that GDB first checks for a low/high attribute pair, and if
that is present, this defines the address range for the block.  Only
if there is no DW_AT_low_pc do we check for the DW_AT_ranges, and use
that to define the extent of the block.  And this makes sense, section
3.5 of the DWARF-5 spec says:

  The lexical block entry may have either a DW_AT_low_pc and DW_AT_high_pc
  pair of attributes or a DW_AT_ranges attribute whose values encode the
  contiguous or non-contiguous address ranges, respectively, of the machine
  instructions generated for the lexical block...

Section 3.5 is specifically about lexical blocks, but the same
wording, about it being either low/high OR ranges is repeated for
other DW_TAG_ types.

So this explains why GDB doesn't use the ranges to expand the problem
blocks ranges; as the first DIE has low/high addresses, these are
used, and the ranges is not consulted.

It is only later in dwarf2_record_block_ranges that we create a range
based off the low/high pc, and then also process the ranges data, this
allows the problem block to exist with ranges that are outside the
low/high range.

To solve this I considered a number of options:

1. Prevent loading certain attributes from an abstract instance.

Section 3.3.8.1 of the DWARF-5 spec talks about which attributes are
appropriate to place in an abstract instance.  Any attribute that
might vary between instances should not appear in an abstract
instance.  DW_AT_ranges is included as an example in the
non-exhaustive list of attributes that should not appear in an
abstract instance.

Currently in dwarf2_attr (dwarf2/read.c), when we see a
DW_AT_abstract_origin attribute, we always follow this to try and find
the attribute we are looking for.  But we could change this function
so that we prevent this following for attributes that we know should
not be looked up in an abstract instance.  This would solve the
problem in this case by preventing us finding the DW_AT_ranges in the
incorrect abstract instance.

2. Filter the ranges.

Having established a blocks low/high address range in
dwarf_get_pc_bounds_ranges_or_highlow_pc, we could allow
dwarf2_record_block_ranges to parse the ranges, but we could reject
any range that extends outside the blocks defined start and end
addresses.

For well behaved DWARF where we have either low/high or ranges, then
the blocks start/end are defined from the range data, and so, by
definition, every range would be acceptable.

But in our problem case we would reject all of the invalid ranges.

This is my least favourite solution as it feels like rejecting the
ranges is tackling the problem too late on.

3. Don't try to parse ranges when we have low/high attributes.

This option involves updating dwarf2_record_block_ranges to match the
behaviour of dwarf_get_pc_bounds_ranges_or_highlow_pc, and, I believe,
to match the DWARF spec: don't try to read range data from
DW_AT_ranges if we have low/high pc attributes.

In our case this solves the issue because the problematic DIE has the
low/high attributes, and it then links to the wrong DIE which happens
to have DW_AT_ranges.  With this change in place we don't even look
for the DW_AT_ranges.

If the problem were reversed, and the initial DIE had DW_AT_ranges,
but the incorrectly referenced DIE had the low/high pc attributes,
we would pick up the wrong addresses, but this wouldn't trigger any
asserts.  The reason is that dwarf_get_pc_bounds_ranges_or_highlow_pc
would also find the low/high addresses from the incorrectly referenced
DIE, and so we would just end up with a block which had the wrong
address ranges, but the block would be self consistent, which is
different to the problem we hit here.

In the end, in this commit I went with solution #3, having
dwarf_get_pc_bounds_ranges_or_highlow_pc and
dwarf2_record_block_ranges be consistent seems sensible.  However, I
do wonder if in the future we might want to explore solution #1 as an
additional safety feature.

With this patch in place I'm able to run the gdb.base/break-probes.exp
without seeing the assert that CI testing highlighted.  I see no
regressions when testing on x86-64 GNU/Linux with gcc  9.3.1.

Note: the diff in this commit looks big, but it's really just me
indenting the code.
---
 gdb/dwarf2/read.c                             |  68 +++--
 .../gdb.dwarf2/dw2-bad-abstract-origin.c      |  83 ++++++
 .../gdb.dwarf2/dw2-bad-abstract-origin.exp    | 274 ++++++++++++++++++
 3 files changed, 403 insertions(+), 22 deletions(-)
 create mode 100644 gdb/testsuite/gdb.dwarf2/dw2-bad-abstract-origin.c
 create mode 100644 gdb/testsuite/gdb.dwarf2/dw2-bad-abstract-origin.exp


base-commit: 5930bcb592a599ec7182cdc9c2f47511e6082a5e
  

>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:

Andrew> In v2:
Andrew>   - Added a DWARF assembler test.

Andrew> Given this patch is fixing a regression (on older distributions) I do
Andrew> plan to merge this fix later this week unless someone jumps in with
Andrew> any concerns.  As always, I'm happy to address issues after I commit,
Andrew> if nobody wants to review ahead of time.

We encountered this assertion failure on various builds after importing
gdb-head this week.  E.g., many internal tests fail on Ubuntu 18.

I tested this patch and can confirm it fixes the problems we are seeing.
Thank you.

Approved-By: Tom Tromey <tom@tromey.com>

Andrew> 1. Prevent loading certain attributes from an abstract instance.

Andrew> Section 3.3.8.1 of the DWARF-5 spec talks about which attributes are
Andrew> appropriate to place in an abstract instance.  Any attribute that
Andrew> might vary between instances should not appear in an abstract
Andrew> instance.  DW_AT_ranges is included as an example in the
Andrew> non-exhaustive list of attributes that should not appear in an
Andrew> abstract instance.

Andrew> Currently in dwarf2_attr (dwarf2/read.c), when we see a
Andrew> DW_AT_abstract_origin attribute, we always follow this to try and find
Andrew> the attribute we are looking for.  But we could change this function
Andrew> so that we prevent this following for attributes that we know should
Andrew> not be looked up in an abstract instance.  This would solve the
Andrew> problem in this case by preventing us finding the DW_AT_ranges in the
Andrew> incorrect abstract instance.

Andrew> However, I do wonder if in the future we might want to explore
Andrew> solution #1 as an additional safety feature.

Yeah, I wonder this as well.

Or perhaps this particular spot should be changed to explicitly not
follow links to an abstract origin, rather than making it conditional on
the particular attribute?  I am not sure if that's better or worse.

Tom
  

Tom Tromey <tromey@adacore.com> writes:

>>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:
>
> Andrew> In v2:
> Andrew>   - Added a DWARF assembler test.
>
> Andrew> Given this patch is fixing a regression (on older distributions) I do
> Andrew> plan to merge this fix later this week unless someone jumps in with
> Andrew> any concerns.  As always, I'm happy to address issues after I commit,
> Andrew> if nobody wants to review ahead of time.
>
> We encountered this assertion failure on various builds after importing
> gdb-head this week.  E.g., many internal tests fail on Ubuntu 18.
>
> I tested this patch and can confirm it fixes the problems we are seeing.
> Thank you.
>
> Approved-By: Tom Tromey <tom@tromey.com>

Sorry for the breakage.  I've now pushed this patch.

>
> Andrew> 1. Prevent loading certain attributes from an abstract instance.
>
> Andrew> Section 3.3.8.1 of the DWARF-5 spec talks about which attributes are
> Andrew> appropriate to place in an abstract instance.  Any attribute that
> Andrew> might vary between instances should not appear in an abstract
> Andrew> instance.  DW_AT_ranges is included as an example in the
> Andrew> non-exhaustive list of attributes that should not appear in an
> Andrew> abstract instance.
>
> Andrew> Currently in dwarf2_attr (dwarf2/read.c), when we see a
> Andrew> DW_AT_abstract_origin attribute, we always follow this to try and find
> Andrew> the attribute we are looking for.  But we could change this function
> Andrew> so that we prevent this following for attributes that we know should
> Andrew> not be looked up in an abstract instance.  This would solve the
> Andrew> problem in this case by preventing us finding the DW_AT_ranges in the
> Andrew> incorrect abstract instance.
>
> Andrew> However, I do wonder if in the future we might want to explore
> Andrew> solution #1 as an additional safety feature.
>
> Yeah, I wonder this as well.
>
> Or perhaps this particular spot should be changed to explicitly not
> follow links to an abstract origin, rather than making it conditional on
> the particular attribute?  I am not sure if that's better or worse.

I've added an item to my todo list to revisit this in the new year.

Thanks,
Andrew