[PATCH v3] [PR gdb/17315] fix until behavior with trailing !is_stmt lines

[PATCH v3] [PR gdb/17315] fix until behavior with trailing !is_stmt lines

[Bruno Larsen via Gdb-patches]

When using the command "until", it is expected that GDB will exit a
loop if the current instruction is the last one related to that loop.
However, if there were trailing non-statement instructions, "until"
would just behave as "next".  This was most noticeable in clang-compiled
code, but might happen with gcc-compiled as well.  PR gdb/17315 relates
to this problem, as running gdb.base/watchpoint.exp with clang
would fail for this reason.

The current patch fixes this by adding an extra check to the
until_next_command function, going through all the following
instructions: if the next instruction relates to the same line and is
not marked as a statement, the end of the execution range is moved to
the end of this next instruction.

This patch also adds a test case that can be run with gcc to test that
this functionality is not silently broken in future updates.
---
 gdb/infcmd.c                                  |  23 ++++
 gdb/testsuite/gdb.base/until-trailing-insns.c |  53 ++++++++
 .../gdb.base/until-trailing-insns.exp         | 118 ++++++++++++++++++
 3 files changed, 194 insertions(+)
 create mode 100644 gdb/testsuite/gdb.base/until-trailing-insns.c
 create mode 100644 gdb/testsuite/gdb.base/until-trailing-insns.exp

diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 9f4ed8bff13..5e57437a4cb 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -1346,6 +1346,29 @@ until_next_command (int from_tty)
 
       tp->control.step_range_start = BLOCK_ENTRY_PC (SYMBOL_BLOCK_VALUE (func));
       tp->control.step_range_end = sal.end;
+
+      /* By setting the step_range_end based on the current pc, we implicitly
+	 assume that the last instruction for any given line must have is_stmt set.
+	 This is not necessarily true.  Clang-13, for example, would compile
+	 the following code:
+
+for(int i=0; i<10; i++)
+  {
+    foo();
+  }
+
+	 with 2 entries after the last is_stmt linetable entry.  To fix this, we 
+	 iterate over the instruction related to the end PC, until we find an sal
+	 related to a different line, and set that pc as the step_range_end. */
+
+      struct symtab_and_line final_sal;
+      final_sal = find_pc_line (tp->control.step_range_end, 0);
+
+      while (final_sal.line == sal.line && !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/testsuite/gdb.base/until-trailing-insns.c b/gdb/testsuite/gdb.base/until-trailing-insns.c
new file mode 100644
index 00000000000..68a2033f517
--- /dev/null
+++ b/gdb/testsuite/gdb.base/until-trailing-insns.c
@@ -0,0 +1,53 @@
+/* Copyright 2022 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+/* This test aims to recreate clang-13 behavior in a more controllable
+   environment.  We want to create a linetable like so:
+
+   line | code    | is_stmt |
+    1     i = 0;	Y
+    2     while(1){	N
+    3       if(...)	Y
+    4       a = i;	Y
+    5       i++;	Y
+    6     }		N
+
+    where all lines, with the exception of line 4, all point to the loop code
+    on line 2, effectively creating a "for" loop.  GDB's until command had a
+    problem with this setup, because when we're stopped at line 5 the range
+    of execution would not include line 6 - a jump instruction still related
+    to line 2, making us stop at the next instruction marked as a statement
+    and not related to the current line (in this example, line 4).  This test
+    exercises the command "until" in this complicated situation.
+
+ */
+
+int main(){/* main prologue */
+    asm("main_label: .globl main_label"); /* This is required */
+    asm("loop_start: .globl loop_start");
+    int a, i;
+    i = 0;						/* loop assignment */
+    while (1) {						/* loop line */
+	asm("loop_condition: .globl loop_condition");
+	if (i >= 10) break;				/* loop condition */
+	asm("loop_code: .globl loop_code");
+	a = i;						/* loop code */
+	asm("loop_increment: .globl loop_increment");
+	i ++;						/* loop increment */
+	asm("loop_jump: .globl loop_jump");
+    }
+    asm("main_return: .globl main_return");
+    return 0; /* main return */
+}
diff --git a/gdb/testsuite/gdb.base/until-trailing-insns.exp b/gdb/testsuite/gdb.base/until-trailing-insns.exp
new file mode 100644
index 00000000000..ea341ccd5a5
--- /dev/null
+++ b/gdb/testsuite/gdb.base/until-trailing-insns.exp
@@ -0,0 +1,118 @@
+# Copyright 2022 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+load_lib dwarf.exp
+
+# This test can only be run on targets which support DWARF-2 and use gas.
+if {![dwarf2_support]} {
+    unsupported "dwarf2 support required for this test"
+    return 0
+}
+if [get_compiler_info] {
+    return -1
+}
+# dwarf assembler requires gcc compiled.
+if !$gcc_compiled {
+    unsupported "gcc is required for this test"
+	return 0
+}
+
+standard_testfile .c .S
+
+if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile}] } {
+    return -1
+}
+
+
+set asm_file [standard_output_file $srcfile2]
+Dwarf::assemble $asm_file {
+    global srcdir subdir srcfile
+    declare_labels integer_label L
+    set int_size [get_sizeof "int" 4]
+
+    # Find start address and length for our functions.
+    lassign [function_range main [list ${srcdir}/${subdir}/$srcfile]] \
+	main_start main_len
+    set main_end "$main_start + $main_len"
+
+    cu {} {
+	compile_unit {
+	    {language @DW_LANG_C}
+	    {name until-trailing-isns.c}
+	    {stmt_list $L DW_FORM_sec_offset}
+	    {low_pc 0 addr}
+	} {
+	    subprogram {
+		{external 1 flag}
+		{name main}
+		{low_pc $main_start addr}
+		{high_pc $main_len DW_FORM_data4}
+	    }
+	}
+    }
+
+    lines {version 2 default_is_stmt 1} L {
+	include_dir "${srcdir}/${subdir}"
+	file_name "$srcfile" 1
+
+	# Generate a line table program.  This mimicks clang-13's behavior
+	# of adding some !is_stmt at the end of a loop line, making until
+	# not work properly.
+	program {
+	    {DW_LNE_set_address $main_start}
+	    {line [gdb_get_line_number "main prologue"]}
+	    {DW_LNS_copy}
+	    {DW_LNE_set_address loop_start}
+	    {line [gdb_get_line_number "loop line"]}
+	    {DW_LNS_copy}
+	    {DW_LNE_set_address loop_condition}
+	    {line [gdb_get_line_number "loop line"]}
+	    {DW_LNS_negate_stmt}
+	    {DW_LNS_copy}
+	    {DW_LNE_set_address loop_code}
+	    {line [gdb_get_line_number "loop code"]}
+	    {DW_LNS_negate_stmt}
+	    {DW_LNS_copy}
+	    {DW_LNE_set_address loop_increment}
+	    {line [gdb_get_line_number "loop line"]}
+	    {DW_LNS_copy}
+	    {DW_LNE_set_address loop_jump}
+	    {line [gdb_get_line_number "loop line"]}
+	    {DW_LNS_negate_stmt}
+	    {DW_LNS_copy}
+	    {DW_LNE_set_address main_return}
+	    {line [gdb_get_line_number "main return"]}
+	    {DW_LNS_negate_stmt}
+	    {DW_LNS_copy}
+	    {DW_LNE_set_address $main_end}
+	    {line [expr [gdb_get_line_number "main return"] + 1]}
+	    {DW_LNS_copy}
+	    {DW_LNE_end_sequence}
+	}
+    }
+
+}
+
+if { [prepare_for_testing "failed to prepare" ${testfile} \
+	[list $srcfile $asm_file] {nodebug} ] } {
+    return -1
+}
+
+if ![runto_main] {
+    return -1
+}
+
+gdb_test "next" ".* loop code .*" "inside the loop"
+gdb_test "next" ".* loop line .*" "ending of loop"
+gdb_test "until" ".* return 0; .*" "left loop"
-- 
2.31.1

[PING] [PATCH v3] [PR gdb/17315] fix until behavior with trailing !is_stmt lines

[Bruno Larsen via Gdb-patches]

ping

