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

[Jan Kratochvil <jan dot kratochvil at redhat dot com>]

On Tue, 15 Jun 2010 17:08:19 +0200, Pedro Alves wrote:
> Do you think it would be hard to split this into smaller pieces?

Yes, I will do - just later.  IMO we should probably more talk about its goal
than about specifics of the implementation.


> I've tried to review this a couple
> of times already, but it looks tricky and easy to miss something.

I wrote some notes for it before, attached them for illustration.  I believe
one should not follow them when reviewing the patch - for obvious reasons.


>  - It should be possible to fix PR 9436 without

We can pretend PR 9436 does not exist.  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.


> In fact, it's not clear to me yet why is the new interface better.

The interface was better (=completely removed) in the original post:
	[patch 3/3] bpstat_what removal
	http://sourceware.org/ml/gdb-patches/2010-05/msg00050.html

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

"names and functionality exactly matching" - in the case of elements like
`ss_print_no' one can single-key jump on its definition to see
`ecs->event_thread->stop_step value 1.', naming the element
ecs_event_thread_stop_step_value_1 would be probably worse but I can change it
to the longer form if anyone would prefer it that way.

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

Moreover I find this patch only as a first step for further simplifications.
For instance at least ecs->event_thread->stop_step and stop_print_frame should
definitely be changed and also simplified.  This is not a part of this patch
(and I admit I currently do not a plan to do such next part soon).
Nullifying the layer makes it easier for futher simplifications downwards.


> To recap, IMO, the current problem with bpstat_main_action is that a few of
> its values aren't really independent and mutually exclusive
> with the others -- BPSTAT_WHAT_CHECK_SHLIBS and BPSTAT_WHAT_CHECK_JIT.

What if a new breakpoint type wants to stop?  What if a new breakpoint type
does not want to stop?  And how they combine if they happend together with
other events?  While there exists answer for it I do not know offhand.  I know
offhand with my implementation.  I should post a patch introducing new
breakpoint types in both variants of the bpstat_what implementation.


> How about fixing the PR that way, and adding the new testcase without all
> the revamping?  That'd be surely a step in the right direction.

The revamping was the goal.  I only found the PR to have some advocacy for its
acceptance but to be honest I would prefer to forget about the PR now at all.


>  - I feel that even getting rid of the table bpstat_what_main_action
> table

Yes, it has to be done.


> could be done without changing the interface between breakpoint.c
> and infrun.c (removing bpstat_main_action), and other way around too.

Changing the interface was also the goal - the goal of simplifying GDB.

So far I thought the patches should have the goal of making the patched code
the best we can (in this case to make it more simple).  If we track the goal
of a minimal patchset to reach some functionality we can completely drop this
patch as its primary goal is no new functionality but just a code
simplification (plus simplification of future code extensions).


> That is, it feels like we could tackle these changes independently.
> Not sure though.

Probably it can although I find such patch more difficult than this one.
I just mapped the effect of the current code and wrote a new code behaving
(hopefully) the same.


> Let me know what you think, and if you disagree, I'll try harder at
> reviewing this.

I believe we should try to find the shared goal of such patch first.



Thanks for reply!
Jan

------------------------------------------------------------------------------

