Re: [PATCH] gdb: handle DW_AT_entry_pc pointing at an empty sub-range

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

On 11/26/24 18:48, Andrew Burgess wrote:
> Bernd Edlinger <bernd.edlinger@hotmail.de> writes:
> 
>> On 11/25/24 15:30, Andrew Burgess wrote:
>>> Bernd Edlinger <bernd.edlinger@hotmail.de> writes:
>>>
>>>> Okay, I just wanted to point out that in my opinion the debug info which
>>>> points at the end of a sub-range is not incorrect, just maybe on a border
>>>> line, where the dwarf spec is unclear.  So you should not say:
>>>> "after all, the DWARF spec is clear that such a range covers no code."
>>>>
>>>> But there are obviously not only cases where the entry_pc points at
>>>> an empty sub-range, but also in very rare cases the entry_pc points at
>>>> the end of a non-empty sub-range.
>>>> So could you please change the check in dwarf2_addr_in_block_ranges
>>>> from addr >= start && addr < end to addr >= start && addr <= end.
>>>
>>> Could you expand on why you believe that the DWARF spec is unclear in
>>> this regard.  I came to my conclusion based on this text within the
>>> DWARF-5 specification, section 2.17.3 Non-Contiguous Address Ranges:
>>>
>>>   Bounded range. This kind of entry defines an address range that is
>>>   included in the range list. The starting address is the lowest address
>>>   of the address range. The ending address is the address of the first
>>>   location past the highest address of the address range
>>>
>>> This seems pretty clear (to me) that the end address is not part of the
>>> region covered by a range.
>>>
>>
>> Yes, but on the other hand, when we look at line table entries, each has a
>> PC and a VIEW number, and even the DW_AT_entry_pc has a DW_AT_GNU_entry_view,
>> just the range list does not have a view number, and that is inconsistent
>> with the concept of location views.
>>
>> Consider as a simple example an inline function:
>>
>> int f(int x)
>> {
>>  x++;
>>  return x;
>> }
>>
>> it will most likely just be compiled into one "inc eax" or similar,
>> and of course you may want to set a break point on the return statement,
>> to inspect 'x' after the increment, but that will be on 'pc == end' !
>>
>> But if the location view number would not be missing from the rnglist
>> it would be obvious whether the corresponding view number is still within
>> subroutine and not outside.  So in my opinion it is a defect in the
>> specification that it does not reflect this use case.
> 
> You make an interesting argument that the specification is deficient.
> But I'm not sure how this helps with this discussion.  I would like to
> avoid derailing this conversation with discussion of missing DWARF
> features.
> 
>>
>>> Additionally, if we start to accept 'addr == end' then this is going to
>>> cause problems elsewhere.  GDB will place a b/p at the 'end' address,
>>> but when GDB then performs block lookup, GDB will not return the block
>>> we expect, and so GDB will not report the inferior as having stopped in
>>> the scope that the user expects.
>>>
>>
>> No, because this is exactly what the core of my patch does, admittedly
>> I also modified the block lookup code a bit, to handle that case.
>> So I strongly disagree here: we have to accept 'addr == end' and other
>> corner cases, otherwise my patch won't work in the end, regardless of in
>> how many small bug-fixes it can be split up, because it depends exactly
>> on not ignoring any information while parsing the debug info.
> 
> But accepting 'addr == end' only works if you also change the block
> lookup mechanism, which isn't part of this patch.  This patch is based
> on the state of block lookup as it exists today.
> 
> I've included a patch below which applies on top of this patch (i.e. the
> one this thread is about), it changes the check to accept 'addr == end'
> as you suggest.  It also updates the test so that an inline function
> (bar) has DW_AT_entry_pc point at the 'end' address of a non-empty
> sub-range.
> 
> Here's a GDB session with that patch applied:
> 
>   (gdb) b bar
>   Breakpoint 1 at 0x401137
>   (gdb) r
>   Starting program: /tmp/gdb/testsuite/outputs/gdb.dwarf2/dw2-entry-pc-in-empty-range/dw2-entry-pc-in-empty-range-4 
>   
>   Breakpoint 1, 0x0000000000401137 in foo ()
>   (gdb) maintenance info blocks 
>   Blocks at 0x401137:
>     from objfile: [(objfile *) 0x3b11720] /tmp/gdb/testsuite/outputs/gdb.dwarf2/dw2-entry-pc-in-empty-range/dw2-entry-pc-in-empty-range-4
>   
>   [(block *) 0x357e680] 0x401106..0x401185
>     entry pc: 0x401106
>     is global block
>     symbol count: 1
>     is contiguous
>   [(block *) 0x357e630] 0x401106..0x401185
>     entry pc: 0x401106
>     is static block
>     is contiguous
>   [(block *) 0x357e5e0] 0x401106..0x401185
>     entry pc: 0x401106
>     function: foo
>     is contiguous
>   (gdb) 
> 
> As you can see, GDB stops at an address which doesn't then resolve to
> the bar block.
> 