On 1/26/22 10:08, Bruno Larsen wrote:
> When using the command "until", it is expected that GDB will exit a
> loop if the current instruction is the last one related to that loop.
> However, if there were trailing non-statement instructions, "until"
> would just behave as "next".  This was most noticeable in clang-compiled
> code, but might happen with gcc-compiled as well.  PR gdb/17315 relates
> to this problem, as running gdb.base/watchpoint.exp with clang
> would fail for this reason.
> 
> The current patch fixes this by adding an extra check to the
> until_next_command function, going through all the following
> instructions: if the next instruction relates to the same line and is
> not marked as a statement, the end of the execution range is moved to
> the end of this next instruction.
> 
> This patch also adds a test case that can be run with gcc to test that
> this functionality is not silently broken in future updates.
> ---
>   gdb/infcmd.c                                  |  23 ++++
>   gdb/testsuite/gdb.base/until-trailing-insns.c |  53 ++++++++
>   .../gdb.base/until-trailing-insns.exp         | 118 ++++++++++++++++++
>   3 files changed, 194 insertions(+)
>   create mode 100644 gdb/testsuite/gdb.base/until-trailing-insns.c
>   create mode 100644 gdb/testsuite/gdb.base/until-trailing-insns.exp
> 
> diff --git a/gdb/infcmd.c b/gdb/infcmd.c
> index 9f4ed8bff13..5e57437a4cb 100644
> --- a/gdb/infcmd.c
> +++ b/gdb/infcmd.c
> @@ -1346,6 +1346,29 @@ until_next_command (int from_tty)
>   
>         tp->control.step_range_start = BLOCK_ENTRY_PC (SYMBOL_BLOCK_VALUE (func));
>         tp->control.step_range_end = sal.end;
> +
> +      /* By setting the step_range_end based on the current pc, we implicitly
> +	 assume that the last instruction for any given line must have is_stmt set.
> +	 This is not necessarily true.  Clang-13, for example, would compile
> +	 the following code:
> +
> +for(int i=0; i<10; i++)
> +  {
> +    foo();
> +  }
> +
> +	 with 2 entries after the last is_stmt linetable entry.  To fix this, we
> +	 iterate over the instruction related to the end PC, until we find an sal
> +	 related to a different line, and set that pc as the step_range_end. */
> +
> +      struct symtab_and_line final_sal;
> +      final_sal = find_pc_line (tp->control.step_range_end, 0);
> +
> +      while (final_sal.line == sal.line && !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/testsuite/gdb.base/until-trailing-insns.c b/gdb/testsuite/gdb.base/until-trailing-insns.c
> new file mode 100644
> index 00000000000..68a2033f517
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/until-trailing-insns.c
> @@ -0,0 +1,53 @@
> +/* Copyright 2022 Free Software Foundation, Inc.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +/* This test aims to recreate clang-13 behavior in a more controllable
> +   environment.  We want to create a linetable like so:
> +
> +   line | code    | is_stmt |
> +    1     i = 0;	Y
> +    2     while(1){	N
> +    3       if(...)	Y
> +    4       a = i;	Y
> +    5       i++;	Y
> +    6     }		N
> +
> +    where all lines, with the exception of line 4, all point to the loop code
> +    on line 2, effectively creating a "for" loop.  GDB's until command had a
> +    problem with this setup, because when we're stopped at line 5 the range
> +    of execution would not include line 6 - a jump instruction still related
> +    to line 2, making us stop at the next instruction marked as a statement
> +    and not related to the current line (in this example, line 4).  This test
> +    exercises the command "until" in this complicated situation.
> +
> + */
> +
> +int main(){/* main prologue */
> +    asm("main_label: .globl main_label"); /* This is required */
> +    asm("loop_start: .globl loop_start");
> +    int a, i;
> +    i = 0;						/* loop assignment */
> +    while (1) {						/* loop line */
> +	asm("loop_condition: .globl loop_condition");
> +	if (i >= 10) break;				/* loop condition */
> +	asm("loop_code: .globl loop_code");
> +	a = i;						/* loop code */
> +	asm("loop_increment: .globl loop_increment");
> +	i ++;						/* loop increment */
> +	asm("loop_jump: .globl loop_jump");
> +    }
> +    asm("main_return: .globl main_return");
> +    return 0; /* main return */
> +}
> diff --git a/gdb/testsuite/gdb.base/until-trailing-insns.exp b/gdb/testsuite/gdb.base/until-trailing-insns.exp
> new file mode 100644
> index 00000000000..ea341ccd5a5
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/until-trailing-insns.exp
> @@ -0,0 +1,118 @@
> +# Copyright 2022 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +load_lib dwarf.exp
> +
> +# This test can only be run on targets which support DWARF-2 and use gas.
> +if {![dwarf2_support]} {
> +    unsupported "dwarf2 support required for this test"
> +    return 0
> +}
> +if [get_compiler_info] {
> +    return -1
> +}
> +# dwarf assembler requires gcc compiled.
> +if !$gcc_compiled {
> +    unsupported "gcc is required for this test"
> +	return 0
> +}
> +
> +standard_testfile .c .S
> +
> +if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile}] } {
> +    return -1
> +}
> +
> +
> +set asm_file [standard_output_file $srcfile2]
> +Dwarf::assemble $asm_file {
> +    global srcdir subdir srcfile
> +    declare_labels integer_label L
> +    set int_size [get_sizeof "int" 4]
> +
> +    # Find start address and length for our functions.
> +    lassign [function_range main [list ${srcdir}/${subdir}/$srcfile]] \
> +	main_start main_len
> +    set main_end "$main_start + $main_len"
> +
> +    cu {} {
> +	compile_unit {
> +	    {language @DW_LANG_C}
> +	    {name until-trailing-isns.c}
> +	    {stmt_list $L DW_FORM_sec_offset}
> +	    {low_pc 0 addr}
> +	} {
> +	    subprogram {
> +		{external 1 flag}
> +		{name main}
> +		{low_pc $main_start addr}
> +		{high_pc $main_len DW_FORM_data4}
> +	    }
> +	}
> +    }
> +
> +    lines {version 2 default_is_stmt 1} L {
> +	include_dir "${srcdir}/${subdir}"
> +	file_name "$srcfile" 1
> +
> +	# Generate a line table program.  This mimicks clang-13's behavior
> +	# of adding some !is_stmt at the end of a loop line, making until
> +	# not work properly.
> +	program {
> +	    {DW_LNE_set_address $main_start}
> +	    {line [gdb_get_line_number "main prologue"]}
> +	    {DW_LNS_copy}
> +	    {DW_LNE_set_address loop_start}
> +	    {line [gdb_get_line_number "loop line"]}
> +	    {DW_LNS_copy}
> +	    {DW_LNE_set_address loop_condition}
> +	    {line [gdb_get_line_number "loop line"]}
> +	    {DW_LNS_negate_stmt}
> +	    {DW_LNS_copy}
> +	    {DW_LNE_set_address loop_code}
> +	    {line [gdb_get_line_number "loop code"]}
> +	    {DW_LNS_negate_stmt}
> +	    {DW_LNS_copy}
> +	    {DW_LNE_set_address loop_increment}
> +	    {line [gdb_get_line_number "loop line"]}
> +	    {DW_LNS_copy}
> +	    {DW_LNE_set_address loop_jump}
> +	    {line [gdb_get_line_number "loop line"]}
> +	    {DW_LNS_negate_stmt}
> +	    {DW_LNS_copy}
> +	    {DW_LNE_set_address main_return}
> +	    {line [gdb_get_line_number "main return"]}
> +	    {DW_LNS_negate_stmt}
> +	    {DW_LNS_copy}
> +	    {DW_LNE_set_address $main_end}
> +	    {line [expr [gdb_get_line_number "main return"] + 1]}
> +	    {DW_LNS_copy}
> +	    {DW_LNE_end_sequence}
> +	}
> +    }
> +
> +}
> +
> +if { [prepare_for_testing "failed to prepare" ${testfile} \
> +	[list $srcfile $asm_file] {nodebug} ] } {
> +    return -1
> +}
> +
> +if ![runto_main] {
> +    return -1
> +}
> +
> +gdb_test "next" ".* loop code .*" "inside the loop"
> +gdb_test "next" ".* loop line .*" "ending of loop"
> +gdb_test "until" ".* return 0; .*" "left loop"


-- 
Cheers!
Bruno Larsen

Re: [PATCH v3] [PR gdb/17315] fix until behavior with trailing !is_stmt lines

[Andrew Burgess via Gdb-patches]

* Bruno Larsen via Gdb-patches <gdb-patches@sourceware.org> [2022-01-26 10:08:13 -0300]:

> When using the command "until", it is expected that GDB will exit a
> loop if the current instruction is the last one related to that loop.
> However, if there were trailing non-statement instructions, "until"
> would just behave as "next".  This was most noticeable in clang-compiled
> code, but might happen with gcc-compiled as well.  PR gdb/17315 relates
> to this problem, as running gdb.base/watchpoint.exp with clang
> would fail for this reason.
> 
> The current patch fixes this by adding an extra check to the
> until_next_command function, going through all the following
> instructions: if the next instruction relates to the same line and is
> not marked as a statement, the end of the execution range is moved to
> the end of this next instruction.
> 
> This patch also adds a test case that can be run with gcc to test that
> this functionality is not silently broken in future updates.

Hi Bruno!

Thanks for working on this.  I had some comments, see inline below.

> ---
>  gdb/infcmd.c                                  |  23 ++++
>  gdb/testsuite/gdb.base/until-trailing-insns.c |  53 ++++++++
>  .../gdb.base/until-trailing-insns.exp         | 118 ++++++++++++++++++
>  3 files changed, 194 insertions(+)
>  create mode 100644 gdb/testsuite/gdb.base/until-trailing-insns.c
>  create mode 100644 gdb/testsuite/gdb.base/until-trailing-insns.exp
> 
> diff --git a/gdb/infcmd.c b/gdb/infcmd.c
> index 9f4ed8bff13..5e57437a4cb 100644
> --- a/gdb/infcmd.c
> +++ b/gdb/infcmd.c
> @@ -1346,6 +1346,29 @@ until_next_command (int from_tty)
>  
>        tp->control.step_range_start = BLOCK_ENTRY_PC (SYMBOL_BLOCK_VALUE (func));
>        tp->control.step_range_end = sal.end;
> +
> +      /* By setting the step_range_end based on the current pc, we implicitly
> +	 assume that the last instruction for any given line must have is_stmt set.
> +	 This is not necessarily true.  Clang-13, for example, would compile
> +	 the following code:
> +
> +for(int i=0; i<10; i++)
> +  {
> +    foo();
> +  }

Code samples like this should be indented to the same level as the
comment, and now kept on the left.  Also, unless there's good reason
not too, code samples within comments should follow the same GNU
standard as the rest of GDB, so white space before '(' and around '='
and '<' for example.

> +
> +	 with 2 entries after the last is_stmt linetable entry.  To fix this, we 

You have trailing white space here.

> +	 iterate over the instruction related to the end PC, until we find an sal
> +	 related to a different line, and set that pc as the step_range_end. */

Two spaces before '*/'.

> +
> +      struct symtab_and_line final_sal;
> +      final_sal = find_pc_line (tp->control.step_range_end, 0);
> +
> +      while (final_sal.line == sal.line && !final_sal.is_stmt)

