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

[Bernd Edlinger <bernd.edlinger@hotmail.de>]

Hi Andrew,

On 10/29/24 15:49, Andrew Burgess wrote:
> In v4:
> 
>   - I still have one failed being reported from Linaro CI,
>     gdb.stabs/gdb11479.exp.  I've been unable to reproduce this
>     failure.  However, on re-reading this patch I did notice that, if
>     the m_entry_pc field was not set then we would still end up giving
>     it a value during relocation, this would result in m_entry_pc
>     holding an address which had been relocated twice.
> 
>     Fixed this by moving the block's address relocation logic into the
>     block class, and only relocating the m_entry_pc variable if it
>     actually has been assigned a value.
> 
> In v3:
> 
>   - Entry PC values are now clamped to the block's start/end range.
>     This was causing failures in the self-tests when GDB was compiled
>     with GCC using default optimisation, as GCC emits an (invalid?)
>     entry-pc which is outside the blocks start/end range.
> 
> In v2:
> 
>   - Don't use std::optional<CORE_ADDR> for block::m_entry_pc, instead
>     just use CORE_ADDR.  This does mean that targets where 0 is a
>     valid code address might run into problems, but those targets
>     likely have plenty of other problems, and I'd rather land this fix
>     than get hung up on this edge case.
> 
>     In the future it might be possible to remove block::m_start and
>     block::m_end as Klaus suggested, but this doesn't look like a
>     simple task.  If we did drop those fields then it might be
>     possible to space a word for some flag bits which would allow us
>     to remove the 0 address special case.
> 
>   - Fixes to the DWARF assembler testcase to handle compiling with
>     '-pie', the Linaro CI exposed these issues.  The changes are
>     pretty minor, mostly using label names rather than addresses when
>     building the DWARF.
> 
> ---
> 
> 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 to a specific value, this is the value returned.
> 
> I have not removed the old code in block::entry_pc().  If the specific
> entry PC value was not set then we still fall back on the old approach
> of just returning the base address.  Doing this feels more useful than
> just claiming there is no entry PC.  This should also mean that we
> shouldn't see any change in behaviour for compilers that don't emit
> the DW_AT_entry_pc, or for debug readers (e.g. stabs) that don't set
> the entry PC value within a block.
> 
> 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.
> 
> There was one issue that I ran into when testing this patch.  GCC
> seems to be quite bad at tracking code ranges when the default level
> of optimisation is applied.  As a result I was seeing failures in
> gdb.gdb/python-helper.exp and gdb.gdb/selftest.exp when we debug the
> generated GDB executable.
> 
> What happens is that the DW_AT_entry_pc is given as an entry address
> which is outside of any of the blocks ranges.  Previously we ignored
> the DW_AT_entry_pc and when we later ask for the entry-pc we just
> report the first range's start address.
> 
> After this commit we track the declared entry-pc, which is then what
> we report.
> 
> What this means is that when the user (or DejaGNU) tries to place a
> breakpoint on an inline function, the breakpoint is placed at the
> entry-pc.  Then when GDB stops at this address and tries to figure out
> which block it's in, GDB doesn't think it's actually within the inline
> function that the user asked to stop in.
> 
> In the test I mention about we place a breakpoint within GDB's
> 'captured_main' function.  In the builds I was looked at, this
> function was inline within 'gdb_main', but the DW_AT_entry_pc for the
> 'captured_main' function was not within the declared ranges of
> 'captured_main'.  As a result when the breakpoint was hit, GDB would
> report the inferior as within 'gdb_main' instead of 'captured_main'.
> 

I think you might have hit a case that I mentioned on part 3 of my patch:

    Additionally it may happen that one inline sub-range
    is empty or the inline is completely empty.  But
    filtering that information away is not the right
    solution, since although there is no actual code
    from the inline, it is still possible that variables
    from an inline function can be inspected here.

You probably had the entry_pc pointing to an empty sub-range,
that is usually the lowest address, currently that is filtered
away by the dwarf reader, but part 2+3 change that behavior,
but since I did not try to solve the entry_pc problem completely,
there was no apparent dependency between part 1 and part 2+3.
But probably this is no longer the case, with your patch,
since the checks are more strict, you could try to layer
your entry_pc path over part 2+3.

> For now I propose fixing this by checking the entry-pc against the
> blocks start address.  If the entry-pc is before the start address
> then we move the entry-pc to be the start address.  This effectively
> restores the pre-patch behaviour for this problem case.
> 

I think this plausibilty check is not strict enough, instead of just
checking against the low and hig bounds, you should only accept the
entry_pc value if it is inside the bounds of any of the sub-ranges,
since we have no chance to get the correct behaviour otherwise, and
the previous state is to prefer in that case.


Thanks
Bernd.