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

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

[-- Attachment #1: Type: text/plain, Size: 7570 bytes --]

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.

Thanks,
- Tom

> I posted the fix as a new thread because it's not "fix a tiny bug" type
> thing that I'm happy to just commit without giving others a chance to
> fully review it.
> 
> Thanks,
> Andrew
> 

[-- Attachment #2: gdb.log --]
[-- Type: text/x-log, Size: 6251 bytes --]

Test run by vries on Fri Nov 15 09:04:46 2024
Native configuration is x86_64-pc-linux-gnu

		=== gdb tests ===

Schedule of variations:
    unix

Running target unix
Using /usr/share/dejagnu/baseboards/unix.exp as board description file for target.
Using /usr/share/dejagnu/config/unix.exp as generic interface file for target.
Using /data/vries/gdb/src/gdb/testsuite/config/unix.exp as tool-and-target-specific interface file.
Running /data/vries/gdb/src/gdb/testsuite/gdb.opt/inline-entry.exp ...
Executing on build: rm -rf /data/vries/gdb/leap-15-5/build/gdb/testsuite/outputs/gdb.opt/inline-entry    (timeout = 300)
builtin_spawn -ignore SIGHUP rm -rf /data/vries/gdb/leap-15-5/build/gdb/testsuite/outputs/gdb.opt/inline-entry
gdb_do_cache: universal_compile_options_c (  )
Executing on host: gcc   -fdiagnostics-color=never -c -o /data/vries/gdb/leap-15-5/build/gdb/testsuite/temp/2484/ccopts.o /data/vries/gdb/leap-15-5/build/gdb/testsuite/temp/2484/ccopts.c    (timeout = 300)
builtin_spawn -ignore SIGHUP gcc -fdiagnostics-color=never -c -o /data/vries/gdb/leap-15-5/build/gdb/testsuite/temp/2484/ccopts.o /data/vries/gdb/leap-15-5/build/gdb/testsuite/temp/2484/ccopts.c
get_compiler_info: gcc-7-5-0
Executing on host: gcc  -fno-stack-protector  -fdiagnostics-color=never -I/data/vries/gdb/src/gdb/testsuite/lib -c -g -O2  -o /data/vries/gdb/leap-15-5/build/gdb/testsuite/outputs/gdb.opt/inline-entry/inline-entry0.o /data/vries/gdb/src/gdb/testsuite/gdb.opt/inline-entry.c    (timeout = 300)
builtin_spawn -ignore SIGHUP gcc -fno-stack-protector -fdiagnostics-color=never -I/data/vries/gdb/src/gdb/testsuite/lib -c -g -O2 -o /data/vries/gdb/leap-15-5/build/gdb/testsuite/outputs/gdb.opt/inline-entry/inline-entry0.o /data/vries/gdb/src/gdb/testsuite/gdb.opt/inline-entry.c
gdb_do_cache: universal_compile_options_c (  )
Executing on host: gcc  -fno-stack-protector /data/vries/gdb/leap-15-5/build/gdb/testsuite/outputs/gdb.opt/inline-entry/inline-entry0.o  -fdiagnostics-color=never -I/data/vries/gdb/src/gdb/testsuite/lib -g -O2  -lm   -o /data/vries/gdb/leap-15-5/build/gdb/testsuite/outputs/gdb.opt/inline-entry/inline-entry    (timeout = 300)
builtin_spawn -ignore SIGHUP gcc -fno-stack-protector /data/vries/gdb/leap-15-5/build/gdb/testsuite/outputs/gdb.opt/inline-entry/inline-entry0.o -fdiagnostics-color=never -I/data/vries/gdb/src/gdb/testsuite/lib -g -O2 -lm -o /data/vries/gdb/leap-15-5/build/gdb/testsuite/outputs/gdb.opt/inline-entry/inline-entry
builtin_spawn /data/vries/gdb/leap-15-5/build/gdb/testsuite/../../gdb/gdb -nw -nx -q -iex set height 0 -iex set width 0 -data-directory /data/vries/gdb/leap-15-5/build/gdb/data-directory
(gdb) set height 0
(gdb) set width 0
(gdb) dir
Reinitialize source path to empty? (y or n) y
Source directories searched: $cdir:$cwd
(gdb) dir /data/vries/gdb/src/gdb/testsuite/gdb.opt
Source directories searched: /data/vries/gdb/src/gdb/testsuite/gdb.opt:$cdir:$cwd
(gdb) kill
The program is not being run.
(gdb) file /data/vries/gdb/leap-15-5/build/gdb/testsuite/outputs/gdb.opt/inline-entry/inline-entry
Reading symbols from /data/vries/gdb/leap-15-5/build/gdb/testsuite/outputs/gdb.opt/inline-entry/inline-entry...
(gdb) delete breakpoints
(gdb) info breakpoints
No breakpoints, watchpoints, tracepoints, or catchpoints.
(gdb) break -qualified main
Breakpoint 1 at 0x4003d0: file /data/vries/gdb/src/gdb/testsuite/gdb.opt/inline-entry.c, line 38.
(gdb) run 
Starting program: /data/vries/gdb/leap-15-5/build/gdb/testsuite/outputs/gdb.opt/inline-entry/inline-entry 

Breakpoint 1, main () at /data/vries/gdb/src/gdb/testsuite/gdb.opt/inline-entry.c:38
38	  if ((global && bar (1)) || bar (2))
(gdb) info source
Current source file is /data/vries/gdb/src/gdb/testsuite/gdb.opt/inline-entry.c
Compilation directory is /data/vries/gdb/leap-15-5/build/gdb/testsuite
Located in /data/vries/gdb/binutils-gdb.git/gdb/testsuite/gdb.opt/inline-entry.c
Contains 41 lines.
Source language is c.
Producer is GNU C11 7.5.0 -mtune=generic -march=x86-64 -g -O2 -fno-stack-protector.
Compiled with DWARF 4 debugging format.
Does not include preprocessor macro info.
(gdb) break bar
Breakpoint 2 at 0x4003d8: bar. (2 locations)
(gdb) print /d $bpnum
$1 = 2
(gdb) PASS: gdb.opt/inline-entry.exp: get number of bar breakpoint
break foo
Breakpoint 3 at 0x4004e0: file /data/vries/gdb/src/gdb/testsuite/gdb.opt/inline-entry.c, line 23.
(gdb) print /d $bpnum
$2 = 3
(gdb) PASS: gdb.opt/inline-entry.exp: get number of foo breakpoint
continue
Continuing.

Breakpoint 2.1, bar (val=<optimized out>) at /data/vries/gdb/src/gdb/testsuite/gdb.opt/inline-entry.c:29
29	  if (global == val)
(gdb) PASS: gdb.opt/inline-entry.exp: continue to bar
continue
Continuing.

Breakpoint 2.2, bar (val=2) at /data/vries/gdb/src/gdb/testsuite/gdb.opt/inline-entry.c:29
29	  if (global == val)
(gdb) FAIL: gdb.opt/inline-entry.exp: continue to foo
continue
Continuing.

Breakpoint 3, foo (arg=arg@entry=1) at /data/vries/gdb/src/gdb/testsuite/gdb.opt/inline-entry.c:23
23	  global += arg;
(gdb) FAIL: gdb.opt/inline-entry.exp: continue until exit
testcase /data/vries/gdb/src/gdb/testsuite/gdb.opt/inline-entry.exp completed in 0 seconds

		=== gdb Summary ===

# of expected passes		3
# of unexpected failures	2
Executing on host: /data/vries/gdb/leap-15-5/build/gdb/testsuite/../../gdb/gdb -nw -nx -q -iex "set height 0" -iex "set width 0" -data-directory /data/vries/gdb/leap-15-5/build/gdb/data-directory --version    (timeout = 300)
builtin_spawn -ignore SIGHUP /data/vries/gdb/leap-15-5/build/gdb/testsuite/../../gdb/gdb -nw -nx -q -iex set height 0 -iex set width 0 -data-directory /data/vries/gdb/leap-15-5/build/gdb/data-directory --version
GNU gdb (GDB) 16.0.50.20241114-git
Copyright (C) 2024 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.
/data/vries/gdb/leap-15-5/build/gdb/gdb version  16.0.50.20241114-git -nw -nx -q -iex "set height 0" -iex "set width 0" -data-directory /data/vries/gdb/leap-15-5/build/gdb/data-directory 

runtest completed at Fri Nov 15 09:04:46 2024