I think we should also be checking final_sal.symtab == sal.symtab
here.  If we find a line for the same line number but from a different
symtab (unlikely as that may be) we should probably not step over that
code.

> +        {
> +	  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/testsuite/gdb.base/until-trailing-insns.c b/gdb/testsuite/gdb.base/until-trailing-insns.c
> new file mode 100644
> index 00000000000..68a2033f517
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/until-trailing-insns.c
> @@ -0,0 +1,53 @@
> +/* Copyright 2022 Free Software Foundation, Inc.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +/* This test aims to recreate clang-13 behavior in a more controllable
> +   environment.  We want to create a linetable like so:
> +
> +   line | code    | is_stmt |
> +    1     i = 0;	Y
> +    2     while(1){	N
> +    3       if(...)	Y
> +    4       a = i;	Y
> +    5       i++;	Y
> +    6     }		N
> +
> +    where all lines, with the exception of line 4, all point to the loop code
> +    on line 2, effectively creating a "for" loop.  GDB's until command had a
> +    problem with this setup, because when we're stopped at line 5 the range
> +    of execution would not include line 6 - a jump instruction still related
> +    to line 2, making us stop at the next instruction marked as a statement
> +    and not related to the current line (in this example, line 4).  This test
> +    exercises the command "until" in this complicated situation.
> +
> + */

The .exp file should include a comment explaining what the test does,
so I think this comment would be better moved to there.

> +
> +int main(){/* main prologue */
> +    asm("main_label: .globl main_label"); /* This is required */
> +    asm("loop_start: .globl loop_start");
> +    int a, i;
> +    i = 0;						/* loop assignment */
> +    while (1) {						/* loop line */
> +	asm("loop_condition: .globl loop_condition");
> +	if (i >= 10) break;				/* loop condition */
> +	asm("loop_code: .globl loop_code");
> +	a = i;						/* loop code */
> +	asm("loop_increment: .globl loop_increment");
> +	i ++;						/* loop increment */
> +	asm("loop_jump: .globl loop_jump");
> +    }
> +    asm("main_return: .globl main_return");
> +    return 0; /* main return */
> +}

This code would ideally follow the GNU coding standard as much as possible.

> diff --git a/gdb/testsuite/gdb.base/until-trailing-insns.exp b/gdb/testsuite/gdb.base/until-trailing-insns.exp
> new file mode 100644
> index 00000000000..ea341ccd5a5
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/until-trailing-insns.exp
> @@ -0,0 +1,118 @@
> +# Copyright 2022 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +load_lib dwarf.exp
> +
> +# This test can only be run on targets which support DWARF-2 and use gas.
> +if {![dwarf2_support]} {
> +    unsupported "dwarf2 support required for this test"
> +    return 0
> +}
> +if [get_compiler_info] {
> +    return -1
> +}
> +# dwarf assembler requires gcc compiled.
> +if !$gcc_compiled {
> +    unsupported "gcc is required for this test"
> +	return 0
> +}
> +
> +standard_testfile .c .S
> +
> +if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile}] } {
> +    return -1
> +}
> +
> +
> +set asm_file [standard_output_file $srcfile2]
> +Dwarf::assemble $asm_file {
> +    global srcdir subdir srcfile
> +    declare_labels integer_label L
> +    set int_size [get_sizeof "int" 4]
> +
> +    # Find start address and length for our functions.
> +    lassign [function_range main [list ${srcdir}/${subdir}/$srcfile]] \
> +	main_start main_len
> +    set main_end "$main_start + $main_len"
> +
> +    cu {} {
> +	compile_unit {
> +	    {language @DW_LANG_C}
> +	    {name until-trailing-isns.c}
> +	    {stmt_list $L DW_FORM_sec_offset}
> +	    {low_pc 0 addr}
> +	} {
> +	    subprogram {
> +		{external 1 flag}
> +		{name main}
> +		{low_pc $main_start addr}
> +		{high_pc $main_len DW_FORM_data4}
> +	    }
> +	}
> +    }
> +
> +    lines {version 2 default_is_stmt 1} L {
> +	include_dir "${srcdir}/${subdir}"
> +	file_name "$srcfile" 1
> +
> +	# Generate a line table program.  This mimicks clang-13's behavior
> +	# of adding some !is_stmt at the end of a loop line, making until
> +	# not work properly.
> +	program {
> +	    {DW_LNE_set_address $main_start}
> +	    {line [gdb_get_line_number "main prologue"]}
> +	    {DW_LNS_copy}
> +	    {DW_LNE_set_address loop_start}
> +	    {line [gdb_get_line_number "loop line"]}
> +	    {DW_LNS_copy}
> +	    {DW_LNE_set_address loop_condition}
> +	    {line [gdb_get_line_number "loop line"]}
> +	    {DW_LNS_negate_stmt}
> +	    {DW_LNS_copy}
> +	    {DW_LNE_set_address loop_code}
> +	    {line [gdb_get_line_number "loop code"]}

I found that this test was failing for me.  The reason being that
"loop code" actually appears inside the comment within the .c file.  I
found that if I delete the comment in the .c file then the test
started passing again (only with your fix of course).

> +	    {DW_LNS_negate_stmt}
> +	    {DW_LNS_copy}
> +	    {DW_LNE_set_address loop_increment}
> +	    {line [gdb_get_line_number "loop line"]}
> +	    {DW_LNS_copy}
> +	    {DW_LNE_set_address loop_jump}
> +	    {line [gdb_get_line_number "loop line"]}
> +	    {DW_LNS_negate_stmt}
> +	    {DW_LNS_copy}
> +	    {DW_LNE_set_address main_return}
> +	    {line [gdb_get_line_number "main return"]}
> +	    {DW_LNS_negate_stmt}
> +	    {DW_LNS_copy}
> +	    {DW_LNE_set_address $main_end}
> +	    {line [expr [gdb_get_line_number "main return"] + 1]}
> +	    {DW_LNS_copy}
> +	    {DW_LNE_end_sequence}
> +	}
> +    }
> +
> +}
> +
> +if { [prepare_for_testing "failed to prepare" ${testfile} \
> +	[list $srcfile $asm_file] {nodebug} ] } {
> +    return -1
> +}
> +
> +if ![runto_main] {
> +    return -1
> +}
> +
> +gdb_test "next" ".* loop code .*" "inside the loop"
> +gdb_test "next" ".* loop line .*" "ending of loop"
> +gdb_test "until" ".* return 0; .*" "left loop"

While reviewing this patch I ended up making the changes I recommended
above as I went along.

I also rewrote and extended some of the text describing the problem we
are hitting.  I always find these things easier to understand if there
is a really simple worked example included, so that's what I did.

I've also added a 'Bug: ' link within the commit message.

Could you take a look at the patch below, if you are happy with it
then I propose that we merge this.

Thanks,
Andrew

---

commit 209580c6befd577b2946a09bc2e024d10d7f0a54
Author: Bruno Larsen via Gdb-patches <gdb-patches@sourceware.org>
Date:   Wed Jan 26 10:08:13 2022 -0300

    gdb: fix until behavior with trailing !is_stmt lines
    
    When using the command "until", it is expected that GDB will exit a
    loop if the current instruction is the last one related to that loop.
    However, if there were trailing non-statement instructions, "until"
    would just behave as "next".  This was noticeable in clang-compiled
    code, but might happen with gcc-compiled as well.  PR gdb/17315 relates
    to this problem, as running gdb.base/watchpoint.exp with clang
    would fail for this reason.
    
    To better understand this issue, consider the following source code,
    with line numbers marked on the left:
    
      10:   for (i = 0; i < 10; ++i)
      11:     loop_body ();
      12:   other_stuff ();
    
    If we transform this to pseudo-assembler, and generate a line table,
    we could end up with something like this:
    
      Address | Pseudo-Assembler | Line | Is-Statement?
    
      0x100   | i = 0            | 10   | Yes
      0x104   | loop_body ()     | 11   | Yes
      0x108   | i = i + 1        | 10   | Yes
      0x10c   | if (i < 10):     | 10   | No
      0x110   |     goto 0x104   | 10   | No
      0x114   | other_stuff ()   | 12   | Yes
    
    Notice the two non-statement instructions at the end of the loop.
    
    The problem is that when we reach address 0x108 and use 'until',
    hoping to leave the loop, GDB sets up a stepping range that runs from
    the start of the function (0x100 in our example) to the end of the
    current line table entry, that is 0x10c in our example.  GDB then
    starts stepping forward.
    
    When 0x10c is reached GDB spots that we have left the stepping range,
    that the new location is not a statement, and that the new location is
    associated with the same source line number as the previous stepping
    range.  GDB then sets up a new stepping range that runs from 0x10c to
    0x114, and continues stepping forward.
    
    Within that stepping range the inferior hits the goto (at 0x110) and
    loops back to address 0x104.
    
    At 0x104 GDB spots that we have left the previous stepping range, that
    the new address is marked as a statement, and that the new address is
    for a different source line.  As a result, GDB stops and returns
    control to the user.  This is not what the user was expecting, they
    expected GDB to exit the loop.
    
    The fix proposed in this patch, is that, when the user issues the
    'until' command, and GDB sets up the initial stepping range, GDB will
    check subsequent SALs (symtab_and_lines) to see if they are
    non-statements associated with the same line number.  If they are then
    the end of the initial stepping range is extended to the end of the
    non-statement SALs.
    
    In our example above, the user is at 0x108 and uses 'until', GDB now
    sets up a stepping range from the start of the function 0x100 to
    0x114, the first address associated with a different line.
    
    Now as GDB steps around the loop it never leaves the initial stepping
    range.  It is only when GDB exits the loop that we leave the stepping
    range, and the stepping finishes at address 0x114.
    
    This patch also adds a test case that can be run with gcc to test that
    this functionality is not broken in the future.
    
    Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=17315

diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 48dda9b23ee..a25d024d24c 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -1346,6 +1346,45 @@ until_next_command (int from_tty)
 
       tp->control.step_range_start = BLOCK_ENTRY_PC (SYMBOL_BLOCK_VALUE (func));
       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 will not
+	 extend beyond the loop and the until command will exit the
+	 stepping range, and find the is_stmt true location corresponding
+	 to 'loop_body ()', this will cause GDB to stop inside the loop on
+	 the 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/testsuite/gdb.base/until-trailing-insns.c b/gdb/testsuite/gdb.base/until-trailing-insns.c
new file mode 100644
index 00000000000..749695b3db1
--- /dev/null
+++ b/gdb/testsuite/gdb.base/until-trailing-insns.c
@@ -0,0 +1,35 @@
+/* Copyright 2022 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+int
+main ()
+{							/* TAG: main prologue */
+    asm ("main_label: .globl main_label");
+    asm ("loop_start: .globl loop_start");
+    int a, i;
+    i = 0;						/* TAG: loop assignment */
+    while (1)						/* TAG: loop line */
+      {
+	asm ("loop_condition: .globl loop_condition");
+	if (i >= 10) break;				/* TAG: loop condition */
+	asm ("loop_code: .globl loop_code");
+	a = i;						/* TAG: loop code */
+	asm ("loop_increment: .globl loop_increment");
+	i++;						/* TAG: loop increment */
+	asm ("loop_jump: .globl loop_jump");
+      }
+    asm ("main_return: .globl main_return");
+    return 0;						/* TAG: main return */
+}
diff --git a/gdb/testsuite/gdb.base/until-trailing-insns.exp b/gdb/testsuite/gdb.base/until-trailing-insns.exp
new file mode 100644
index 00000000000..109bf5892e9
--- /dev/null
+++ b/gdb/testsuite/gdb.base/until-trailing-insns.exp
@@ -0,0 +1,183 @@
+# Copyright 2022 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# This test sets up debug information for a loop as we see in some cases
+# from clang-13.  In this situation, instructions at both the start and end
+# of the loop are associated (in the line table), with the header line of
+# the loop (line 10 in the example below).
+#
+# At the end of the loop we see some instructions marked as not a statement,
+# but still associated with the same loop header line.  For example,
+# consider the following C code:
+#
+# 10:	for (i = 0; i < 10; ++i)
+# 11:     loop_body ();
+# 12:   ...
+#
+# Transformed into the following pseudo-assembler, with associated line table:
+#
+# Address | Pseudo-Assembler | Line | Is-Statement?
+#
+# 0x100   | i = 0            | 10   | Yes
+# 0x104   | loop_body ()     | 11   | Yes
+# 0x108   | i = i + 1        | 10   | Yes
+# 0x10c   | if (i < 10):     | 10   | No
+# 0x110   |     goto 0x104   | 10   | No
+# 0x114   | ....             | 12   | Yes
+#
+# Notice the two non-statement instructions at the end of the loop.
+#
+# The problem here is that when we reach address 0x108 and use 'until',
+# hoping to leave the loop, GDB sets up a stepping range that runs from the
+# start of the function (0x100 in our example) to the end of the current
+# line table entry, that is 0x10c in our example.  GDB then starts stepping
+# forward.
+#
+# When 0x10c is reached GDB spots that we have left the stepping range, that
+# the new location is not a statement, and that the new location is
+# associated with the same source line number as the previous stepping
+# range.  GDB then sets up a new stepping range that runs from 0x10c to
+# 0x114, and continues stepping forward.
+#
+# Within that stepping range the inferior hits the goto and loops back to
+# address 0x104.
+#
+# At 0x104 GDB spots that we have left the previous stepping range, that the
+# new address is marked as a statement, and that the new address is for a
+# different source line.  As a result, GDB stops and returns control to the
+# user.  This is not what the user was expecting, they expected GDB not to
+# stop until they were outside of the loop.
+#
+# The fix is that, when the user issues the 'until' command, and GDB sets up
+# the initial stepping range, GDB will check subsequent SALs to see if they
+# are non-statements associated with the same line number.  If they are then
+# the end of the initial stepping range is pushed out to the end of the
+# non-statement SALs.
+#
+# In our example above, the user is at 0x108 and uses 'until'.  GDB now sets
+# up a stepping range from the start of the function 0x100 to 0x114, the
+# first address associated with a different line.
+#
+# Now as GDB steps around the loop it never leaves the initial stepping
+# range.  It is only when GDB exits the loop that we leave the stepping
+# range, and the stepping finishes at address 0x114.
+#
+# This test checks this behaviour using the DWARF assembler.
+
+load_lib dwarf.exp
+
+# This test can only be run on targets which support DWARF-2 and use gas.
+if {![dwarf2_support]} {
+    unsupported "dwarf2 support required for this test"
+    return 0
+}
+
+if [get_compiler_info] {
+    return -1
+}
+
+# The DWARF assembler requires the gcc compiler.
+if {!$gcc_compiled} {
+    unsupported "gcc is required for this test"
+    return 0
+}
+
+standard_testfile .c .S
+
+if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile}] } {
+    return -1
+}
+
+set asm_file [standard_output_file $srcfile2]
+Dwarf::assemble $asm_file {
+    global srcdir subdir srcfile
+    declare_labels integer_label L
+    set int_size [get_sizeof "int" 4]
+
+    # Find start address and length for our functions.
+    lassign [function_range main [list ${srcdir}/${subdir}/$srcfile]] \
+	main_start main_len
+    set main_end "$main_start + $main_len"
+
+    cu {} {
+	compile_unit {
+	    {language @DW_LANG_C}
+	    {name until-trailing-isns.c}
+	    {stmt_list $L DW_FORM_sec_offset}
+	    {low_pc 0 addr}
+	} {
+	    subprogram {
+		{external 1 flag}
+		{name main}
+		{low_pc $main_start addr}
+		{high_pc $main_len DW_FORM_data4}
+	    }
+	}
+    }
+
+    lines {version 2 default_is_stmt 1} L {
+	include_dir "${srcdir}/${subdir}"
+	file_name "$srcfile" 1
+
+	# Generate a line table program.  This mimicks clang-13's behavior
+	# of adding some !is_stmt at the end of a loop line, making until
+	# not work properly.
+	program {
+	    {DW_LNE_set_address $main_start}
+	    {line [gdb_get_line_number "TAG: main prologue"]}
+	    {DW_LNS_copy}
+	    {DW_LNE_set_address loop_start}
+	    {line [gdb_get_line_number "TAG: loop line"]}
+	    {DW_LNS_copy}
+	    {DW_LNE_set_address loop_condition}
+	    {line [gdb_get_line_number "TAG: loop line"]}
+	    {DW_LNS_negate_stmt}
+	    {DW_LNS_copy}
+	    {DW_LNE_set_address loop_code}
+	    {line [gdb_get_line_number "TAG: loop code"]}
+	    {DW_LNS_negate_stmt}
+	    {DW_LNS_copy}
+	    {DW_LNE_set_address loop_increment}
+	    {line [gdb_get_line_number "TAG: loop line"]}
+	    {DW_LNS_copy}
+	    {DW_LNE_set_address loop_jump}
+	    {line [gdb_get_line_number "TAG: loop line"]}
+	    {DW_LNS_negate_stmt}
+	    {DW_LNS_copy}
+	    {DW_LNE_set_address main_return}
+	    {line [gdb_get_line_number "TAG: main return"]}
+	    {DW_LNS_negate_stmt}
+	    {DW_LNS_copy}
+	    {DW_LNE_set_address $main_end}
+	    {line [expr [gdb_get_line_number "TAG: main return"] + 1]}
+	    {DW_LNS_copy}
+	    {DW_LNE_end_sequence}
+	}
+    }
+
+}
+
+if { [prepare_for_testing "failed to prepare" ${testfile} \
+	[list $srcfile $asm_file] {nodebug} ] } {
+    return -1
+}
+
+if ![runto_main] {
+    return -1
+}
+
+gdb_test "next" ".* TAG: loop code .*" "inside the loop"
+gdb_test "next" ".* TAG: loop line .*" "ending of loop"
+gdb_test "until" ".* TAG: main return .*" "left loop"

Re: [PATCH v3] [PR gdb/17315] fix until behavior with trailing !is_stmt lines

[Bruno Larsen via Gdb-patches]

On 2/10/22 09:44, Andrew Burgess wrote:
> * Bruno Larsen via Gdb-patches <gdb-patches@sourceware.org> [2022-01-26 10:08:13 -0300]:
> 
>> When using the command "until", it is expected that GDB will exit a
>> loop if the current instruction is the last one related to that loop.
>> However, if there were trailing non-statement instructions, "until"
>> would just behave as "next".  This was most noticeable in clang-compiled
>> code, but might happen with gcc-compiled as well.  PR gdb/17315 relates
>> to this problem, as running gdb.base/watchpoint.exp with clang
>> would fail for this reason.
>>
>> The current patch fixes this by adding an extra check to the
>> until_next_command function, going through all the following
>> instructions: if the next instruction relates to the same line and is
>> not marked as a statement, the end of the execution range is moved to
>> the end of this next instruction.
>>
>> This patch also adds a test case that can be run with gcc to test that
>> this functionality is not silently broken in future updates.
> 
> Hi Bruno!
> 
> Thanks for working on this.  I had some comments, see inline below.
> 

Hi Andrew,

Thanks for the review. I agree with your comments, and I think that most of the explanations are indeed better, with one exception mentioned below.

