Re: [PATCH 3/3] [gdb/symtab] Tweak fix-up of truncated inline function block ranges

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

On 3/19/26 4:39 PM, Andrew Burgess wrote:
> 
> Hi Tom,
> 
> Thanks for looking at this.
> 

Hi Andrew,

thanks for the review.

> Tom de Vries <tdevries@suse.de> writes:
> 
>> Consider test-case gdb.cp/step-and-next-inline.exp on ppc64le-linux.
>>
>> The corresponding source file step-and-next-inline.cc contains functions
>> tree_check and get_alias_set:
>> ...
>>      35	#define TREE_TYPE(NODE) (*tree_check (NODE, 0))
>>      36
>>      37	inline tree *
>>      38	tree_check (tree *t, int i)
>>      39	{
>>      40	  if (t->x != i)
>>      41	    abort();
>>      42	  tree *x = t;
>>      43	  return x;
>>      44	}
>>      ...
>>      48	int __attribute__((noinline, noclone))
>>      49	get_alias_set (tree *t)
>>      50	{
>>      51	  if (t != NULL
>>      52	      && TREE_TYPE (t).z != 1
>>      53	      && TREE_TYPE (t).z != 2
>>      54	      && TREE_TYPE (t).z != 3)
>>      55	    return 0;
>>      56	  return 1;
>>      57	}
>> ...
>> as well as a trivial function main calling get_alias_set.
>>
>> Say we step into the first call to tree_check, and then step to the return
>> at line 43:
>> ...
>> (gdb) s
>> tree_check (i=0, t=0x10020030 <xx>) at step-and-next-inline.cc:40
>> 40	  if (t->x != i)
>> (gdb) s
>> 43	  return x;
>> (gdb)
>> ...
>>
>> At that point, we have pc 0x1000071c:
>> ...
>> (gdb) p $pc
>> $1 = (void (*)(void)) 0x1000071c <get_alias_set(tree*)+28>
>> (gdb)
>> ...
>> and the backtrace looks like this:
>> ...
>>   (gdb) bt
>>   #0  tree_check (i=<optimized out>, t=<optimized out>) at
>>         step-and-next-inline.cc:43
>>   #1  get_alias_set (t=t@entry=0x10020030 <xx>) at step-and-next-inline.cc:52
>>   #2  0x0000000010000560 in main () at step-and-next-inline.cc:64
>>   (gdb)
>> ...
>> which shows all 3 functions.
>>
>> This seems trivial, but it's not.
>>
>> All three calls to tree_check are inlined, and the first call is represented
>> by:
>> ...
>>   <2><877>: Abbrev Number: 40 (DW_TAG_inlined_subroutine)
>>      <878>   DW_AT_abstract_origin: <0x967>
>>      <87c>   DW_AT_entry_pc    : 0x10000710
>>      <884>   DW_AT_GNU_entry_view: 0
>>      <885>   DW_AT_ranges      : 0xc
>>      <88a>   DW_AT_call_line   : 52
>> ...
>> with DW_AT_ranges referring to:
>> ...
>> Contents of the .debug_rnglists section:
>>
>>      Offset   Begin    End
>>      0000000c 0000000010000710 (base address)
>>      00000015 0000000010000710 000000001000071c
>>      00000018 000000001000077c 000000001000077c (start == end)
>>      0000001b 0000000010000788 0000000010000790
>>      0000001f <End of list>
>> ...
>>
>> The range at offset 0x15 is [0x10000710, 0x1000071c), so address 0x1000071c
>> does not fall in the range, and consequently the debug info does not consider
>> 0x1000071c part of the inlined tree_check.
>>
>> However, since commit 8efed40efd6 ("gdb: fix-up truncated inline function
>> block ranges"), gdb contains a fix in lnp_state_machine::record_line:
>> ...
>>    if (m_address != m_last_address
>>        && m_stmt_at_address
>>        && m_cu->producer_is_gcc ()
>>        && (m_flags & LEF_IS_STMT) == 0)
>>      dwarf_find_and_extend_inline_block_range (m_cu, m_last_address,
>>                                                m_address, m_line);
>> ...
>> that looks at the corresponding line number information:
>> ...
>> File name                    Line number    Starting address    View    Stmt
>> step-and-next-inline.cc               42          0x1000071c               x
>> step-and-next-inline.cc               43          0x1000071c       1       x
>> step-and-next-inline.cc               43          0x1000071c       2
>> step-and-next-inline.cc               52          0x1000071c       3
>> step-and-next-inline.cc               52          0x10000720
>> ...
>> and extends the range of the inlined tree_check to include
>> [0x1000071c, 0x10000720).
>>
>> [ Please read the commit message of aforementioned commit to understand why
>> the fix is correct. ]
> 
> I think it's worth discussing the idea behind the aforementioned commit
> at this point because it is the reason behind my later thoughts.
> 
> What that commit does is modify the debug information.  It has GDB
> trying to decide that it, GDB, knows better than the compiler, what the
> debug information should look like.  This is always going to come with a
> degree of risk.  To restrictive and the "fix" will not trigger often
> enough to provide real benefits to the user.  To loose and the "fix"
> ends up triggering in cases where it shouldn't, making the debug
> experience worse.
> 

Ack.

> The pattern I observed, and sort-of tried to match (more on this below),
> is as follows:
> 
>    1. A series of line entries at the end address for (one of) an inline
>       function's ranges.
> 
>    2. The first of these line entries is a STMT, while the subsequent
>       entries are non-STMT.
> 
>    3. The last of these line entries is a non-STMT and is associated with
>       the calling line of the inline function.
> 
>    4. The next line entry is a non-STMT, and is also associated with the
>       calling line of the function.
> 

Thanks for spelling this out, this is helpful for me.

I understand that you're explaining here what you tried to match, but I 
wonder if the 4th item is indeed necessary.  That is, we're trying to 
match something like this:
...
Line number    Starting address    View    Stmt
          42          0x1000071c               x
          43          0x1000071c       1       x
          43          0x1000071c       2
          52          0x1000071c       3
          52          0x10000720
...
but if you annotate each entry using the following address like this:
...
Line number              Address range    View    Stmt
          42    [0x1000071c,0x1000071c)               x
          43    [0x1000071c,0x1000071c)       1       x
          43    [0x1000071c,0x1000071c)       2
          52    [0x1000071c,0x10000720)       3
          52    [0x10000720,...)
...
then would we still look at the last item at all?  ISTM we just need the 
address.

> Now I said above that I 'sort-of' tried to match this pattern.
> Honestly, I don't think I did a great job, only some of these
> characteristics are actually matched.  Lets go through and see which
> characteristics are, or are not actually checked for:
> 
>    1. This is checked inside dwarf_find_and_extend_inline_block_range
>       where we check if original_address matches the end address of an
>       inline function range.
> 
>    2. For this we check m_stmt_at_address, but this isn't exactly
>       correct. If _any_ of the line entries at the previous address are
>       marked as STMT then this will be true (and count at a match), this
>       is probably OK in most cases except for the next point....
> 
>    3. We don't really check this at all; as you point out we check the
>       current line, not the previous one, and m_stmt_at_address will be
>       true if the previous entry is a STMT, which I think would be bad.

We could add an m_last_is_stmt field, and use that.

> 
>    4. We do check the non-STMT part, and we check the line number part.
> 
> So I was a little sloppy with the STMT checking, and should have taken
> care to add both line number checks.
> 
> I mention this because at this point your commit message seem (IMHO) to
> focus on the is-stmt logic even though that's not really the part you
> end up fixing.
> 

Yes, the part about the is-stmt was merely a reflection of my difficulty 
in understanding it.  I've submitted a v2 that has two refactoring 
patches added that focus on that part, leaving the commit message of 
this patch much cleaner, I hope.

>>
>> Let's look in more detail at how the call to
>> dwarf_find_and_extend_inline_block_range is activated for the fix.  It's
>> activated for the last entry (52/0x10000720), with:
>> - m_last_address == 0x1000071c,
>> - m_address == 0x10000720, and
>> - m_line == 52 (matching the DW_AT_call_line).
>>
>> It's easy to see that for the last entry, (m_flags & LEF_IS_STMT) == 0 holds,
>> because it doesn't have an x in the "Stmt" column.
>>
>> I found it less obvious that m_stmt_at_address also holds.
>>
>> [ The documentation clarifies that this is related to m_last_address:
>> ....
>>    /* Set to true when a previous line at the same address (using
>>       m_last_address) had LEF_IS_STMT set in m_flags.  This is reset to false
>>       when a line entry at a new address (m_address different to
>>       m_last_address) is processed.  */
>>    bool m_stmt_at_address = false;
>> ...
>>
>> To get maximum clarity, I checked the value for each entry:
>> ...
>> address         m_stmt_at_address
>> ---------------------------------
>> before          false
>> 42/0x1000071c   false->true
>> 43/0x1000071c/1 true
>> 43/0x1000071c/2 true
>> 52/0x1000071c/3 true
>> 52/0x10000720   true->false
>> ...
>>
>> Again it's easy to relate the transitions for particular entries to the "Stmt"
>> column.
>>
>> The tricky bit is that the transition takes place at the end of
>> lnp_state_machine::record_line, so while processing entry 52/0x10000720, we
>> sample m_stmt_at_address while it's still true. ]
>>
>> Likewise, the fix works for the second inlined call.
>>
>> But not for the third.  The debug info has the same problem, but the fix is
>> not applied.
>>
>> The corresponding line info looks slightly different:
>> ...
>> File name                    Line number    Starting address    View    Stmt
>> step-and-next-inline.cc               42          0x1000074c               x
>> step-and-next-inline.cc               43          0x1000074c       1       x
>> step-and-next-inline.cc               43          0x1000074c       2
>> step-and-next-inline.cc               54          0x1000074c       3
>> step-and-next-inline.cc               55          0x10000750
>> ...
>>
>> In this case, dwarf_find_and_extend_inline_block_range gets called with:
>> - m_last_address == 0x1000074c,
>> - m_address == 0x10000750, and
>> - m_line == 55,
>> but since line 55 doesn't match DW_AT_call_line 54:
> 
> Your discussion of the is-stmt handling / tracking is all correct, but
> the cause of the problem is captured in these last 5 lines, the next
> line is not associated with the function's calling line, but with the
> next source line.
> 
>> ...
>>   <2><918>: Abbrev Number: 44 (DW_TAG_inlined_subroutine)
>>      <919>   DW_AT_abstract_origin: <0x967>
>>      <91d>   DW_AT_entry_pc    : 0x10000740
>>      <925>   DW_AT_GNU_entry_view: 0
>>      <926>   DW_AT_low_pc      : 0x10000740
>>      <92e>   DW_AT_high_pc     : 0xc
>>      <937>   DW_AT_call_line   : 54
>> ...
>> the fix is not applied.
>>
>> I'm proposing the following simple tweak, to handle the third inlined call as
>> well: instead of using m_line, use m_last_line.
> 
> If I'd done my job better when I wrote the original commit then we would
> have been checking BOTH lines already, and requiring that they both
> match the calling line; that after all was the original pattern I
> spotted.
> 
> So in my mind the question is, if I had done a better job of matching
> the pattern, would it be OK to loosen that matching?  I'm a little
> conflicted on this, but I think it's probably worth the risk.
> 
>>
>> Tested on x86_64-linux and ppc64le-linux.
>>
>> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=33930
>> ---
>>   gdb/dwarf2/line-program.c | 15 ++++++++++++++-
>>   1 file changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/gdb/dwarf2/line-program.c b/gdb/dwarf2/line-program.c
>> index b9d93bb7b0e..5aa97ea3714 100644
>> --- a/gdb/dwarf2/line-program.c
>> +++ b/gdb/dwarf2/line-program.c
>> @@ -445,12 +445,25 @@ lnp_state_machine::record_line (bool end_sequence)
>>   		  (end_sequence ? "\t(end sequence)" : ""));
>>       }
>>   
>> +  /*  Activate dwarf_find_and_extend_inline_block_range for line number info:
>> +
>> +	Line number    Starting address	   View	   Stmt
>> +		 42	     0x1000074c		      x
>> +		 43	     0x1000074c	      1	      x
>> +		 43	     0x1000074c	      2
>> +		 54	     0x1000074c	      3
>> +		 55	     0x10000750
>> +
>> +      at entry 55/0x10000750 with:
>> +      - m_last_address == 0x1000074c
>> +      - m_address == 0x10000750
>> +      - m_last_line == 54.  */
>>     if (m_address != m_last_address
>>         && m_stmt_at_address
>>         && m_cu->producer_is_gcc ()
>>         && (m_flags & LEF_IS_STMT) == 0)
> 
> I wonder if should extend the condition with `&& m_line >= m_last_line`?
> 

As I tried to explain above, I don't think so, and having said that, I 
think we could probably also drop the '(m_flags & LEF_IS_STMT) == 0' 
part.  Haven't done so in a v2 though.

> Under my original scheme I _should_ have been checking `& m_line ==
> m_last_line` and your change is to relax that constraint to `>=`.
> 
>>       dwarf_find_and_extend_inline_block_range (m_cu, m_last_address,
>> -					      m_address, m_line);
>> +					      m_address, m_last_line);
>>   
> 
> This patch will need rebasing onto your frame printing patch once that's
> merged, and I guess you'll be enabling the additional tests that are in
> that patch and currently disabled so that this commit gets some tests.
> 
> But I think this change is the right way to go, we just need to be
> mindful that as we relax the match criteria we run the risk of having
> GDB change the debug info in non-helpful ways.
> 

Agreed.

V2 submitted here ( 
https://sourceware.org/pipermail/gdb-patches/2026-April/226646.html ).

Thanks,
- Tom


> Thanks,
> Andrew
>