From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 2910 invoked by alias); 2 Jun 2014 23:16:38 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 2894 invoked by uid 89); 2 Jun 2014 23:16:37 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.2 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 02 Jun 2014 23:16:34 +0000 Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s52NGU6n011840 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Mon, 2 Jun 2014 19:16:30 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx12.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id s52NGSTB012055; Mon, 2 Jun 2014 19:16:29 -0400 Message-ID: <538D05CC.8050608@redhat.com> Date: Mon, 02 Jun 2014 23:16:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.5.0 MIME-Version: 1.0 To: Joel Brobecker CC: gdb-patches@sourceware.org Subject: Re: [RFA/7.8] user breakpoint not inserted if software-single-step at same location References: <1401394280-14999-1-git-send-email-brobecker@adacore.com> <5387BFF0.6010208@redhat.com> <20140530122253.GC4289@adacore.com> <53887ED5.5050603@redhat.com> <20140530132659.GD4289@adacore.com> <20140530193549.GF4289@adacore.com> In-Reply-To: <20140530193549.GF4289@adacore.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-SW-Source: 2014-06/txt/msg00052.txt.bz2 On 05/30/2014 08:35 PM, Joel Brobecker wrote: > Hi Pedro, > >>> For 7.8, I'm thinking it's really safer to avoid resending >>> duplicate Z0 packets to stubs that don't support conditional >>> breakpoint evaluation on the target end. So I think we should >>> handle the "insert" path too. >> >> OK - I will take care of that. > > New patch attached... > > gdb/ChangeLog: > > PR breakpoints/17000 > * breakpoint.c (non_sss_software_breakpoint_inserted_here_p): > New function, extracted from software_breakpoint_inserted_here_p. > (software_breakpoint_inserted_here_p): Remove factored out code > by call to non_sss_software_breakpoint_inserted_here_p. > (insert_single_step_breakpoint): Do nothing if the target > does not support target-side breakpoint condition evaluation, > a a non-software- single-step breakpoint was already inserted > at the same address. > (remove_single_step_breakpoints): Adjust to take into account > the fact that the first software single-step may not have been > inserted. Do not remove the raw breakpoint is a user software > breakpoint is still inserted at the same location. > > Tested on ppc-aix with AdaCore's testsuite. Tested on x86_64-linux > with the official testsuite. Also tested on x86_64-linux through > Pedro's branch enabling software single-stepping on that platform. > > Does it look better to you? Hmm, this doesn't handle the background execution cases we discussed earlier. That is, with: > @@ -15149,6 +15162,20 @@ insert_single_step_breakpoint (struct gdbarch *gdbarch, > { > void **bpt_p; > > + /* Unless the target supports target-side evaluation of breakpoint > + conditions, there is no need to insert an additional raw breakpoint > + if a non-software-single-step breakpoint is already inserted > + at that location. > + > + For targets supporting target-side evaluation of breakpoint > + conditions, the issue is that those breakpoints cannot be > + relied on to trigger during our software-single-step operation. > + So we have to insert our own to make sure. */ > + > + if (!target_supports_evaluation_of_breakpoint_conditions () > + && non_sss_software_breakpoint_inserted_here_p (aspace, next_pc)) > + return; ... when we hit this early return, the "b *0xaddr_where_sss_will_be_placed; s&; del" case may well result in removing the sss breakpoint from the target before the "step" actually finishes. So I think we should still create a bp_target_info for the single-step breakpoint, even though we don't actually insert it. I think it'd then be better if we do this in deprecated_insert_raw_breakpoint/deprecated_remove_raw_breakpoint instead, and to handle the mirror scenario you identified ("when the user deletes his breakpoints, since the breakpoint chain doesn't know about the SSS breakpoint, wouldn't it remove our SSS breakpoint insn?") in bkpt_insert_location/bkpt_remove_location. Note that with that early return as it is in the patch, single_step_breakpoints_inserted returns the wrong thing: int single_step_breakpoints_inserted (void) { return (single_step_breakpoints[0] != NULL || single_step_breakpoints[1] != NULL); } E.g., assume we only have inserted one sss breakpoint, and it happened to be on top of another breakpoint. Then single_step_breakpoints_inserted returns false, which confuses the callers. > One thing that we might have to consider > is the fact that z0 and Z0 packets are no longer balanced; not sure > if it would matter to any stubb in practice. I think we're into > grey waters anyway... About unbalanced: you're seeing that against gdbserver, right? If so, that's OK. However, we shouldn't see unbalanced Z0/z0 with "set remote conditional-breakpoints-packet off". If we do, that'd better be addressed. See also . > @@ -15189,22 +15216,28 @@ single_step_breakpoints_inserted (void) > void > remove_single_step_breakpoints (void) > { > - gdb_assert (single_step_breakpoints[0] != NULL); I think it's best if this assertion is preserved. > - > - /* See insert_single_step_breakpoint for more about this deprecated > - call. */ > - deprecated_remove_raw_breakpoint (single_step_gdbarch[0], > - single_step_breakpoints[0]); > - single_step_gdbarch[0] = NULL; > - single_step_breakpoints[0] = NULL; > + int i; > > - if (single_step_breakpoints[1] != NULL) > - { > - deprecated_remove_raw_breakpoint (single_step_gdbarch[1], > - single_step_breakpoints[1]); > - single_step_gdbarch[1] = NULL; > - single_step_breakpoints[1] = NULL; > - } > + for (i = 0; i < 2; i++) > + if (single_step_breakpoints[i] != NULL) > + { > + struct bp_target_info *bp_tgt = single_step_breakpoints[i]; > + CORE_ADDR pc = bp_tgt->placed_address; > + struct address_space *aspace = bp_tgt->placed_address_space; > + > + /* Only remove the raw breakpoint if there are no other > + non-software-single-step breakpoints still inserted > + at this location. Otherwise, we would be effectively > + disabling those breakpoints. */ > + if (!non_sss_software_breakpoint_inserted_here_p (aspace, pc)) > + { > + /* See insert_single_step_breakpoint for more about > + this deprecated call. */ > + deprecated_remove_raw_breakpoint (single_step_gdbarch[i], bp_tgt); > + } there should be an "else xfree" here, otherwise it's a leak. But, the patch below ends up not touching this function though. Let me know what you think, and feel free to push if it looks OK. Tested on all combinations of native|gdbserver X hardware-|software- stepping. At least, I think I did. If not this exact version, some other minor variation. :-) 8<-------- Subject: [PATCH] User breakpoint ignored if software-single-step at same location with the following code... 12 Nested; -- break #1 13 return I; -- break #2 14 end; (line 12 is a call to function Nested) ... we have noticed the following errorneous behavior on ppc-aix, where, after having inserted a breakpoint at line 12 and line 13, and continuing from the breakpoint at line 12, the program never stops at line 13, running away until the program terminates: % gdb -q func (gdb) b func.adb:12 Breakpoint 1 at 0x10000a24: file func.adb, line 12. (gdb) b func.adb:13 Breakpoint 2 at 0x10000a28: file func.adb, line 13. (gdb) run Starting program: /[...]/func Breakpoint 1, func () at func.adb:12 12 Nested; -- break #1 (gdb) c Continuing. [Inferior 1 (process 4128872) exited with code 02] When resuming from the first breakpoint, GDB first tries to step out of that first breakpoint. We rely on software single-stepping on this platform, and it just so happens that the address of the first software single-step breakpoint is the same as the user's breakpoint #2 (0x10000a28). So, with infrun and target traces turned on (but uninteresting traces snip'ed off), the "continue" operation looks like this: (gdb) c ### First, we insert the user breakpoints (the second one is an internal ### breakpoint on __pthread_init). The first user breakpoint is not ### inserted as we need to step out of it first. target_insert_breakpoint (0x0000000010000a28, xxx) = 0 target_insert_breakpoint (0x00000000d03f3800, xxx) = 0 ### Then we proceed with the step-out-of-breakpoint... infrun: resume (step=1, signal=GDB_SIGNAL_0), trap_expected=1, current thread [process 15335610] at 0x10000a24 ### That's when we insert the SSS breakpoints... target_insert_breakpoint (0x0000000010000a28, xxx) = 0 target_insert_breakpoint (0x00000000100009ac, xxx) = 0 ### ... then let the inferior resume... target_resume (15335610, continue, 0) infrun: wait_for_inferior () target_wait (-1, status, options={}) = 15335610, status->kind = stopped, signal = GDB_SIGNAL_TRAP infrun: target_wait (-1, status) = infrun: 15335610 [process 15335610], infrun: status->kind = stopped, signal = GDB_SIGNAL_TRAP infrun: infwait_normal_state infrun: TARGET_WAITKIND_STOPPED infrun: stop_pc = 0x100009ac ### At this point, we stopped at the second SSS breakpoint... target_stopped_by_watchpoint () = 0 ### We remove the SSS breakpoints... target_remove_breakpoint (0x0000000010000a28, xxx) = 0 target_remove_breakpoint (0x00000000100009ac, xxx) = 0 target_stopped_by_watchpoint () = 0 ### We find that we're not done, so we resume.... infrun: no stepping, continue ### And thus insert the user breakpoints again, except we're not ### inserting the second breakpoint?!? target_insert_breakpoint (0x0000000010000a24, xxx) = 0 infrun: resume (step=0, signal=GDB_SIGNAL_0), trap_expected=0, current thread [process 15335610] at 0x100009ac target_resume (-1, continue, 0) infrun: prepare_to_wait target_wait (-1, status, options={}) = 15335610, status->kind = exited, status = 2 What happens is that the removal of the software single-step breakpoints effectively removed the breakpoint instruction from inferior memory. But because such breakpoints are inserted directly as raw breakpoints rather than through the normal chain of breakpoints, we fail to notice that one of the user breakpoints points to the same address and that this user breakpoint is therefore effectively un-inserted. When resuming after the single-step, GDB thinks that the user breakpoint is still inserted and therefore does not need to insert it again. This patch teaches the insert and remove routines of both regular and raw breakpoints to be aware of each other. Special care needs to be applied in case the target supports evaluation of breakpoint conditions or commands. gdb/ChangeLog: PR breakpoints/17000 * breakpoint.c (find_non_raw_software_breakpoint_inserted_here): New function, extracted from software_breakpoint_inserted_here_p. (software_breakpoint_inserted_here_p): Replace factored out code by call to find_non_raw_software_breakpoint_inserted_here. (bkpt_insert_location): Handle the case of a single-step breakpoint already inserted at the same address. (bkpt_remove_location): Handle the case of a single-step breakpoint still inserted at the same address. (deprecated_insert_raw_breakpoint): Handle the case of non-raw breakpoint already inserted at the same address. (deprecated_remove_raw_breakpoint): Handle the case of a non-raw breakpoint still inserted at the same address. (single_step_breakpoint_inserted_here_p): (find_single_step_breakpoint_inserted_here): New function, fatored out from single_step_breakpoint_inserted_here_p. (single_step_breakpoint_inserted_here_p): Reimplement. gdb/testsuite/ChangeLog: PR breakpoints/17000 * gdb.base/sss-bp-on-user-bp.exp: Remove kfail. Tested on ppc-aix with AdaCore's testsuite. Tested on x86_64-linux, (native and gdbserver) with the official testsuite. Also tested on x86_64-linux through Pedro's branch enabling software single-stepping on that platform (native and gdbserver). --- gdb/breakpoint.c | 91 +++++++++++++++++++++++++--- gdb/testsuite/gdb.base/sss-bp-on-user-bp.exp | 3 +- 2 files changed, 82 insertions(+), 12 deletions(-) diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index 4e6c627..0043b06 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -4165,12 +4165,12 @@ breakpoint_inserted_here_p (struct address_space *aspace, CORE_ADDR pc) return 0; } -/* This function returns non-zero iff there is a software breakpoint - inserted at PC. */ +/* Ignoring deprecated raw breakpoints, return non-zero iff there is a + software breakpoint inserted at PC. */ -int -software_breakpoint_inserted_here_p (struct address_space *aspace, - CORE_ADDR pc) +static struct bp_location * +find_non_raw_software_breakpoint_inserted_here (struct address_space *aspace, + CORE_ADDR pc) { struct bp_location *bl, **blp_tmp; @@ -4188,10 +4188,23 @@ software_breakpoint_inserted_here_p (struct address_space *aspace, && !section_is_mapped (bl->section)) continue; /* unmapped overlay -- can't be a match */ else - return 1; + return bl; } } + return NULL; +} + +/* This function returns non-zero iff there is a software breakpoint + inserted at PC. */ + +int +software_breakpoint_inserted_here_p (struct address_space *aspace, + CORE_ADDR pc) +{ + if (find_non_raw_software_breakpoint_inserted_here (aspace, pc) != NULL) + return 1; + /* Also check for software single-step breakpoints. */ if (single_step_breakpoint_inserted_here_p (aspace, pc)) return 1; @@ -13119,8 +13132,18 @@ bkpt_insert_location (struct bp_location *bl) return target_insert_hw_breakpoint (bl->gdbarch, &bl->target_info); else - return target_insert_breakpoint (bl->gdbarch, - &bl->target_info); + { + struct bp_target_info *bp_tgt = &bl->target_info; + int ret; + + /* There is no need to insert a breakpoint if an unconditional + raw/sss breakpoint is already inserted at that location. */ + if (single_step_breakpoint_inserted_here_p (bp_tgt->placed_address_space, + bp_tgt->placed_address)) + return 0; + + return target_insert_breakpoint (bl->gdbarch, bp_tgt); + } } static int @@ -13129,7 +13152,19 @@ bkpt_remove_location (struct bp_location *bl) if (bl->loc_type == bp_loc_hardware_breakpoint) return target_remove_hw_breakpoint (bl->gdbarch, &bl->target_info); else - return target_remove_breakpoint (bl->gdbarch, &bl->target_info); + { + struct bp_target_info *bp_tgt = &bl->target_info; + struct address_space *aspace = bp_tgt->placed_address_space; + CORE_ADDR address = bp_tgt->placed_address; + + /* Only remove the breakpoint if there is no raw/sss breakpoint + still inserted at this location. Otherwise, we would be + effectively disabling the raw/sss breakpoint. */ + if (single_step_breakpoint_inserted_here_p (aspace, address)) + return 0; + + return target_remove_breakpoint (bl->gdbarch, bp_tgt); + } } static int @@ -15138,12 +15173,27 @@ deprecated_insert_raw_breakpoint (struct gdbarch *gdbarch, struct address_space *aspace, CORE_ADDR pc) { struct bp_target_info *bp_tgt; + struct bp_location *bl; bp_tgt = XCNEW (struct bp_target_info); bp_tgt->placed_address_space = aspace; bp_tgt->placed_address = pc; + /* If an unconditional non-raw breakpoint is already inserted at + that location, there's no need to insert another. However, with + target-side evaluation of breakpoint conditions, if the + breakpoint that is currently inserted on the target is + conditional, we need to make it unconditional. Note that a + breakpoint with target-side commands is not reported even if + unconditional, so we need to remove the commands from the target + as well. */ + bl = find_non_raw_software_breakpoint_inserted_here (aspace, pc); + if (bl != NULL + && VEC_empty (agent_expr_p, bl->target_info.conditions) + && VEC_empty (agent_expr_p, bl->target_info.tcommands)) + return bp_tgt; + if (target_insert_breakpoint (gdbarch, bp_tgt) != 0) { /* Could not insert the breakpoint. */ @@ -15161,9 +15211,30 @@ int deprecated_remove_raw_breakpoint (struct gdbarch *gdbarch, void *bp) { struct bp_target_info *bp_tgt = bp; + struct address_space *aspace = bp_tgt->placed_address_space; + CORE_ADDR address = bp_tgt->placed_address; + struct bp_location *bl; int ret; - ret = target_remove_breakpoint (gdbarch, bp_tgt); + bl = find_non_raw_software_breakpoint_inserted_here (aspace, address); + + /* Only remove the raw breakpoint if there are no other non-raw + breakpoints still inserted at this location. Otherwise, we would + be effectively disabling those breakpoints. */ + if (bl == NULL) + ret = target_remove_breakpoint (gdbarch, bp_tgt); + else if (!VEC_empty (agent_expr_p, bl->target_info.conditions) + || !VEC_empty (agent_expr_p, bl->target_info.tcommands)) + { + /* The target is evaluating conditions, and when we inserted the + software single-step breakpoint, we had made the breakpoint + unconditional and command-less on the target side. Reinsert + to restore the conditions/commands. */ + ret = target_insert_breakpoint (bl->gdbarch, &bl->target_info); + } + else + ret = 0; + xfree (bp_tgt); return ret; diff --git a/gdb/testsuite/gdb.base/sss-bp-on-user-bp.exp b/gdb/testsuite/gdb.base/sss-bp-on-user-bp.exp index 62415e7..2a12ad6 100644 --- a/gdb/testsuite/gdb.base/sss-bp-on-user-bp.exp +++ b/gdb/testsuite/gdb.base/sss-bp-on-user-bp.exp @@ -47,6 +47,5 @@ gdb_test "si" "Breakpoint .* bar break .*" # If the breakpoint is still correctly inserted, then this jump should # re-trigger it. Otherwise, GDB will lose control and the program -# will exit. -setup_kfail "breakpoints/17000" "*-*-*" +# will exit. See PR breakpoints/17000. gdb_test "jump *\$pc" "Breakpoint .* bar break .*" -- 1.9.0