Stop breakpoint commands from poping the target

Stop breakpoint commands from poping the target

[Pedro Alves]

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

This patch goes on top of Vladimir's pending async fixes patch.

commands.exp triggers the problem this patch addresses, in
async mode.

In that test, we have a watch on a local variable, and then we
add a command to that watch to print the local variable's
value.  When the variable goes out of scope, gdb prints that,
and also runs the associated commands with the watch.  Since
the variable is out of scope, and error is thrown.  In sync
targets, the exception just ends the breakpoints command
processing, and goes on to proceed with the command loop.  But in
async mode, the exception ends up in
inf-loop.c:inferior_event_handler/INF_REG_EVENT, which considers exceptions
fatal, and pops the target.  The fix is to catch the exception earlier.

Imagine the following command list associated with a breakpoint:
  non_existing_command
  info target

As and example, in sync mode, trying to execute
non_existing_command causes an error that stops the
interpreting of all subsequent commands, hence info target is
never executed.  The patch installs the same behaviour for
async targets.

commands.exp passes cleanly with the linux native async patch
I'll post next.

-- 
Pedro Alves

[-- Attachment #2: bpstat_do_actions_dont_escape_exception.diff --]
[-- Type: text/x-diff, Size: 1713 bytes --]

2008-03-14  Pedro Alves  <pedro@codesourcery.com>

	* top.c (bpstat_do_actions_wrapper): New.
	(command_line_handler_continuation): Wrap call to
	bpstat_do_actions in catch_errors.

---
 gdb/top.c |   17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

Index: src/gdb/top.c
===================================================================
--- src.orig/gdb/top.c	2008-03-14 07:10:51.000000000 +0000
+++ src/gdb/top.c	2008-03-14 07:22:10.000000000 +0000
@@ -364,6 +364,14 @@ do_chdir_cleanup (void *old_dir)
 }
 #endif
 
+static int
+bpstat_do_actions_wrapper (void *arg)
+{
+  bpstat *bsp = arg;
+  bpstat_do_actions (bsp);
+  return 1;
+}
+
 /* Do any commands attached to breakpoint we stopped at. Only if we
    are always running synchronously. Or if we have just executed a
    command that doesn't start the target. */
@@ -376,7 +384,12 @@ command_line_handler_continuation (struc
   long time_at_cmd_start  = arg->data.longint;
   long space_at_cmd_start = arg->next->data.longint;
 
-  bpstat_do_actions (&stop_bpstat);
+  /* Don't let exceptions escape into
+     inferior_event_handler/INF_REG_EVENT, as that pops the
+     target.  */
+  catch_errors (bpstat_do_actions_wrapper, (void *) &stop_bpstat,
+		"", RETURN_MASK_ALL);
+
   /*do_cleanups (old_chain); *//*?????FIXME????? */
 
   if (display_time)
@@ -542,7 +555,7 @@ execute_command (char *p, int from_tty)
 	}
     }
 
-  /* Set things up for this function to be compete later, once the
+  /* Set things up for this function to be finished later, once the
      execution has completed, if we are doing an execution command,
      otherwise, just go ahead and finish. */
   if (target_can_async_p () && target_executing)

Re: Stop breakpoint commands from poping the target

[Vladimir Prus]

Pedro Alves wrote:

> This patch goes on top of Vladimir's pending async fixes patch.
> 
> commands.exp triggers the problem this patch addresses, in
> async mode.
> 
> In that test, we have a watch on a local variable, and then we
> add a command to that watch to print the local variable's
> value.  When the variable goes out of scope, gdb prints that,
> and also runs the associated commands with the watch.  Since
> the variable is out of scope, and error is thrown.  In sync
> targets, the exception just ends the breakpoints command
> processing, and goes on to proceed with the command loop.  But in
> async mode, the exception ends up in
> inf-loop.c:inferior_event_handler/INF_REG_EVENT, which considers exceptions
> fatal, and pops the target.  The fix is to catch the exception earlier.
> 
> Imagine the following command list associated with a breakpoint:
>   non_existing_command
>   info target
> 
> As and example, in sync mode, trying to execute
> non_existing_command causes an error that stops the
> interpreting of all subsequent commands, hence info target is
> never executed.  The patch installs the same behaviour for
> async targets.
> 
> commands.exp passes cleanly with the linux native async patch
> I'll post next.

