From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 1CB793858D1E for ; Fri, 11 Feb 2022 15:17:20 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 1CB793858D1E Received: from mail-wm1-f72.google.com (mail-wm1-f72.google.com [209.85.128.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-473-xtJVBTI_PiqJnrMNihn4yw-1; Fri, 11 Feb 2022 10:17:15 -0500 X-MC-Unique: xtJVBTI_PiqJnrMNihn4yw-1 Received: by mail-wm1-f72.google.com with SMTP id i204-20020a1c3bd5000000b0037bb9f6feeeso535714wma.5 for ; Fri, 11 Feb 2022 07:17:14 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=2/yFHMTgnYLEJM1jc8Fm/aHzKqe3c8uOo7pRAvahxN8=; b=goADi38hgpuoz7R9Fv/0djsRodd3h9pxSTp8Rmr8yWONNyCaoxqtzmTxamb+NtzSSg AM9znfpZOGnf8VjV0IiGpJmHCLt3T5poRCN7a9NM3rvqRCP06IaDzd9r+2VwgjJyrLrl n7jdh4wXzygnlXw/k+69LxboNeCWg1uuLZv1TJzuEu/4fRdA4A4WqFPFcc4EwzwIfrkb DmsLOCzlPTYOiAlD63hlvDrC3eARyU9t2Wrme+eK7MT40dn1/7jWBNsiq690EAECsups vt2cy9J1Ebfm3RGoJqGYGFrepSMAxaVCZytwaMPgvp6oQxSmXdKphIF618xHFOpSk+Tl Jb7Q== X-Gm-Message-State: AOAM530dooiMovXRKuIyJjpOUTPJMdsEB4w+s0bSdn/d0ljCNfR4cibP IC88zK4g16vakmqCPvbUK9EnvthUjaNKbnfhbWWob8dk06kGmEQbPUQ1W/8IhrDVwlDlCg9yqTo H4AWT4ThbvkExLG7n9TVRNQ== X-Received: by 2002:a05:6000:1568:: with SMTP id 8mr1684860wrz.583.1644592633233; Fri, 11 Feb 2022 07:17:13 -0800 (PST) X-Google-Smtp-Source: ABdhPJxJHbTsxbmMWzWkYgzli8oyRrPrlF3buJbY/fzAQGgPscCJcyh8DgbOlbuiWEaPGKuTjrafDQ== X-Received: by 2002:a05:6000:1568:: with SMTP id 8mr1684820wrz.583.1644592632699; Fri, 11 Feb 2022 07:17:12 -0800 (PST) Received: from localhost (host86-134-151-224.range86-134.btcentralplus.com. [86.134.151.224]) by smtp.gmail.com with ESMTPSA id t2sm3494272wrr.55.2022.02.11.07.17.12 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 11 Feb 2022 07:17:12 -0800 (PST) Date: Fri, 11 Feb 2022 15:17:11 +0000 From: Andrew Burgess To: Bruno Larsen Cc: gdb-patches@sourceware.org Subject: Re: [PATCH v3] [PR gdb/17315] fix until behavior with trailing !is_stmt lines Message-ID: <20220211151711.GC2571@redhat.com> References: <20220126130813.24716-1-blarsen@redhat.com> <20220210124411.GB2768@redhat.com> <40482a62-1fe6-a19e-d0dc-3d79bc8165a5@redhat.com> MIME-Version: 1.0 In-Reply-To: <40482a62-1fe6-a19e-d0dc-3d79bc8165a5@redhat.com> X-Operating-System: Linux/5.8.18-100.fc31.x86_64 (x86_64) X-Uptime: 15:16:14 up 4:55, 1 X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=us-ascii Content-Disposition: inline X-Spam-Status: No, score=-9.7 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, KAM_STOCKGEN, RCVD_IN_BARRACUDACENTRAL, RCVD_IN_DNSWL_LOW, SPF_HELO_NONE, SPF_NONE, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 11 Feb 2022 15:17:23 -0000 * Bruno Larsen via Gdb-patches [2022-02-10 10:46:01 -0300]: > On 2/10/22 09:44, Andrew Burgess wrote: > > * Bruno Larsen via Gdb-patches [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 . */ > > > + > > > +/* 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 . > > > +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 > > 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