[patch 2/2] Do not bpstat_clear_actions on throw_exception #3

[patch 2/2] Do not bpstat_clear_actions on throw_exception #3

[Jan Kratochvil]

Hi Pedro,

On Thu, 18 Aug 2011 18:42:19 +0200, Pedro Alves wrote:
> Unfortunately, the hook-stop handling is in normal_stop.
> Your patch clears the breakpoint commands before get get a chance
> to run if the user installs a hook-stop.  E.g., before:

OK, I agree, I have made a new testcase.


> This looks tricky to get right without changing gdb's behavior :-(

The question is how big changed you were thinking about.

One problem I find one cannot use "step" and other such commands in the
breakpoints commands lists.  This may be due to GDB trying not to overflow its
stack.  I gues with async mode it could be implementable as some
stack-in-data-structure.

But that seems to be out of scope of this patch.


> We could try pushing bpstat_do_actions to the end of an execution
> command's run, but e.g., that would change the behavior of
> breakpoint commands in command lists, and things like "step N".
> OTOH, maybe that'd be the right thing to do (the current
> behavior could be seen as buggy --- more thought is needed).

I was playing with various changes but it had various side-effects.

Do you have anything against this patch?  I hope I have caught all the cases
where exceptions can be thrown.  Otherwise IMO everything is caught by
execute_command anyway.


Thanks,
Jan


gdb/
2011-08-21  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* breakpoint.c (bpstat_do_actions): Wrap it by TRY_CATCH, new variable
	except, possibly call bpstat_clear_actions.
	* top.c (bpstat_clear_actions_cleanup): New function.
	(execute_command): New variable cleanup_if_error, register there
	bpstat_clear_actions_cleanup, discard it.

gdb/testsuite/
2011-08-21  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.base/commands.exp (error_clears_commands_left): New function.
	(): Call it.

--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -3352,17 +3352,27 @@ bpstat_do_actions_1 (bpstat *bsp)
 void
 bpstat_do_actions (void)
 {
-  /* Do any commands attached to breakpoint we are stopped at.  */
-  while (!ptid_equal (inferior_ptid, null_ptid)
-	 && target_has_execution
-	 && !is_exited (inferior_ptid)
-	 && !is_executing (inferior_ptid))
-    /* Since in sync mode, bpstat_do_actions may resume the inferior,
-       and only return when it is stopped at the next breakpoint, we
-       keep doing breakpoint actions until it returns false to
-       indicate the inferior was not resumed.  */
-    if (!bpstat_do_actions_1 (&inferior_thread ()->control.stop_bpstat))
-      break;
+  volatile struct gdb_exception except;
+
+  TRY_CATCH (except, RETURN_MASK_ALL)
+    {
+      /* Do any commands attached to breakpoint we are stopped at.  */
+      while (!ptid_equal (inferior_ptid, null_ptid)
+	     && target_has_execution
+	     && !is_exited (inferior_ptid)
+	     && !is_executing (inferior_ptid))
+	/* Since in sync mode, bpstat_do_actions may resume the inferior,
+	   and only return when it is stopped at the next breakpoint, we
+	   keep doing breakpoint actions until it returns false to
+	   indicate the inferior was not resumed.  */
+	if (!bpstat_do_actions_1 (&inferior_thread ()->control.stop_bpstat))
+	  break;
+    }
+  if (except.reason < 0)
+    {
+      bpstat_clear_actions ();
+      throw_exception (except);
+    }
 }
 
 /* Print out the (old or new) value associated with a watchpoint.  */
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -362,18 +362,27 @@ prepare_execute_command (void)
   return cleanup;
 }
 