Heh, we seem to have a mid-flight collision here -- I've a local
patch moving the call to bpstat_do_actions into 
inferior_event_handler. The call you modify, in 
command_line_handler_continuation, runs for CLI only and it's
quite possible to have commands on breakpoints even if GDB uses
MI on the top level.

But that only chances which line of code must be changed, while
this patch is still needed. One tweak though -- I think it's
better to use TRY_CATCH, not catch_exceptions -- it's just
fewer lines.

- Volodya

Re: Stop breakpoint commands from poping the target

[Daniel Jacobowitz]

On Fri, Mar 14, 2008 at 11:15:30AM +0300, Vladimir Prus wrote:
> But that only chances which line of code must be changed, while
> this patch is still needed. One tweak though -- I think it's
> better to use TRY_CATCH, not catch_exceptions -- it's just
> fewer lines.

I'm with Vladimir on this one.

-- 
Daniel Jacobowitz
CodeSourcery

Re: Stop breakpoint commands from poping the target

[Pedro Alves]

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

A Friday 14 March 2008 08:15:30, Vladimir Prus wrote:
> Pedro Alves wrote:
> > This patch goes on top of Vladimir's pending async fixes patch.
> >
> > commands.exp triggers the problem this patch addresses, in
> > async mode.
> >
> > In that test, we have a watch on a local variable, and then we
> > add a command to that watch to print the local variable's
> > value.  When the variable goes out of scope, gdb prints that,
> > and also runs the associated commands with the watch.  Since
> > the variable is out of scope, and error is thrown.  In sync
> > targets, the exception just ends the breakpoints command
> > processing, and goes on to proceed with the command loop.  But in
> > async mode, the exception ends up in
> > inf-loop.c:inferior_event_handler/INF_REG_EVENT, which considers
> > exceptions fatal, and pops the target.  The fix is to catch the exception
> > earlier.
> >
> > Imagine the following command list associated with a breakpoint:
> >   non_existing_command
> >   info target
> >
> > As and example, in sync mode, trying to execute
> > non_existing_command causes an error that stops the
> > interpreting of all subsequent commands, hence info target is
> > never executed.  The patch installs the same behaviour for
> > async targets.
> >
> > commands.exp passes cleanly with the linux native async patch
> > I'll post next.
>
> Heh, we seem to have a mid-flight collision here -- I've a local
> patch moving the call to bpstat_do_actions into
> inferior_event_handler. The call you modify, in
> command_line_handler_continuation, runs for CLI only and it's
> quite possible to have commands on breakpoints even if GDB uses
> MI on the top level.
>
> But that only chances which line of code must be changed, while
> this patch is still needed. One tweak though -- I think it's
> better to use TRY_CATCH, not catch_exceptions -- it's just
> fewer lines.
>

I have been using this instead now.  Should I go ahead while you
don't post your patch (if it looks ok)?  Or is it coming out soon (,
hopefully with a similar fix in)?  I'm fine either way.

-- 
Pedro Alves

