Re: [patch 3/3] bpstat_what removal

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

On Tue, 04 May 2010 16:09:52 +0200, Stan Shebs wrote:
> Jan Kratochvil wrote:
> >Hi,
> >
> >the simple idea is to inline bpstat_what into handle_inferior_event.  This
> >removes enum bpstat_what_main_action and struct bpstat_what currently acting
> >just as an interface between these two functions.
> 
> There's a reason for this actually, which is that it helps keep the
> myriad of random breakpoint types from infecting the rest of GDB.

This way from existing 24 bp_* breakpoint types you create new artificial
9 BPSTAT_* types with associated 3 STOP_* types, therefore effectively you
create new artificial 11 BPSTAT/STOP_* breakpoint events.


> Breakpoint types are visible globally, and individual breakpoint
> types are mentioned here and there in the code, but I think it's
> worthwhile to keep the type enumerations / switches in breakpoint.c
> as much as possible.

I understand the idea.  If there would be 3 or 4 BPSTAT/STOP_* events I would
say it is worth it.  But 11 BPSTAT/STOP_* events I find as a too thick
interface to overweight the cost of a new artificial interface at all.

While this citation may be controversial I find it appropriate here:

% And my point is that multiple interfaces are BAD.
% 
% There is one interface we _have_ to have: the traditional
% [subject replaced>>>] bp_* types [<<<] one. That one we can't get away from.
% 
% "Multiple interfaces" on its own is just confusion with no upside.
% 
% You need a _reason_ to have other interfaces. They need to have that
% killer feature. Just being "different" is not a feature at all.
-- Linus Torvalds

Currently already the bp_* types references are not encapsulated in
breakpoint.[ch]:
	386 breakpoint.[ch]
	 51 other files
	= 12%

By inlining bpstat_what from breakpoint.c into infrun.c thus moving the 24
bp_* types references from the first line to the second line it gets worse: 18%

Still 12% to 18% is not any radical design change.

Moreover I do not notice in which file which function resides at least myself.
With ctags navigation (+vim :grep -rw) one jumps the references without
noticing any file boundaries.  I understand some cscope or some Emacs
navigation is even more seamless.


> There is a pretty good chance that we're going to be doing some
> refactoring on how breakpoint types are handled - people are
> interested in the idea of "tracing watchpoints" (or "watching
> tracepoints" :-) ) for instance - so there's a practical reason to
> provide interfaces that define the net effect of types, rather than
> requiring callers to know aracane details of each.

I see the opposite examples where new breakpoint types always require also new
infrun behavior:

off-trunk archer-jankratochvil-ifunc:
  bp_gnu_ifunc_resolver: Place new breakpoint at the return point.
  bp_gnu_ifunc_resolver_return: Move target breakpoint to the right address.

off-trunk archer-pmuldoon-next-over-throw2 [by tromey]:
  bp_exception: Place bp_exception_resume breakpoint.
  bp_exception_resume: Delete the breakpoint and check if we should stop.

These cases mean that each new bp_* type requires new BPSTAT/STOP_* event,
thus nullifying the artificial bpstat_what interface separation.


Still I can keep the BPSTAT_* interface, remove the STOP_* symbols, still
remove the state machine table and keep the bp_* -> BPSTAT_* conversion in
existing bpstat_what.  The state machine table has been the primary target of
this patchset.  The BPSTAT_* symbols should be still renamed to more
technically describe both their infrun effect and their bp_* types relation.


Thanks,
Jan