Re: [PATCH 1/4] 'catch syscall' feature -- Architecture-independent part

[Joel Brobecker <brobecker@adacore.com>]

Hi Sergio,

> If I understood correctly, you think it's better to create a "generic"
> bptype so that not only "catch syscall" can use it, right?

That's correct. The short answer is that, if we make your catchpoint
use a more generic type and base the actual implementation of
breakpoint_ops methods, then, later on, when someone decides to
implement a new kind of catchpoint with similar functionality,
then all he should have to do is create a new breakpoint_ops vector
with appropriate methods, and voila.

This is pretty much what happened when I implemented catchpoints
on Ada exception.  If you had a look at my patch, you'll notice
that I didn't have to sprinkle "case bp_my_new_kind" everywhere
to make it work.  To me, this makes me so much more confident
that I didn't miss an update somewhere. But my situation was simpler
than yours, however, because my catchpoints were barely more than just
an elaborated breakpoint.

The longer answer is that I have been wondering about making the
breakpoint structure a little more object-oriented. I have this
Vision (no mushrooms involved so far) where there are really two
types of catchpoints:
  (a): Those that are implemented using one or more breakpoints
  (b): Those that are implemented without the use of breakpoins
Either way, I would like to see these breakpoints as objects,
with methods that we would call:
  (1) insert: Insert the physical breakpoint, or activate the
      associated trace, or...
  (2) remove: The opposite of insert
  (3) is_mind: Return true if an event is recognized by the catchpoint
  (3) print_stop_action, print_one, print_mention.
      (the methods already provided by the breakpoint_ops)
  (4) any other method?
So, when the debugger needs to insert breakpoints, he doesn't have
to know how syscall catchpoints are implemented.  It just calls
the associated "insert" method. Same for removing.  When we receive
an event, we could rely on the "is_mine" method to determine that
we stopped due to this catchpoint, and thus call the associated
methods to tell the user about them. Etc.

The thing is, I haven't designed this part of the code, and I don't
know if the breakpoint_ops were meant to be used such that we could
make the core code completely abstract of which breakpoint was hit.
Nor am I certain that it is actually possible.  But it's definitely
something that I would like to try.

Now, going back to your patch, I think it's important not to let
best be the enemy of good either.  I think that the feature that
you are proposing is interesting and would like to make sure that
we don't lose your work.  I am about to go away on a trip, so will
have little time to review the details of your patch, but looks like
Michael has started on that. I would love for you to investigate
my (ahem) Vision (double-ahem), and I'm ready to guide you as much
as I can.  But I am equally happy to incorporate your approach
if another maintainer looks at your code and OKays it.

> The way I see, there's no problem to implement "catch syscall" using
> another way. However, and I think you already know that, the "catch
> fork", "catch vfork" and "catch exec" are implemented using an entry on
> "enum bptype" (and that's specific why I've chosen to do this). Do you
> think we should change this, too?

Not as part of your patch.  But in the long term, I would like to
migrate them to the breakpoint_ops approach if possible.  That's if
we have determined that it's doable, that is.

> I'm not sure if I understood this part correctly. Maybe my GDB-fu isn't
> good enought yet :-). I'll take a look at breakpoint_ops to see if I can
> understand better what you want.

Hopefully the explainations above might have clarified a little bit
the context. Let me know if there are parts that you'd like me to
expand more.

-- 
Joel