bs->breakpoint_at == NULL                  no_effect    kc   BPSTAT_WHAT_KEEP_CHECKING
bp_none                                    no_effect    kc   BPSTAT_WHAT_KEEP_CHECKING
bp_watchpoint          && !stop            no_effect    kc   BPSTAT_WHAT_KEEP_CHECKING
bp_hardware_watchpoint && !stop            no_effect    kc   BPSTAT_WHAT_KEEP_CHECKING
bp_read_watchpoint     && !stop            no_effect    kc   BPSTAT_WHAT_KEEP_CHECKING
bp_access_watchpoint   && !stop            no_effect    kc   BPSTAT_WHAT_KEEP_CHECKING
bp_catchpoint          && !stop            no_effect    kc   BPSTAT_WHAT_KEEP_CHECKING
bp_watchpoint          && stop && !print   wp_silent    ss   BPSTAT_WHAT_STOP_SILENT
bp_hardware_watchpoint && stop && !print   wp_silent    ss   BPSTAT_WHAT_STOP_SILENT
bp_read_watchpoint     && stop && !print   wp_silent    ss   BPSTAT_WHAT_STOP_SILENT
bp_access_watchpoint   && stop && !print   wp_silent    ss   BPSTAT_WHAT_STOP_SILENT
bp_breakpoint          && stop && !print   bp_silent    ss   BPSTAT_WHAT_STOP_SILENT
bp_hardware_breakpoint && stop && !print   bp_silent    ss   BPSTAT_WHAT_STOP_SILENT
bp_until               && stop && !print   bp_silent    ss   BPSTAT_WHAT_STOP_SILENT
bp_finish              && stop && !print   bp_silent    ss   BPSTAT_WHAT_STOP_SILENT
bp_catchpoint          && stop && !print   bp_silent    ss   BPSTAT_WHAT_STOP_SILENT
bp_call_dummy                              bp_silent    ss   BPSTAT_WHAT_STOP_SILENT    STOP_STACK_DUMMY
bp_std_terminate                           bp_silent    ss   BPSTAT_WHAT_STOP_SILENT    STOP_STD_TERMINATE
bp_watchpoint          && stop && print    wp_noisy     sn   BPSTAT_WHAT_STOP_NOISY
bp_hardware_watchpoint && stop && print    wp_noisy     sn   BPSTAT_WHAT_STOP_NOISY
bp_read_watchpoint     && stop && print    wp_noisy     sn   BPSTAT_WHAT_STOP_NOISY
bp_access_watchpoint   && stop && print    wp_noisy     sn   BPSTAT_WHAT_STOP_NOISY
bp_breakpoint          && stop && print    bp_noisy     sn   BPSTAT_WHAT_STOP_NOISY
bp_hardware_breakpoint && stop && print    bp_noisy     sn   BPSTAT_WHAT_STOP_NOISY
bp_until               && stop && print    bp_noisy     sn   BPSTAT_WHAT_STOP_NOISY
bp_finish              && stop && print    bp_noisy     sn   BPSTAT_WHAT_STOP_NOISY
bp_catchpoint          && stop && print    bp_noisy     sn   BPSTAT_WHAT_STOP_NOISY
bs->breakpoint_at->owner == NULL           bp_nostop    sgl  BPSTAT_WHAT_SINGLE
bp_breakpoint          && !stop            bp_nostop    sgl  BPSTAT_WHAT_SINGLE
bp_hardware_breakpoint && !stop            bp_nostop    sgl  BPSTAT_WHAT_SINGLE
bp_until               && !stop            bp_nostop    sgl  BPSTAT_WHAT_SINGLE
bp_finish              && !stop            bp_nostop    sgl  BPSTAT_WHAT_SINGLE
bp_step_resume         && !stop            bp_nostop    sgl  BPSTAT_WHAT_SINGLE
bp_watchpoint_scope                        bp_nostop    sgl  BPSTAT_WHAT_SINGLE
bp_thread_event                            bp_nostop    sgl  BPSTAT_WHAT_SINGLE
bp_overlay_event                           bp_nostop    sgl  BPSTAT_WHAT_SINGLE
bp_longjmp_master                          bp_nostop    sgl  BPSTAT_WHAT_SINGLE
bp_std_terminate_master                    bp_nostop    sgl  BPSTAT_WHAT_SINGLE
bp_longjmp                                 long_jump    slr  BPSTAT_WHAT_SET_LONGJMP_RESUME
bp_longjmp_resume                          long_resume  clr  BPSTAT_WHAT_CLEAR_LONGJMP_RESUME
bp_step_resume         && stop             step_resume  sr   BPSTAT_WHAT_STEP_RESUME
bp_shlib_event                             shlib        shl  BPSTAT_WHAT_CHECK_SHLIBS
bp_jit_event                               jit_event    jit  BPSTAT_WHAT_CHECK_JIT
bp_tracepoint                              internal_error ("bpstat_what: tracepoint encountered")
bp_fast_tracepoint                         internal_error ("bpstat_what: tracepoint encountered")