>> ---
>>   gdb/infcmd.c                                  |  23 ++++
>>   gdb/testsuite/gdb.base/until-trailing-insns.c |  53 ++++++++
>>   .../gdb.base/until-trailing-insns.exp         | 118 ++++++++++++++++++
>>   3 files changed, 194 insertions(+)
>>   create mode 100644 gdb/testsuite/gdb.base/until-trailing-insns.c
>>   create mode 100644 gdb/testsuite/gdb.base/until-trailing-insns.exp
>>
>> diff --git a/gdb/infcmd.c b/gdb/infcmd.c
>> index 9f4ed8bff13..5e57437a4cb 100644
>> --- a/gdb/infcmd.c
>> +++ b/gdb/infcmd.c
>> @@ -1346,6 +1346,29 @@ until_next_command (int from_tty)
>>   
>>         tp->control.step_range_start = BLOCK_ENTRY_PC (SYMBOL_BLOCK_VALUE (func));
>>         tp->control.step_range_end = sal.end;
>> +
>> +      /* By setting the step_range_end based on the current pc, we implicitly
>> +	 assume that the last instruction for any given line must have is_stmt set.
>> +	 This is not necessarily true.  Clang-13, for example, would compile
>> +	 the following code:
>> +
>> +for(int i=0; i<10; i++)
>> +  {
>> +    foo();
>> +  }
> 
> Code samples like this should be indented to the same level as the
> comment, and now kept on the left.  Also, unless there's good reason
> not too, code samples within comments should follow the same GNU
> standard as the rest of GDB, so white space before '(' and around '='
> and '<' for example.
> 
>> +
>> +	 with 2 entries after the last is_stmt linetable entry.  To fix this, we
> 
> You have trailing white space here.
> 
>> +	 iterate over the instruction related to the end PC, until we find an sal
>> +	 related to a different line, and set that pc as the step_range_end. */
> 
> Two spaces before '*/'.
> 
>> +
>> +      struct symtab_and_line final_sal;
>> +      final_sal = find_pc_line (tp->control.step_range_end, 0);
>> +
>> +      while (final_sal.line == sal.line && !final_sal.is_stmt)
> 
> I think we should also be checking final_sal.symtab == sal.symtab
> here.  If we find a line for the same line number but from a different
> symtab (unlikely as that may be) we should probably not step over that
> code.
> 
>> +        {
>> +	  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/testsuite/gdb.base/until-trailing-insns.c b/gdb/testsuite/gdb.base/until-trailing-insns.c
>> new file mode 100644
>> index 00000000000..68a2033f517
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.base/until-trailing-insns.c
>> @@ -0,0 +1,53 @@
>> +/* Copyright 2022 Free Software Foundation, Inc.
>> +
>> +   This program is free software; you can redistribute it and/or modify
>> +   it under the terms of the GNU General Public License as published by
>> +   the Free Software Foundation; either version 3 of the License, or
>> +   (at your option) any later version.
>> +
>> +   This program is distributed in the hope that it will be useful,
>> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +   GNU General Public License for more details.
>> +
>> +   You should have received a copy of the GNU General Public License
>> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
>> +
>> +/* This test aims to recreate clang-13 behavior in a more controllable
>> +   environment.  We want to create a linetable like so:
>> +
>> +   line | code    | is_stmt |
>> +    1     i = 0;	Y
>> +    2     while(1){	N
>> +    3       if(...)	Y
>> +    4       a = i;	Y
>> +    5       i++;	Y
>> +    6     }		N
>> +
>> +    where all lines, with the exception of line 4, all point to the loop code
>> +    on line 2, effectively creating a "for" loop.  GDB's until command had a
>> +    problem with this setup, because when we're stopped at line 5 the range
>> +    of execution would not include line 6 - a jump instruction still related
>> +    to line 2, making us stop at the next instruction marked as a statement
>> +    and not related to the current line (in this example, line 4).  This test
>> +    exercises the command "until" in this complicated situation.
>> +
>> + */
> 
> The .exp file should include a comment explaining what the test does,
> so I think this comment would be better moved to there.
> 
>> +
>> +int main(){/* main prologue */
>> +    asm("main_label: .globl main_label"); /* This is required */
>> +    asm("loop_start: .globl loop_start");
>> +    int a, i;
>> +    i = 0;						/* loop assignment */
>> +    while (1) {						/* loop line */
>> +	asm("loop_condition: .globl loop_condition");
>> +	if (i >= 10) break;				/* loop condition */
>> +	asm("loop_code: .globl loop_code");
>> +	a = i;						/* loop code */
>> +	asm("loop_increment: .globl loop_increment");
>> +	i ++;						/* loop increment */
>> +	asm("loop_jump: .globl loop_jump");
>> +    }
>> +    asm("main_return: .globl main_return");
>> +    return 0; /* main return */
>> +}
> 
> This code would ideally follow the GNU coding standard as much as possible.
> 
>> diff --git a/gdb/testsuite/gdb.base/until-trailing-insns.exp b/gdb/testsuite/gdb.base/until-trailing-insns.exp
>> new file mode 100644
>> index 00000000000..ea341ccd5a5
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.base/until-trailing-insns.exp
>> @@ -0,0 +1,118 @@
>> +# Copyright 2022 Free Software Foundation, Inc.
>> +
>> +# This program is free software; you can redistribute it and/or modify
>> +# it under the terms of the GNU General Public License as published by
>> +# the Free Software Foundation; either version 3 of the License, or
>> +# (at your option) any later version.
>> +#
>> +# This program is distributed in the hope that it will be useful,
>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +# GNU General Public License for more details.
>> +#
>> +# You should have received a copy of the GNU General Public License
>> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
>> +load_lib dwarf.exp
>> +
>> +# This test can only be run on targets which support DWARF-2 and use gas.
>> +if {![dwarf2_support]} {
>> +    unsupported "dwarf2 support required for this test"
>> +    return 0
>> +}
>> +if [get_compiler_info] {
>> +    return -1
>> +}
>> +# dwarf assembler requires gcc compiled.
>> +if !$gcc_compiled {
>> +    unsupported "gcc is required for this test"
>> +	return 0
>> +}
>> +
>> +standard_testfile .c .S
>> +
>> +if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile}] } {
>> +    return -1
>> +}
>> +
>> +
>> +set asm_file [standard_output_file $srcfile2]
>> +Dwarf::assemble $asm_file {
>> +    global srcdir subdir srcfile
>> +    declare_labels integer_label L
>> +    set int_size [get_sizeof "int" 4]
>> +
>> +    # Find start address and length for our functions.
>> +    lassign [function_range main [list ${srcdir}/${subdir}/$srcfile]] \
>> +	main_start main_len
>> +    set main_end "$main_start + $main_len"
>> +
>> +    cu {} {
>> +	compile_unit {
>> +	    {language @DW_LANG_C}
>> +	    {name until-trailing-isns.c}
>> +	    {stmt_list $L DW_FORM_sec_offset}
>> +	    {low_pc 0 addr}
>> +	} {
>> +	    subprogram {
>> +		{external 1 flag}
>> +		{name main}
>> +		{low_pc $main_start addr}
>> +		{high_pc $main_len DW_FORM_data4}
>> +	    }
>> +	}
>> +    }
>> +
>> +    lines {version 2 default_is_stmt 1} L {
>> +	include_dir "${srcdir}/${subdir}"
>> +	file_name "$srcfile" 1
>> +
>> +	# Generate a line table program.  This mimicks clang-13's behavior
>> +	# of adding some !is_stmt at the end of a loop line, making until
>> +	# not work properly.
>> +	program {
>> +	    {DW_LNE_set_address $main_start}
>> +	    {line [gdb_get_line_number "main prologue"]}
>> +	    {DW_LNS_copy}
>> +	    {DW_LNE_set_address loop_start}
>> +	    {line [gdb_get_line_number "loop line"]}
>> +	    {DW_LNS_copy}
>> +	    {DW_LNE_set_address loop_condition}
>> +	    {line [gdb_get_line_number "loop line"]}
>> +	    {DW_LNS_negate_stmt}
>> +	    {DW_LNS_copy}
>> +	    {DW_LNE_set_address loop_code}
>> +	    {line [gdb_get_line_number "loop code"]}
> 
> I found that this test was failing for me.  The reason being that
> "loop code" actually appears inside the comment within the .c file.  I
> found that if I delete the comment in the .c file then the test
> started passing again (only with your fix of course).
> 
>> +	    {DW_LNS_negate_stmt}
>> +	    {DW_LNS_copy}
>> +	    {DW_LNE_set_address loop_increment}
>> +	    {line [gdb_get_line_number "loop line"]}
>> +	    {DW_LNS_copy}
>> +	    {DW_LNE_set_address loop_jump}
>> +	    {line [gdb_get_line_number "loop line"]}
>> +	    {DW_LNS_negate_stmt}
>> +	    {DW_LNS_copy}
>> +	    {DW_LNE_set_address main_return}
>> +	    {line [gdb_get_line_number "main return"]}
>> +	    {DW_LNS_negate_stmt}
>> +	    {DW_LNS_copy}
>> +	    {DW_LNE_set_address $main_end}
>> +	    {line [expr [gdb_get_line_number "main return"] + 1]}
>> +	    {DW_LNS_copy}
>> +	    {DW_LNE_end_sequence}
>> +	}
>> +    }
>> +
>> +}
>> +
>> +if { [prepare_for_testing "failed to prepare" ${testfile} \
>> +	[list $srcfile $asm_file] {nodebug} ] } {
>> +    return -1
>> +}
>> +
>> +if ![runto_main] {
>> +    return -1
>> +}
>> +
>> +gdb_test "next" ".* loop code .*" "inside the loop"
>> +gdb_test "next" ".* loop line .*" "ending of loop"
>> +gdb_test "until" ".* return 0; .*" "left loop"
> 
> While reviewing this patch I ended up making the changes I recommended
> above as I went along.
> 
> I also rewrote and extended some of the text describing the problem we
> are hitting.  I always find these things easier to understand if there
> is a really simple worked example included, so that's what I did.
> 
> I've also added a 'Bug: ' link within the commit message.
> 
> Could you take a look at the patch below, if you are happy with it
> then I propose that we merge this.
> 
> Thanks,
> Andrew
> 
> ---
> 
> commit 209580c6befd577b2946a09bc2e024d10d7f0a54
> Author: Bruno Larsen via Gdb-patches <gdb-patches@sourceware.org>
> Date:   Wed Jan 26 10:08:13 2022 -0300
> 
>      gdb: fix until behavior with trailing !is_stmt lines
>      
>      When using the command "until", it is expected that GDB will exit a
>      loop if the current instruction is the last one related to that loop.
>      However, if there were trailing non-statement instructions, "until"
>      would just behave as "next".  This was noticeable in clang-compiled
>      code, but might happen with gcc-compiled as well.  PR gdb/17315 relates
>      to this problem, as running gdb.base/watchpoint.exp with clang
>      would fail for this reason.
>      
>      To better understand this issue, consider the following source code,
>      with line numbers marked on the left:
>      
>        10:   for (i = 0; i < 10; ++i)
>        11:     loop_body ();
>        12:   other_stuff ();
>      
>      If we transform this to pseudo-assembler, and generate a line table,
>      we could end up with something like this:
>      
>        Address | Pseudo-Assembler | Line | Is-Statement?
>      
>        0x100   | i = 0            | 10   | Yes
>        0x104   | loop_body ()     | 11   | Yes
>        0x108   | i = i + 1        | 10   | Yes
>        0x10c   | if (i < 10):     | 10   | No
>        0x110   |     goto 0x104   | 10   | No
>        0x114   | other_stuff ()   | 12   | Yes
>      
>      Notice the two non-statement instructions at the end of the loop.
>      
>      The problem is that when we reach address 0x108 and use 'until',
>      hoping to leave the loop, GDB sets up a stepping range that runs from
>      the start of the function (0x100 in our example) to the end of the
>      current line table entry, that is 0x10c in our example.  GDB then
>      starts stepping forward.
>      
>      When 0x10c is reached GDB spots that we have left the stepping range,
>      that the new location is not a statement, and that the new location is
>      associated with the same source line number as the previous stepping
>      range.  GDB then sets up a new stepping range that runs from 0x10c to
>      0x114, and continues stepping forward.
>      
>      Within that stepping range the inferior hits the goto (at 0x110) and
>      loops back to address 0x104.
>      
>      At 0x104 GDB spots that we have left the previous stepping range, that
>      the new address is marked as a statement, and that the new address is
>      for a different source line.  As a result, GDB stops and returns
>      control to the user.  This is not what the user was expecting, they
>      expected GDB to exit the loop.
>      
>      The fix proposed in this patch, is that, when the user issues the
>      'until' command, and GDB sets up the initial stepping range, GDB will
>      check subsequent SALs (symtab_and_lines) to see if they are
>      non-statements associated with the same line number.  If they are then
>      the end of the initial stepping range is extended to the end of the
>      non-statement SALs.
>      
>      In our example above, the user is at 0x108 and uses 'until', GDB now
>      sets up a stepping range from the start of the function 0x100 to
>      0x114, the first address associated with a different line.
>      
>      Now as GDB steps around the loop it never leaves the initial stepping
>      range.  It is only when GDB exits the loop that we leave the stepping
>      range, and the stepping finishes at address 0x114.
>      
>      This patch also adds a test case that can be run with gcc to test that
>      this functionality is not broken in the future.
>      
>      Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=17315
> 
> diff --git a/gdb/infcmd.c b/gdb/infcmd.c
> index 48dda9b23ee..a25d024d24c 100644
> --- a/gdb/infcmd.c
> +++ b/gdb/infcmd.c
> @@ -1346,6 +1346,45 @@ until_next_command (int from_tty)
>   
>         tp->control.step_range_start = BLOCK_ENTRY_PC (SYMBOL_BLOCK_VALUE (func));
>         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 will not
> +	 extend beyond the loop and the until command will exit the
> +	 stepping range, and find the is_stmt true location corresponding
> +	 to 'loop_body ()', this will cause GDB to stop inside the loop on
> +	 the which is not what we wanted.

