From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 12883 invoked by alias); 18 Jun 2010 11:42:46 -0000 Received: (qmail 12874 invoked by uid 22791); 18 Jun 2010 11:42:45 -0000 X-SWARE-Spam-Status: No, hits=-1.4 required=5.0 tests=AWL,BAYES_05,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (38.113.113.100) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 18 Jun 2010 11:42:40 +0000 Received: (qmail 6902 invoked from network); 18 Jun 2010 11:42:39 -0000 Received: from unknown (HELO orlando.localnet) (pedro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 18 Jun 2010 11:42:39 -0000 From: Pedro Alves To: Jan Kratochvil Subject: Re: [patch 3/3] bpstat_what removal [rediff] Date: Fri, 18 Jun 2010 11:42:00 -0000 User-Agent: KMail/1.13.2 (Linux/2.6.32-22-generic; KDE/4.4.2; x86_64; ; ) Cc: gdb-patches@sourceware.org, Stan Shebs References: <20100503200217.GD30386@host0.dyn.jankratochvil.net> <201006162013.31064.pedro@codesourcery.com> <20100618104129.GA11850@host0.dyn.jankratochvil.net> In-Reply-To: <20100618104129.GA11850@host0.dyn.jankratochvil.net> MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Message-Id: <201006181242.30972.pedro@codesourcery.com> X-IsSubscribed: yes 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 X-SW-Source: 2010-06/txt/msg00404.txt.bz2 On Friday 18 June 2010 11:41:29, Jan Kratochvil wrote: > > I do not understand offhand what does exactly mean BPSTAT_WHAT_SINGLE. > While > in my proposed interface I find `this.stepping_over_breakpoint = 1;' is much > easier to understand it will probably set > `ecs->event_thread->stepping_over_breakpoint = 1;'. That change will need to be well thought out. That's a behaviour change, and there are tricky code paths in infrun that want to rehit a breakpoint, as the one you've shown. Let me reiterate --- I'm not against your patch; I found it hard to review because a change like that (and others) has subtle handle_inferior_event behaviour changes, that I'd prefer having that isolated from the bpstat_what internals rework to not use the table. (I don't think these behaviour changes and how they were correct and desirable were mentioned or explained in either the patch descriptions, or in the code itself.) > That is it would be more correct to stop using BPSTAT_WHAT_STOP_NOISY > immediately without the last resume() call. If we stop at 0x400481 and > a breakpoint is placed there we can stop and print it. OTOH if the breakpoint > is there left placed in the inferior GDB will stop again on it anyway so it > has no user visible effect. Yeah, I realised that shortly after my last post. There are code paths in handle_inferior_event that _want_ to resume until a breakpoint is rehit, related to (nested) signals. Not sure they apply in this case, I/We'll need to dig further. (another reason for wanting to have that change separated). > My patch does: (...) > But former BPSTAT_WHAT_STEP_RESUME and BPSTAT_WHAT_SET_LONGJMP_RESUME > (therefore those using `keep_going (ecs); return;' make a mess there as they > cancel lower priorities wanting to stop. Well, not cancel, but postpone. I agree that this can be fixed/simplified, and is yet another instance of "moving solib out of BPSTAT_WHAT_... makes it easier to see what we need to do". I have trouble thinking how you'd have a simultaneous BPSTAT_WHAT_STEP_RESUME along with an solib or jit event, but, let's put thread event breakpoints and other kind of event breakpoints we'll come up with, or even gdb side tracepoints in the mix. All these breakpoints have the property that you do want to avoid considering rehits introduced by these "spurious" resumes as separate hits. Easier to think about if you consider tracepoints (you'd have double, or more collects for the same tracepoint hit), and that does indeed suggest something needs to change. > In general when I forget about my patch I find your solution definitely as an > improvement over current FSF GDB HEAD state. I still would like to review it > more, just hand waving it is correct in this mail. To be clear, my (well, our) patch was an attempt at splitting your patch in two (well, three, the solib-event patch split didn't happen yet) --- one that gets the table out of the way, and another that reworks the bpstat_what interface. The latter would conceivably be somewhat like the interdiff between your and my patch. > > > Probably it can although I find such patch more difficult than this one. > > > > I'll prove you wrong. :-) > > Your patch has a regression against my patch therefore I do not find it > proven. ?? What I was "proving" was that splitting from the larger patch a patch that just removes the bpstat table without changing gdb's behaviour in any way, was both possible, and actually simple, not that is was superior to your patch. The goal was getting the table our of the way, which is supposedly a non-behaviour (almost mechanical actually) change, so we can concentrate on the interface and infrun issues. > > You'll notice that this bpstat_what version is even a bit more simplified > > than yours, because it centralizes BPSTAT_WHAT_SINGLE handling (it's really > > a property of whether there's a breakpoint at PC when we keep going, > > This violates the goal of my patch to make its reviewing easier by not > changing the behavior in any way for the cases only a single event happens. (while making it hard to review because it changes the behavior when multiple breakpoints happen :-) ) > I would prefer to remove it from this your patch. That was supposedly not changing gdb's behaviour in _any_ way, either when single or multiple simultaneous events happen, but I'll agree to split that bit out. Baby steps! -- Pedro Alves