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 23B7D395147A for ; Sat, 19 Sep 2020 03:05:37 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 23B7D395147A 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 C43491E599; Fri, 18 Sep 2020 23:05:33 -0400 (EDT) Subject: Re: [PATCH v2 1/2] gdb/breakpoint: disable a bp location if condition is invalid at that location To: Tankut Baris Aktemur , gdb-patches@sourceware.org References: From: Simon Marchi Message-ID: Date: Fri, 18 Sep 2020 23:05:31 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.12.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=-9.8 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_STATUS, NICE_REPLY_A, 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: Sat, 19 Sep 2020 03:05:39 -0000 Hi Baris, I don't have much time right now, but I also don't want to delay looking at this further, so here are a few comments. Overall, I think that this is very well thought through and I like the behavior. On 2020-08-20 5:24 p.m., Tankut Baris Aktemur via Gdb-patches wrote: > Currently, for a conditional breakpoint, GDB checks if the condition > can be evaluated in the context of the first symtab and line (SAL). > In case of an error, defining the conditional breakpoint is aborted. > This prevents having a conditional breakpoint whose condition may > actually be meaningful for some of the location contexts. This patch > makes it possible to define conditional BPs by checking all location > contexts. If the condition is meaningful for even one context, the > breakpoint is defined. The locations for which the condition gives > errors are disabled. > > The bp_location struct is introduced a new field, 'disabled_by_cond'. > This field denotes whether the location is disabled automatically > because the condition was non-evaluatable. Disabled-by-cond locations > cannot be enabled by the user. But locations that are not > disabled-by-cond can be enabled/disabled by the user manually as > before. > > For a concrete example, consider 3 contexts; func1, func2, and func3. > Each context contains instructions coming from the same source. > For instance: > > void > func1 (int *value) > { > int a = 10; > value += a; > int b = 1; > #include "included.c" > } > > void > func2 (int *value) > { > int b = 2; > #include "included.c" > } > > void > func3 (int *value) > { > int c = 30; > value += c; > int b = 3; > #include "included.c" > } A C++ example with overloaded functions would be simpler to understand, I think. > > Note that > > * the variable 'a' is defined only in the context defined by func1. > * the variable 'c' is defined only in the context defined by func3. > * the variable 'b' is defined in all the three contexts. > > With the existing GDB, it's not possible to define a conditional > breakpoint at "included.c:1" if the condition refers to 'a' or 'c': > > (gdb) break included.c:1 if a == 10 > No symbol "a" in current context. > (gdb) break included.c:1 if c == 30 > No symbol "c" in current context. > (gdb) info breakpoints > No breakpoints or watchpoints. > > With this patch, it becomes possible: > > (gdb) break included.c:1 if a == 10 > warning: disabling breakpoint location 2: No symbol "a" in current context. > warning: disabling breakpoint location 3: No symbol "a" in current context. Though it would make the message a bit longer, I think it would be important to mention in it that the location is disabled because GDB failed to resolve (is the right word?) the condition at this location. Most of the time it will be obvious, but I'm sure that in some cases it won't be, and the user will be wondering what that error on the right is about. Just one example, if the condition expression involves a macro that hides some identifiers: #define SOME_EXPR s and (gdb) b bar if SOME_EXPR warning: disabling breakpoint location 2: No symbol "s" in current context. Breakpoint 1 at 0x1120: bar. (2 locations) (gdb) i b Num Type Disp Enb Address What 1 breakpoint keep y stop only if SOME_EXPR 1.1 n 0x0000000000001120 in bar(int) at test.c:11 1.2 y 0x000000000000112b in bar(char const*) at test.c:15 I'm not sure what format is better: warning: failed to resolve condition at location 2, disabing: No symbol "a" in current context. vs disabling breakpoint location 2: failed to resolve condition: No symbol "a" in current context. vs warning: failed to resolve condition at location 2: No symbol "a" in current context. warning: disabling breakpoint location 2. It's a bit annoying that the exception message is capitalized and has a period, otherwise this would have been a good candidate: warning: failed to resolve condition at location 2 (no symbol "a" in current context), disabling. > Breakpoint 1 at 0x117d: included.c:1. (3 locations) > (gdb) break included.c:1 if c == 30 > Note: breakpoint 1 also set at pc 0x117d. > warning: disabling breakpoint location 1: No symbol "c" in current context. > Note: breakpoint 1 also set at pc 0x119c. > warning: disabling breakpoint location 2: No symbol "c" in current context. > Note: breakpoint 1 also set at pc 0x11cf. > Breakpoint 2 at 0x117d: included.c:1. (3 locations) > (gdb) info break > Num Type Disp Enb Address What > 1 breakpoint keep y > stop only if a == 10 > 1.1 y 0x000000000000117d in func1 at included.c:1 > 1.2 n 0x000000000000119c in func2 at included.c:1 > 1.3 n 0x00000000000011cf in func3 at included.c:1 > 2 breakpoint keep y > stop only if c == 30 > 2.1 n 0x000000000000117d in func1 at included.c:1 > 2.2 n 0x000000000000119c in func2 at included.c:1 > 2.3 y 0x00000000000011cf in func3 at included.c:1 Should we somehow show in the listing that the locations disabled because of the condition are disabled and can't be enabled? For example, a capital N in the "Enb" column? > > Executing the code hits the breakpoints 1.1 and 2.3 as expected. > > Defining a condition on an unconditional breakpoint gives the same > behavior above: > > (gdb) break included.c:1 > Breakpoint 1 at 0x117d: included.c:1. (3 locations) > (gdb) cond 1 a == 10 > warning: disabling breakpoint 1.2: No symbol "a" in current context. > warning: disabling breakpoint 1.3: No symbol "a" in current context. > (gdb) info breakpoints > Num Type Disp Enb Address What > 1 breakpoint keep y > stop only if a == 10 > 1.1 y 0x000000000000117d in func1 at included.c:1 > 1.2 n 0x000000000000119c in func2 at included.c:1 > 1.3 n 0x00000000000011cf in func3 at included.c:1 > > Locations that are disabled because of a condition cannot be enabled > by the user: > > ... > (gdb) enable 1.2 > Location is disabled because of the condition; cannot enable manually. "because of the condition" does not sound clear to me. It does not say what about the condition prevents the location from getting enabled. What about something like "Breakpoint condition is invalid at location 1.2, cannot enable". Ideally, we should make sure that the messages we output at different times should use the same vocabulary. > > Resetting the condition enables the locations back: > > ... > (gdb) cond 1 > Breakpoint 1.2 is now enabled. > Breakpoint 1.3 is now enabled. Likewise, this doesn't say why these locations suddenly get enabled. Should it? Something like "Breakpoint condition now resolves at location 1.2, enabling.". Or is it obvious, because the user is already using the "condition" command? > Breakpoint 1 now unconditional. > (gdb) info breakpoints > Num Type Disp Enb Address What > 1 breakpoint keep y > 1.1 y 0x000000000000117d in func1 at included.c:1 > 1.2 y 0x000000000000119c in func2 at included.c:1 > 1.3 y 0x00000000000011cf in func3 at included.c:1 > > If a location is disabled by the user, a condition can still be defined > but the location will remain disabled even if the condition is meaningful > for the disabled location: > > ... > (gdb) disable 1.1 > (gdb) cond 1 a == 10 > warning: disabling breakpoint 1.2: No symbol "a" in current context. > warning: disabling breakpoint 1.3: No symbol "a" in current context. > (gdb) info breakpoints > Num Type Disp Enb Address What > 1 breakpoint keep y > stop only if a == 10 > 1.1 n 0x000000000000117d in func1 at included.c:1 > 1.2 n 0x000000000000119c in func2 at included.c:1 > 1.3 n 0x00000000000011cf in func3 at included.c:1 If a location is already user-disabled and we install a condition that condition-disables it, we print a "disabling" message: (gdb) b bar Breakpoint 1 at 0x1120: bar. (2 locations) (gdb) disable 1.1 1.2 (gdb) i b Num Type Disp Enb Address What 1 breakpoint keep y 1.1 n 0x0000000000001120 in bar(int) at test.c:9 1.2 n 0x000000000000112b in bar(char const*) at test.c:13 (gdb) cond 1 s warning: disabling breakpoint 1.1: No symbol "s" in current context. (gdb) i b Num Type Disp Enb Address What 1 breakpoint keep y stop only if s 1.1 n 0x0000000000001120 in bar(int) at test.c:9 1.2 n 0x000000000000112b in bar(char const*) at test.c:13 Should the "disabling breakpoint" message not be printed here, since it doesn't really make sense to disable an already disabled breakpoint? If the user tries to enable it back, then they'll get the "can't enable" message. > diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c > index 977599db1db..7abfd510abc 100644 > --- a/gdb/breakpoint.c > +++ b/gdb/breakpoint.c > @@ -830,6 +830,48 @@ get_first_locp_gte_addr (CORE_ADDR address) > return locp_found; > } > > +/* Parse COND_STRING in the context of LOC and set as the condition > + expression of LOC. BP_NUM is the number of LOC's owner, LOC_NUM is > + the number of LOC within its owner. In case of parsing error, mark > + LOC as DISABLED_BY_COND. In case of success, unset DISABLED_BY_COND. */ > + > +static void > +set_breakpoint_location_condition (const char *cond_string, bp_location *loc, > + int bp_num, int loc_num) > +{ > + bool has_junk = false; > + try > + { > + expression_up new_exp = parse_exp_1 (&cond_string, loc->address, > + block_for_pc (loc->address), 0); > + if (*cond_string != 0) > + has_junk = true; > + else > + { > + loc->cond = std::move (new_exp); > + if (loc->disabled_by_cond && loc->enabled) > + printf_filtered (_("Breakpoint %d.%d is now enabled.\n"), > + bp_num, loc_num); > + > + loc->disabled_by_cond = false; > + } > + } > + catch (const gdb_exception_error &e) > + { > + if (bp_num != 0) > + warning (_("disabling breakpoint %d.%d: %s"), > + bp_num, loc_num, e.what ()); > + else > + warning (_("disabling breakpoint location %d: %s"), > + loc_num, e.what ()); When is bp_num 0? > @@ -14157,6 +14258,10 @@ enable_disable_bp_num_loc (int bp_num, int loc_num, bool enable) > struct bp_location *loc = find_location_by_number (bp_num, loc_num); > if (loc != NULL) > { > + if (loc->disabled_by_cond && enable) > + error(_("Location is disabled because of the condition; " Space after "error". Simon