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 34FE33875411 for ; Wed, 22 Jul 2020 16:06:31 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 34FE33875411 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 661481E111; Wed, 22 Jul 2020 12:06:30 -0400 (EDT) Subject: Re: [PATCH 2/3] gdb/breakpoint: set the condition exp after parsing the condition successfully To: "Aktemur, Tankut Baris" , "gdb-patches@sourceware.org" References: <341eb335-db50-82ce-4dc9-2b5d53765dd6@simark.ca> <368bbad3-67c4-1422-9006-8a37ac49be34@simark.ca> From: Simon Marchi Message-ID: Date: Wed, 22 Jul 2020 12:06:26 -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=-3.8 required=5.0 tests=BAYES_00, 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 16:06:32 -0000 On 2020-07-22 11:29 a.m., Aktemur, Tankut Baris wrote: > On Wednesday, July 22, 2020 3:28 PM, Simon Marchi wrote: >> 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? > > Ahh, yes. The diff for the part above should have been: > > struct bp_location *loc; > > + /* Parse and set condition expressions. We make two passes. > + In the first, we parse the condition string to see if it > + is valid in all locations. If so, the condition would be > + accepted. So we go ahead and set the locations' > + conditions. In case a failing case is found, we throw > + the error and the condition string will be rejected. > + This two-pass approach is taken to avoid setting the > + state of locations in case of a reject. */ > + for (loc = b->loc; loc; loc = loc->next) > + { > + arg = exp; > + parse_exp_1 (&arg, loc->address, > + block_for_pc (loc->address), 0); > + if (*arg != 0) > + error (_("Junk at end of expression")); > + } > + > + /* If we reach here, the condition is valid at all locations. */ > for (loc = b->loc; loc; loc = loc->next) > { > arg = exp; > loc->cond = > parse_exp_1 (&arg, loc->address, > block_for_pc (loc->address), 0); > - if (*arg) > - error (_("Junk at end of expression")); > } > >> 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? > > I'm in fact working on a follow-up patch on this topic, where the two-pass approach above > is used (hence I forgot to include it in this series). > > Currently, GDB expects the condition to be valid at all locations. The patch that I'll > soon post proposes to accept the condition if there exist locations where it's valid. > The locations where the condition is invalid are disabled. But in the current state, the > condition has to make sense at all locations. Ok, so do you want to wait and post everything together, or do still want to consider merging this one on its own, since it's still a step forward? Simon