[-- Attachment #2: bpstat_do_actions_dont_escape_exception.diff --]
[-- Type: text/x-diff, Size: 1255 bytes --]

2008-03-17  Pedro Alves  <pedro@codesourcery.com>

	* top.c (command_line_handler_continuation): Wrap call to
	bpstat_do_actions in TRY_CATCH.

---
 gdb/top.c |    9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

Index: src/gdb/top.c
===================================================================
--- src.orig/gdb/top.c	2008-03-14 22:36:07.000000000 +0000
+++ src/gdb/top.c	2008-03-14 22:46:02.000000000 +0000
@@ -376,7 +376,12 @@ command_line_handler_continuation (struc
   long time_at_cmd_start  = arg->data.longint;
   long space_at_cmd_start = arg->next->data.longint;
 
-  bpstat_do_actions (&stop_bpstat);
+  volatile struct gdb_exception exception;
+  TRY_CATCH (exception, RETURN_MASK_ALL)
+    {
+      /* Don't propagate errors to inferior_event_handler/INF_REG_EVENT.  */
+      bpstat_do_actions (&stop_bpstat);
+    }
 
   if (display_time)
     {
@@ -539,7 +544,7 @@ execute_command (char *p, int from_tty)
 	}
     }
 
-  /* Set things up for this function to be compete later, once the
+  /* Set things up for this function to be finished later, once the
      execution has completed, if we are doing an execution command,
      otherwise, just go ahead and finish. */
   if (target_can_async_p () && target_executing)

Re: Stop breakpoint commands from poping the target

[Daniel Jacobowitz]

On Mon, Mar 17, 2008 at 04:43:43PM +0000, Pedro Alves wrote:
> I have been using this instead now.  Should I go ahead while you
> don't post your patch (if it looks ok)?  Or is it coming out soon (,
> hopefully with a similar fix in)?  I'm fine either way.

If this is OK with Vladimir, it's OK with me.

-- 
Daniel Jacobowitz
CodeSourcery

Re: Stop breakpoint commands from poping the target

[Vladimir Prus]

Pedro Alves wrote:

> This patch goes on top of Vladimir's pending async fixes patch.
> 
> commands.exp triggers the problem this patch addresses, in
> async mode.
> 
> In that test, we have a watch on a local variable, and then we
> add a command to that watch to print the local variable's
> value.  When the variable goes out of scope, gdb prints that,
> and also runs the associated commands with the watch.  Since
> the variable is out of scope, and error is thrown.  In sync
> targets, the exception just ends the breakpoints command
> processing, and goes on to proceed with the command loop.  But in
> async mode, the exception ends up in
> inf-loop.c:inferior_event_handler/INF_REG_EVENT, which considers exceptions
> fatal, and pops the target.  The fix is to catch the exception earlier.

Why do we pop the target in the first place? It does not seem like
a good idea to do that if any exception is thrown.

I actually agree that if breakpoint commands fail, it's not
then end of world, and we can go on with continuations.  But at the same
time, popping target on any unhandled exception sounds a little bit drastic.

- Volodya

Re: Stop breakpoint commands from poping the target

[Pedro Alves]

A Saturday 15 March 2008 15:58:59, Vladimir Prus escreveu:
> Pedro Alves wrote:
> > This patch goes on top of Vladimir's pending async fixes patch.
> >
> > commands.exp triggers the problem this patch addresses, in
> > async mode.
> >
> > In that test, we have a watch on a local variable, and then we
> > add a command to that watch to print the local variable's
> > value.  When the variable goes out of scope, gdb prints that,
> > and also runs the associated commands with the watch.  Since
> > the variable is out of scope, and error is thrown.  In sync
> > targets, the exception just ends the breakpoints command
> > processing, and goes on to proceed with the command loop.  But in
> > async mode, the exception ends up in
> > inf-loop.c:inferior_event_handler/INF_REG_EVENT, which considers
> > exceptions fatal, and pops the target.  The fix is to catch the exception
> > earlier.
>
> Why do we pop the target in the first place? It does not seem like
> a good idea to do that if any exception is thrown.
>
> I actually agree that if breakpoint commands fail, it's not
> then end of world, and we can go on with continuations.  But at the same
> time, popping target on any unhandled exception sounds a little bit
> drastic.
>

I don't know why we're doing that.  Maybe it should have been an
internal error, or maybe we'll find a case where an exception must
not be caught there.

It's not even a good job in doing the popping it either.  The target
is left in some undefined state.  

-- 
Pedro Alves