From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by sourceware.org (Postfix) with ESMTPS id 08D9638930C9 for ; Mon, 29 Jun 2020 13:49:27 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 08D9638930C9 IronPort-SDR: 1dk6lbA2a4KDptr1++C0BcvNHm/+aga6jpF8i3okfafTF87AMV6oPeikukgX/fu9qWNMQGsEuW QuT9syrdiuiw== X-IronPort-AV: E=McAfee;i="6000,8403,9666"; a="147513309" X-IronPort-AV: E=Sophos;i="5.75,294,1589266800"; d="scan'208";a="147513309" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga003.jf.intel.com ([10.7.209.27]) by orsmga102.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 29 Jun 2020 06:49:26 -0700 IronPort-SDR: Co/9KY57VHlUsHwCJ8OLOswbv2ucy6IZY13yAeOa9p1JlkAD4QyWRlRankHi9zbhYzHQKdKTBO Rpw4SiiVaHiA== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.75,294,1589266800"; d="scan'208";a="277086030" Received: from irvmail001.ir.intel.com ([163.33.26.43]) by orsmga003.jf.intel.com with ESMTP; 29 Jun 2020 06:49:26 -0700 Received: from ulvlx001.iul.intel.com (ulvlx001.iul.intel.com [172.28.207.17]) by irvmail001.ir.intel.com (8.14.3/8.13.6/MailSET/Hub) with ESMTP id 05TDnPaX028066; Mon, 29 Jun 2020 14:49:25 +0100 Received: from ulvlx001.iul.intel.com (localhost [127.0.0.1]) by ulvlx001.iul.intel.com with ESMTP id 05TDnPFp025814; Mon, 29 Jun 2020 15:49:25 +0200 Received: (from taktemur@localhost) by ulvlx001.iul.intel.com with LOCAL id 05TDnP2V025810; Mon, 29 Jun 2020 15:49:25 +0200 From: Tankut Baris Aktemur To: gdb-patches@sourceware.org Subject: [PATCH 2/3] gdb/breakpoint: set the condition exp after parsing the condition successfully Date: Mon, 29 Jun 2020 15:48:27 +0200 Message-Id: X-Mailer: git-send-email 1.7.0.7 In-Reply-To: References: In-Reply-To: References: X-Spam-Status: No, score=-22.6 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_NONE, KAM_DMARC_STATUS, KAM_LAZY_DOMAIN_SECURITY, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, 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: Mon, 29 Jun 2020 13:49:30 -0000 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. */ + } + } if (from_tty) printf_filtered (_("Breakpoint %d now unconditional.\n"), b->number); @@ -872,9 +872,10 @@ set_breakpoint_condition (struct breakpoint *b, const char *exp, innermost_block_tracker tracker; arg = exp; - w->cond_exp = parse_exp_1 (&arg, 0, 0, 0, &tracker); + expression_up new_exp = parse_exp_1 (&arg, 0, 0, 0, &tracker); if (*arg) error (_("Junk at end of expression")); + w->cond_exp = std::move (new_exp); w->cond_exp_valid_block = tracker.block (); } else @@ -884,11 +885,12 @@ set_breakpoint_condition (struct breakpoint *b, const char *exp, for (loc = b->loc; loc; loc = loc->next) { arg = exp; - loc->cond = - parse_exp_1 (&arg, loc->address, - block_for_pc (loc->address), 0); + expression_up new_exp + = parse_exp_1 (&arg, loc->address, + block_for_pc (loc->address), 0); if (*arg) error (_("Junk at end of expression")); + loc->cond = std::move (new_exp); } } diff --git a/gdb/testsuite/gdb.base/condbreak-bad.exp b/gdb/testsuite/gdb.base/condbreak-bad.exp index a01ba2a9340..84d32a0f15d 100644 --- a/gdb/testsuite/gdb.base/condbreak-bad.exp +++ b/gdb/testsuite/gdb.base/condbreak-bad.exp @@ -38,3 +38,91 @@ gdb_test "info break" \ "${decimal}${fill}breakpoint${fill}keep y${fill}:${bp_location}"] \ "breakpoint is unconditional" +# Now define a valid condition. Attempt to override that with a 'bad' +# condition again. The condition should be preserved. +with_test_prefix "with run" { + gdb_test_no_output "cond $bpnum a == 10" + + gdb_test "cond $bpnum gibberish" \ + "No symbol \"gibberish\" in current context." \ + "attempt a bad condition" + + gdb_test "info break" \ + [multi_line \ + "Num${fill}What" \ + "${decimal}${fill}breakpoint${fill}keep y${fill}:${bp_location}" \ + "${fill}stop only if a == 10${fill}"] \ + "breakpoint condition is preserved" + + # Run the code. We should hit the breakpoint, because the + # condition evaluates to true. + + gdb_run_cmd + gdb_test "" ".*reakpoint .*, main .*${srcfile}.*" "run to the bp" +} + +# Restart. Repeat the test above after the program has started. +# This is needed to check a scenario where the breakpoints are no +# longer re-inserted due to solib events. Note that runto_main +# deletes the breakpoints. +with_test_prefix "with continue 1" { + if {![runto_main]} { + fail "could not run to main" + return -1 + } + + gdb_breakpoint "$bp_location" + set bpnum [get_integer_valueof "\$bpnum" 0 "get bpnum"] + + gdb_test_no_output "cond $bpnum a == 10" + + gdb_test "cond $bpnum gibberish" \ + "No symbol \"gibberish\" in current context." \ + "attempt a bad condition" + + # Resume. We should hit the breakpoint, because the + # condition evaluates to true. + gdb_continue_to_breakpoint "${srcfile}:${bp_location}" +} + +# Repeat with a condition that evaluates to false. +with_test_prefix "with continue 2" { + if {![runto_main]} { + fail "could not run to main" + return -1 + } + + gdb_breakpoint "$bp_location" + set bpnum [get_integer_valueof "\$bpnum" 0 "get bpnum"] + + gdb_test_no_output "cond $bpnum a == 999" + + gdb_test "cond $bpnum gibberish" \ + "No symbol \"gibberish\" in current context." \ + "attempt a bad condition" + + # Resume. We should *not* hit the breakpoint, because the + # condition evaluates to false. + gdb_continue_to_end +} + +# Repeat with a condition that contains junk at the end. +with_test_prefix "with junk" { + if {![runto_main]} { + fail "could not run to main" + return -1 + } + + gdb_breakpoint "$bp_location" + set bpnum [get_integer_valueof "\$bpnum" 0 "get bpnum"] + + gdb_test_no_output "cond $bpnum a == 999" + + gdb_test "cond $bpnum a == 10 if" \ + "Junk at end of expression" \ + "attempt a bad condition" + + # Resume. We should *not* hit the breakpoint, because the + # condition evaluates to false. + gdb_continue_to_end +} -- 2.17.1