Re: [patch] Do not bpstat_clear_actions on throw_exception

[Jan Kratochvil <jan.kratochvil@redhat.com>]

On Mon, 08 Aug 2011 17:44:08 +0200, Pedro Alves wrote:
> and run to foo.  After seeing the error, try any other
> command, like "p 5".  You'll see the breakpoint commands
> of the other breakpoint running...

Thank you very much, I agree now, created a testcase for it.


> Things get more complicated with any error that is thrown
> from between bpstat_stop_status, and actually running the
> breakpoint commands.  You'll also leave bs->commands_left
> set then.

The code path execute_command...bpstat_do_actions is multiple times in GDB
(CLI, MI, bpstat_do_actions from continuations etc.).   The path
execute_command...bpstat_do_actions is currently unprotected by any TRY_CATCH,
therefore I see the only possibility to clear commands_left _before_
execute_command.  One could protect execute_command...bpstat_do_actions but
that is present at multiple places and I find the patch below also OK.

I rather cleared commands/commands_left before starting a new command as it
seems easily unified in GDB.

There is also normal_stop but that is before the commands get executed at all.


> Maybe bs->commands_left shouldn't be set by
> bpstat_stop_status at all.

It already needs to check for the "silent" statement.  Besides it if
"commands" are fetched in bpstat_do_actions_1 the unwanted execution you have
shown to me can also happen.

Or could you describe more the idea you had?


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


Thanks,
Jan


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

	* exceptions.c (throw_exception): Move variable tp, its initialization
	and the call of bpstat_clear_actions to ...
	* top.c (prepare_execute_command): ... here.

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

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

--- a/gdb/exceptions.c
+++ b/gdb/exceptions.c
@@ -207,22 +207,9 @@ exceptions_state_mc_action_iter_1 (void)
 void
 throw_exception (struct gdb_exception exception)
 {
-  struct thread_info *tp = NULL;
-
   quit_flag = 0;
   immediate_quit = 0;
 
-  if (!ptid_equal (inferior_ptid, null_ptid))
-    tp = find_thread_ptid (inferior_ptid);
-
-  /* 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).  */
-  if (tp != NULL)
-    {
-      /* Clear queued breakpoint commands.  */
-      bpstat_clear_actions (tp->control.stop_bpstat);
-    }
-
   do_cleanups (ALL_CLEANUPS);
 
   /* Jump to the containing catch_errors() call, communicating REASON
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -348,6 +348,7 @@ prepare_execute_command (void)
 {
   struct value *mark;
   struct cleanup *cleanup;
+  struct thread_info *tp = NULL;
 
   mark = value_mark ();
   cleanup = make_cleanup_value_free_to_mark (mark);
@@ -359,6 +360,17 @@ prepare_execute_command (void)
   if (non_stop)
     target_dcache_invalidate ();
 
+  if (!ptid_equal (inferior_ptid, null_ptid))
+    tp = find_thread_ptid (inferior_ptid);
+
+  /* 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).  */
+  if (tp != NULL)
+    {
+      /* Clear queued breakpoint commands.  */
+      bpstat_clear_actions (tp->control.stop_bpstat);
+    }
+
   return cleanup;
 }
 
--- a/gdb/testsuite/gdb.base/commands.exp
+++ b/gdb/testsuite/gdb.base/commands.exp
@@ -678,6 +678,61 @@ 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 {} {
+    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 "" "cmd1\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 +813,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