minimalistic MI catch support

minimalistic MI catch support

[Markus Schiltknecht]

[-- Attachment #1: Type: text/plain, Size: 565 bytes --]

Hello gdb hackers,

working on an automated debugging layer on top of gdb I figured the
catchpoint functions in the MI interface is missing. The attached patch
against current CVS at least makes gdb report a catchpoint-break via MI.
This is already sufficient for my application. However, I would
appreciate a complete catchpoint implementation for the MI interface. I
haven't figured out how to implement the '-break-catch' command. If
somebody more knowledgable could do that I'd be thankfull.

Please CC me in responses, as I'm not subscribed.

Regards

Markus


[-- Attachment #2: mi-catch.diff --]
[-- Type: text/x-patch, Size: 1893 bytes --]

Index: breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.220
diff -u -r1.220 breakpoint.c
--- breakpoint.c	16 Jan 2006 12:55:18 -0000	1.220
+++ breakpoint.c	28 Jan 2006 12:52:59 -0000
@@ -2167,17 +2167,41 @@
 
     case bp_catch_fork:
       annotate_catchpoint (bs->breakpoint_at->number);
-      printf_filtered (_("\nCatchpoint %d (forked process %d), "),
-		       bs->breakpoint_at->number, 
-		       bs->breakpoint_at->forked_inferior_pid);
+      if (ui_out_is_mi_like_p (uiout))
+      {
+	ui_out_text (uiout, "\nBreakpoint ");
+	ui_out_field_string (uiout, "reason", "fork");
+	ui_out_field_int (uiout, "bkptno", bs->breakpoint_at->number);
+	ui_out_field_int (uiout, "forked-process",
+			  bs->breakpoint_at->forked_inferior_pid);
+	ui_out_text (uiout, ", ");
+      }
+      else
+      {
+	printf_filtered (_("\nCatchpoint %d (forked process %d), "),
+			 bs->breakpoint_at->number, 
+			 bs->breakpoint_at->forked_inferior_pid);
+      }
       return PRINT_SRC_AND_LOC;
       break;
 
     case bp_catch_vfork:
       annotate_catchpoint (bs->breakpoint_at->number);
-      printf_filtered (_("\nCatchpoint %d (vforked process %d), "),
-		       bs->breakpoint_at->number, 
-		       bs->breakpoint_at->forked_inferior_pid);
+      if (ui_out_is_mi_like_p (uiout))
+      {
+	ui_out_text (uiout, "\nBreakpoint ");
+	ui_out_field_string (uiout, "reason", "vfork");
+	ui_out_field_int (uiout, "bkptno", bs->breakpoint_at->number);
+	ui_out_field_int (uiout, "forked-process",
+			  bs->breakpoint_at->forked_inferior_pid);
+	ui_out_text (uiout, ", ");
+      }
+      else
+      {
+	printf_filtered (_("\nCatchpoint %d (vforked process %d), "),
+			 bs->breakpoint_at->number, 
+			 bs->breakpoint_at->forked_inferior_pid);
+      }
       return PRINT_SRC_AND_LOC;
       break;
 

Re: minimalistic MI catch support

[Daniel Jacobowitz]

On Sat, Jan 28, 2006 at 02:04:24PM +0100, Markus Schiltknecht wrote:
> Hello gdb hackers,
> 
> working on an automated debugging layer on top of gdb I figured the
> catchpoint functions in the MI interface is missing. The attached patch
> against current CVS at least makes gdb report a catchpoint-break via MI.
> This is already sufficient for my application. However, I would
> appreciate a complete catchpoint implementation for the MI interface. I
> haven't figured out how to implement the '-break-catch' command. If
> somebody more knowledgable could do that I'd be thankfull.
> 
> Please CC me in responses, as I'm not subscribed.

Hi Markus,

For the -break-catch command, I recommend you file an issue in our bug
system; that won't help it get done any sooner, but at least it won't
get lost.

For the patch, in general it's better to avoid ui_out_is_mi_like_p when
we can.  Because the text outputs are ignored in non-MI mode, this is
usually pretty easy; see the attached.

This changes from:

*stopped,thread-id="0",frame={addr="0x00002aaaaaeb6462",func="fork",args=[],from="/lib/libc.so.6"}

to:

*stopped,reason="breakpoint-hit",bkptno="1",forked-process="6570",thread-id="0",frame={addr="0x00002aaaaaeb6462",func="fork",args=[],from="/lib/libc.so.6"}

You used fork and vfork for the reasons; I'm not sure whether these
should have separate reasons or be marked as catchpoint-hit.  Bob,
Nick, any opinions on that?

-- 
Daniel Jacobowitz
CodeSourcery

Index: breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.221
diff -u -p -r1.221 breakpoint.c
--- breakpoint.c	6 Feb 2006 21:55:05 -0000	1.221
+++ breakpoint.c	6 Feb 2006 23:28:32 -0000
@@ -2167,17 +2167,29 @@ print_it_typical (bpstat bs)
 
     case bp_catch_fork:
       annotate_catchpoint (bs->breakpoint_at->number);
-      printf_filtered (_("\nCatchpoint %d (forked process %d), "),
-		       bs->breakpoint_at->number, 
-		       bs->breakpoint_at->forked_inferior_pid);
+      ui_out_text (uiout, "\nCatchpoint ");
+      if (ui_out_is_mi_like_p (uiout))
+	ui_out_field_string (uiout, "reason",
+			     async_reason_lookup (EXEC_ASYNC_BREAKPOINT_HIT));
+      ui_out_field_int (uiout, "bkptno", bs->breakpoint_at->number);
+      ui_out_text (uiout, " (forked process ");
+      ui_out_field_int (uiout, "forked-process",
+			bs->breakpoint_at->forked_inferior_pid);
+      ui_out_text (uiout, "), ");
       return PRINT_SRC_AND_LOC;
       break;
 
     case bp_catch_vfork:
       annotate_catchpoint (bs->breakpoint_at->number);
-      printf_filtered (_("\nCatchpoint %d (vforked process %d), "),
-		       bs->breakpoint_at->number, 
-		       bs->breakpoint_at->forked_inferior_pid);
+      ui_out_text (uiout, "\nCatchpoint ");
+      if (ui_out_is_mi_like_p (uiout))
+	ui_out_field_string (uiout, "reason",
+			     async_reason_lookup (EXEC_ASYNC_BREAKPOINT_HIT));
+      ui_out_field_int (uiout, "bkptno", bs->breakpoint_at->number);
+      ui_out_text (uiout, " (vforked process ");
+      ui_out_field_int (uiout, "vforked-process",
+			bs->breakpoint_at->forked_inferior_pid);
+      ui_out_text (uiout, "), ");
       return PRINT_SRC_AND_LOC;
       break;
 

Re: minimalistic MI catch support

[Bob Rossi]

On Mon, Feb 06, 2006 at 06:29:13PM -0500, Daniel Jacobowitz wrote:
> On Sat, Jan 28, 2006 at 02:04:24PM +0100, Markus Schiltknecht wrote:
> > Hello gdb hackers,
> > 
> > working on an automated debugging layer on top of gdb I figured the
> > catchpoint functions in the MI interface is missing. The attached patch
> > against current CVS at least makes gdb report a catchpoint-break via MI.
> > This is already sufficient for my application. However, I would
> > appreciate a complete catchpoint implementation for the MI interface. I
> > haven't figured out how to implement the '-break-catch' command. If
> > somebody more knowledgable could do that I'd be thankfull.
> > 
> > Please CC me in responses, as I'm not subscribed.
>
> For the patch, in general it's better to avoid ui_out_is_mi_like_p when
> we can.  Because the text outputs are ignored in non-MI mode, this is
> usually pretty easy; see the attached.

Daniel, could you please explain the above in more detail. In
particular, this patch adds specific functionality when in MI mode. Why
do we care about how it acts in non-MI mode? Just curious.

> This changes from:
> 
> *stopped,thread-id="0",frame={addr="0x00002aaaaaeb6462",func="fork",args=[],from="/lib/libc.so.6"}
> 
> to:
> 
> *stopped,reason="breakpoint-hit",bkptno="1",forked-process="6570",thread-id="0",frame={addr="0x00002aaaaaeb6462",func="fork",args=[],from="/lib/libc.so.6"}
> 
> You used fork and vfork for the reasons; I'm not sure whether these
> should have separate reasons or be marked as catchpoint-hit.  Bob,
> Nick, any opinions on that?

Defiantly this reason is better than the original patch. I think it would be 
best if we added a new enumeration called EXEC_ASYNC_CATCHPOINT_HIT, and 
returned that as the reason.  In the future, we could easily return the kind 
of catchpoint that was caught. For instance,

*stopped,reason="catchpoint-hit",catchpoint-kind="vfork",bkptno="1",forked-process="6570",thread-id="0",frame={addr="0x00002aaaaaeb6462",func="fork",args=[],from="/lib/libc.so.6"}

Finally, it irks me that all of the bp_catch_* commands are not being
implemented in this patch. I see these enumeration values:
  bp_catch_load,
  bp_catch_unload,
  bp_catch_fork,
  bp_catch_vfork,
  bp_catch_exec,
  bp_catch_catch,
  bp_catch_throw

I would really like to see a patch that supports all of these cases, or
none. It's just confusing to FE's to have 2/7'ths of the case's
implemented.

If no one has time to do this, I will. I certainly am running thin
though.

Thanks,
Bob Rossi

Re: minimalistic MI catch support

[Daniel Jacobowitz]

On Mon, Feb 06, 2006 at 08:16:01PM -0500, Bob Rossi wrote:
> On Mon, Feb 06, 2006 at 06:29:13PM -0500, Daniel Jacobowitz wrote:
> > For the patch, in general it's better to avoid ui_out_is_mi_like_p when
> > we can.  Because the text outputs are ignored in non-MI mode, this is
> > usually pretty easy; see the attached.
> 
> Daniel, could you please explain the above in more detail. In
> particular, this patch adds specific functionality when in MI mode. Why
> do we care about how it acts in non-MI mode? Just curious.

Because it prevents the two from diverging, for one thing.  It makes
sure that there is no non-cosmetic output between the two.  This would
be useful for implementing the CLI output on top of the MI output.

> Defiantly this reason is better than the original patch. I think it would be 
> best if we added a new enumeration called EXEC_ASYNC_CATCHPOINT_HIT, and 
> returned that as the reason.  In the future, we could easily return the kind 
> of catchpoint that was caught. For instance,
> 
> *stopped,reason="catchpoint-hit",catchpoint-kind="vfork",bkptno="1",forked-process="6570",thread-id="0",frame={addr="0x00002aaaaaeb6462",func="fork",args=[],from="/lib/libc.so.6"}

I like this.

> Finally, it irks me that all of the bp_catch_* commands are not being
> implemented in this patch. I see these enumeration values:
>   bp_catch_load,
>   bp_catch_unload,
>   bp_catch_fork,
>   bp_catch_vfork,
>   bp_catch_exec,
>   bp_catch_catch,
>   bp_catch_throw
> 
> I would really like to see a patch that supports all of these cases, or
> none. It's just confusing to FE's to have 2/7'ths of the case's
> implemented.
> 
> If no one has time to do this, I will. I certainly am running thin
> though.

The problem with this is that some of those others are ill-defined
and/or broken.  Fork and vfork only work on some targets, and they're
the only ones I would say worked "right".  Load/unload should be
implemented but aren't, exec presents very tricky user interface
and symbol table problems, and catch/throw are hard to present
the correct context to the user.

-- 
Daniel Jacobowitz
CodeSourcery

Re: minimalistic MI catch support

[Markus Schiltknecht]

Hello Daniel,

thank you for the corrected patch.

On Mon, 2006-02-06 at 22:00 -0500, Daniel Jacobowitz wrote:
> The problem with this is that some of those others are ill-defined
> and/or broken.  Fork and vfork only work on some targets, and they're
> the only ones I would say worked "right".

Could you tell me more about that? I was running into a problem and
wondering what happened. Because I did not find anything about that I
filed a bug report: gdb/2078

Regards

Markus

Re: minimalistic MI catch support

[Nick Roberts]

[I srewed up the header first time, hopefully not this time, apologies if you
get two copies.  I'm not subscribed to gdb-patches and would like to be cc'ed
in any threads on MI, if possible.]

I've not used catchpoints, so I just have a couple of minor observations.


>      case bp_catch_vfork:
>        annotate_catchpoint (bs->breakpoint_at->number);
> -      printf_filtered (_("\nCatchpoint %d (vforked process %d), "),
> -		       bs->breakpoint_at->number, 
> -		       bs->breakpoint_at->forked_inferior_pid);
> +      ui_out_text (uiout, "\nCatchpoint ");
> +      if (ui_out_is_mi_like_p (uiout))
> +	ui_out_field_string (uiout, "reason",
> +			     async_reason_lookup (EXEC_ASYNC_BREAKPOINT_HIT));
> +      ui_out_field_int (uiout, "bkptno", bs->breakpoint_at->number);
> +      ui_out_text (uiout, " (vforked process ");
> +      ui_out_field_int (uiout, "vforked-process",
> +			bs->breakpoint_at->forked_inferior_pid);

I realise that this method of combining CLI and MI output is already used in
breakpoint.c but presumably it has the unfortunate side effect of not
translating some CLI output intended for the user.  For example "breakpoint"
and "catchpoint may be translated in some places but not others.

The manual says:

    `exec'
          A call to `exec'.  This is currently only available for HP-UX.

    `fork'
          A call to `fork'.  This is currently only available for HP-UX.

    `vfork'
          A call to `vfork'.  This is currently only available for
          HP-UX.

Is that still true?  I can certainly set catchpoints for these with GNU/Linux.

> > *stopped,reason="catchpoint-hit",catchpoint-kind="vfork",bkptno="1",forked-process="6570",thread-id="0",frame={addr="0x00002aaaaaeb6462",func="fork",args=[],from="/lib/libc.so.6"}

> I like this.

With reason="breakpoint-hit" it's nature (catch vfork) can be deduced from
bkptno="1" and -break-list or just "-break-info 1" but I guess there's no harm
in redundant information.


Nick                                           http://www.inet.net.nz/~nickrob

Re: minimalistic MI catch support

[Nick Roberts]

A few further remark about catchpoints:

(gdb) catch catch
Catchpoint 2 (catch)
(gdb) catch throw
Catchpoint 3 (throw)
(gdb) catch fork
Catchpoint 4 (fork)
(gdb) catch exec
Catchpoint 5 (exec)
(gdb) info break
Num Type           Disp Enb Address    What
2   breakpoint     keep y   0x07dae1aa exception catch
3   breakpoint     keep y   0x07daf001 exception throw
4   catch fork     keep y
5   catch exec     keep y

The first two are of type breakpoint and their nature is given under the
heading "What".

The second two have their own type: "catch fork" and "catch exec" (I must
admit that I don't understand how this gives differerent behaviour from using
"break fork" and "break exec").  Also for some reason, they have no address.

This inconsistent description probably doesn't matter for CLI, but it is
also used for MI output, for which it does seem inappropriate.

Would it be a good idea to change it to:

Num Type           Disp Enb Address    What
2   breakpoint     keep y   0x07dae1aa exception catch
3   breakpoint     keep y   0x07daf001 exception throw
4   breakpoint     keep y              fork
5   breakpoint     keep y              exec

or something like:

Num Type             Disp Enb Address    What
2   exception catch  keep y   0x07dae1aa 
3   exception throw  keep y   0x07daf001 
4   catch fork       keep y
5   catch exec       keep y

to make it easier to parse the MI output.

Nick

Re: minimalistic MI catch support

[Eli Zaretskii]

> From: Nick Roberts <nickrob@snap.net.nz>
> Date: Fri, 10 Feb 2006 19:33:28 +1300
> 
> (gdb) catch catch
> Catchpoint 2 (catch)
> (gdb) catch throw
> Catchpoint 3 (throw)
> (gdb) catch fork
> Catchpoint 4 (fork)
> (gdb) catch exec
> Catchpoint 5 (exec)
> (gdb) info break
> Num Type           Disp Enb Address    What
> 2   breakpoint     keep y   0x07dae1aa exception catch
> 3   breakpoint     keep y   0x07daf001 exception throw
> 4   catch fork     keep y
> 5   catch exec     keep y
> 
> The first two are of type breakpoint and their nature is given under the
> heading "What".
> 
> The second two have their own type: "catch fork" and "catch exec" (I must
> admit that I don't understand how this gives differerent behaviour from using
> "break fork" and "break exec").  Also for some reason, they have no address.

I think this is a bug (or undocumented misfeature) in
breakpoint.c:handle_gnu_v3_exceptions.  You will see that it sets the
type of the raw breakpoint it creates to bp_breakpoint, instead of
using the argument ex_event that is passed by the caller.  In
addition, for some reason, it forces a special function to be used for
printing these catchpoints, and that function
(print_one_exception_catchpoint) doesn't use the list of descriptions
in the bptypes[] array used by the more general code in
print_one_breakpoint.  I think it's bad to have more than one place to
have these description strings.

Can someone explain why we should use special code for gnu_v3
exceptions?

As for the address not being printed, can you step with a debugger
through the function that prints these breakpoints and see why they
are skipped?  I cannot for the moment see any reason, just looking at
the code.

Re: minimalistic MI catch support

[Daniel Jacobowitz]

On Fri, Feb 10, 2006 at 01:25:28PM +0200, Eli Zaretskii wrote:
> > The second two have their own type: "catch fork" and "catch exec" (I must
> > admit that I don't understand how this gives differerent behaviour from using
> > "break fork" and "break exec").

They are associated with the system events reported for fork and exec. 
That means that in addition to the above, they'll catch things like
clone with appropriate arguments, execlp, direct use of syscall()...

Also, they give GDB external knowledge about what's going on, enabling
it to track the child of the fork, or the target of the exec.  That
latter is currently disabled because the code and user interface
for following exec were such a mess.

> >  Also for some reason, they have no address.

Unlike a breakpoint, they are associated with an event and not an
address.  They aren't at all like breakpoints.

> I think this is a bug (or undocumented misfeature) in
> breakpoint.c:handle_gnu_v3_exceptions.  You will see that it sets the
> type of the raw breakpoint it creates to bp_breakpoint, instead of
> using the argument ex_event that is passed by the caller.  In
> addition, for some reason, it forces a special function to be used for
> printing these catchpoints, and that function
> (print_one_exception_catchpoint) doesn't use the list of descriptions
> in the bptypes[] array used by the more general code in
> print_one_breakpoint.  I think it's bad to have more than one place to
> have these description strings.

That's not a bug, it's supposed to be a progression!  The idea was to,
over time, make the breakpoint layer more object-oriented.  Just
because no one has had time to go back to it yet...  One of my goals
was to break up the huge switch statements on breakpoint type.

ex_event is not suitable for two reasons; one is that EX_EVENT_CATCH
has little direct relationship to the bp_* constants, and the other is
that the type of the breakpoint is used to handle it, e.g. in
bpstat_stop_status.  "catch fork" is handled by a system event; on old
HP systems, "catch throw" was too, which I suspect is why it was
implemented as a catchpoint.  But on GNU systems, EH catchpoints are
just magical breakpoints in the runtime library.  Ideally, after
hitting them, we change frames a little ways up into the user's code;
IIRC that's not implemented because of some problem I had getting
it to work, but I can't remember the details.

As background, the overall reason for making it more object oriented is
the distinction between "what the user asked for" and "how gdb
satisfies that request".  For instance, a longstanding shortcoming in
my humble opinion is that GDB can't use hardware breakpoint resources
automatically - there's code that associates things like "bp_longjmp"
with "oh, that's really a bp_breakpoint and I'll handle it like one",
instead of information in the breakpoint that says "oh, this is really
a software breakpoint".

That's not 100% true any more now that I added bp->loc->loc_type, which
explicitly says the breakpoint has type bp_loc_software_breakpoint. 
But the two are not yet fully decoupled.

I'm not sure what "info breakpoints" should have to say about it;
whether it should show the underlying implementation and address,
or hide them as if it were a true catchpoint.  If Nick's got a
convincing argument to display "catchpoint" instead of "breakpoint",
I'm sure we can change that.

> Can someone explain why we should use special code for gnu_v3
> exceptions?

I hope the above covers this.

> As for the address not being printed, can you step with a debugger
> through the function that prints these breakpoints and see why they
> are skipped?  I cannot for the moment see any reason, just looking at
> the code.

[Now we're talking about fork catchpoints rather than EH.]

There's no meangingful address.  See this in breakpoint.h:

  /* Note that zero is a perfectly valid code address on some platforms
     (for example, the mn10200 (OBSOLETE) and mn10300 simulators).  NULL
     is not a special value for this field.  Valid for all types except
     bp_loc_other.  */
  CORE_ADDR address;

That's used for:

  - Software watchpoints.  I still plan someday to finish refactoring
    this so that a single watchpoint, software or otherwise, has
    multiple bp_location entries, each with a valid address.  But there
    will still be some software watchpoints that don't, e.g. I believe
    it is possible to set a software watchpoint on the value of a
    register.
  - Fork, vfork, and exec catchpoints.
  - Catch and throw catchpoints, if their type is bp_catch_*, which it
    is for the non-GNU implementation, which hasn't been tested in
    years.

The last two are associated with system events, just like DLL events
in Windows, not with a specific PC.

-- 
Daniel Jacobowitz
CodeSourcery

Re: minimalistic MI catch support

[Eli Zaretskii]

> Date: Fri, 10 Feb 2006 09:11:38 -0500
> From: Daniel Jacobowitz <drow@false.org>
> Cc: Nick Roberts <nickrob@snap.net.nz>, gdb-patches@sourceware.org
> 
> > I think this is a bug (or undocumented misfeature) in
> > breakpoint.c:handle_gnu_v3_exceptions.  You will see that it sets the
> > type of the raw breakpoint it creates to bp_breakpoint, instead of
> > using the argument ex_event that is passed by the caller.  In
> > addition, for some reason, it forces a special function to be used for
> > printing these catchpoints, and that function
> > (print_one_exception_catchpoint) doesn't use the list of descriptions
> > in the bptypes[] array used by the more general code in
> > print_one_breakpoint.  I think it's bad to have more than one place to
> > have these description strings.
> 
> That's not a bug

Which part?  I was talking about two things, not one, and only called
the first one a possible bug.

> ex_event is not suitable for two reasons; one is that EX_EVENT_CATCH
> has little direct relationship to the bp_* constants

??? Isn't there a 1:1 correspondence between them?

> and the other is
> that the type of the breakpoint is used to handle it, e.g. in
> bpstat_stop_status.  "catch fork" is handled by a system event; on old
> HP systems, "catch throw" was too, which I suspect is why it was
> implemented as a catchpoint.  But on GNU systems, EH catchpoints are
> just magical breakpoints in the runtime library.  Ideally, after
> hitting them, we change frames a little ways up into the user's code;
> IIRC that's not implemented because of some problem I had getting
> it to work, but I can't remember the details.

Thanks for the explanations, but I still cannot see what does this
have to do with the issue at hand.  The issue at hand is consistency
of the UI.  IMHO, the details of the implementation do not matter from
UI's point of view; whatever hoops GDB is jumping through behind the
scenes, we should still present the same consistent UI to the user.
That means the format we use to report all the catchpoints should be
the same no matter how those catchpoints are implemented.

In other words, we should use the our knowledge about these
breakpoints being ``magical'' to present a consistent view to the
users on all systems.

> I'm not sure what "info breakpoints" should have to say about it;
> whether it should show the underlying implementation and address,
> or hide them as if it were a true catchpoint.

The latter, IMO; the former is suitable for a "maint" command, not for
a user-level command.

> > Can someone explain why we should use special code for gnu_v3
> > exceptions?
> 
> I hope the above covers this.

I'd suggest some comments saying that this area is in flux and that
the intent is to make all the bp_* types look like catch and throw.
Otherwise, it's impossible to know where we are aiming, and this could
bite someone who will want to submit patches in that area.

> > As for the address not being printed, can you step with a debugger
> > through the function that prints these breakpoints and see why they
> > are skipped?  I cannot for the moment see any reason, just looking at
> > the code.
> 
> [Now we're talking about fork catchpoints rather than EH.]
> 
> There's no meangingful address.

Yes, but what code knows about that? i.e. where in the code is it
clear that the address isn't displayed?

>   CORE_ADDR address;
> 
> That's used for:
> 
>   - Software watchpoints.  I still plan someday to finish refactoring
>     this so that a single watchpoint, software or otherwise, has
>     multiple bp_location entries, each with a valid address.

??? Under what circumstances would a watchpoint have to have several
locations?  It's watching data, and data has one address only, right?
What am I missing?

>     I believe it is possible to set a software watchpoint on the
>     value of a register.

Yes, it's possible.  I've even used that once, I don't remember in
what situation, but I do remember that this was the only method to
track something tricky.

Re: minimalistic MI catch support

[Daniel Jacobowitz]

On Fri, Feb 10, 2006 at 05:41:18PM +0200, Eli Zaretskii wrote:
> > Date: Fri, 10 Feb 2006 09:11:38 -0500
> > From: Daniel Jacobowitz <drow@false.org>
> > Cc: Nick Roberts <nickrob@snap.net.nz>, gdb-patches@sourceware.org
> > 
> > > I think this is a bug (or undocumented misfeature) in
> > > breakpoint.c:handle_gnu_v3_exceptions.  You will see that it sets the
> > > type of the raw breakpoint it creates to bp_breakpoint, instead of
> > > using the argument ex_event that is passed by the caller.  In
> > > addition, for some reason, it forces a special function to be used for
> > > printing these catchpoints, and that function
> > > (print_one_exception_catchpoint) doesn't use the list of descriptions
> > > in the bptypes[] array used by the more general code in
> > > print_one_breakpoint.  I think it's bad to have more than one place to
> > > have these description strings.
> > 
> > That's not a bug
> 
> Which part?  I was talking about two things, not one, and only called
> the first one a possible bug.

Well, I was responding to "I think this is a bug in", which I
associated with "You will see that".

> > ex_event is not suitable for two reasons; one is that EX_EVENT_CATCH
> > has little direct relationship to the bp_* constants
> 
> ??? Isn't there a 1:1 correspondence between them?

Nope.  I agree this is unclear.

EX_EVENT_CATCH means "the user has asked for a 'catch catch'
operation".  bp_catch_catch means "we have set an OS-specific
catchpoint on the 'an exception has been caught' event".  In
the original case, these two did correspond; but for modern C++ ABIs
(ignoring the issue of Windows SEH here for the moment...) they no
longer correspond, and there is no event external to the process
that we can capture.  Instead, we set a breakpoint at an ABI-defined
location.

It Might Be Nice If the bp_catch_catch event described the user's
intent and its ->loc described the implementation.  I'm not sure.
Anyway, the two are not yet sufficiently separated.

> Thanks for the explanations, but I still cannot see what does this
> have to do with the issue at hand.  The issue at hand is consistency
> of the UI.  IMHO, the details of the implementation do not matter from
> UI's point of view; whatever hoops GDB is jumping through behind the
> scenes, we should still present the same consistent UI to the user.
> That means the format we use to report all the catchpoints should be
> the same no matter how those catchpoints are implemented.
> 
> In other words, we should use the our knowledge about these
> breakpoints being ``magical'' to present a consistent view to the
> users on all systems.

Well, you asked about the non-use of bp_catch_catch :-)  That's the
implementation.  I've got no objection to changing the info breakpoints
output to describe them as catchpoints, if that's considered superior.

> > I'm not sure what "info breakpoints" should have to say about it;
> > whether it should show the underlying implementation and address,
> > or hide them as if it were a true catchpoint.
> 
> The latter, IMO; the former is suitable for a "maint" command, not for
> a user-level command.

Hmm.  Good idea.  I don't think it really fits into "maint info
breakpoints" at the moment, but maybe we can rearrange... I think
we ought to defer this until the one-to-many breakpoint issues are
finally addressed, which will hopefully be this year.

> I'd suggest some comments saying that this area is in flux and that
> the intent is to make all the bp_* types look like catch and throw.
> Otherwise, it's impossible to know where we are aiming, and this could
> bite someone who will want to submit patches in that area.

That's a good idea.  I'll leave myself a note to write such, unless
someone else wants to.

> > > As for the address not being printed, can you step with a debugger
> > > through the function that prints these breakpoints and see why they
> > > are skipped?  I cannot for the moment see any reason, just looking at
> > > the code.
> > 
> > [Now we're talking about fork catchpoints rather than EH.]
> > 
> > There's no meangingful address.
> 
> Yes, but what code knows about that? i.e. where in the code is it
> clear that the address isn't displayed?

For instance,

      case bp_catch_fork:
      case bp_catch_vfork:
        /* Field 4, the address, is omitted (which makes the columns
           not line up too nicely with the headers, but the effect
           is relatively readable).  */
        if (addressprint)
          ui_out_field_skip (uiout, "addr");
        annotate_field (5);
        if (b->forked_inferior_pid != 0)
          {
            ui_out_text (uiout, "process ");
            ui_out_field_int (uiout, "what", b->forked_inferior_pid);
            ui_out_spaces (uiout, 1);
          }
        break;

> >   CORE_ADDR address;
> > 
> > That's used for:
> > 
> >   - Software watchpoints.  I still plan someday to finish refactoring
> >     this so that a single watchpoint, software or otherwise, has
> >     multiple bp_location entries, each with a valid address.
> 
> ??? Under what circumstances would a watchpoint have to have several
> locations?  It's watching data, and data has one address only, right?
> What am I missing?

Oh, there's plenty of examples.  Here's an easy one.  Suppose we have
an int **ptr, and the user sets a watchpoint on **ptr.  That's
implemented with two watchpoints: one on ptr, and one on *ptr.  When
ptr changes, the watchpoint on *ptr has to be moved.

We do this with both software and hardware watchpoints.

      case bp_watchpoint:
      case bp_hardware_watchpoint:
      case bp_read_watchpoint:
      case bp_access_watchpoint:
        /* Field 4, the address, is omitted (which makes the columns
           not line up too nicely with the headers, but the effect
           is relatively readable).  */

I'm not sure of the address field is used for hardware watchpoints
or not, actually, but in any case it is not displayed.  Just the
expression is.

-- 
Daniel Jacobowitz
CodeSourcery

Re: minimalistic MI catch support

[Eli Zaretskii]

> Date: Fri, 10 Feb 2006 11:48:11 -0500
> From: Daniel Jacobowitz <drow@false.org>
> Cc: Nick Roberts <nickrob@snap.net.nz>, gdb-patches@sourceware.org
> 
> On Fri, Feb 10, 2006 at 05:41:18PM +0200, Eli Zaretskii wrote:
> > > Date: Fri, 10 Feb 2006 09:11:38 -0500
> > > From: Daniel Jacobowitz <drow@false.org>
> > > Cc: Nick Roberts <nickrob@snap.net.nz>, gdb-patches@sourceware.org
> > > 
> > > > I think this is a bug (or undocumented misfeature) in
> > > > breakpoint.c:handle_gnu_v3_exceptions.  You will see that it sets the
> > > > type of the raw breakpoint it creates to bp_breakpoint, instead of
> > > > using the argument ex_event that is passed by the caller.  In
> > > > addition, for some reason, it forces a special function to be used for
> > > > printing these catchpoints, and that function
> > > > (print_one_exception_catchpoint) doesn't use the list of descriptions
> > > > in the bptypes[] array used by the more general code in
> > > > print_one_breakpoint.  I think it's bad to have more than one place to
> > > > have these description strings.
> > > 
> > > That's not a bug
> > 
> > Which part?  I was talking about two things, not one, and only called
> > the first one a possible bug.
> 
> Well, I was responding to "I think this is a bug in", which I
> associated with "You will see that".
> 
> > > ex_event is not suitable for two reasons; one is that EX_EVENT_CATCH
> > > has little direct relationship to the bp_* constants
> > 
> > ??? Isn't there a 1:1 correspondence between them?
> 
> Nope.  I agree this is unclear.
> 
> EX_EVENT_CATCH means "the user has asked for a 'catch catch'
> operation".  bp_catch_catch means "we have set an OS-specific
> catchpoint on the 'an exception has been caught' event".  In
> the original case, these two did correspond; but for modern C++ ABIs
> (ignoring the issue of Windows SEH here for the moment...) they no
> longer correspond, and there is no event external to the process
> that we can capture.  Instead, we set a breakpoint at an ABI-defined
> location.

Thanks.  This is so confusing that it ought to be documented
somewhere.

> I've got no objection to changing the info breakpoints output to
> describe them as catchpoints, if that's considered superior.

Nick, would you like to make this change?

> Hmm.  Good idea.  I don't think it really fits into "maint info
> breakpoints" at the moment, but maybe we can rearrange... I think
> we ought to defer this until the one-to-many breakpoint issues are
> finally addressed, which will hopefully be this year.

I'm okay with deferring this, let's just not forget it when the time
comes.

> > I'd suggest some comments saying that this area is in flux and that
> > the intent is to make all the bp_* types look like catch and throw.
> > Otherwise, it's impossible to know where we are aiming, and this could
> > bite someone who will want to submit patches in that area.
> 
> That's a good idea.  I'll leave myself a note to write such, unless
> someone else wants to.

Thanks.

> > > [Now we're talking about fork catchpoints rather than EH.]
> > > 
> > > There's no meangingful address.
> > 
> > Yes, but what code knows about that? i.e. where in the code is it
> > clear that the address isn't displayed?
> 
> For instance,
> 
>       case bp_catch_fork:
>       case bp_catch_vfork:
>         /* Field 4, the address, is omitted (which makes the columns
>            not line up too nicely with the headers, but the effect
>            is relatively readable).  */
>         if (addressprint)
>           ui_out_field_skip (uiout, "addr");

Yes, I was fooled by a similar code that does the same for
bp_catch_catch (which I thought was run for "catch catch", whose
address _is_ displayed).  Also, to _not_ print when the variable
addressprint is non-zero doesn't really help to read the code, and the
comment (repeated N times elsewhere) doesn't really explain anything,
IMHO.

Anyway, if there's no meaningful address in this case, then we
shouldn't print one for "catch catch", either.  I take it that this is
currently hard to do, since there's no sign in the breakpoint data
structure that this breakpoint is special, right?  I guess this all
boils down to adding two more bp_* enum members, one each for the
new-style catch and throw breakpoints.  Then the print routines will
know to DTRT (and the code will be clearer as well).

> > >   - Software watchpoints.  I still plan someday to finish refactoring
> > >     this so that a single watchpoint, software or otherwise, has
> > >     multiple bp_location entries, each with a valid address.
> > 
> > ??? Under what circumstances would a watchpoint have to have several
> > locations?  It's watching data, and data has one address only, right?
> > What am I missing?
> 
> Oh, there's plenty of examples.  Here's an easy one.  Suppose we have
> an int **ptr, and the user sets a watchpoint on **ptr.  That's
> implemented with two watchpoints: one on ptr, and one on *ptr.  When
> ptr changes, the watchpoint on *ptr has to be moved.

You are talking about watching expressions, and you are talking
intermittently about high-level and raw watchpoints.  I was thinking
only about raw watchpoints we insert on the target side, thus the
misunderstanding.

So do you suggest to show all the raw watchpoints used to watch an
expression?  That could mean an awfully large list of addresses.

> We do this with both software and hardware watchpoints.

Sure, this is common to them all.

>       case bp_watchpoint:
>       case bp_hardware_watchpoint:
>       case bp_read_watchpoint:
>       case bp_access_watchpoint:
>         /* Field 4, the address, is omitted (which makes the columns
>            not line up too nicely with the headers, but the effect
>            is relatively readable).  */
> 
> I'm not sure of the address field is used for hardware watchpoints
> or not, actually, but in any case it is not displayed.  Just the
> expression is.

I don't see why not display for the watchpoints the address of the
first value on the value chain.  This will at least show the right
address when the expression is actually a simple variable.  WDYT?

Re: minimalistic MI catch support

[Daniel Jacobowitz]

On Fri, Feb 10, 2006 at 08:40:30PM +0200, Eli Zaretskii wrote:
> > > > [Now we're talking about fork catchpoints rather than EH.]
> > > > 
> > > > There's no meangingful address.
> > > 
> > > Yes, but what code knows about that? i.e. where in the code is it
> > > clear that the address isn't displayed?
> > 
> > For instance,
> > 
> >       case bp_catch_fork:
> >       case bp_catch_vfork:
> >         /* Field 4, the address, is omitted (which makes the columns
> >            not line up too nicely with the headers, but the effect
> >            is relatively readable).  */
> >         if (addressprint)
> >           ui_out_field_skip (uiout, "addr");
> 
> Yes, I was fooled by a similar code that does the same for
> bp_catch_catch (which I thought was run for "catch catch", whose
> address _is_ displayed).  Also, to _not_ print when the variable
> addressprint is non-zero doesn't really help to read the code, and the
> comment (repeated N times elsewhere) doesn't really explain anything,
> IMHO.

If addressprint is not set, nothing will print addresses, not even
breakpoints.  If it is set, then breakpoints will print addresses,
and catchpoints will explicitly skip the field.

> Anyway, if there's no meaningful address in this case, then we
> shouldn't print one for "catch catch", either.

For "catch catch" there is a relevant address, if breakpoints
are used to implement it.  Still, I've got no objection to skipping it
for consistency.  Unless GDB makes a mistake, the address is
irrelevant.

> I take it that this is
> currently hard to do, since there's no sign in the breakpoint data
> structure that this breakpoint is special, right?  I guess this all
> boils down to adding two more bp_* enum members, one each for the
> new-style catch and throw breakpoints.  Then the print routines will
> know to DTRT (and the code will be clearer as well).

No, not at all.  It's not hard to do currently; that was part of my
point in object-ifying this.  If you scroll a page or so up, to before
that big switch statement, you'll find:

  if (b->ops != NULL && b->ops->print_one != NULL)
    b->ops->print_one (b, last_addr);

Of course, it's called after the description field has been printed. 
So we've got the option of adding a description string to the
breakpoint ops, or making print_one duplicate more of the work of
print_one_breakpoint; I think the former is probably the way to go.

  if (b->ops && b->ops->description)
    print it
  else
    current table lookup

> > > >   - Software watchpoints.  I still plan someday to finish refactoring
> > > >     this so that a single watchpoint, software or otherwise, has
> > > >     multiple bp_location entries, each with a valid address.
> > > 
> > > ??? Under what circumstances would a watchpoint have to have several
> > > locations?  It's watching data, and data has one address only, right?
> > > What am I missing?
> > 
> > Oh, there's plenty of examples.  Here's an easy one.  Suppose we have
> > an int **ptr, and the user sets a watchpoint on **ptr.  That's
> > implemented with two watchpoints: one on ptr, and one on *ptr.  When
> > ptr changes, the watchpoint on *ptr has to be moved.
> 
> You are talking about watching expressions, and you are talking
> intermittently about high-level and raw watchpoints.  I was thinking
> only about raw watchpoints we insert on the target side, thus the
> misunderstanding.

We were talking about the bp_* constants, the "struct breakpoint",
and the "struct bp_location".  These are all mapped to "high level"
breakpoints at the moment - whether they should be or not is another
question.

> So do you suggest to show all the raw watchpoints used to watch an
> expression?  That could mean an awfully large list of addresses.

Hardly, compared to breakpoints on inlined functions.  If anything, I'd
propose to treat these the same way we ought to treat those: if there
isn't a sensible address, don't show one, and allow the user to drill
down and see the set of addresses that make up their breakpoint if they
want to.

> I don't see why not display for the watchpoints the address of the
> first value on the value chain.  This will at least show the right
> address when the expression is actually a simple variable.  WDYT?

*shrug* No opinion.

-- 
Daniel Jacobowitz
CodeSourcery

Re: minimalistic MI catch support

[Nick Roberts]

 > > > The second two have their own type: "catch fork" and "catch exec" (I
 > > > must admit that I don't understand how this gives differerent behaviour
 > > > from using "break fork" and "break exec").
 > 
 > They are associated with the system events reported for fork and exec. 
 > That means that in addition to the above, they'll catch things like
 > clone with appropriate arguments, execlp, direct use of syscall()...
 > 
 > Also, they give GDB external knowledge about what's going on, enabling
 > it to track the child of the fork, or the target of the exec.  That
 > latter is currently disabled because the code and user interface
 > for following exec were such a mess.
 > 
 > > >  Also for some reason, they have no address.
 > 
 > Unlike a breakpoint, they are associated with an event and not an
 > address.  They aren't at all like breakpoints.

OK thanks for explaining the difference.  I would still say that they are
a _bit_ like breakpoints (they stop execution if fork or exec are called)
and I guess thats why their details are given in "info breakpoints".

Are you saying "catch fork" and "catch exec" aren't at all like
"catch catch" and "catch throw" which do have addresses?  If so, perhaps
some clearer distinction could be made to the user/in MI output.

Nick

Re: minimalistic MI catch support

[Daniel Jacobowitz]

On Sat, Feb 11, 2006 at 09:15:58AM +1300, Nick Roberts wrote:
> OK thanks for explaining the difference.  I would still say that they are
> a _bit_ like breakpoints (they stop execution if fork or exec are called)
> and I guess thats why their details are given in "info breakpoints".
> 
> Are you saying "catch fork" and "catch exec" aren't at all like
> "catch catch" and "catch throw" which do have addresses?  If so, perhaps
> some clearer distinction could be made to the user/in MI output.

I think Eli's convinced me on this point; we should hide the addresses,
at least in "info breakpoints".

"catch catch" is a pretty generic concept.  When implemented on a GNU
v3 binary, it uses a breakpoint; when implemented on an old HP-UX aCC
binary, apparently, it used an out-of-band event just like "catch
fork".  Thus the difference.

-- 
Daniel Jacobowitz
CodeSourcery