The wording in this paragraph seems a bit odd to me. My suggestion for an improvement is:

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/testsuite/gdb.base/until-trailing-insns.c b/gdb/testsuite/gdb.base/until-trailing-insns.c
> new file mode 100644
> index 00000000000..749695b3db1
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/until-trailing-insns.c
> @@ -0,0 +1,35 @@
> +/* Copyright 2022 Free Software Foundation, Inc.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +int
> +main ()
> +{							/* TAG: main prologue */
> +    asm ("main_label: .globl main_label");
> +    asm ("loop_start: .globl loop_start");
> +    int a, i;
> +    i = 0;						/* TAG: loop assignment */
> +    while (1)						/* TAG: loop line */
> +      {
> +	asm ("loop_condition: .globl loop_condition");
> +	if (i >= 10) break;				/* TAG: loop condition */
> +	asm ("loop_code: .globl loop_code");
> +	a = i;						/* TAG: loop code */
> +	asm ("loop_increment: .globl loop_increment");
> +	i++;						/* TAG: loop increment */
> +	asm ("loop_jump: .globl loop_jump");
> +      }
> +    asm ("main_return: .globl main_return");
> +    return 0;						/* TAG: main return */
> +}
> diff --git a/gdb/testsuite/gdb.base/until-trailing-insns.exp b/gdb/testsuite/gdb.base/until-trailing-insns.exp
> new file mode 100644
> index 00000000000..109bf5892e9
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/until-trailing-insns.exp
> @@ -0,0 +1,183 @@
> +# Copyright 2022 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +# This test sets up debug information for a loop as we see in some cases
> +# from clang-13.  In this situation, instructions at both the start and end
> +# of the loop are associated (in the line table), with the header line of
> +# the loop (line 10 in the example below).
> +#
> +# At the end of the loop we see some instructions marked as not a statement,
> +# but still associated with the same loop header line.  For example,
> +# consider the following C code:
> +#
> +# 10:	for (i = 0; i < 10; ++i)
> +# 11:     loop_body ();
> +# 12:   ...
> +#
> +# Transformed into the following pseudo-assembler, with associated line table:
> +#
> +# Address | Pseudo-Assembler | Line | Is-Statement?
> +#
> +# 0x100   | i = 0            | 10   | Yes
> +# 0x104   | loop_body ()     | 11   | Yes
> +# 0x108   | i = i + 1        | 10   | Yes
> +# 0x10c   | if (i < 10):     | 10   | No
> +# 0x110   |     goto 0x104   | 10   | No
> +# 0x114   | ....             | 12   | Yes
> +#
> +# Notice the two non-statement instructions at the end of the loop.
> +#
> +# The problem here is that when we reach address 0x108 and use 'until',
> +# hoping to leave the loop, GDB sets up a stepping range that runs from the
> +# start of the function (0x100 in our example) to the end of the current
> +# line table entry, that is 0x10c in our example.  GDB then starts stepping
> +# forward.
> +#
> +# When 0x10c is reached GDB spots that we have left the stepping range, that
> +# the new location is not a statement, and that the new location is
> +# associated with the same source line number as the previous stepping
> +# range.  GDB then sets up a new stepping range that runs from 0x10c to
> +# 0x114, and continues stepping forward.
> +#
> +# Within that stepping range the inferior hits the goto and loops back to
> +# address 0x104.
> +#
> +# At 0x104 GDB spots that we have left the previous stepping range, that the
> +# new address is marked as a statement, and that the new address is for a
> +# different source line.  As a result, GDB stops and returns control to the
> +# user.  This is not what the user was expecting, they expected GDB not to
> +# stop until they were outside of the loop.
> +#
> +# The fix is that, when the user issues the 'until' command, and GDB sets up
> +# the initial stepping range, GDB will check subsequent SALs to see if they
> +# are non-statements associated with the same line number.  If they are then
> +# the end of the initial stepping range is pushed out to the end of the
> +# non-statement SALs.
> +#
> +# In our example above, the user is at 0x108 and uses 'until'.  GDB now sets
> +# up a stepping range from the start of the function 0x100 to 0x114, the
> +# first address associated with a different line.
> +#
> +# Now as GDB steps around the loop it never leaves the initial stepping
> +# range.  It is only when GDB exits the loop that we leave the stepping
> +# range, and the stepping finishes at address 0x114.
> +#
> +# This test checks this behaviour using the DWARF assembler.
> +
> +load_lib dwarf.exp
> +
> +# This test can only be run on targets which support DWARF-2 and use gas.
> +if {![dwarf2_support]} {
> +    unsupported "dwarf2 support required for this test"
> +    return 0
> +}
> +
> +if [get_compiler_info] {
> +    return -1
> +}
> +
> +# The DWARF assembler requires the gcc compiler.
> +if {!$gcc_compiled} {
> +    unsupported "gcc is required for this test"
> +    return 0
> +}
> +
> +standard_testfile .c .S
> +
> +if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile}] } {
> +    return -1
> +}
> +
> +set asm_file [standard_output_file $srcfile2]
> +Dwarf::assemble $asm_file {
> +    global srcdir subdir srcfile
> +    declare_labels integer_label L
> +    set int_size [get_sizeof "int" 4]
> +
> +    # Find start address and length for our functions.
> +    lassign [function_range main [list ${srcdir}/${subdir}/$srcfile]] \
> +	main_start main_len
> +    set main_end "$main_start + $main_len"
> +
> +    cu {} {
> +	compile_unit {
> +	    {language @DW_LANG_C}
> +	    {name until-trailing-isns.c}
> +	    {stmt_list $L DW_FORM_sec_offset}
> +	    {low_pc 0 addr}
> +	} {
> +	    subprogram {
> +		{external 1 flag}
> +		{name main}
> +		{low_pc $main_start addr}
> +		{high_pc $main_len DW_FORM_data4}
> +	    }
> +	}
> +    }
> +
> +    lines {version 2 default_is_stmt 1} L {
> +	include_dir "${srcdir}/${subdir}"
> +	file_name "$srcfile" 1
> +
> +	# Generate a line table program.  This mimicks clang-13's behavior
> +	# of adding some !is_stmt at the end of a loop line, making until
> +	# not work properly.
> +	program {
> +	    {DW_LNE_set_address $main_start}
> +	    {line [gdb_get_line_number "TAG: main prologue"]}
> +	    {DW_LNS_copy}
> +	    {DW_LNE_set_address loop_start}
> +	    {line [gdb_get_line_number "TAG: loop line"]}
> +	    {DW_LNS_copy}
> +	    {DW_LNE_set_address loop_condition}
> +	    {line [gdb_get_line_number "TAG: loop line"]}
> +	    {DW_LNS_negate_stmt}
> +	    {DW_LNS_copy}
> +	    {DW_LNE_set_address loop_code}
> +	    {line [gdb_get_line_number "TAG: loop code"]}
> +	    {DW_LNS_negate_stmt}
> +	    {DW_LNS_copy}
> +	    {DW_LNE_set_address loop_increment}
> +	    {line [gdb_get_line_number "TAG: loop line"]}
> +	    {DW_LNS_copy}
> +	    {DW_LNE_set_address loop_jump}
> +	    {line [gdb_get_line_number "TAG: loop line"]}
> +	    {DW_LNS_negate_stmt}
> +	    {DW_LNS_copy}
> +	    {DW_LNE_set_address main_return}
> +	    {line [gdb_get_line_number "TAG: main return"]}
> +	    {DW_LNS_negate_stmt}
> +	    {DW_LNS_copy}
> +	    {DW_LNE_set_address $main_end}
> +	    {line [expr [gdb_get_line_number "TAG: main return"] + 1]}
> +	    {DW_LNS_copy}
> +	    {DW_LNE_end_sequence}
> +	}
> +    }
> +
> +}
> +
> +if { [prepare_for_testing "failed to prepare" ${testfile} \
> +	[list $srcfile $asm_file] {nodebug} ] } {
> +    return -1
> +}
> +
> +if ![runto_main] {
> +    return -1
> +}
> +
> +gdb_test "next" ".* TAG: loop code .*" "inside the loop"
> +gdb_test "next" ".* TAG: loop line .*" "ending of loop"
> +gdb_test "until" ".* TAG: main return .*" "left loop"
> 


