[PINGv4][PATCH] gdb: Guarantee that an SAL's end is right before the next statement

[Guinevere Larsen <blarsen@redhat.com>]

Ping!

-- 
Cheers,
Guinevere Larsen
She/Her/Hers

On 27/10/2023 10:57, Guinevere Larsen wrote:
> When examining a failure that happens when testing
> gdb.python/py-symtab.c with clang, I noticed that it was going wrong
> because the test assumed that whenever we get an SAL, its end would
> always be right before statement in the line table. This is true for GCC
> compiled binaries, since gcc only adds statements to the line table, but
> not true for clang compiled binaries.
>
> This is the second time I run into a problem where GDB doesn't handle
> non-statement line table entries correctly. The other was eventually
> committed as 9ab50efc463ff723b8e9102f1f68a6983d320517: "gdb: fix until
> behavior with trailing !is_stmt lines", but that commit only changes the
> behavior for the 'until' command. In this patch I propose a more general
> solution, making it so every time we generate the SAL for a given pc, we
> set the end of the SAL to before the next statement or the first
> instruciton in the next line, instead of naively assuming that to be the
> case.
>
> With this new change, the edge case is removed from the processing of
> the 'until' command without regressing the accompanying test case, and
> no other regressions were observed in the testsuite.
>
> Regression tested on fedora 37 with GCC and clang.
>
> ---
>   gdb/infcmd.c | 39 ---------------------------------------
>   gdb/symtab.c | 10 ++++++++--
>   2 files changed, 8 insertions(+), 41 deletions(-)
>
> diff --git a/gdb/infcmd.c b/gdb/infcmd.c
> index cf8cd527955..72dc8231523 100644
> --- a/gdb/infcmd.c
> +++ b/gdb/infcmd.c
> @@ -1363,45 +1363,6 @@ until_next_command (int from_tty)
>   
>         tp->control.step_range_start = func->value_block ()->entry_pc ();
>         tp->control.step_range_end = sal.end;
> -
> -      /* By setting the step_range_end based on the current pc, we are
> -	 assuming that the last line table entry for any given source line
> -	 will have is_stmt set to true.  This is not necessarily the case,
> -	 there may be additional entries for the same source line with
> -	 is_stmt set false.  Consider the following code:
> -
> -	 for (int i = 0; i < 10; i++)
> -	   loop_body ();
> -
> -	 Clang-13, will generate multiple line table entries at the end of
> -	 the loop all associated with the 'for' line.  The first of these
> -	 entries is marked is_stmt true, but the other entries are is_stmt
> -	 false.
> -
> -	 If we only use the values in SAL, then our stepping range may not
> -	 extend to the end of the loop. The until command will reach the
> -	 end of the range, find a non is_stmt instruction, and step to the
> -	 next is_stmt instruction. This stopping point, however, will be
> -	 inside the loop, which is not what we wanted.
> -
> -	 Instead, we now check any subsequent line table entries to see if
> -	 they are for the same line.  If they are, and they are marked
> -	 is_stmt false, then we extend the end of our stepping range.
> -
> -	 When we finish this process the end of the stepping range will
> -	 point either to a line with a different line number, or, will
> -	 point at an address for the same line number that is marked as a
> -	 statement.  */
> -
> -      struct symtab_and_line final_sal
> -	= find_pc_line (tp->control.step_range_end, 0);
> -
> -      while (final_sal.line == sal.line && final_sal.symtab == sal.symtab
> -	     && !final_sal.is_stmt)
> -	{
> -	  tp->control.step_range_end = final_sal.end;
> -	  final_sal = find_pc_line (final_sal.end, 0);
> -	}
>       }
>     tp->control.may_range_step = 1;
>   
> diff --git a/gdb/symtab.c b/gdb/symtab.c
> index 5ec56f4f2af..090f6415af6 100644
> --- a/gdb/symtab.c
> +++ b/gdb/symtab.c
> @@ -3207,10 +3207,16 @@ find_pc_sect_line (CORE_ADDR pc, struct obj_section *section, int notcurrent)
>   	       unrelocated_addr (pc - objfile->text_section_offset ()),
>   	       pc_compare));
>         if (item != first)
> -	prev = item - 1;		/* Found a matching item.  */
> +	{
> +	  prev = item - 1;		/* Found a matching item.  */
> +	  /* At this point, prev is a line whose address is <= pc.  However, we
> +	     don't know if ITEM is pointing to the same statement or not.  */
> +	  while (item != last && prev->line == item->line && !item->is_stmt)
> +	    item++;
> +	}
>   
>         /* At this point, prev points at the line whose start addr is <= pc, and
> -	 item points at the next line.  If we ran off the end of the linetable
> +	 item points at the next statement.  If we ran off the end of the linetable
>   	 (pc >= start of the last line), then prev == item.  If pc < start of
>   	 the first line, prev will not be set.  */
>