[v5,2/3] Introduce a new line table flag is_weak

  This introduces a new line table flag is_weak.
The line entries at the end of a subroutine range,
use this to indicate that they may possibly
be part of the previous subroutine.

When there is a sequence of line entries at the
same address where an inline range ends, and the
last item has is_stmt = 0, we force all previous
items to have is_weak = 1.

Additionally this adds a "fake" end sequence to the
record_line function, that is line number -1.
That will be used in the next patch.

Finally this adds a handling for empty ranges to
record_block_range.  Currently this function is
not called with empty ranges, but that will be used
in the next patch.

There should be no functional changes after this commit.

Well, except the new is-weak flag in the line table of course.
As an example consider the following test code:

$ cat test.c
inline int test ()
{
  asm ("nop");
  return 0;
}

int main ()
{
  int x = test ();
  return x;
}
$ gcc -g -O2 test.c

This will receive the following line table:

(gdb) maintenance info line-table
INDEX  LINE   REL-ADDRESS        UNREL-ADDRESS      IS-STMT IS-WEAK PROLOGUE-END EPILOGUE-BEGIN
0      8      0x0000555555555040 0x0000000000001040 Y
1      9      0x0000555555555040 0x0000000000001040 Y
2      1      0x0000555555555040 0x0000000000001040 Y
3      3      0x0000555555555040 0x0000000000001040 Y
4      4      0x0000555555555041 0x0000000000001041 Y       Y
5      4      0x0000555555555041 0x0000000000001041         Y  <---+ set is_weak
6      10     0x0000555555555041 0x0000000000001041 Y       Y      ^
7      11     0x0000555555555041 0x0000000000001041           <----+ no is-stmt
8      END    0x0000555555555044 0x0000000000001044 Y
---
 gdb/buildsym.c                               | 106 ++++++++++++++++---
 gdb/buildsym.h                               |   3 +
 gdb/jit.c                                    |   1 +
 gdb/symmisc.c                                |   6 +-
 gdb/symtab.h                                 |   5 +
 gdb/testsuite/gdb.dwarf2/dw2-ranges-base.exp |   2 +-
 gdb/xcoffread.c                              |   1 +
 7 files changed, 109 insertions(+), 15 deletions(-)
  

On 9/2/24 11:58 AM, Bernd Edlinger wrote:
> This introduces a new line table flag is_weak.
> The line entries at the end of a subroutine range,
> use this to indicate that they may possibly
> be part of the previous subroutine.
>
> When there is a sequence of line entries at the
> same address where an inline range ends, and the
> last item has is_stmt = 0, we force all previous
> items to have is_weak = 1.
>
> Additionally this adds a "fake" end sequence to the
> record_line function, that is line number -1.
> That will be used in the next patch.
>
> Finally this adds a handling for empty ranges to
> record_block_range.  Currently this function is
> not called with empty ranges, but that will be used
> in the next patch.
>
> There should be no functional changes after this commit.
>
> Well, except the new is-weak flag in the line table of course.
> As an example consider the following test code:
>
> $ cat test.c
> inline int test ()
> {
>    asm ("nop");
>    return 0;
> }
>
> int main ()
> {
>    int x = test ();
>    return x;
> }
> $ gcc -g -O2 test.c
>
> This will receive the following line table:
>
> (gdb) maintenance info line-table
> INDEX  LINE   REL-ADDRESS        UNREL-ADDRESS      IS-STMT IS-WEAK PROLOGUE-END EPILOGUE-BEGIN
> 0      8      0x0000555555555040 0x0000000000001040 Y
> 1      9      0x0000555555555040 0x0000000000001040 Y
> 2      1      0x0000555555555040 0x0000000000001040 Y
> 3      3      0x0000555555555040 0x0000000000001040 Y
> 4      4      0x0000555555555041 0x0000000000001041 Y       Y
> 5      4      0x0000555555555041 0x0000000000001041         Y  <---+ set is_weak
> 6      10     0x0000555555555041 0x0000000000001041 Y       Y      ^
> 7      11     0x0000555555555041 0x0000000000001041           <----+ no is-stmt
> 8      END    0x0000555555555044 0x0000000000001044 Y

I've been trying to review this patch, but I've come to the conclusion 
that the conceptual side is out of my depth. I will leave you and Andrew 
to hash out the high level overview.

If you decide to go with this approach, I have a couple comments about 
the specifics, but I wouldn't recommend that you act on these comments 
until a decision is made if this is the direction to solve inlined 
functions.

