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

[Andrew Burgess <aburgess@redhat.com>]

Hi Tom,

Thanks for looking at this.

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.

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.

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.

  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.

>
> 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`?

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.

Thanks,
Andrew