err:
long_jump   && clr
long_resume && sgl
long_resume && slr
long_resume && clr


#define kc BPSTAT_WHAT_KEEP_CHECKING
	break;
#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 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 sr BPSTAT_WHAT_STEP_RESUME
	special
	if (special2)
		ecs->event_thread->stepping_over_breakpoint = 1;
		keep_going (ecs);
		return;
	else
		break;
#define shl BPSTAT_WHAT_CHECK_SHLIBS
	special
	if (stop_on_solib_events || is (STOP_STACK_DUMMY) || is (STOP_STD_TERMINATE))
		stop_stepping (ecs);
		return;
	else
		ecs->event_thread->stepping_over_breakpoint = 1;
		break;
#define jit BPSTAT_WHAT_CHECK_JIT
	special
	ecs->event_thread->stepping_over_breakpoint = 1;
	break;

BPSTAT_WHAT_LAST
#define err BPSTAT_WHAT_STOP_NOISY

default = force keep-going | check reasons why to stop? | always stop
default = stepping_over_breakpoint no | yes
ecs->event_thread->stop_step default=0 | 1 | 0
stop_print_frame default == 1 | 0 | 1

< BPSTAT_WHAT_KEEP_CHECKING: Check reasons why to stop otherwise keep-going.
	< BPSTAT_WHAT_SINGLE: Check reasons why to stop, otherwise force stepping over breakpoint.

< STEPPING_KEEP_GOING: Force stepping over breakpoint, suppressing checking reasons why to stop.

< STOP_STEP: Always stop (not printing the source line).
	< STOP_STEPPING: Stronger than STOP_STEP by printing the source line (stop_step == 0).

< BPSTAT_WHAT_STOP_SILENT: Always stop (stop_print_frame == 0).
	< BPSTAT_WHAT_STOP_NOISY: Stronger than BPSTAT_WHAT_STOP_SILENT by stop_print_frame == 1.
------------------------------------------------------------------------------
STOP_STEPPING
	stop_stepping (ecs);
	return;

STOP_STEP
	ecs->event_thread->stop_step = 1;
	stop_stepping (ecs);
	return;

STEPPING_KEEP_GOING
	ecs->event_thread->stepping_over_breakpoint = 1;
	keep_going (ecs);
	return;

#define kc BPSTAT_WHAT_KEEP_CHECKING
	break;

#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 sgl BPSTAT_WHAT_SINGLE
	ecs->event_thread->stepping_over_breakpoint = 1;
	break;
------------------------------------------------------------------------------

#define slr BPSTAT_WHAT_SET_LONGJMP_RESUME
	special
	STEPPING_KEEP_GOING
#define clr BPSTAT_WHAT_CLEAR_LONGJMP_RESUME
	special
	STOP_STEP
#define sr BPSTAT_WHAT_STEP_RESUME
	special
	if (special2)
		STEPPING_KEEP_GOING
	else
		BPSTAT_WHAT_KEEP_CHECKING
#define shl BPSTAT_WHAT_CHECK_SHLIBS
	special
	if (stop_on_solib_events || is (STOP_STACK_DUMMY) || is (STOP_STD_TERMINATE))
		STOP_STEPPING
	else
		BPSTAT_WHAT_SINGLE
#define jit BPSTAT_WHAT_CHECK_JIT
	special
	BPSTAT_WHAT_SINGLE



KEEP_CHECKING
STOP_SILENT
STOP_NOISY
SINGLE
SET_LONGJMP_RESUME
CLEAR_LONGJMP_RESUME
STEP_RESUME
CHECK_SHLIBS
CHECK_JIT