> ---
>   gdb/buildsym.c                               | 106 ++++++++++++++++---
>   gdb/buildsym.h                               |   3 +
>   gdb/jit.c                                    |   1 +
>   gdb/symmisc.c                                |   6 +-
>   gdb/symtab.h                                 |   5 +
>   gdb/testsuite/gdb.dwarf2/dw2-ranges-base.exp |   2 +-
>   gdb/xcoffread.c                              |   1 +
>   7 files changed, 109 insertions(+), 15 deletions(-)
>
> diff --git a/gdb/buildsym.c b/gdb/buildsym.c
> index 02d6848f3a4..2e552fa9046 100644
> --- a/gdb/buildsym.c
> +++ b/gdb/buildsym.c
> @@ -413,6 +413,16 @@ buildsym_compunit::record_block_range (struct block *block,
>         || end_inclusive + 1 != block->end ())
>       m_pending_addrmap_interesting = true;
>   
> +  if (block->inlined_p ())
> +    {
> +      m_inline_end_vector.push_back (end_inclusive + 1);
> +      if (end_inclusive + 1 == start)
> +	{
> +	  end_inclusive = start;
> +	  m_pending_addrmap_interesting = true;
> +	}
> +    }
> +
>     m_pending_addrmap.set_empty (start, end_inclusive, block);
>   }
>   
> @@ -627,19 +637,16 @@ buildsym_compunit::record_line (struct subfile *subfile, int line,
>   {
>     m_have_line_numbers = true;
>   
> -  /* Normally, we treat lines as unsorted.  But the end of sequence
> -     marker is special.  We sort line markers at the same PC by line
> -     number, so end of sequence markers (which have line == 0) appear
> -     first.  This is right if the marker ends the previous function,
> -     and there is no padding before the next function.  But it is
> -     wrong if the previous line was empty and we are now marking a
> -     switch to a different subfile.  We must leave the end of sequence
> -     marker at the end of this group of lines, not sort the empty line
> -     to after the marker.  The easiest way to accomplish this is to
> -     delete any empty lines from our table, if they are followed by
> -     end of sequence markers.  All we lose is the ability to set
> -     breakpoints at some lines which contain no instructions
> -     anyway.  */
> +  /* The end of sequence marker is special.  We need to delete any
> +     previous lines at the same PC, otherwise these lines may cause
> +     problems since they might be at the same address as the following
> +     function.  For instance suppose a function calls abort there is no
> +     reason to emit a ret after that point (no joke).
> +     So the label may be at the same address where the following
> +     function begins.  There is also a fake end of sequence marker (-1)
> +     that we emit internally when switching between different CUs
> +     In this case, duplicate line table entries shall not be deleted.
> +     We simply set the is_weak marker in this case.  */
>     if (line == 0)
>       {
>         std::optional<int> last_line;
> @@ -659,15 +666,84 @@ buildsym_compunit::record_line (struct subfile *subfile, int line,
>         if (!last_line.has_value () || *last_line == 0)
>   	return;
>       }
> +  else if (line == -1)
> +    {
> +      line = 0;
> +      auto e = subfile->line_vector_entries.end ();
> +      while (e > subfile->line_vector_entries.begin ())
> +	{
> +	  e--;
> +	  if (e->unrelocated_pc () != pc)
> +	    break;
> +	  e->is_weak = 1;
> +	}
> +    }

This part of the change (and the comment above) should definitely go on 
patch 3.

If record_line is only called with line == -1 in patch 3, not having 
this hunk in that patch makes it very confusing to review those changes, 
and could make anyone in the future very confused when trying to 
understand the commit, especially if finding these email threads turns 
out to be difficult.

>   
>     linetable_entry &e = subfile->line_vector_entries.emplace_back ();
>     e.line = line;
>     e.is_stmt = (flags & LEF_IS_STMT) != 0;
> +  e.is_weak = false;
>     e.set_unrelocated_pc (pc);
>     e.prologue_end = (flags & LEF_PROLOGUE_END) != 0;
>     e.epilogue_begin = (flags & LEF_EPILOGUE_BEGIN) != 0;
>   }
>   
> +
> +/* Patch the is_stmt bits at the given inline end address.
> +   The line table has to be already sorted.  */
Firstly, this comment is incorrect, you're patching is_weak, not 
is_stmt, but more importantly below:
> +
> +static void
> +patch_inline_end_pos (struct subfile *subfile, struct objfile *objfile,
> +		      CORE_ADDR end)

I think this comment+function name only make sense because we're deep in 
this conversation. Someone who hasn't had this context hitting this 
function will be very confused.

There is nowhere in the code that concisely explains is_weak. Since this 
seems like the main place is_weak is set, the function comment could 
explain it some more. I would suggest the following:

/* Patch all the line table entries at the final PC of an inline 
function, marking them as "weak".
     weak statements are used to identify line table entries that likely 
belong to an inner block,
     while not weak statements likely below to the outermost block 
containing that PC.
     The line table has to be sorted already.  */
static void
patch_weak_linetable_entries (...)

This way, in 5-10+ years it will still be easy to understand what this 
code is aiming to do, even if we decide to change version control 
systems or lose access to mailing list archives (yes, I've been having 
trouble with lost history, how can you tell?)

> +{
> +  std::vector<linetable_entry> &items = subfile->line_vector_entries;
> +  int a = 2, b = items.size () - 1;
> +
> +  /* We need at least two items with pc = end in the table.
> +     The lowest usable items are at pos 0 and 1, the highest
> +     usable items are at pos b - 2 and b - 1.  */
> +  if (a > b
> +      || end < items[1].pc (objfile)
> +      || end > items[b - 2].pc (objfile))
> +    return;
> +
> +  /* Look for the first item with pc > end in the range [a,b].
> +     The previous element has pc = end or there is no match.
> +     We set a = 2, since we need at least two consecutive elements
> +     with pc = end to do anything useful.
> +     We set b = items.size () - 1, since we are not interested
> +     in the last element which should be an end of sequence
> +     marker with line = 0 and is_stmt = true.  */
> +  while (a < b)
> +    {
> +      int c = (a + b) / 2;
> +
> +      if (end < items[c].pc (objfile))
> +	b = c;
> +      else
> +	a = c + 1;
> +    }
> +
> +  a--;
> +  if (items[a].pc (objfile) != end || items[a].is_stmt)
> +    return;
> +
> +  /* When there is a sequence of line entries at the same address
> +     where an inline range ends, and the last item has is_stmt = 0,
> +     we force all previous items to have is_weak = true as well.  */
> +  do
> +    {
> +      /* We stop at the first line entry with a different address,
> +	 or when we see an end of sequence marker.  */
> +      a--;
> +      if (items[a].pc (objfile) != end || items[a].line == 0)
> +	break;
> +
> +      items[a].is_weak = true;
> +    }
> +  while (a > 0);
> +}
> +
>   
>   /* Subroutine of end_compunit_symtab to simplify it.  Look for a subfile that
>      matches the main source file's basename.  If there is only one, and
> @@ -892,6 +968,10 @@ buildsym_compunit::end_compunit_symtab_with_blockvector
>   	     relationships, this is why std::stable_sort is used.  */
>   	  std::stable_sort (subfile->line_vector_entries.begin (),
>   			    subfile->line_vector_entries.end ());
> +
> +	  for (int i = 0; i < m_inline_end_vector.size (); i++)
> +	    patch_inline_end_pos (subfile, m_objfile,
> +				  m_inline_end_vector[i]);
>   	}
>   
>         /* Allocate a symbol table if necessary.  */
> diff --git a/gdb/buildsym.h b/gdb/buildsym.h
> index c1eed247d25..edf76c8b17c 100644
> --- a/gdb/buildsym.h
> +++ b/gdb/buildsym.h
> @@ -446,6 +446,9 @@ struct buildsym_compunit
>   
>     /* Pending symbols that are local to the lexical context.  */
>     struct pending *m_local_symbols = nullptr;
> +
> +  /* Pending inline end range addresses.  */
> +  std::vector<CORE_ADDR> m_inline_end_vector;
>   };
>   
>   
> diff --git a/gdb/jit.c b/gdb/jit.c
> index 89c967db615..3e9353e206b 100644
> --- a/gdb/jit.c
> +++ b/gdb/jit.c
> @@ -495,6 +495,7 @@ jit_symtab_line_mapping_add_impl (struct gdb_symbol_callbacks *cb,
>   	(unrelocated_addr (map[i].pc));
>         stab->linetable->item[i].line = map[i].line;
>         stab->linetable->item[i].is_stmt = true;
> +      stab->linetable->item[i].is_weak = false;
>       }
>   }
>   
> diff --git a/gdb/symmisc.c b/gdb/symmisc.c
> index 7f8141588b7..eb1487b39b0 100644
> --- a/gdb/symmisc.c
> +++ b/gdb/symmisc.c
> @@ -265,6 +265,8 @@ dump_symtab_1 (struct symtab *symtab, struct ui_file *outfile)
>   	  gdb_puts (paddress (gdbarch, l->item[i].pc (objfile)), outfile);
>   	  if (l->item[i].is_stmt)
>   	    gdb_printf (outfile, "\t(stmt)");
> +	  if (l->item[i].is_weak)
> +	    gdb_printf (outfile, "\t(weak)");
>   	  gdb_printf (outfile, "\n");
>   	}
>       }
> @@ -972,12 +974,13 @@ maintenance_print_one_line_table (struct symtab *symtab, void *data)
>         /* Leave space for 6 digits of index and line number.  After that the
>   	 tables will just not format as well.  */
>         struct ui_out *uiout = current_uiout;
> -      ui_out_emit_table table_emitter (uiout, 7, -1, "line-table");
> +      ui_out_emit_table table_emitter (uiout, 8, -1, "line-table");
>         uiout->table_header (6, ui_left, "index", _("INDEX"));
>         uiout->table_header (6, ui_left, "line", _("LINE"));
>         uiout->table_header (18, ui_left, "rel-address", _("REL-ADDRESS"));
>         uiout->table_header (18, ui_left, "unrel-address", _("UNREL-ADDRESS"));
>         uiout->table_header (7, ui_left, "is-stmt", _("IS-STMT"));
> +      uiout->table_header (7, ui_left, "is-weak", _("IS-WEAK"));
>         uiout->table_header (12, ui_left, "prologue-end", _("PROLOGUE-END"));
>         uiout->table_header (14, ui_left, "epilogue-begin", _("EPILOGUE-BEGIN"));
>         uiout->table_body ();
> @@ -998,6 +1001,7 @@ maintenance_print_one_line_table (struct symtab *symtab, void *data)
>   	  uiout->field_core_addr ("unrel-address", objfile->arch (),
>   				  CORE_ADDR (item->unrelocated_pc ()));
>   	  uiout->field_string ("is-stmt", item->is_stmt ? "Y" : "");
> +	  uiout->field_string ("is-weak", item->is_weak ? "Y" : "");
>   	  uiout->field_string ("prologue-end", item->prologue_end ? "Y" : "");
>   	  uiout->field_string ("epilogue-begin", item->epilogue_begin ? "Y" : "");
>   	  uiout->text ("\n");
> diff --git a/gdb/symtab.h b/gdb/symtab.h
> index 4197a3a164c..188d7e68923 100644
> --- a/gdb/symtab.h
> +++ b/gdb/symtab.h
> @@ -1656,6 +1656,9 @@ struct linetable_entry
>     /* True if this PC is a good location to place a breakpoint for LINE.  */
>     bool is_stmt : 1;
>   
> +  /* True if this PC is at a subroutine range end.  */
> +  bool is_weak : 1;
> +
>     /* True if this location is a good location to place a breakpoint after a
>        function prologue.  */
>     bool prologue_end : 1;
> @@ -2412,6 +2415,8 @@ struct symtab_and_line
>     /* If the line number information is valid, then this indicates if this
>        line table entry had the is-stmt flag set or not.  */
>     bool is_stmt = false;
> +  /* True if this PC is at a subroutine range end.  */
> +  bool is_weak = false;
>   
>     /* The probe associated with this symtab_and_line.  */
>     probe *prob = NULL;
> diff --git a/gdb/testsuite/gdb.dwarf2/dw2-ranges-base.exp b/gdb/testsuite/gdb.dwarf2/dw2-ranges-base.exp
> index 1a3d53c2116..403bc6b3f72 100644
> --- a/gdb/testsuite/gdb.dwarf2/dw2-ranges-base.exp
> +++ b/gdb/testsuite/gdb.dwarf2/dw2-ranges-base.exp
> @@ -168,7 +168,7 @@ gdb_test_multiple "maint info line-table gdb.dwarf2/dw2-ranges-base.c" \
>   	-re ".*linetable: \\(\\(struct linetable \\*\\) 0x0\\):\r\nNo line table.\r\n" {
>   	    exp_continue
>   	}
> -	-re ".*linetable: \\(\\(struct linetable \\*\\) $hex\\):\r\nINDEX\[ \t\]+LINE\[ \t\]+REL-ADDRESS\[ \t\]+UNREL-ADDRESS\[ \t\]+IS-STMT\[ \t\]PROLOGUE-END\[ \t\]EPILOGUE-BEGIN *\r\n" {
> +	-re ".*linetable: \\(\\(struct linetable \\*\\) $hex\\):\r\nINDEX\[ \t\]+LINE\[ \t\]+REL-ADDRESS\[ \t\]+UNREL-ADDRESS\[ \t\]+IS-STMT\[ \t\]IS-WEAK\[ \t\]PROLOGUE-END\[ \t\]EPILOGUE-BEGIN *\r\n" {
>   	    exp_continue
>   	}
>       }
> diff --git a/gdb/xcoffread.c b/gdb/xcoffread.c
> index 639dd5b8adc..1642bc12dfb 100644
> --- a/gdb/xcoffread.c
> +++ b/gdb/xcoffread.c
> @@ -431,6 +431,7 @@ arrange_linetable (std::vector<linetable_entry> &old_linetable)
>   	  linetable_entry &e = fentries.emplace_back ();
>   	  e.line = ii;
>   	  e.is_stmt = true;
> +	  e.is_weak = false;
>   	  e.set_unrelocated_pc (old_linetable[ii].unrelocated_pc ());
>   	}
>       }