+/* Call bpstat_clear_actions with make_cleanup compatible prototype.  */
+
+static void
+bpstat_clear_actions_cleanup (void *unused)
+{
+  bpstat_clear_actions ();
+}
+
 /* Execute the line P as a command, in the current user context.
    Pass FROM_TTY as second argument to the defining function.  */
 
 void
 execute_command (char *p, int from_tty)
 {
-  struct cleanup *cleanup;
+  struct cleanup *cleanup_if_error, *cleanup;
   struct cmd_list_element *c;
   enum language flang;
   static int warned = 0;
   char *line;
 
+  cleanup_if_error = make_cleanup (bpstat_clear_actions_cleanup, NULL);
   cleanup = prepare_execute_command ();
 
   /* Force cleanup of any alloca areas if using C alloca instead of
@@ -477,7 +486,8 @@ execute_command (char *p, int from_tty)
 	}
     }
 
-    do_cleanups (cleanup);
+  do_cleanups (cleanup);
+  discard_cleanups (cleanup_if_error);
 }
 
 /* Run execute_command for P and FROM_TTY.  Capture its output into the
--- a/gdb/testsuite/gdb.base/commands.exp
+++ b/gdb/testsuite/gdb.base/commands.exp
@@ -678,6 +678,74 @@ proc if_commands_test {} {
     }
 }
 
+# Verify an error during "commands" commands execution will prevent any other
+# "commands" from other breakpoints at the same location to be executed.
+
+proc error_clears_commands_left {} {
+    set test "hook-stop 1"
+    gdb_test_multiple {define hook-stop} $test {
+	-re "End with a line saying just \"end\"\\.\r\n>$" {
+	    pass $test
+	}
+    }
+    set test "hook-stop 1a"
+    gdb_test_multiple {echo hook-stop1\n} $test {
+	-re "\r\n>$" {
+	    pass $test
+	}
+    }
+    gdb_test_no_output "end" "hook-stop 1b"
+
+    delete_breakpoints
+    gdb_breakpoint "main"
+
+    set test "main commands 1"
+    gdb_test_multiple {commands $bpnum} $test {
+	-re "End with a line saying just \"end\"\\.\r\n>$" {
+	    pass $test
+	}
+    }
+    set test "main commands 1a"
+    gdb_test_multiple {echo cmd1\n} $test {
+	-re "\r\n>$" {
+	    pass $test
+	}
+    }
+    set test "main commands 1b"
+    gdb_test_multiple {errorcommandxy\n} $test {
+	-re "\r\n>$" {
+	    pass $test
+	}
+    }
+    gdb_test_no_output "end" "main commands 1c"
+
+    gdb_breakpoint "main"
+    set test "main commands 2"
+    gdb_test_multiple {commands $bpnum} $test {
+	-re "End with a line saying just \"end\"\\.\r\n>$" {
+	    pass $test
+	}
+    }
+    set test "main commands 2a"
+    gdb_test_multiple {echo cmd2\n} $test {
+	-re "\r\n>$" {
+	    pass $test
+	}
+    }
+    set test "main commands 2b"
+    gdb_test_multiple {errorcommandyz\n} $test {
+	-re "\r\n>$" {
+	    pass $test
+	}
+    }
+    gdb_test_no_output "end" "main commands 2c"
+
+    gdb_run_cmd
+    gdb_test "" "\r\nhook-stop1\r\n.*\r\ncmd1\r\nUndefined command: \"errorcommandxy\"\\.  Try \"help\"\\." "cmd1 error"
+
+    gdb_test {echo idle\n} "\r\nidle" "no cmd2"
+}
+
 proc redefine_hook_test {} {
     global gdb_prompt
 
@@ -758,6 +826,7 @@ stray_arg0_test
 source_file_with_indented_comment
 recursive_source_test
 if_commands_test
+error_clears_commands_left
 redefine_hook_test
 # This one should come last, as it redefines "backtrace".
 redefine_backtrace_test

Re: [patch 2/2] Do not bpstat_clear_actions on throw_exception #3

[Pedro Alves]

On Monday 22 August 2011 15:51:50, Jan Kratochvil wrote:
> On Thu, 18 Aug 2011 18:42:19 +0200, Pedro Alves wrote:
> > Unfortunately, the hook-stop handling is in normal_stop.
> > Your patch clears the breakpoint commands before get get a chance
> > to run if the user installs a hook-stop.  E.g., before:
> 
> OK, I agree, I have made a new testcase.

Thanks.

> > This looks tricky to get right without changing gdb's behavior :-(
> 
> The question is how big changed you were thinking about.
> 
> One problem I find one cannot use "step" and other such commands in the
> breakpoints commands lists.  This may be due to GDB trying not to overflow its
> stack.  I gues with async mode it could be implementable as some
> stack-in-data-structure.

Yes, I going in that direction with
<http://sourceware.org/ml/gdb-patches/2011-06/msg00158.html>,
but there are other places I hadn't made fully state-machined
on that patch.  Unfortunately, I can't afford finishing
that one now, and I flipped to a plan B.

> But that seems to be out of scope of this patch.

For sure.

> > We could try pushing bpstat_do_actions to the end of an execution
> > command's run, but e.g., that would change the behavior of
> > breakpoint commands in command lists, and things like "step N".
> > OTOH, maybe that'd be the right thing to do (the current
> > behavior could be seen as buggy --- more thought is needed).
> 
> I was playing with various changes but it had various side-effects.
> 
> Do you have anything against this patch?  

No, looks almost good enough.  I like that it's simple.

> I hope I have caught all the cases
> where exceptions can be thrown.  Otherwise IMO everything is caught by
> execute_command anyway.

Not all cases.  In async mode, handle_inferior_event is called
_outside_ of execute_command, directly by the event loop (well, almost
directly).  Thus any exception thrown between bpstat_stop_status is called,
and the bpstat_do_actions call in inf-loop.c, will leave the thread
with a dangling bpstat too.  Might be good enough to wrap
handle_inferior_event with a similar cleanup?

-- 
Pedro Alves

Re: [patch 2/2] Do not bpstat_clear_actions on throw_exception #3

[Jan Kratochvil]

On Mon, 22 Aug 2011 19:06:15 +0200, Pedro Alves wrote:
> Not all cases.  In async mode, handle_inferior_event is called
> _outside_ of execute_command, directly by the event loop (well, almost
> directly).  Thus any exception thrown between bpstat_stop_status is called,
> and the bpstat_do_actions call in inf-loop.c, will leave the thread
> with a dangling bpstat too.  Might be good enough to wrap
> handle_inferior_event with a similar cleanup?

Done, thanks.  On top of the same patch 1/2 as before.

Testing is more difficult, going to post now patch 3/2 but in fact the real
continuation is not tested there as the testcase gets caught by
execute_command.  I haven't done a proper testcase for the async mode.

No regressions on {x86_64,x86_64-m32,i686}-fedora16pre-linux-gnu.


Thanks,
Jan


gdb/
2011-08-21  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* breakpoint.c (bpstat_do_actions): New variable cleanup_if_error, call
	make_bpstat_clear_actions_cleanup and discard_cleanups for it.
	* defs.h (make_bpstat_clear_actions_cleanup): New declaration.
	* exceptions.c (throw_exception): Remove the bpstat_clear_actions call.
	* inf-loop.c (inferior_event_handler): New variable cleanup_if_error,
	call make_bpstat_clear_actions_cleanup and discard_cleanups for it.
	Call bpstat_clear_actions for failed fetch_inferior_event_wrapper.
	* top.c (execute_command): New variable cleanup_if_error, call
	make_bpstat_clear_actions_cleanup and discard_cleanups for it.
	* utils.c (do_bpstat_clear_actions_cleanup)
	(make_bpstat_clear_actions_cleanup): New functions.

gdb/testsuite/
2011-08-21  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.base/commands.exp (error_clears_commands_left): New function.
	(): Call it.

--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -3352,6 +3352,8 @@ bpstat_do_actions_1 (bpstat *bsp)
 void
 bpstat_do_actions (void)
 {
+  struct cleanup *cleanup_if_error = make_bpstat_clear_actions_cleanup ();
+
   /* Do any commands attached to breakpoint we are stopped at.  */
   while (!ptid_equal (inferior_ptid, null_ptid)
 	 && target_has_execution
@@ -3363,6 +3365,8 @@ bpstat_do_actions (void)
        indicate the inferior was not resumed.  */
     if (!bpstat_do_actions_1 (&inferior_thread ()->control.stop_bpstat))
       break;
+
+  discard_cleanups (cleanup_if_error);
 }
 
 /* Print out the (old or new) value associated with a watchpoint.  */
--- a/gdb/defs.h
+++ b/gdb/defs.h
@@ -429,6 +429,8 @@ extern const char *gdb_bfd_errmsg (bfd_error_type error_tag, char **matching);
 
 extern int parse_pid_to_attach (char *args);
 
+extern struct cleanup *make_bpstat_clear_actions_cleanup (void);
+
 /* From demangle.c */
 
 extern void set_demangling_style (char *);
--- a/gdb/exceptions.c
+++ b/gdb/exceptions.c
@@ -210,10 +210,6 @@ throw_exception (struct gdb_exception exception)
   quit_flag = 0;
   immediate_quit = 0;
 
-  /* Perhaps it would be cleaner to do this via the cleanup chain (not sure
-     I can think of a reason why that is vital, though).  */
-  bpstat_clear_actions ();
-
   do_cleanups (ALL_CLEANUPS);
 
   /* Jump to the containing catch_errors() call, communicating REASON
--- a/gdb/inf-loop.c
+++ b/gdb/inf-loop.c
@@ -42,6 +42,7 @@ inferior_event_handler (enum inferior_event_type event_type,
 {
   struct gdb_exception e;
   int was_sync = 0;
+  struct cleanup *cleanup_if_error = make_bpstat_clear_actions_cleanup ();
 
   switch (event_type)
     {
@@ -53,6 +54,7 @@ inferior_event_handler (enum inferior_event_type event_type,
       if (!catch_errors (fetch_inferior_event_wrapper, 
 			 client_data, "", RETURN_MASK_ALL))
 	{
+	  bpstat_clear_actions ();
 	  do_all_intermediate_continuations (1);
 	  do_all_continuations (1);
 	  async_enable_stdin ();
@@ -142,6 +144,8 @@ inferior_event_handler (enum inferior_event_type event_type,
       printf_unfiltered (_("Event type not recognized.\n"));
       break;
     }
+
+  discard_cleanups (cleanup_if_error);
 }
 
 static int 
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -368,12 +368,13 @@ prepare_execute_command (void)
 void
 execute_command (char *p, int from_tty)
 {
-  struct cleanup *cleanup;
+  struct cleanup *cleanup_if_error, *cleanup;
   struct cmd_list_element *c;
   enum language flang;
   static int warned = 0;
   char *line;
 
+  cleanup_if_error = make_bpstat_clear_actions_cleanup ();
   cleanup = prepare_execute_command ();
 
   /* Force cleanup of any alloca areas if using C alloca instead of
@@ -477,7 +478,8 @@ execute_command (char *p, int from_tty)
 	}
     }
 
-    do_cleanups (cleanup);
+  do_cleanups (cleanup);
+  discard_cleanups (cleanup_if_error);
 }
 
 /* Run execute_command for P and FROM_TTY.  Capture its output into the
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -3674,6 +3674,23 @@ parse_pid_to_attach (char *args)
   return pid;
 }
 
+/* Helper for make_bpstat_clear_actions_cleanup.  */
+
+static void
+do_bpstat_clear_actions_cleanup (void *unused)
+{
+  bpstat_clear_actions ();
+}
+
+/* Call bpstat_clear_actions for the case an exception is throw.  You should
+   discard_cleanups if no exception is caught.  */
+
+struct cleanup *
+make_bpstat_clear_actions_cleanup (void)
+{
+  return make_cleanup (do_bpstat_clear_actions_cleanup, NULL);
+}
+
 /* Provide a prototype to silence -Wmissing-prototypes.  */
 extern initialize_file_ftype _initialize_utils;
 
--- a/gdb/testsuite/gdb.base/commands.exp
+++ b/gdb/testsuite/gdb.base/commands.exp
@@ -678,6 +678,74 @@ proc if_commands_test {} {
     }
 }
 
