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

[Bruno Larsen <blarsen@redhat.com>]

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