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 4FD25385700B for ; Wed, 22 Jul 2020 13:21:41 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 4FD25385700B 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) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id C24161E794; Wed, 22 Jul 2020 09:21:40 -0400 (EDT) Subject: Re: [PATCH 2/3] gdb/breakpoint: set the condition exp after parsing the condition successfully To: Tankut Baris Aktemur , gdb-patches@sourceware.org References: From: Simon Marchi Message-ID: <341eb335-db50-82ce-4dc9-2b5d53765dd6@simark.ca> Date: Wed, 22 Jul 2020 09:21:37 -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, 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:21:42 -0000 On 2020-06-29 9:48 a.m., Tankut Baris Aktemur via Gdb-patches wrote: > In 'set_breakpoint_condition', GDB resets the condition expressions > before parsing the condition input by the user. This leads to the > problem of losing the condition expressions if the new condition > does not parse successfully and is thus rejected. > > For instance: > > $ gdb ./test > Reading symbols from ./test... > (gdb) start > Temporary breakpoint 1 at 0x114e: file test.c, line 4. > Starting program: test > > Temporary breakpoint 1, main () at test.c:4 > 4 int a = 10; > (gdb) break 5 > Breakpoint 2 at 0x555555555155: file test.c, line 5. > > Now define a condition that would evaluate to false. Next, attempt > to overwrite that with an invalid condition: > > (gdb) cond 2 a == 999 > (gdb) cond 2 gibberish > No symbol "gibberish" in current context. > (gdb) info breakpoints > Num Type Disp Enb Address What > 2 breakpoint keep y 0x0000555555555155 in main at test.c:5 > stop only if a == 999 > > It appears as if the bad condition is successfully rejected. But if we > resume the program, we see that we hit the breakpoint although the condition > would evaluate to false. > > (gdb) continue > Continuing. > > Breakpoint 2, main () at test.c:5 > 5 a = a + 1; /* break-here */ > > Fix the problem by not resetting the condition expressions before > parsing the condition input. > > Suppose the fix is applied. A similar problem could occur if the > condition is valid, but has "junk" at the end. In this case, parsing > succeeds, but an error is raised immediately after. It is too late, > though; the condition expression is already updated. > > For instance: > > $ gdb ./test > Reading symbols from ./test... > (gdb) start > Temporary breakpoint 1 at 0x114e: file test.c, line 4. > Starting program: test > > Temporary breakpoint 1, main () at test.c:4 > 4 int a = 10; > (gdb) break 5 > Breakpoint 2 at 0x555555555155: file test.c, line 5. > (gdb) cond 2 a == 999 > (gdb) cond 2 a == 10 if > Junk at end of expression > (gdb) info breakpoints > Num Type Disp Enb Address What > 2 breakpoint keep y 0x0000555555555155 in main at test.c:5 > stop only if a == 999 > (gdb) c > Continuing. > > Breakpoint 2, main () at test.c:5 > 5 a = a + 1; /* break-here */ > (gdb) > > We should not have hit the breakpoint because the condition would > evaluate to false. > > Fix this problem by updating the condition expression of the breakpoint > after parsing the input successfully and checking that there is no > remaining junk. > > gdb/ChangeLog: > 2020-06-29 Tankut Baris Aktemur > > * breakpoint.c (set_breakpoint_condition): Update the condition > expressions after checking that the input condition string parses > successfully and does not contain junk at the end. > > gdb/testsuite/ChangeLog: > 2020-06-29 Tankut Baris Aktemur > > * gdb.base/condbreak-bad.exp: Extend the test with scenarios > that attempt to overwrite and existing condition with a condition > that fails parsing and also with a condition that parses fine > but contains junk at the end. > --- > gdb/breakpoint.c | 46 +++++++------ > gdb/testsuite/gdb.base/condbreak-bad.exp | 88 ++++++++++++++++++++++++ > 2 files changed, 112 insertions(+), 22 deletions(-) > > diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c > index 1fc2d1b8966..abda470fe21 100644 > --- a/gdb/breakpoint.c > +++ b/gdb/breakpoint.c > @@ -834,30 +834,30 @@ void > set_breakpoint_condition (struct breakpoint *b, const char *exp, > int from_tty) > { > - if (is_watchpoint (b)) > - { > - struct watchpoint *w = (struct watchpoint *) b; > - > - w->cond_exp.reset (); > - } > - else > + if (*exp == 0) > { > - struct bp_location *loc; > + xfree (b->cond_string); > + b->cond_string = nullptr; > > - for (loc = b->loc; loc; loc = loc->next) > + if (is_watchpoint (b)) > { > - loc->cond.reset (); > + struct watchpoint *w = (struct watchpoint *) b; > > - /* No need to free the condition agent expression > - bytecode (if we have one). We will handle this > - when we go through update_global_location_list. */ > + w->cond_exp.reset (); > } > - } > + else > + { > + struct bp_location *loc; > > - if (*exp == 0) > - { > - xfree (b->cond_string); > - b->cond_string = nullptr; > + for (loc = b->loc; loc; loc = loc->next) > + { > + loc->cond.reset (); > + > + /* No need to free the condition agent expression > + bytecode (if we have one). We will handle this > + when we go through update_global_location_list. */ > + } > + } Since you touch this, might as well declare the `bp_location *loc` in the for loop and use `loc != nullptr`. Otherwise, this LGTM. Simon