[patch 3/3] bpstat_what removal [rediff]

[Jan Kratochvil]

On Wed, 16 Jun 2010 21:13:30 +0200, Pedro Alves wrote:
> On Tuesday 15 June 2010 22:54:02, Jan Kratochvil wrote:
> > I noted a wish of removing bpstat_what
> > already on #gdb 2008-12-31T22:51:10Z I believe not aware of this PR that time.
> > I believe the wish lasts longer than I have expressed it on #gdb.
> 
> The comment mentioning that the table is not really needed is there
> since 5.0.  That's like 10 years ago.  :-)

My plan was to remove the whole needless bpstat_what interface but that has
been rejected after my first post.  Anyway it is offtopic now.


> > Currently the interface was created with the goal of not introducing anything
> > new.  All its element have both their names and functionality exactly matching
> > the already existing (and currently required to stay in place) GDB data
> > structures.  They had to copy them to comply with the separation requirements:
> > 	Re: [patch 3/3] bpstat_what removal
> > 	http://sourceware.org/ml/gdb-patches/2010-05/msg00186.html
> 
> I don't see how that relates, sorry.

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;'.

But I understand a code readability improvement can be postponed into
different future patches.


> > Contrary to it what exactly does BPSTAT_WHAT_STEP_RESUME and how to combine it
> > with other events is unclear to me.
> >     /* Clear step resume breakpoint, and keep checking.  */
> 
> Yes, there's an issue with step-resume's priority over other events.
> step-resume takes priority over all other actions, even stops for other
> breakpoints.  It usually just works (breakpoint on top of step-resume
> breakpoint, for instance), because when you hit the step-resume, the step
> range _also_ ends, so we stop anyway.  I can see that the (infrun.c)
> optimization around:
> 
>       /* We are at the start of a different line.  So stop.  Note that
>          we don't stop if we step into the middle of a different line.
>          That is said to make things like for (;;) statements work
>          better.  */
> 
> can make us miss a breakpoint in some rare cases.  Acknowledging this
> doesn't mean we can't tackle one issue at a time.

==> 1.c <==
extern int f (void);

int
main (void)
{
  int i;

  i = f ();
  return 0;
}

==> 2.c <==
int
f (void)
{
  return 1;
}

gcc -Wall -c -o 2.o 2.c; gcc -o 1 1.c 2.o -Wall -g
# 2.o has no DWARF!
gdb -nx -ex 'set breakpoint always-inserted on' -ex 'b *0x400481' -ex start -ex 'set debug infrun 1' -ex step -ex disass -ex 'p/x $pc' 1

8	  i = f ();
   0x000000000040047c <+8>:	callq  0x40048c <f>
   0x0000000000400481 <+13>:	mov    %eax,-0x4(%rbp)
9	  return 0;
   0x0000000000400484 <+16>:	mov    $0x0,%eax

infrun: prepare_to_wait
infrun: target_wait (-1, status) =
infrun:   1622 [process 1622],
infrun:   status->kind = stopped, signal = SIGTRAP
infrun: infwait_normal_state
infrun: TARGET_WAITKIND_STOPPED
infrun: stop_pc = 0x400481
infrun: BPSTAT_WHAT_STEP_RESUME
infrun: stepping inside range [0x40047c-0x400484]
infrun: resume (step=1, signal=0), trap_expected=0
infrun: prepare_to_wait
infrun: target_wait (-1, status) =
infrun:   1622 [process 1622],
infrun:   status->kind = stopped, signal = SIGTRAP
infrun: infwait_normal_state
infrun: TARGET_WAITKIND_STOPPED
infrun: stop_pc = 0x400481
infrun: BPSTAT_WHAT_STOP_NOISY
infrun: stop_stepping

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.

My patch does:

infrun: prepare_to_wait
infrun: target_wait (-1, status) =
infrun:   4037 [process 4037],
infrun:   status->kind = stopped, signal = SIGTRAP
infrun: infwait_normal_state
infrun: TARGET_WAITKIND_STOPPED
infrun: stop_pc = 0x400481
infrun: bp_breakpoint: print_frame pf_yes
infrun: bp_breakpoint: perform pe_stop
infrun: bp_step_resume: perform pe_check_more
infrun: bp_step_resume: bp_step_resume_on_stop yes
infrun: summary: print_frame pf_yes
infrun: summary: stop_step ss_default (ss_print_yes (stop_step 0))
infrun: summary: perform pe_stop
infrun: summary: stepping_over_breakpoint no
infrun: summary: bp_longjmp no
infrun: summary: bp_longjmp_resume no
infrun: summary: bp_step_resume_on_stop yes
infrun: stop_stepping

