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

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

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