From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.polymtl.ca (smtp.polymtl.ca [132.207.4.11]) by sourceware.org (Postfix) with ESMTPS id 3B253385801D for ; Wed, 7 Apr 2021 22:08:39 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 3B253385801D Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id 137M8VC2031896 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Wed, 7 Apr 2021 18:08:36 -0400 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca 137M8VC2031896 Received: from [10.0.0.11] (192-222-157-6.qc.cable.ebox.net [192.222.157.6]) (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 E60611E01F; Wed, 7 Apr 2021 18:08:30 -0400 (EDT) Subject: Re: [PATCH 3/4] gdb/breakpoint: add a 'force_condition' parameter to 'create_breakpoint' To: Tankut Baris Aktemur , gdb-patches@sourceware.org References: <40773bd923ed8cb1f9773ecd29a5af01e204d801.1617806598.git.tankut.baris.aktemur@intel.com> From: Simon Marchi Message-ID: <8f9e3ef4-22c0-a872-68e4-20389e4fb746@polymtl.ca> Date: Wed, 7 Apr 2021 18:08:30 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.7.1 MIME-Version: 1.0 In-Reply-To: <40773bd923ed8cb1f9773ecd29a5af01e204d801.1617806598.git.tankut.baris.aktemur@intel.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Wed, 7 Apr 2021 22:08:31 +0000 X-Spam-Status: No, score=-10.2 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, NICE_REPLY_A, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, 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, 07 Apr 2021 22:08:41 -0000 On 2021-04-07 10:55 a.m., Tankut Baris Aktemur via Gdb-patches wrote: > The 'create_breakpoint' function takes a 'parse_extra' argument that > determines whether the condition, thread, and force-condition > specifiers should be parsed from the extra string or be used from the > function arguments. However, for the case when 'parse_extra' is > false, there is no way to pass the force-condition specifier. This > patch adds it as a new argument. > > Also, in the case when parse_extra is false, the current behavior is > as if the condition is being forced. This is a bug. The default > behavior should reject the breakpoint. See below for a demo of this > incorrect behavior. (The MI command '-break-insert' uses the > 'create_breakpoint' function with parse_extra=0.) > > $ gdb -q --interpreter=mi2 /tmp/simple > =thread-group-added,id="i1" > =cmd-param-changed,param="history save",value="on" > =cmd-param-changed,param="auto-load safe-path",value="/" > ~"Reading symbols from /tmp/simple...\n" > (gdb) > -break-insert -c junk -f main > &"warning: failed to validate condition at location 1, disabling:\n " > &"No symbol \"junk\" in current context.\n" > ^done,bkpt={number="1",type="breakpoint",disp="keep",enabled="y",addr="",cond="junk",times="0",original-location="main"},{number="1.1",enabled="N*",addr="0x000000000000114e",func="main",file="simple.c",fullname="/tmp/simple.c",line="2",thread-groups=["i1"]} > (gdb) > break main if junk > &"break main if junk\n" > &"No symbol \"junk\" in current context.\n" > ^error,msg="No symbol \"junk\" in current context." > (gdb) > break main -force-condition if junk > &"break main -force-condition if junk\n" > ~"Note: breakpoint 1 also set at pc 0x114e.\n" > &"warning: failed to validate condition at location 1, disabling:\n " > &"No symbol \"junk\" in current context.\n" > ~"Breakpoint 2 at 0x114e: file simple.c, line 2.\n" > =breakpoint-created,bkpt={number="2",type="breakpoint",disp="keep",enabled="y",addr="",cond="junk",times="0",original-location="main"},{number="2.1",enabled="N*",addr="0x000000000000114e",func="main",file="simple.c",fullname="/tmp/simple.c",line="2",thread-groups=["i1"]} > ^done > (gdb) > > After applying this patch, we get the behavior below: > > (gdb) > -break-insert -c junk -f main > ^error,msg="No symbol \"junk\" in current context." You can also mention that this restores the previous behavior (present in existing releases). > diff --git a/gdb/testsuite/gdb.mi/mi-break.exp b/gdb/testsuite/gdb.mi/mi-break.exp > index 9d3d7ade6dc..ac13e4d1e09 100644 > --- a/gdb/testsuite/gdb.mi/mi-break.exp > +++ b/gdb/testsuite/gdb.mi/mi-break.exp > @@ -224,6 +224,19 @@ proc test_error {} { > mi_gdb_test "-break-insert -c i==4 \"callme if i < 4\"" \ > ".*\\^error,msg=\"Garbage 'if i < 4' at end of location\"" \ > "conditional breakpoint with garbage after location" > + > + # Try using an invalid condition. > + mi_gdb_test "-break-insert -c bad callme" \ > + ".*\\^error,msg=\"No symbol \\\\\"bad\\\\\" in current context.\"" \ > + "breakpoint with bad condition" > + > + mi_gdb_test "-dprintf-insert -c bad callme 123" \ > + ".*\\^error,msg=\"No symbol \\\\\"bad\\\\\" in current context.\"" \ > + "dprintf with bad condition" > + > + mi_gdb_test "-break-condition 5 bad" \ > + ".*\\^error,msg=\"No symbol \\\\\"bad\\\\\" in current context.\"" \ > + "invalid condition" Here, does the 5 refer to an existing breakpoint number created by another test function? It's a bit annoying when test functions rely on the state left by other test functions, that makes them harder to debug in isolation. In my ideal world, each test_* function in this file (except test_break, which is just the driver) would spawn a fresh GDB, such that if you need to debug one of them, you can comment out all the other ones. The patch is still OK though, and thanks for taking the time to write a test. Simon