+# Verify an error during "commands" commands execution will prevent any other
+# "commands" from other breakpoints at the same location to be executed.
+
+proc error_clears_commands_left {} {
+    set test "hook-stop 1"
+    gdb_test_multiple {define hook-stop} $test {
+	-re "End with a line saying just \"end\"\\.\r\n>$" {
+	    pass $test
+	}
+    }
+    set test "hook-stop 1a"
+    gdb_test_multiple {echo hook-stop1\n} $test {
+	-re "\r\n>$" {
+	    pass $test
+	}
+    }
+    gdb_test_no_output "end" "hook-stop 1b"
+
+    delete_breakpoints
+    gdb_breakpoint "main"
+
+    set test "main commands 1"
+    gdb_test_multiple {commands $bpnum} $test {
+	-re "End with a line saying just \"end\"\\.\r\n>$" {
+	    pass $test
+	}
+    }
+    set test "main commands 1a"
+    gdb_test_multiple {echo cmd1\n} $test {
+	-re "\r\n>$" {
+	    pass $test
+	}
+    }
+    set test "main commands 1b"
+    gdb_test_multiple {errorcommandxy\n} $test {
+	-re "\r\n>$" {
+	    pass $test
+	}
+    }
+    gdb_test_no_output "end" "main commands 1c"
+
+    gdb_breakpoint "main"
+    set test "main commands 2"
+    gdb_test_multiple {commands $bpnum} $test {
+	-re "End with a line saying just \"end\"\\.\r\n>$" {
+	    pass $test
+	}
+    }
+    set test "main commands 2a"
+    gdb_test_multiple {echo cmd2\n} $test {
+	-re "\r\n>$" {
+	    pass $test
+	}
+    }
+    set test "main commands 2b"
+    gdb_test_multiple {errorcommandyz\n} $test {
+	-re "\r\n>$" {
+	    pass $test
+	}
+    }
+    gdb_test_no_output "end" "main commands 2c"
+
+    gdb_run_cmd
+    gdb_test "" "\r\nhook-stop1\r\n.*\r\ncmd1\r\nUndefined command: \"errorcommandxy\"\\.  Try \"help\"\\." "cmd1 error"
+
+    gdb_test {echo idle\n} "\r\nidle" "no cmd2"
+}
+
 proc redefine_hook_test {} {
     global gdb_prompt
 
@@ -758,6 +826,7 @@ stray_arg0_test
 source_file_with_indented_comment
 recursive_source_test
 if_commands_test
+error_clears_commands_left
 redefine_hook_test
 # This one should come last, as it redefines "backtrace".
 redefine_backtrace_test

Re: [patch 2/2] Do not bpstat_clear_actions on throw_exception #3

[Pedro Alves]

On Tuesday 23 August 2011 21:32:58, Jan Kratochvil wrote:
> On Mon, 22 Aug 2011 19:06:15 +0200, Pedro Alves wrote:
> > Not all cases.  In async mode, handle_inferior_event is called
> > _outside_ of execute_command, directly by the event loop (well, almost
> > directly).  Thus any exception thrown between bpstat_stop_status is called,
> > and the bpstat_do_actions call in inf-loop.c, will leave the thread
> > with a dangling bpstat too.  Might be good enough to wrap
> > handle_inferior_event with a similar cleanup?
> 
> Done, thanks.  On top of the same patch 1/2 as before.
> 
> Testing is more difficult, going to post now patch 3/2 but in fact the real
> continuation is not tested there as the testcase gets caught by
> execute_command.  I haven't done a proper testcase for the async mode.

I think a hook-stop that errors would be sufficient to leave
breakpoint commands dangling (normal_stop is called after
handle_inferior_event, from fetch_inferior_event)

> No regressions on {x86_64,x86_64-m32,i686}-fedora16pre-linux-gnu.

Almost there, but not yet, sorry. :-(  The cleanup is installed before
fetch_inferior_event (non-stop) switches inferior_ptid to the event thread
with a cleanup that restores back to the previous selected thread, which
means, this happens:

inferior_event_handler
  -> install cleanup to clear the bpstat of the current thread
  -> fetch_inferior_event
    -> install cleanup to restore the selected thread 
    -> switch to the event thread
    -> handle_inferior_event
       -> bpstat_stop_status on the now current event thread
       -> some error is thrown, e.g., while unwinding for step/next.
            -> the cleanup to restore the previous thread is ran
            -> the cleanup to clear the bpstat of the current
               thread is ran, but the current thread is not the
               event thread.  The bpstat of the event thread is left
               handling.

Installing the cleanup in fetch_inferior_event _after_
make_cleanup_restore_current_thread would fix this.

All-stop doesn't switch to the event thread until within
handle_inferior_event, so that still leaves a window where an
exception would run the cleanup with the old thread selected, but I'm
not seeing a scenario where that'd be a problem, and plus, the
current code (clearing bpstat actions from throw_exception) gets
that wrong too.

So the patch looks okay to me with that change.  Thanks.

-- 
Pedro Alves

Re: [patch 2/2] Do not bpstat_clear_actions on throw_exception #3

[Jan Kratochvil]

On Wed, 24 Aug 2011 12:19:03 +0200, Pedro Alves wrote:
> On Tuesday 23 August 2011 21:32:58, Jan Kratochvil wrote:
> > Testing is more difficult, going to post now patch 3/2 but in fact the real
> > continuation is not tested there as the testcase gets caught by
> > execute_command.  I haven't done a proper testcase for the async mode.
> 
> I think a hook-stop that errors would be sufficient to leave
> breakpoint commands dangling (normal_stop is called after
> handle_inferior_event, from fetch_inferior_event)

The problem is from hook-stop one runs execute_command which already catches
the error and does bpstat_clear_actions on its own.

Just FYI, it would be better but not everything needs to have a testcase.


I started this thread to support entry-values-not-available implemented via an
exception.  It may even have been inspired by your NOT_AVAILABLE_ERROR.

When evaluating all TRY_CATCH cases where should be done bpstat_clear_actions
and where not I have found it is very subjective.  gdb_target_find_new_threads
for example may catch exception due to corrupted thread list when analysing
a core file, therefore it currently does bpstat_clear_actions.  But maybe it
was expected the thread list is corrupted (it may have been the reason a core
file got dumped).

bpstat_clear_actions is not so strong as script_from_file abort but I find it
similar and maybe confusing their conditions are not the same.  I find wrong
that `gdb -nx -x ./cmd' stops on first error as I have to run `gdb -nx <cmd'
in such cases while that mode of run has other kinds of disadvantages.

Even if this patch gets fixed the way we were leading to it would still make
changes in GDB behavior as GDB would much less call bpstat_clear_actions.


The tracepoint example below on FSF GDB HEAD does:

Found trace frame 0, tracepoint 4
#0  globals_test_func () at ./gdb.trace/unavailable.cc:319
Backtrace stopped: Not enough registers or memory available to unwind further
No longer looking at any trace frame
(gdb) _

while it does not print "still-executing" which is IMO a bug, do you agree?


If not printing "still-executing" is really a bug proposing to forget about
the current patch and instead to:

(a) throw_exception will call bpstat_clear_actions only if exception.error is
    not NOT_AVAILABLE_ERROR (adding later my new ENTRY_VALUE_NOT_AVAILABLE).

(b) bpstat_clear_actions should also abort script_from_file.

(c) There should be a new setting "set error-stops-script" with default "on"
    (backward compatibility) where "off" would disable bpstat_clear_actions
    completely.  And I will happily use "set error-stops-script off" in my
    ~/.gdbinit so that I can finally run `gdb -nx -x ./transcript.1'.

I will try to code it.


Thanks,
Jan


cat >cmd <<EOH
file gdb.trace/unavailable
target remote localhost:1234
break begin
break end
commands
  tstop
  tfind start
  bt
  tfind none
end
break end
commands
  echo still-executing\n
end
trace 319
actions
collect a
end
continue
tstart
continue
EOH

./gdbserver --once :1234 ../testsuite/gdb.trace/unavailable
../gdb -nx -x cmd 

Re: [patch 2/2] Do not bpstat_clear_actions on throw_exception #3

[Pedro Alves]

On Friday 26 August 2011 11:13:59, Jan Kratochvil wrote:
> On Wed, 24 Aug 2011 12:19:03 +0200, Pedro Alves wrote:
> > On Tuesday 23 August 2011 21:32:58, Jan Kratochvil wrote:
> > > Testing is more difficult, going to post now patch 3/2 but in fact the real
> > > continuation is not tested there as the testcase gets caught by
> > > execute_command.  I haven't done a proper testcase for the async mode.
> > 
> > I think a hook-stop that errors would be sufficient to leave
> > breakpoint commands dangling (normal_stop is called after
> > handle_inferior_event, from fetch_inferior_event)
> 
> The problem is from hook-stop one runs execute_command which already catches
> the error and does bpstat_clear_actions on its own.
> 
> Just FYI, it would be better but not everything needs to have a testcase.

I agree.

> I started this thread to support entry-values-not-available implemented via an
> exception.  It may even have been inspired by your NOT_AVAILABLE_ERROR.
> 
> When evaluating all TRY_CATCH cases where should be done bpstat_clear_actions
> and where not I have found it is very subjective.  gdb_target_find_new_threads
> for example may catch exception due to corrupted thread list when analysing
> a core file, therefore it currently does bpstat_clear_actions.  But maybe it
> was expected the thread list is corrupted (it may have been the reason a core
> file got dumped).

Yes, the fact that that does bpstat_clear_actions is a bug, because
whatever error happened was handled, and the command the user
entered was not cancelled.  At the CLI level, the internal handling
of the error is an implementation detail of the command --- you
could instead reimplement gdb_target_find_new_threads to not
catch errors, but instead pass return error codes around, avoiding
any throw, but ending up behaving the same, that is, not consider
a corrupt thread list a fatal error.

I think that canceptually, what really matters for this patch is:

   (1) - some execution command is invoked at the cli prompt (e.g, "next").
   (2) - thread stops for breakpoint, and breakpoint has commands
   (3) - breakpoint commands are ran
   (4) - user enters some other command at the prompt

What we want is that the commands in 2-3 are either fully cancelled
or fully ran by the time we get to 4.  If some specific command should
handle errors itself gracefully or throw an error back at the interpreter
doesn't matter here --- the fact remains that some commands will
throw errors, others handle them gracefully.

Implementation wise, the fact that it is currently throw_exception that
clears the bpstat actions list is simply because #1 thread->stop_stat was
once a global, and, #2 gdb didn't use to have the exception scheme we
have nowadays, we only had a single setjmp at the top level, and
everywhere there was an error, we'd longjmp to the top level.  But nowadays
we can have errors that are caught by intermediate layers, and handled
there, so those should not cause the breakpoint command list to be
cancelled, because the error was handled before escaping out of the
command, making it non-fatal for the command's life.  Internal
NOT_AVAILABLE_ERROR handling by the printing layer is one such
example.  Another would be, say, MEMORY_ERRORs handled by e.g., 
"watch *0" (the watch command succeeds, so a command list
should move on to the next command).  Yet another would be the
general errors caught when printing values, that translate the errors
to "<error reading variable: %s>".  All these intermediate error
cases are breaking breakpoint command lists.
We're not terribly consistent in the printing stuff --- in some cases
we end up showing that "<error reading variable: %s>", while in
other cases we let the error escape, like in "p *0".

> bpstat_clear_actions is not so strong as script_from_file abort 
> but I find it similar and maybe confusing their conditions are not the same.  I find wrong
> that `gdb -nx -x ./cmd' stops on first error 

I'd find it wrong that a command sequence continues
blindly ignoring previous errors by default.

I think the neatest fix would be to add some try/catch/finaly
syntax to the cli.  There was a patch for that posted eons ago:

http://sourceware.org/ml/gdb-patches/2001-12/msg00449.html

I don't know what happened to it.  (Cagney's reply raises
some questions that would apply to a global continue-on-errors
flag equally.)

> as I have to run `gdb -nx <cmd'
> in such cases while that mode of run has other kinds of disadvantages.
> 
> Even if this patch gets fixed the way we were leading to it would still make
> changes in GDB behavior as GDB would much less call bpstat_clear_actions.

I think most if not all those changes are actually bug fixes.

> The tracepoint example below on FSF GDB HEAD does:
> 
> Found trace frame 0, tracepoint 4
> #0  globals_test_func () at ./gdb.trace/unavailable.cc:319
> Backtrace stopped: Not enough registers or memory available to unwind further
> No longer looking at any trace frame
> (gdb) _
> 
> while it does not print "still-executing" which is IMO a bug, do you agree?

I agree it's a bug.  The backtrace stopped gracefully, and completed,
it didn't throw any error back to the interpreter.

> If not printing "still-executing" is really a bug proposing to forget about
> the current patch and instead to:

But we're so close to having this fixed!  :-(  Did you find some
legitimate use case the patch breaks?

> (a) throw_exception will call bpstat_clear_actions only if exception.error is
>     not NOT_AVAILABLE_ERROR (adding later my new ENTRY_VALUE_NOT_AVAILABLE).

I don't like this at all.  I've given examples above how it's not
really just NOT_AVAILABLE_ERROR that matters.

> (b) bpstat_clear_actions should also abort script_from_file.

Hmm?

> (c) There should be a new setting "set error-stops-script" with default "on"
>     (backward compatibility) where "off" would disable bpstat_clear_actions
>     completely.  And I will happily use "set error-stops-script off" in my
>     ~/.gdbinit so that I can finally run `gdb -nx -x ./transcript.1'.

Some patch for something like that has been posted before too:

 <http://sourceware.org/ml/gdb-patches/2005-08/msg00120.html>

> 
> I will try to code it.
> 
> 
> Thanks,
> Jan
> 
> 
> cat >cmd <<EOH
> file gdb.trace/unavailable
> target remote localhost:1234
> break begin
> break end
> commands
>   tstop
>   tfind start
>   bt
>   tfind none
> end
> break end
> commands
>   echo still-executing\n
> end
> trace 319
> actions
> collect a
> end
> continue
> tstart
> continue
> EOH
> 
> ./gdbserver --once :1234 ../testsuite/gdb.trace/unavailable
> ../gdb -nx -x cmd 
> 
> 

-- 
Pedro Alves

Re: [patch 2/2] Do not bpstat_clear_actions on throw_exception #3

[Jan Kratochvil]

On Fri, 26 Aug 2011 13:45:32 +0200, Pedro Alves wrote:
> We're not terribly consistent in the printing stuff --- in some cases
> we end up showing that "<error reading variable: %s>", while in
> other cases we let the error escape, like in "p *0".
+
> I'd find it wrong that a command sequence continues
> blindly ignoring previous errors by default.

The problem is this "inconsistence" affects when the command sequences break
vs. when they continue.

What about save_gdb_index_command TRY_CATCH there?
TRY_CATCH + exception_fprintf
	Error while writing index for `objfile->name': except.message
This means the command succeeds even if some .gdb_index files could not be
produced.  I guess in this case there should have been rather final
throw_error so that save_gdb_index_command as whole throws one exception.


But it is a nice direction that any command with uncaught exception should
stop execution + clear all bpstats.  And any command that has all the
exceptions caught should continue execution + must not clear any bpstats.

Whatever violates these rules is a bug in the command implementation and not
in the GDB scripts execution control.


> I think the neatest fix would be to add some try/catch/finaly
> syntax to the cli.  There was a patch for that posted eons ago:
> 
> http://sourceware.org/ml/gdb-patches/2001-12/msg00449.html

I find this more as obsolete now, for advanced scripting there is already
Python or a control via MI by Perl, it does not make sense to further extend
the canned sequences of commands.


> I think most if not all those changes are actually bug fixes.
+
> I agree it's a bug.  The backtrace stopped gracefully, and completed,
> it didn't throw any error back to the interpreter.

I was considered any change to be a regression; great if it is the opposite.


> But we're so close to having this fixed!  :-(  Did you find some
> legitimate use case the patch breaks?

For example it changes the tracepoint example but it is great if it is
considered a bugfix.  It will also execute more bpstats now but therefore it
is not a bug but a feature.

In fact it also unifies the conditions under which bpstats gets clear and
canned commands sequence gets aborted.


> > (b) bpstat_clear_actions should also abort script_from_file.
> 
> Hmm?

Currently a caught exception:
Calls bpstat_clear_actions but it continues execution of script_from_file.
and uncaught exception:
Calls bpstat_clear_actions and also abort execution of script_from_file.

I was proposing that any exception - even the caught one should:
Call bpstat_clear_actions and also abort execution of script_from_file.

You are proposing and I am going to follow that a caught exception:
Does not call bpstat_clear_actions, it continues execution of script_from_file.
while an uncaught exception:
Call bpstat_clear_actions and also abort execution of script_from_file.

My goal was to unify bpstat_clear_actions and script_from_file abortion which
gets accomplished also with your proposal.


> > (c) There should be a new setting "set error-stops-script" with default "on"
> >     (backward compatibility) where "off" would disable bpstat_clear_actions
> >     completely.  And I will happily use "set error-stops-script off" in my
> >     ~/.gdbinit so that I can finally run `gdb -nx -x ./transcript.1'.
> 
> Some patch for something like that has been posted before too:
> 
>  <http://sourceware.org/ml/gdb-patches/2005-08/msg00120.html>

This simple patch looks cooked almost in acceptable state, I will hopefully
finish / check it in to make the testsuite more debuggable.


Thanks,
Jan

Re: [patch 2/2] Do not bpstat_clear_actions on throw_exception #3

[Pedro Alves]

On Friday 26 August 2011 14:16:58, Jan Kratochvil wrote:
> On Fri, 26 Aug 2011 13:45:32 +0200, Pedro Alves wrote:
> > We're not terribly consistent in the printing stuff --- in some cases
> > we end up showing that "<error reading variable: %s>", while in
> > other cases we let the error escape, like in "p *0".
> +
> > I'd find it wrong that a command sequence continues
> > blindly ignoring previous errors by default.
> 
> The problem is this "inconsistence" affects when the command sequences break
> vs. when they continue.
> 
> What about save_gdb_index_command TRY_CATCH there?
> TRY_CATCH + exception_fprintf
> 	Error while writing index for `objfile->name': except.message
> This means the command succeeds even if some .gdb_index files could not be
> produced.  I guess in this case there should have been rather final
> throw_error so that save_gdb_index_command as whole throws one exception.

Yeah, this one's not clear.  The command loops over all objfiles, so
some indexes may have been written.  Maybe it's a matter of documenting
the behavior.

> But it is a nice direction that any command with uncaught exception should
> stop execution + clear all bpstats.  And any command that has all the
> exceptions caught should continue execution + must not clear any bpstats.
>
> Whatever violates these rules is a bug in the command implementation and not
> in the GDB scripts execution control.

Exactly.

> > I think the neatest fix would be to add some try/catch/finaly
> > syntax to the cli.  There was a patch for that posted eons ago:
> > 
> > http://sourceware.org/ml/gdb-patches/2001-12/msg00449.html
> 
> I find this more as obsolete now, for advanced scripting there is already
> Python or a control via MI by Perl, it does not make sense to further extend
> the canned sequences of commands.

I don't agree, but I'm not going to pursue it either now.

I did have a dream of replacing the whole CLI implentation
by a tcl based one (since the syntax is quite similar) sometime
ago.  :-)

> > > (b) bpstat_clear_actions should also abort script_from_file.
> > 
> > Hmm?
> 
> Currently a caught exception:
> Calls bpstat_clear_actions but it continues execution of script_from_file.
> and uncaught exception:
> Calls bpstat_clear_actions and also abort execution of script_from_file.
> 
> I was proposing that any exception - even the caught one should:
> Call bpstat_clear_actions and also abort execution of script_from_file.
> 
> You are proposing and I am going to follow that a caught exception:
> Does not call bpstat_clear_actions, it continues execution of script_from_file.
> while an uncaught exception:
> Call bpstat_clear_actions and also abort execution of script_from_file.
> 
> My goal was to unify bpstat_clear_actions and script_from_file abortion which
> gets accomplished also with your proposal.

Great, thanks.

> > > (c) There should be a new setting "set error-stops-script" with default "on"
> > >     (backward compatibility) where "off" would disable bpstat_clear_actions
> > >     completely.  And I will happily use "set error-stops-script off" in my
> > >     ~/.gdbinit so that I can finally run `gdb -nx -x ./transcript.1'.
> > 
> > Some patch for something like that has been posted before too:
> > 
> >  <http://sourceware.org/ml/gdb-patches/2005-08/msg00120.html>
> 
> This simple patch looks cooked almost in acceptable state, I will hopefully
> finish / check it in to make the testsuite more debuggable.

-- 
Pedro Alves

Re: [patch 2/2] Do not bpstat_clear_actions on throw_exception #3

[Tom Tromey]

Pedro> I think the neatest fix would be to add some try/catch/finaly
Pedro> syntax to the cli.  There was a patch for that posted eons ago:
Pedro> http://sourceware.org/ml/gdb-patches/2001-12/msg00449.html

This is also languishing in bugzilla.

I think it would be a decent addition.  While I tend to think that more
serious scripting should be done in Python now, I think there is also
room for CLI stuff.  (I don't have a hard-and-fast rule here; I don't
want to try to expand the CLI into a full-blown language, but I don't
mind additions much either.)

This won't address Jan's use case, but you dug up another patch that
does, so all is well :)

