From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 5868 invoked by alias); 3 Jun 2014 13:35:44 -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 5855 invoked by uid 89); 3 Jun 2014 13:35:43 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.6 required=5.0 tests=AWL,BAYES_00 autolearn=ham version=3.3.2 X-HELO: rock.gnat.com Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-SHA encrypted) ESMTPS; Tue, 03 Jun 2014 13:35:41 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 11E30116140; Tue, 3 Jun 2014 09:35:39 -0400 (EDT) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id 8w31yOUzrzSW; Tue, 3 Jun 2014 09:35:38 -0400 (EDT) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id D7C4B116124; Tue, 3 Jun 2014 09:35:38 -0400 (EDT) Received: by joel.gnat.com (Postfix, from userid 1000) id 01CC440E72; Tue, 3 Jun 2014 06:35:39 -0700 (PDT) Date: Tue, 03 Jun 2014 13:35:00 -0000 From: Joel Brobecker To: Pedro Alves Cc: gdb-patches@sourceware.org Subject: Re: [RFA/7.8] user breakpoint not inserted if software-single-step at same location Message-ID: <20140603133539.GM4289@adacore.com> 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> <538D05CC.8050608@redhat.com> <538D85A9.5010004@redhat.com> <538DC98E.9050004@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <538DC98E.9050004@redhat.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-SW-Source: 2014-06/txt/msg00083.txt.bz2 Hi Pedro, > > Bah, I woke up realizing that the version I posted forgets to > > clone the shadow buffer! Let me fix that and repost... You are producing patches so fast, I am wondering if I will be able to keep up! :-) > 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. > (bp_target_info_copy_insertion_state): New function. > (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. > (find_single_step_breakpoint): New function, extracted from > single_step_breakpoint_inserted_here_p. > (find_single_step_breakpoint): New function, > factored 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. > * gdb.base/sss-bp-on-user-bp-2.exp: Remove kfail. You are making it us realize that the problem is more and more complex than we thought! :-(. And I think we'll need a small adjustment to your patch in order to account for something that may have been missed. See below: > @@ -15138,12 +15196,30 @@ 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)) > + { > + bp_target_info_copy_insertion_state (bp_tgt, &bl->target_info); > + return bp_tgt; > + } > + ISTM that you are assuming that there would only be one other breakpoint inserted at this location. What if there were more? If I am right, I suggest the addition of an extra parameter to find_non_raw_software_breakpoint_inserted_here which would be a pointer to a filtering function. If NULL, no filtering is done, but if not NULL, the filter function must accept the bp_location for find_non_raw_software_breakpoint_inserted_here to return it. > 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; Same here, I think. -- Joel