Re: [PATCHv6] gdb: fix handling of DW_AT_entry_pc of inlined subroutines

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

On 11/13/24 17:59, Andrew Burgess wrote:
> Andrew Burgess <aburgess@redhat.com> writes:
> 
>> Andrew Burgess <aburgess@redhat.com> writes:
>>
>>> The entry PC for a DIE, e.g. an inline function, might not be the base
>>> address of the DIE.  Currently though, in block::entry_pc(), GDB
>>> always returns the base address (low-pc or the first address of the
>>> first range) as the entry PC.
>>>
>>> This commit extends the block class to carry the entry PC as a
>>> separate member variable.  Then the DWARF reader is extended to read
>>> and set the entry PC for the block.  Now in block::entry_pc(), if the
>>> entry PC has been set, this is the value returned.
>>>
>>> If the entry-pc has not been set to a specific value then the old
>>> behaviour of block::entry_pc() remains, GDB will use the block's base
>>> address.  Not every DIE will set the entry-pc, but GDB still needs to
>>> have an entry-pc for every block, so the existing logic supplies the
>>> entry-pc for any block where the entry-pc was not set.
>>>
>>> The DWARF-5 spec for reading the entry PC is a super-set of the spec
>>> as found in DWARF-4.  For example, if there is no DW_AT_entry_pc then
>>> DWARF-4 says to use DW_AT_low_pc while DWARF-5 says to use the base
>>> address, which is DW_AT_low_pc or the first address in the first range
>>> specified by DW_AT_ranges if there is no DW_AT_low_pc.
>>>
>>> I have taken the approach of just implementing the DWARF-5 spec for
>>> everyone.  There doesn't seem to be any benefit to deliberately
>>> ignoring a ranges based entry PC value for DWARF-4.  If some naughty
>>> compiler has emitted that, then lets use it.
>>>
>>> Similarly, DWARF-4 says that DW_AT_entry_pc is an address.  DWARF-5
>>> allows an address or a constant, where the constant is an offset from
>>> the base address.  I allow both approaches for all DWARF versions.
>>> There doesn't seem to be any downsides to this approach.
>>>
>>> I ran into an issue when testing this patch where GCC would have the
>>> DW_AT_entry_pc point to an empty range.  When GDB parses the ranges
>>> any empty ranges are ignored.  As a consequence, the entry-pc appears
>>> to be outside the address range of a block.
>>>
>>> The empty range problem is certainly something that we can, and should
>>> address, but that is not the focus of this patch, so for now I'm
>>> ignoring that problem.  What I have done is added a check: if the
>>> DW_AT_entry_pc is outside the range of a block then the entry-pc is
>>> ignored, GDB will then fall-back to its default algorithm for
>>> computing the entry-pc.
>>>
>>> If/when in the future we address the empty range problem, these
>>> DW_AT_entry_pc attributes will suddenly become valid and GDB will
>>> start using them.  Until then, GDB continues to operate as it always
>>> has.
>>>
>>> An early version of this patch stored the entry-pc within the block
>>> like this:
>>>
>>>    std::optional<CORE_ADDR> m_entry_pc;
>>>
>>> However, a concern was raised that this, on a 64-bit host, effectively
>>> increases the size of block by 16-bytes (8-bytes for the CORE_ADDR,
>>> and 8-bytes for the std::optional's bool plus padding).
>>>
>>> If we remove the std::optional part and just use a CORE_ADDR then we
>>> need to have a "special" address to indicate if m_entry_pc is in use
>>> or not.  I don't really like using special addresses; different
>>> targets can access different address ranges, even zero is a valid
>>> address on some targets.
>>>
>>> However, Bernd Edlinger suggested storing the entry-pc as an offset,
>>> and I think that will resolve my concerns.  So, we store the entry-pc
>>> as a signed offset from the block's base address (the first address of
>>> the first range, or the start() address value if there are now
>>> ranges).  Remember, ranges can be out of order, in which case the
>>> first address of the first range might be greater than the entry-pc.
>>>
>>> When GDB needs to read the entry-pc we can add the offset onto the
>>> blocks base address to recalculate it.
>>>
>>> With this done, on a 64-bit host, block only needs to increase by
>>> 8-bytes.
>>>
>>> The inline-entry.exp test was originally contributed by Bernd here:
>>>
>>>    https://inbox.sourceware.org/gdb-patches/AS1PR01MB94659E4D9B3F4A6006CC605FE4922@AS1PR01MB9465.eurprd01.prod.exchangelabs.com
>>>
>>> though I have made some edits, making more use of lib/gdb.exp
>>> functions, making the gdb_test output patterns a little tighter, and
>>> updating the test to run with Clang.  I also moved the test to
>>> gdb.opt/ as that seemed like a better home for it.
>>>
>>> Co-Authored-By: Bernd Edlinger <bernd.edlinger@hotmail.de>
>>
>> I've pushed the patch attached below.  The only changes are some minor
>> test tweaks in order to get more of the 'check-all-boards' passing.
>> There are still some failures on e.g. gold and fission based boards, but
>> I think these might be due to issues with the board file.  I'll post a
>> patch for those soon.
> 
> I'm aware that this patch has caused a regression on the buildbot[1].  I'm
> investigating this but it will be at least tomorrow before I have fix.
> 

In case this helps, I see the same assert in a large amount of ada 
test-cases.

I reproduced the first one using gdb.ada/access_to_packed_array.exp, 
with system gcc 7 and then with gcc 8.  With gcc 9 - 14, the assert no 
longer reproduces.

Thanks,
- Tom

> Thanks,
> Andrew
> 
> [1] https://builder.sourceware.org/buildbot/#/builders/290/builds/1903
>