And therefore it does not invoke additional needless inferior resume there.

I was trying yesterday to find a user-visible reproducer (not just a `set
debug' output defect) but I have not found one soon enough and I have to move
on other work so just posting this mail.  I still believe in some cases it may
be user visible or will be with the planned new gnu-ifunc, python and other
breakpoint types.


> > What if a new breakpoint type does not want to stop?  
> 
> BPSTAT_WHAT_SINGLE

I do not find this easy to find out.


> > And how they combine if they happend together with
> > other events? 
> 
> The highest priority of the above wins.

You describe your patch here.  Based on it I made an ordered list:

#define kc BPSTAT_WHAT_KEEP_CHECKING
	break;
#define sgl BPSTAT_WHAT_SINGLE
	ecs->event_thread->stepping_over_breakpoint = 1;
	break;
#define slr BPSTAT_WHAT_SET_LONGJMP_RESUME
	special
	ecs->event_thread->stepping_over_breakpoint = 1;
	keep_going (ecs);
	return;
#define clr BPSTAT_WHAT_CLEAR_LONGJMP_RESUME
	special
	ecs->event_thread->stop_step = 1;
	stop_stepping (ecs);
	return;
#define ss BPSTAT_WHAT_STOP_SILENT
	stop_print_frame = 0;
	stop_stepping (ecs);
	return;
#define sn BPSTAT_WHAT_STOP_NOISY
	stop_print_frame = 1;
	stop_stepping (ecs);
	return;
#define sr BPSTAT_WHAT_STEP_RESUME
	special
	if (special2)
		ecs->event_thread->stepping_over_breakpoint = 1;
		keep_going (ecs);
		return;
	else
		break;

We do not want "highest priority of the above wins" but we want "higher
priority is a superset of actions of any lower priority".  This has been
achieved in my patch according to the `set debug'-class reproducer above.


> If you're going to stop, you'll never want to set longjmp or step resume
> breakpoints, or start a step over.  You're just going to stop, period.

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.


> (My feeling is that we should completely move the infrun-specific
> breakpoints handling to infrun (longjmp, longjmp-resume, step-resume),
> though it isn't clear _how_ is best.  I hope that by eliminating
> the table first, we can think of better interfaces afterwards.
> We may even end up with something like yours, I'm not rejecting
> it upfront, to be clear.)

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.


> > >  - I feel that even getting rid of the table bpstat_what_main_action
> > > table
> > 
> > Yes, it has to be done.
> 
> It doesn't _have_ to.  There's nothing wrong with it, if you look at
> it as a state machine (and once you remove the states that should never
> have been put there in the first place).

To make it correct any superset of actions of any element of the enum set
would have to be represented by an element of that enum set.  The enum set
actions would have to be closed for the operation of unification.


> I don't find it hard to read, but you do
> have to think a bit about it to understand it the first time;

Yes, I am mostly used to BPSTAT_WHAT_* now after spending many days with it.
This only proves it must be removed one day to get on par with the
contribution rate of Linux kernel.


> Well, I agree with all that, but it just doesn't look simpler
> to me at first look.  :-)  That's my main worry.

I believe nullifying this layer will make it easier to simplify other parts of
the code this code controls.  That is to simplify code currently controlled by
`ecs->event_thread->stop_step', `stop_print_frame',
ecs->event_thread->stepping_over_breakpoint' (this one is the only one
possibly clear enough) and others.  But I have not even tried to simplify this
part so I may have false predictions on the goals of my bpstat_what rework.
Primarily this future simplification goal was heavily hit by the
re-introduction of bpstat_what interface as requested after my first post.


> > 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.  I was going to try some BPSTAT_WHAT_* patch while keeping it
regression free but when thinking about it now it probably would not be
possible anyway.

Your patch probably (I would have to verify this part first) does not have
a regression against FSF GDB HEAD and therefore it is a good step.


> 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.

I would prefer to remove it from this your patch.


> so it could even be eliminated if we're okay with adding a
> breakpoint_inserted_here call somewhere else; if it stays, it's really
> a property of the breakpoint location, not the breakpoint, it seems.),

This seems as a nice simplifications but it is outside of (my intended) scope
of this patch.



Thanks,
Jan