FWIW I wrote a simple "ignore-errors" command in Python to satisfy my
own occasional need for this.

Jan> (a) throw_exception will call bpstat_clear_actions only if
Jan> exception.error is not NOT_AVAILABLE_ERROR (adding later my new
Jan> ENTRY_VALUE_NOT_AVAILABLE).

Pedro> I don't like this at all.

Me neither.  This code has really got to go.

Tom

Re: [patch 2/2] Do not bpstat_clear_actions on throw_exception #3

[Jan Kratochvil]

On Wed, 24 Aug 2011 12:19:03 +0200, Pedro Alves wrote:
> Almost there, but not yet, sorry. :-(  The cleanup is installed before
> fetch_inferior_event (non-stop) switches inferior_ptid to the event thread
> with a cleanup that restores back to the previous selected thread, which
> means, this happens:
> 
> inferior_event_handler
>   -> install cleanup to clear the bpstat of the current thread
>   -> fetch_inferior_event
>     -> install cleanup to restore the selected thread 
>     -> switch to the event thread
>     -> handle_inferior_event
>        -> bpstat_stop_status on the now current event thread
>        -> some error is thrown, e.g., while unwinding for step/next.
>             -> the cleanup to restore the previous thread is ran
>             -> the cleanup to clear the bpstat of the current
>                thread is ran, but the current thread is not the
>                event thread.  The bpstat of the event thread is left
>                handling.
> 
> Installing the cleanup in fetch_inferior_event _after_
> make_cleanup_restore_current_thread would fix this.

OK this way?  It fixed the reproducer (unsuitable for the testsuite) for me.


Thanks,
Jan


gdb/
2011-08-26  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* breakpoint.c (bpstat_do_actions): New variable cleanup_if_error, call
	make_bpstat_clear_actions_cleanup and discard_cleanups for it.
	* defs.h (make_bpstat_clear_actions_cleanup): New declaration.
	* exceptions.c (throw_exception): Remove the bpstat_clear_actions call.
	* inf-loop.c (inferior_event_handler): New variable cleanup_if_error,
	call make_bpstat_clear_actions_cleanup and discard_cleanups for it.
	Call bpstat_clear_actions for failed fetch_inferior_event_wrapper.
	* infrun.c (fetch_inferior_event): Call
	make_bpstat_clear_actions_cleanup.
	* top.c (execute_command): New variable cleanup_if_error, call
	make_bpstat_clear_actions_cleanup and discard_cleanups for it.
	* utils.c (do_bpstat_clear_actions_cleanup)
	(make_bpstat_clear_actions_cleanup): New functions.

gdb/testsuite/
2011-08-26  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.base/commands.exp (error_clears_commands_left): New function.
	(): Call it.

--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -3352,6 +3352,8 @@ bpstat_do_actions_1 (bpstat *bsp)
 void
 bpstat_do_actions (void)
 {
+  struct cleanup *cleanup_if_error = make_bpstat_clear_actions_cleanup ();
+
   /* Do any commands attached to breakpoint we are stopped at.  */
   while (!ptid_equal (inferior_ptid, null_ptid)
 	 && target_has_execution
@@ -3363,6 +3365,8 @@ bpstat_do_actions (void)
        indicate the inferior was not resumed.  */
     if (!bpstat_do_actions_1 (&inferior_thread ()->control.stop_bpstat))
       break;
+
+  discard_cleanups (cleanup_if_error);
 }
 
 /* Print out the (old or new) value associated with a watchpoint.  */
--- a/gdb/defs.h
+++ b/gdb/defs.h
@@ -429,6 +429,8 @@ extern const char *gdb_bfd_errmsg (bfd_error_type error_tag, char **matching);
 
 extern int parse_pid_to_attach (char *args);
 
+extern struct cleanup *make_bpstat_clear_actions_cleanup (void);
+
 /* From demangle.c */
 
 extern void set_demangling_style (char *);
--- a/gdb/exceptions.c
+++ b/gdb/exceptions.c
@@ -210,10 +210,6 @@ throw_exception (struct gdb_exception exception)
   quit_flag = 0;
   immediate_quit = 0;
 
-  /* Perhaps it would be cleaner to do this via the cleanup chain (not sure
-     I can think of a reason why that is vital, though).  */
-  bpstat_clear_actions ();
-
   do_cleanups (ALL_CLEANUPS);
 
   /* Jump to the containing catch_errors() call, communicating REASON
--- a/gdb/inf-loop.c
+++ b/gdb/inf-loop.c
@@ -42,6 +42,7 @@ inferior_event_handler (enum inferior_event_type event_type,
 {
   struct gdb_exception e;
   int was_sync = 0;
+  struct cleanup *cleanup_if_error = make_bpstat_clear_actions_cleanup ();
 
   switch (event_type)
     {
@@ -53,6 +54,7 @@ inferior_event_handler (enum inferior_event_type event_type,
       if (!catch_errors (fetch_inferior_event_wrapper, 
 			 client_data, "", RETURN_MASK_ALL))
 	{
+	  bpstat_clear_actions ();
 	  do_all_intermediate_continuations (1);
 	  do_all_continuations (1);
 	  async_enable_stdin ();
@@ -142,6 +144,8 @@ inferior_event_handler (enum inferior_event_type event_type,
       printf_unfiltered (_("Event type not recognized.\n"));
       break;
     }
+
+  discard_cleanups (cleanup_if_error);
 }
 
 static int 
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -2779,6 +2779,10 @@ fetch_inferior_event (void *client_data)
   else
     ts_old_chain = make_cleanup (finish_thread_state_cleanup, &ecs->ptid);
 
+  /* Get executed before make_cleanup_restore_current_thread above to apply
+     still for the thread which has thrown the exception.  */
+  make_bpstat_clear_actions_cleanup ();
+
   /* Now figure out what to do with the result of the result.  */
   handle_inferior_event (ecs);
 
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -368,12 +368,13 @@ prepare_execute_command (void)
 void
 execute_command (char *p, int from_tty)
 {
-  struct cleanup *cleanup;
+  struct cleanup *cleanup_if_error, *cleanup;
   struct cmd_list_element *c;
   enum language flang;
   static int warned = 0;
   char *line;
 
+  cleanup_if_error = make_bpstat_clear_actions_cleanup ();
   cleanup = prepare_execute_command ();
 
   /* Force cleanup of any alloca areas if using C alloca instead of
@@ -477,7 +478,8 @@ execute_command (char *p, int from_tty)
 	}
     }
 
-    do_cleanups (cleanup);
+  do_cleanups (cleanup);
+  discard_cleanups (cleanup_if_error);
 }
 
 /* Run execute_command for P and FROM_TTY.  Capture its output into the
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -3674,6 +3674,23 @@ parse_pid_to_attach (char *args)
   return pid;
 }
 
+/* Helper for make_bpstat_clear_actions_cleanup.  */
+
+static void
+do_bpstat_clear_actions_cleanup (void *unused)
+{
+  bpstat_clear_actions ();
+}
+
+/* Call bpstat_clear_actions for the case an exception is throw.  You should
+   discard_cleanups if no exception is caught.  */
+
+struct cleanup *
+make_bpstat_clear_actions_cleanup (void)
+{
+  return make_cleanup (do_bpstat_clear_actions_cleanup, NULL);
+}
+
 /* Provide a prototype to silence -Wmissing-prototypes.  */
 extern initialize_file_ftype _initialize_utils;
 
--- a/gdb/testsuite/gdb.base/commands.exp
+++ b/gdb/testsuite/gdb.base/commands.exp
@@ -678,6 +678,74 @@ proc if_commands_test {} {
     }
 }
 
+# Verify an error during "commands" commands execution will prevent any other
+# "commands" from other breakpoints at the same location to be executed.
+
+proc error_clears_commands_left {} {
+    set test "hook-stop 1"
+    gdb_test_multiple {define hook-stop} $test {
+	-re "End with a line saying just \"end\"\\.\r\n>$" {
+	    pass $test
+	}
+    }
+    set test "hook-stop 1a"
+    gdb_test_multiple {echo hook-stop1\n} $test {
+	-re "\r\n>$" {
+	    pass $test
+	}
+    }
+    gdb_test_no_output "end" "hook-stop 1b"
+
+    delete_breakpoints
+    gdb_breakpoint "main"
+
+    set test "main commands 1"
+    gdb_test_multiple {commands $bpnum} $test {
+	-re "End with a line saying just \"end\"\\.\r\n>$" {
+	    pass $test
+	}
+    }
+    set test "main commands 1a"
+    gdb_test_multiple {echo cmd1\n} $test {
+	-re "\r\n>$" {
+	    pass $test
+	}
+    }
+    set test "main commands 1b"
+    gdb_test_multiple {errorcommandxy\n} $test {
+	-re "\r\n>$" {
+	    pass $test
+	}
+    }
+    gdb_test_no_output "end" "main commands 1c"
+
+    gdb_breakpoint "main"
+    set test "main commands 2"
+    gdb_test_multiple {commands $bpnum} $test {
+	-re "End with a line saying just \"end\"\\.\r\n>$" {
+	    pass $test
+	}
+    }
+    set test "main commands 2a"
+    gdb_test_multiple {echo cmd2\n} $test {
+	-re "\r\n>$" {
+	    pass $test
+	}
+    }
+    set test "main commands 2b"
+    gdb_test_multiple {errorcommandyz\n} $test {
+	-re "\r\n>$" {
+	    pass $test
+	}
+    }
+    gdb_test_no_output "end" "main commands 2c"
+
+    gdb_run_cmd
+    gdb_test "" "\r\nhook-stop1\r\n.*\r\ncmd1\r\nUndefined command: \"errorcommandxy\"\\.  Try \"help\"\\." "cmd1 error"
+
+    gdb_test {echo idle\n} "\r\nidle" "no cmd2"
+}
+
 proc redefine_hook_test {} {
     global gdb_prompt
 
@@ -758,6 +826,7 @@ stray_arg0_test
 source_file_with_indented_comment
 recursive_source_test
 if_commands_test
+error_clears_commands_left
 redefine_hook_test
 # This one should come last, as it redefines "backtrace".
 redefine_backtrace_test

Re: [patch 2/2] Do not bpstat_clear_actions on throw_exception #3

[Pedro Alves]

On Friday 26 August 2011 22:04:20, Jan Kratochvil wrote:
> On Wed, 24 Aug 2011 12:19:03 +0200, Pedro Alves wrote:

> > Installing the cleanup in fetch_inferior_event _after_
> > make_cleanup_restore_current_thread would fix this.
> 
> OK this way?  It fixed the reproducer (unsuitable for the testsuite) for me.

Yes, thanks!

-- 
Pedro Alves

Re: [patch 2/2] Do not bpstat_clear_actions on throw_exception #3

[Jan Kratochvil]

On Fri, 26 Aug 2011 23:36:00 +0200, Pedro Alves wrote:
> Yes, thanks!

Checked in, sorry it took so many mails:
	http://sourceware.org/ml/gdb-cvs/2011-08/msg00120.html


Thanks,
Jan