Ah, okay, thanks for the test case I looked into it and tried it out with
a gdb built with my patch.

A breakpoint an an empty subrange works as expected, but indeed a breakpoint
at the end of a non-empty subrange does behave as you described.
But with real code examples a break point at the end of a non-empty subrange
does work, and that is because my "heuristic" which does the magic, uses
weak line-table entries near the end of a subrange to determine what to do
here.  So because the test case does not have a line table at all, the
test case is not realistic in that aspect, and does not tell you what happens
here in a real world example.

So a patch that allows for start <= addr && addr <= end should be applied,
probably with a line table that looks more like a real line table especially
at the end of a subrange, then the test should probably work out of the box.
But it is okay to do that after the rest of the series is applied.



> If/when the block lookup code is changed as you propose then this
> restriction (addr < end) can be relaxed.  But it doesn't make sense to
> merge the relaxed restriction, without the block lookup changes.
> 
> And no, I don't see that as a reason to merge all of the changes at
> once.  I think splitting the original large change into many small steps
> is the correct approach.  And sometimes that will mean that we check
> something in, and then revise it in a later commit.  That's not a
> problem with this approach, it's an advantage of this approach.  It
> makes it clearer how we got to the final destination.  And each step is
> smaller, and easier to review.  This is the preferred approach for GDB
> patches.
> 
> I feel that, as the author of the original large change, you're looking
> ahead and you're frustrated that this code isn't inline with how you
> feel the code should finally look.  But just because I hope we can check
> this code in first, doesn't mean that I will prevent this code being
> changed later on.  As GDB evolves (e.g. if the block lookup code
> changes) then I'm happy for this code to evolve with it.
> 
> To (I hope) offer you some confidence, I have, on my machine, created a
> branch with this patch (without the 'addr == end' change), followed by
> the next two patches I plan to post (once this is merged), and then, on
> top of that, I have rebased your original series.  This includes all of
> your original tests completely unmodified.
> 
> I have tested this merge with gcc versions 14.2.0, 13.3.0, 12.2.0,
> 11.5.0, 10.5.0, 9.5.0, 9.3.1, 8.4.0, 8.1.0, and in all cases, all of
> your original tests pass.  I'd rather not post this merged branch just
> yet, as I'm worried that this might derail review of this patch even
> more, I really don't want to start discussing the next patches before
> they are even posted, but if it's the only way to move this patch
> forward then I could share the branch.  I do plan to make this unified
> branch available when I post the next two patches I'd like to upstream,
> as I think it will actually help at that point.
> 
> My hope is that you will be willing to accept this change on the
> understanding that this code might need to be modified in the future.
> 
> For my part I also accept that this code might need to change in the
> future.
> 


Well okay, please go ahead.


Thanks
Bernd.

> Thanks,
> Andrew
> 
> ---
> 
> ### Patch to show 'addr == end' doesn't work (yet) ###
> 
> diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
> index c178b13d96d..60daf032036 100644
> --- a/gdb/dwarf2/read.c
> +++ b/gdb/dwarf2/read.c
> @@ -11351,7 +11351,7 @@ dwarf2_addr_in_block_ranges (CORE_ADDR addr, struct block *block)
>    /* Check if ADDR is within any of the block's sub-ranges.  */
>    for (const blockrange &br : block->ranges ())
>      {
> -      if (addr >= br.start () && addr < br.end ())
> +      if (addr >= br.start () && addr <= br.end ())
>  	return true;
>      }
>  
> diff --git a/gdb/testsuite/gdb.dwarf2/dw2-entry-pc-in-empty-range.exp b/gdb/testsuite/gdb.dwarf2/dw2-entry-pc-in-empty-range.exp
> index 79b1783b2ec..9e4fb781a8d 100644
> --- a/gdb/testsuite/gdb.dwarf2/dw2-entry-pc-in-empty-range.exp
> +++ b/gdb/testsuite/gdb.dwarf2/dw2-entry-pc-in-empty-range.exp
> @@ -179,8 +179,8 @@ proc run_test { entry_label dwarf_version } {
>  	     "    $::foo_5\\.\\.$::foo_6"]
>  }
>  
> -foreach_with_prefix entry_label { foo_3 foo_4 } {
> -    foreach_with_prefix dwarf_version { 4 5 } {
> +foreach_with_prefix entry_label { foo_2 } {
> +    foreach_with_prefix dwarf_version { 4 } {
>  	run_test $entry_label $dwarf_version
>      }
>  }
>