This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [patch 3/3] bpstat_what removal [rediff]


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]