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

[Andrew Burgess <aburgess@redhat.com>]

Tom de Vries <tdevries@suse.de> writes:

> On 11/14/24 20:33, Andrew Burgess wrote:
>> Tom de Vries <tdevries@suse.de> writes:
>> 
>>> 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,
>> 
>> I've posted a proposed fix here:
>> 
>>    https://inbox.sourceware.org/gdb-patches/23102df35a659d9ef4725d2d3822f08f2790e52d.1731612468.git.aburgess@redhat.com
>> 
>> I would be really grateful if you could give this a try and let me know
>> if this fixes all the problems you are seeing.
>> 
>
> Hi Andrew,
>
> it does fix all the regressions in ada test-cases.
>
> There's one problem left: a test-case introduced by the commit 
> (gdb.opt/inline-entry.exp) fails with gcc 7 (not with gcc 8 and higher):
> ...
> FAIL: gdb.opt/inline-entry.exp: continue to foo
> FAIL: gdb.opt/inline-entry.exp: continue until exit
> ...
>
> Gdb.log attached.
>
> The test-case sets a breakpoint at foo and bar, and expects that the 
> program:
> - stops at bar,
> - stops at foo, and
> - exits.
>
> This is what I see when using -O0.
>
> What happens instead (with -O2) is that the program:
> - stops at bar,
> - stops at bar again,
> - stops at foo, and
> - exits.
>
> For context, relevant code:
> ...
> inline __attribute__((always_inline)) int
> bar (int val)
> {
>    if (global == val)
>      return 1;
>    foo (1);
>    return 1;
> }
>
> int
> main (void)
> {
>    if ((global && bar (1)) || bar (2))
>      return 0;
>    return 1;
> }
> ...
>
>  From what I can tell, what happened in source terms is that the 
> compiler inlined the call "bar (1)", and then moved the first 
> instruction of bar to before the &&:
>
> In disassembly:
> ...
> 00000000004003c0 <main>:
> 4003c0:  mov    0x1c5e(%rip),%eax  # Load global to reg
> 4003c6:  test   %eax,%eax          # If global is zero, set ZF
> 4003c8:  mov    0x1c56(%rip),%eax  # Local global to reg (1st of bar)
> 4003ce:  je     4003d8 <main+0x18> # If the ZF is set, skip "bar (1)"
> ...
>
> At first glance, gdb is behaving ok.

I agree with your diagnosis.  I'm pretty sure this is a bug in older
versions of gcc, the DW_AT_entry_pc (for older gcc) points to the very
first instruction of bar, even though that instruction has moved before
the &&.

Newer (8+) versions of gcc correctly set DW_AT_entry_pc to the first
instruction after the &&, in your assembler above this is the first
instruction after the 'je' instruction.

GDB is doing the best is can given the DWARF available.

I propose that we just skip this test for older versions of gcc.

There's a patch below for this.

If you agree then I'll get this checked in.  The other patch I'll check
in once it's had time for a fuller review.

Thanks,
Andrew

---

commit b6ad91d9baeeae0fb5275fce2de0cb917ae3ab4f
Author: Andrew Burgess <aburgess@redhat.com>
Date:   Fri Nov 15 13:07:09 2024 +0000

    gdb/testsuite: skip gdb.opt/inline-entry.exp for gcc 7 and older
    
    It was pointed out that the recently added gdb.opt/inline-entry.exp
    test would fail when run using gcc 7 and earlier, on an x86-64 target.
    
    It would appear that these older versions of gcc would emit an
    incorrect DW_AT_entry_pc value which is causing the failure.
    
    Here is the interesting source line from inline-entry.c:
    
      if ((global && bar (1)) || bar (2))
    
    And here's some of the relevant disassembly output:
    
      Dump of assembler code for function main:
         0x401020 <+0>:     mov    0x3006(%rip),%eax        (1)
         0x401026 <+6>:     test   %eax,%eax                (2)
         0x401028 <+8>:     mov    0x2ffe(%rip),%eax        (3)
         0x40102e <+14>:    je     0x401038 <main+24>       (4)
         0x401030 <+16>:    sub    $0x1,%eax                (5)
         0x401033 <+19>:    jne    0x40103d <main+29>       (6)
    
    Lines (1), (2), and (4) represent the check of 'global'.  However,
    line (3) is actually the first instruction for 'bar' which has been
    inlined.  Lines (5) and (6) are also part of the first inlined 'bar'
    function.
    
    If the check of 'global' returns false then the first call to 'bar'
    should never happen, this is accomplished by the branch at (4) being
    taken.
    
    For gcc 8+, gcc generates a DW_AT_entry_pc with the value 0x401030,
    this is where GDB places a breakpoint for 'bar', and this address is
    after the branch at line (4), and so, if the call to 'bar' never
    happens, the breakpoint is never hit.
    
    For gcc 7 and older, gcc generates a DW_AT_entry_pc with the value
    0x401028, which also happens to be the first address associated with
    the inline 'bar' function.  Unfortunately, this address is also before
    the check of 'global' has completed, this means that GDB hits the
    'bar' breakpoint before the inferior has decided if 'bar' should
    actually be called or not.
    
    I don't think there's really much GDB can do in this case, the debug
    information seems to be misleading.
    
    I propose that we skip this test if we are using gcc, and the version
    is 7 or older.  Newer versions of gcc, or Clang, all seem to handle
    this test fine.

diff --git a/gdb/testsuite/gdb.opt/inline-entry.exp b/gdb/testsuite/gdb.opt/inline-entry.exp
index 6f87ea3a00d..4f82fa6ed1c 100644
--- a/gdb/testsuite/gdb.opt/inline-entry.exp
+++ b/gdb/testsuite/gdb.opt/inline-entry.exp
@@ -21,6 +21,9 @@
 # Testing with Clang 9.0.1 and 15.0.2 seemed to indicate that the
 # Clang generated code didn't depend on the entry_pc being parsed.
 
+# Older versions of GCC emit an incorrect DW_AT_entry_pc.
+require {expr [gcc_major_version] >= 8 || ![is_c_compiler_gcc]}
+
 standard_testfile
 
 set options {debug optimize=-O2}