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 572F8385700B for ; Wed, 22 Jul 2020 13:28:06 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 572F8385700B 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 E065C1E794; Wed, 22 Jul 2020 09:28:05 -0400 (EDT) Subject: Re: [PATCH 2/3] gdb/breakpoint: set the condition exp after parsing the condition successfully From: Simon Marchi To: Tankut Baris Aktemur , gdb-patches@sourceware.org References: <341eb335-db50-82ce-4dc9-2b5d53765dd6@simark.ca> Message-ID: <368bbad3-67c4-1422-9006-8a37ac49be34@simark.ca> Date: Wed, 22 Jul 2020 09:28:03 -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: <341eb335-db50-82ce-4dc9-2b5d53765dd6@simark.ca> 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:28:07 -0000 On 2020-07-22 9:21 a.m., Simon Marchi wrote: > 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`. Again, this is taken care of in the next patch, so forget it :). Although, in the breakpoint case, when we have: for (bp_location *loc = b->loc; loc != nullptr; loc = loc->next) { const char *arg = exp; expression_up new_exp = parse_exp_1 (&arg, loc->address, block_for_pc (loc->address), 0); if (*arg != 0) error (_("Junk at end of expression")); loc->cond = std::move (new_exp); } Doesn't that mean that if the expression succeeds to parse for one location and then fails to parse for another location, we'll have updated one location and not the other? How does that work (or should work) when we have a multi-location breakpoint and the condition only makes sense in one of the locations? Simon