From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id 6C18A38930DB for ; Wed, 22 Jul 2020 13:12:49 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 6C18A38930DB Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=simark.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=simark@simark.ca Received: from [10.0.0.11] (173-246-6-90.qc.cable.ebox.net [173.246.6.90]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id BE9D11E794; Wed, 22 Jul 2020 09:12:48 -0400 (EDT) Subject: Re: [PATCH 1/3] gdb/breakpoint: do not update the condition string if parsing the condition fails To: Tankut Baris Aktemur , gdb-patches@sourceware.org References: From: Simon Marchi Message-ID: <6460667d-3906-fd69-e0af-92c79e408fd4@simark.ca> Date: Wed, 22 Jul 2020 09:12:45 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: fr Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-8.8 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_STATUS, KAM_SHORT, SPF_HELO_PASS, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) 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: Wed, 22 Jul 2020 13:12:51 -0000 On 2020-06-29 9:48 a.m., Tankut Baris Aktemur via Gdb-patches wrote: > The condition of a breakpoint can be set with the 'cond' command. If > the condition has errors that make it problematic to evaluate, it > appears like GDB rejects the condition, but updates the breakpoint's > condition string, which causes incorrect/unintuitive behavior. > > For instance: > > $ gdb ./test > Reading symbols from ./test... > (gdb) break 5 > Breakpoint 1 at 0x1155: file test.c, line 5. > (gdb) cond 1 gibberish > No symbol "gibberish" in current context. > > At this point, it looks like the condition was rejected. > But "info breakpoints" shows the following: > > (gdb) info breakpoints > Num Type Disp Enb Address What > 1 breakpoint keep y 0x0000000000001155 in main at test.c:5 > stop only if gibberish > > Running the code gives the following behavior, where re-insertion of > the breakpoint causes failures. > > (gdb) run > Starting program: test > warning: failed to reevaluate condition for breakpoint 1: No symbol "gibberish" in current context. > warning: failed to reevaluate condition for breakpoint 1: No symbol "gibberish" in current context. > warning: failed to reevaluate condition for breakpoint 1: No symbol "gibberish" in current context. > warning: failed to reevaluate condition for breakpoint 1: No symbol "gibberish" in current context. > warning: failed to reevaluate condition for breakpoint 1: No symbol "gibberish" in current context. > [Inferior 1 (process 19084) exited normally] > (gdb) > > This broken behavior occurs because GDB updates the condition string > of the breakpoint *before* checking that it parses successfully. > When parsing fails, the update has already taken place. > > Fix the problem by updating the condition string *after* parsing the > condition. We get the following behavior when this patch is applied: > > $ gdb ./test > Reading symbols from ./test... > (gdb) break 5 > Breakpoint 1 at 0x1155: file test.c, line 5. > (gdb) cond 1 gibberish > No symbol "gibberish" in current context. > (gdb) info breakpoints > Num Type Disp Enb Address What > 1 breakpoint keep y 0x0000000000001155 in main at test.c:5 > (gdb) run > Starting program: test > > Breakpoint 1, main () at test.c:5 > 5 a = a + 1; /* break-here */ > (gdb) c > Continuing. > [Inferior 1 (process 15574) exited normally] > (gdb) > > A side note: The problem does not occur if the condition is given > at the time of breakpoint definition, as in "break 5 if gibberish", > because the parsing of the condition fails during symtab-and-line > creation, before the breakpoint is created. > > Finally, the code included the following comment: > > /* I don't know if it matters whether this is the string the user > typed in or the decompiled expression. */ > > This comment did not make sense to me because the condition string is > the user-typed input. The patch updates this comment, too. > > gdb/ChangeLog: > 2020-06-29 Tankut Baris Aktemur > > * breakpoint.c (set_breakpoint_condition): Update the > condition string after parsing the new condition successfully. > > gdb/testsuite/ChangeLog: > 2020-06-29 Tankut Baris Aktemur > > * gdb.base/condbreak-bad.c: New test. > * gdb.base/condbreak-bad.exp: New file. > --- > gdb/breakpoint.c | 17 +++++----- > gdb/testsuite/gdb.base/condbreak-bad.c | 24 ++++++++++++++ > gdb/testsuite/gdb.base/condbreak-bad.exp | 40 ++++++++++++++++++++++++ > 3 files changed, 73 insertions(+), 8 deletions(-) > create mode 100644 gdb/testsuite/gdb.base/condbreak-bad.c > create mode 100644 gdb/testsuite/gdb.base/condbreak-bad.exp > > diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c > index 6d81323dd92..1fc2d1b8966 100644 > --- a/gdb/breakpoint.c > +++ b/gdb/breakpoint.c > @@ -834,9 +834,6 @@ void > set_breakpoint_condition (struct breakpoint *b, const char *exp, > int from_tty) > { > - xfree (b->cond_string); > - b->cond_string = NULL; > - > if (is_watchpoint (b)) > { > struct watchpoint *w = (struct watchpoint *) b; > @@ -859,6 +856,9 @@ set_breakpoint_condition (struct breakpoint *b, const char *exp, > > if (*exp == 0) > { > + xfree (b->cond_string); > + b->cond_string = nullptr; > + > if (from_tty) > printf_filtered (_("Breakpoint %d now unconditional.\n"), b->number); > } > @@ -866,11 +866,6 @@ set_breakpoint_condition (struct breakpoint *b, const char *exp, > { > const char *arg = exp; > > - /* I don't know if it matters whether this is the string the user > - typed in or the decompiled expression. */ > - b->cond_string = xstrdup (arg); > - b->condition_not_parsed = 0; > - > if (is_watchpoint (b)) > { > struct watchpoint *w = (struct watchpoint *) b; > @@ -896,6 +891,12 @@ set_breakpoint_condition (struct breakpoint *b, const char *exp, > error (_("Junk at end of expression")); > } > } > + > + /* We know that the new condition parsed successfully. The > + condition string of the breakpoint can be safely updated. */ > + xfree (b->cond_string); > + b->cond_string = xstrdup (exp); > + b->condition_not_parsed = 0; > } > mark_breakpoint_modified (b); > > diff --git a/gdb/testsuite/gdb.base/condbreak-bad.c b/gdb/testsuite/gdb.base/condbreak-bad.c > new file mode 100644 > index 00000000000..58283b75ca7 > --- /dev/null > +++ b/gdb/testsuite/gdb.base/condbreak-bad.c > @@ -0,0 +1,24 @@ > +/* This testcase is part of GDB, the GNU debugger. > + > + Copyright 2020 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 . */ > + > +int > +main () > +{ > + int a = 10; > + a = a + 1; /* break-here */ > + return 0; > +} > diff --git a/gdb/testsuite/gdb.base/condbreak-bad.exp b/gdb/testsuite/gdb.base/condbreak-bad.exp > new file mode 100644 > index 00000000000..a01ba2a9340 > --- /dev/null > +++ b/gdb/testsuite/gdb.base/condbreak-bad.exp > @@ -0,0 +1,40 @@ > +# Copyright 2020 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 . > + > +# Test defining bad conditions for breakpoints. > + > +standard_testfile > + > +if {[prepare_for_testing "failed to prepare" ${binfile} ${srcfile}]} { > + return > +} > + > +set bp_location [gdb_get_line_number "break-here"] > +gdb_breakpoint "$bp_location" > +set bpnum [get_integer_valueof "\$bpnum" 0 "get bpnum"] > + > +# Define a 'bad' condition. The breakpoint should stay unconditional. > +gdb_test "cond $bpnum gibberish" \ > + "No symbol \"gibberish\" in current context." \ > + "attempt a bad condition" > + > +set fill "\[^\r\n\]*" > + > +gdb_test "info break" \ > + [multi_line \ > + "Num${fill}What" \ > + "${decimal}${fill}breakpoint${fill}keep y${fill}:${bp_location}"] \ > + "breakpoint is unconditional" > + Here, could you also test that when a valid condition exists and we try to change it for a bad one, the previous condition is still there after the failed attempt to change it? Simon