-- 
Cheers!
Bruno Larsen

Re: [PATCH v3] [PR gdb/17315] fix until behavior with trailing !is_stmt lines

[Andrew Burgess via Gdb-patches]

* Bruno Larsen via Gdb-patches <gdb-patches@sourceware.org> [2022-02-10 10:46:01 -0300]:

> On 2/10/22 09:44, Andrew Burgess wrote:
> > * Bruno Larsen via Gdb-patches <gdb-patches@sourceware.org> [2022-01-26 10:08:13 -0300]:
> > 
> > > When using the command "until", it is expected that GDB will exit a
> > > loop if the current instruction is the last one related to that loop.
> > > However, if there were trailing non-statement instructions, "until"
> > > would just behave as "next".  This was most noticeable in clang-compiled
> > > code, but might happen with gcc-compiled as well.  PR gdb/17315 relates
> > > to this problem, as running gdb.base/watchpoint.exp with clang
> > > would fail for this reason.
> > > 
> > > The current patch fixes this by adding an extra check to the
> > > until_next_command function, going through all the following
> > > instructions: if the next instruction relates to the same line and is
> > > not marked as a statement, the end of the execution range is moved to
> > > the end of this next instruction.
> > > 
> > > This patch also adds a test case that can be run with gcc to test that
> > > this functionality is not silently broken in future updates.
> > 
> > Hi Bruno!
> > 
> > Thanks for working on this.  I had some comments, see inline below.
> > 
> 
> Hi Andrew,
> 
> Thanks for the review. I agree with your comments, and I think that most of the explanations are indeed better, with one exception mentioned below.
> 
> > > ---
> > >   gdb/infcmd.c                                  |  23 ++++
> > >   gdb/testsuite/gdb.base/until-trailing-insns.c |  53 ++++++++
> > >   .../gdb.base/until-trailing-insns.exp         | 118 ++++++++++++++++++
> > >   3 files changed, 194 insertions(+)
> > >   create mode 100644 gdb/testsuite/gdb.base/until-trailing-insns.c
> > >   create mode 100644 gdb/testsuite/gdb.base/until-trailing-insns.exp
> > > 
> > > diff --git a/gdb/infcmd.c b/gdb/infcmd.c
> > > index 9f4ed8bff13..5e57437a4cb 100644
> > > --- a/gdb/infcmd.c
> > > +++ b/gdb/infcmd.c
> > > @@ -1346,6 +1346,29 @@ until_next_command (int from_tty)
> > >         tp->control.step_range_start = BLOCK_ENTRY_PC (SYMBOL_BLOCK_VALUE (func));
> > >         tp->control.step_range_end = sal.end;
> > > +
> > > +      /* By setting the step_range_end based on the current pc, we implicitly
> > > +	 assume that the last instruction for any given line must have is_stmt set.
> > > +	 This is not necessarily true.  Clang-13, for example, would compile
> > > +	 the following code:
> > > +
> > > +for(int i=0; i<10; i++)
> > > +  {
> > > +    foo();
> > > +  }
> > 
> > Code samples like this should be indented to the same level as the
> > comment, and now kept on the left.  Also, unless there's good reason
> > not too, code samples within comments should follow the same GNU
> > standard as the rest of GDB, so white space before '(' and around '='
> > and '<' for example.
> > 
> > > +
> > > +	 with 2 entries after the last is_stmt linetable entry.  To fix this, we
> > 
> > You have trailing white space here.
> > 
> > > +	 iterate over the instruction related to the end PC, until we find an sal
> > > +	 related to a different line, and set that pc as the step_range_end. */
> > 
> > Two spaces before '*/'.
> > 
> > > +
> > > +      struct symtab_and_line final_sal;
> > > +      final_sal = find_pc_line (tp->control.step_range_end, 0);
> > > +
> > > +      while (final_sal.line == sal.line && !final_sal.is_stmt)
> > 
> > I think we should also be checking final_sal.symtab == sal.symtab
> > here.  If we find a line for the same line number but from a different
> > symtab (unlikely as that may be) we should probably not step over that
> > code.
> > 
> > > +        {
> > > +	  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/testsuite/gdb.base/until-trailing-insns.c b/gdb/testsuite/gdb.base/until-trailing-insns.c
> > > new file mode 100644
> > > index 00000000000..68a2033f517
> > > --- /dev/null
> > > +++ b/gdb/testsuite/gdb.base/until-trailing-insns.c
> > > @@ -0,0 +1,53 @@
> > > +/* Copyright 2022 Free Software Foundation, Inc.
> > > +
> > > +   This program is free software; you can redistribute it and/or modify
> > > +   it under the terms of the GNU General Public License as published by
> > > +   the Free Software Foundation; either version 3 of the License, or
> > > +   (at your option) any later version.
> > > +
> > > +   This program is distributed in the hope that it will be useful,
> > > +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > > +   GNU General Public License for more details.
> > > +
> > > +   You should have received a copy of the GNU General Public License
> > > +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> > > +
> > > +/* This test aims to recreate clang-13 behavior in a more controllable
> > > +   environment.  We want to create a linetable like so:
> > > +
> > > +   line | code    | is_stmt |
> > > +    1     i = 0;	Y
> > > +    2     while(1){	N
> > > +    3       if(...)	Y
> > > +    4       a = i;	Y
> > > +    5       i++;	Y
> > > +    6     }		N
> > > +
> > > +    where all lines, with the exception of line 4, all point to the loop code
> > > +    on line 2, effectively creating a "for" loop.  GDB's until command had a
> > > +    problem with this setup, because when we're stopped at line 5 the range
> > > +    of execution would not include line 6 - a jump instruction still related
> > > +    to line 2, making us stop at the next instruction marked as a statement
> > > +    and not related to the current line (in this example, line 4).  This test
> > > +    exercises the command "until" in this complicated situation.
> > > +
> > > + */
> > 
> > The .exp file should include a comment explaining what the test does,
> > so I think this comment would be better moved to there.
> > 
> > > +
> > > +int main(){/* main prologue */
> > > +    asm("main_label: .globl main_label"); /* This is required */
> > > +    asm("loop_start: .globl loop_start");
> > > +    int a, i;
> > > +    i = 0;						/* loop assignment */
> > > +    while (1) {						/* loop line */
> > > +	asm("loop_condition: .globl loop_condition");
> > > +	if (i >= 10) break;				/* loop condition */
> > > +	asm("loop_code: .globl loop_code");
> > > +	a = i;						/* loop code */
> > > +	asm("loop_increment: .globl loop_increment");
> > > +	i ++;						/* loop increment */
> > > +	asm("loop_jump: .globl loop_jump");
> > > +    }
> > > +    asm("main_return: .globl main_return");
> > > +    return 0; /* main return */
> > > +}
> > 
> > This code would ideally follow the GNU coding standard as much as possible.
> > 
> > > diff --git a/gdb/testsuite/gdb.base/until-trailing-insns.exp b/gdb/testsuite/gdb.base/until-trailing-insns.exp
> > > new file mode 100644
> > > index 00000000000..ea341ccd5a5
> > > --- /dev/null
> > > +++ b/gdb/testsuite/gdb.base/until-trailing-insns.exp
> > > @@ -0,0 +1,118 @@
> > > +# Copyright 2022 Free Software Foundation, Inc.
> > > +
> > > +# This program is free software; you can redistribute it and/or modify
> > > +# it under the terms of the GNU General Public License as published by
> > > +# the Free Software Foundation; either version 3 of the License, or
> > > +# (at your option) any later version.
> > > +#
> > > +# This program is distributed in the hope that it will be useful,
> > > +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > > +# GNU General Public License for more details.
> > > +#
> > > +# You should have received a copy of the GNU General Public License
> > > +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> > > +load_lib dwarf.exp
> > > +
> > > +# This test can only be run on targets which support DWARF-2 and use gas.
> > > +if {![dwarf2_support]} {
> > > +    unsupported "dwarf2 support required for this test"
> > > +    return 0
> > > +}
> > > +if [get_compiler_info] {
> > > +    return -1
> > > +}
> > > +# dwarf assembler requires gcc compiled.
> > > +if !$gcc_compiled {
> > > +    unsupported "gcc is required for this test"
> > > +	return 0
> > > +}
> > > +
> > > +standard_testfile .c .S
> > > +
> > > +if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile}] } {
> > > +    return -1
> > > +}
> > > +
> > > +
> > > +set asm_file [standard_output_file $srcfile2]
> > > +Dwarf::assemble $asm_file {
> > > +    global srcdir subdir srcfile
> > > +    declare_labels integer_label L
> > > +    set int_size [get_sizeof "int" 4]
> > > +
> > > +    # Find start address and length for our functions.
> > > +    lassign [function_range main [list ${srcdir}/${subdir}/$srcfile]] \
> > > +	main_start main_len
> > > +    set main_end "$main_start + $main_len"
> > > +
> > > +    cu {} {
> > > +	compile_unit {
> > > +	    {language @DW_LANG_C}
> > > +	    {name until-trailing-isns.c}
> > > +	    {stmt_list $L DW_FORM_sec_offset}
> > > +	    {low_pc 0 addr}
> > > +	} {
> > > +	    subprogram {
> > > +		{external 1 flag}
> > > +		{name main}
> > > +		{low_pc $main_start addr}
> > > +		{high_pc $main_len DW_FORM_data4}
> > > +	    }
> > > +	}
> > > +    }
> > > +
> > > +    lines {version 2 default_is_stmt 1} L {
> > > +	include_dir "${srcdir}/${subdir}"
> > > +	file_name "$srcfile" 1
> > > +
> > > +	# Generate a line table program.  This mimicks clang-13's behavior
> > > +	# of adding some !is_stmt at the end of a loop line, making until
> > > +	# not work properly.
> > > +	program {
> > > +	    {DW_LNE_set_address $main_start}
> > > +	    {line [gdb_get_line_number "main prologue"]}
> > > +	    {DW_LNS_copy}
> > > +	    {DW_LNE_set_address loop_start}
> > > +	    {line [gdb_get_line_number "loop line"]}
> > > +	    {DW_LNS_copy}
> > > +	    {DW_LNE_set_address loop_condition}
> > > +	    {line [gdb_get_line_number "loop line"]}
> > > +	    {DW_LNS_negate_stmt}
> > > +	    {DW_LNS_copy}
> > > +	    {DW_LNE_set_address loop_code}
> > > +	    {line [gdb_get_line_number "loop code"]}
> > 
> > I found that this test was failing for me.  The reason being that
> > "loop code" actually appears inside the comment within the .c file.  I
> > found that if I delete the comment in the .c file then the test
> > started passing again (only with your fix of course).
> > 
> > > +	    {DW_LNS_negate_stmt}
> > > +	    {DW_LNS_copy}
> > > +	    {DW_LNE_set_address loop_increment}
> > > +	    {line [gdb_get_line_number "loop line"]}
> > > +	    {DW_LNS_copy}
> > > +	    {DW_LNE_set_address loop_jump}
> > > +	    {line [gdb_get_line_number "loop line"]}
> > > +	    {DW_LNS_negate_stmt}
> > > +	    {DW_LNS_copy}
> > > +	    {DW_LNE_set_address main_return}
> > > +	    {line [gdb_get_line_number "main return"]}
> > > +	    {DW_LNS_negate_stmt}
> > > +	    {DW_LNS_copy}
> > > +	    {DW_LNE_set_address $main_end}
> > > +	    {line [expr [gdb_get_line_number "main return"] + 1]}
> > > +	    {DW_LNS_copy}
> > > +	    {DW_LNE_end_sequence}
> > > +	}
> > > +    }
> > > +
> > > +}
> > > +
> > > +if { [prepare_for_testing "failed to prepare" ${testfile} \
> > > +	[list $srcfile $asm_file] {nodebug} ] } {
> > > +    return -1
> > > +}
> > > +
> > > +if ![runto_main] {
> > > +    return -1
> > > +}
> > > +
> > > +gdb_test "next" ".* loop code .*" "inside the loop"
> > > +gdb_test "next" ".* loop line .*" "ending of loop"
> > > +gdb_test "until" ".* return 0; .*" "left loop"
> > 
> > While reviewing this patch I ended up making the changes I recommended
> > above as I went along.
> > 
> > I also rewrote and extended some of the text describing the problem we
> > are hitting.  I always find these things easier to understand if there
> > is a really simple worked example included, so that's what I did.
> > 
> > I've also added a 'Bug: ' link within the commit message.
> > 
> > Could you take a look at the patch below, if you are happy with it
> > then I propose that we merge this.
> > 
> > Thanks,
> > Andrew
> > 
> > ---
> > 
> > commit 209580c6befd577b2946a09bc2e024d10d7f0a54
> > Author: Bruno Larsen via Gdb-patches <gdb-patches@sourceware.org>
> > Date:   Wed Jan 26 10:08:13 2022 -0300
> > 
> >      gdb: fix until behavior with trailing !is_stmt lines
> >      When using the command "until", it is expected that GDB will exit a
> >      loop if the current instruction is the last one related to that loop.
> >      However, if there were trailing non-statement instructions, "until"
> >      would just behave as "next".  This was noticeable in clang-compiled
> >      code, but might happen with gcc-compiled as well.  PR gdb/17315 relates
> >      to this problem, as running gdb.base/watchpoint.exp with clang
> >      would fail for this reason.
> >      To better understand this issue, consider the following source code,
> >      with line numbers marked on the left:
> >        10:   for (i = 0; i < 10; ++i)
> >        11:     loop_body ();
> >        12:   other_stuff ();
> >      If we transform this to pseudo-assembler, and generate a line table,
> >      we could end up with something like this:
> >        Address | Pseudo-Assembler | Line | Is-Statement?
> >        0x100   | i = 0            | 10   | Yes
> >        0x104   | loop_body ()     | 11   | Yes
> >        0x108   | i = i + 1        | 10   | Yes
> >        0x10c   | if (i < 10):     | 10   | No
> >        0x110   |     goto 0x104   | 10   | No
> >        0x114   | other_stuff ()   | 12   | Yes
> >      Notice the two non-statement instructions at the end of the loop.
> >      The problem is that when we reach address 0x108 and use 'until',
> >      hoping to leave the loop, GDB sets up a stepping range that runs from
> >      the start of the function (0x100 in our example) to the end of the
> >      current line table entry, that is 0x10c in our example.  GDB then
> >      starts stepping forward.
> >      When 0x10c is reached GDB spots that we have left the stepping range,
> >      that the new location is not a statement, and that the new location is
> >      associated with the same source line number as the previous stepping
> >      range.  GDB then sets up a new stepping range that runs from 0x10c to
> >      0x114, and continues stepping forward.
> >      Within that stepping range the inferior hits the goto (at 0x110) and
> >      loops back to address 0x104.
> >      At 0x104 GDB spots that we have left the previous stepping range, that
> >      the new address is marked as a statement, and that the new address is
> >      for a different source line.  As a result, GDB stops and returns
> >      control to the user.  This is not what the user was expecting, they
> >      expected GDB to exit the loop.
> >      The fix proposed in this patch, is that, when the user issues the
> >      'until' command, and GDB sets up the initial stepping range, GDB will
> >      check subsequent SALs (symtab_and_lines) to see if they are
> >      non-statements associated with the same line number.  If they are then
> >      the end of the initial stepping range is extended to the end of the
> >      non-statement SALs.
> >      In our example above, the user is at 0x108 and uses 'until', GDB now
> >      sets up a stepping range from the start of the function 0x100 to
> >      0x114, the first address associated with a different line.
> >      Now as GDB steps around the loop it never leaves the initial stepping
> >      range.  It is only when GDB exits the loop that we leave the stepping
> >      range, and the stepping finishes at address 0x114.
> >      This patch also adds a test case that can be run with gcc to test that
> >      this functionality is not broken in the future.
> >      Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=17315
> > 
> > diff --git a/gdb/infcmd.c b/gdb/infcmd.c
> > index 48dda9b23ee..a25d024d24c 100644
> > --- a/gdb/infcmd.c
> > +++ b/gdb/infcmd.c
> > @@ -1346,6 +1346,45 @@ until_next_command (int from_tty)
> >         tp->control.step_range_start = BLOCK_ENTRY_PC (SYMBOL_BLOCK_VALUE (func));
> >         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 will not
> > +	 extend beyond the loop and the until command will exit the
> > +	 stepping range, and find the is_stmt true location corresponding
> > +	 to 'loop_body ()', this will cause GDB to stop inside the loop on
> > +	 the which is not what we wanted.
> 
> The wording in this paragraph seems a bit odd to me. My suggestion for an improvement is:
> 
> 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.

I made this change, and pushed the patch.

Thanks,
Andrew