[PATCH v3] [PR gdb/17315] fix until behavior with trailing !is_stmt lines
Andrew Burgess
aburgess@redhat.com
Fri Feb 11 15:17:11 GMT 2022
* 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
More information about the Gdb-patches
mailing list