Don't throw quit while handling inferior events, part II

I noticed that if Ctrl-C was typed just while GDB is evaluating a
breakpoint condition in the background, and GDB ends up reaching out
to the Python interpreter, then the breakpoint condition would still
fail, like:

  c&
  Continuing.
  (gdb) Error in testing breakpoint condition:
  Quit

That happens because while evaluating the breakpoint condition, we
enter Python, and end up calling PyErr_SetInterrupt (it's called by
gdbpy_set_quit_flag, in frame #0):

 (top-gdb) bt
 #0  gdbpy_set_quit_flag (extlang=0x558c68f81900 <extension_language_python>) at ../../src/gdb/python/python.c:288
 #1  0x0000558c6845f049 in set_quit_flag () at ../../src/gdb/extension.c:785
 #2  0x0000558c6845ef98 in set_active_ext_lang (now_active=0x558c68f81900 <extension_language_python>) at ../../src/gdb/extension.c:743
 #3  0x0000558c686d3e56 in gdbpy_enter::gdbpy_enter (this=0x7fff2b70bb90, gdbarch=0x558c6ab9eac0, language=0x0) at ../../src/gdb/python/python.c:212
 #4  0x0000558c68695d49 in python_on_memory_change (inferior=0x558c6a830b00, addr=0x555555558014, len=4, data=0x558c6af8a610 "") at ../../src/gdb/python/py-inferior.c:146
 #5  0x0000558c6823a071 in std::__invoke_impl<void, void (*&)(inferior*, unsigned long, long, unsigned char const*), inferior*, unsigned long, long, unsigned char const*> (__f=@0x558c6a8ecd98: 0x558c68695d01 <python_on_memory_change(inferior*, CORE_ADDR, ssize_t, bfd_byte const*)>) at /usr/include/c++/11/bits/invoke.h:61
 #6  0x0000558c68237591 in std::__invoke_r<void, void (*&)(inferior*, unsigned long, long, unsigned char const*), inferior*, unsigned long, long, unsigned char const*> (__fn=@0x558c6a8ecd98: 0x558c68695d01 <python_on_memory_change(inferior*, CORE_ADDR, ssize_t, bfd_byte const*)>) at /usr/include/c++/11/bits/invoke.h:111
 #7  0x0000558c68233e64 in std::_Function_handler<void (inferior*, unsigned long, long, unsigned char const*), void (*)(inferior*, unsigned long, long, unsigned char const*)>::_M_invoke(std::_Any_data const&, inferior*&&, unsigned long&&, long&&, unsigned char const*&&) (__functor=..., __args#0=@0x7fff2b70bd40: 0x558c6a830b00, __args#1=@0x7fff2b70bd38: 93824992247828, __args#2=@0x7fff2b70bd30: 4, __args#3=@0x7fff2b70bd28: 0x558c6af8a610 "") at /usr/include/c++/11/bits/std_function.h:290
 #8  0x0000558c6830a96e in std::function<void (inferior*, unsigned long, long, unsigned char const*)>::operator()(inferior*, unsigned long, long, unsigned char const*) const (this=0x558c6a8ecd98, __args#0=0x558c6a830b00, __args#1=93824992247828, __args#2=4, __args#3=0x558c6af8a610 "") at /usr/include/c++/11/bits/std_function.h:590
 #9  0x0000558c6830a620 in gdb::observers::observable<inferior*, unsigned long, long, unsigned char const*>::notify (this=0x558c690828c0 <gdb::observers::memory_changed>, args#0=0x558c6a830b00, args#1=93824992247828, args#2=4, args#3=0x558c6af8a610 "") at ../../src/gdb/../gdbsupport/observable.h:166
 #10 0x0000558c68309d95 in write_memory_with_notification (memaddr=0x555555558014, myaddr=0x558c6af8a610 "", len=4) at ../../src/gdb/corefile.c:363
 #11 0x0000558c68904224 in value_assign (toval=0x558c6afce910, fromval=0x558c6afba6c0) at ../../src/gdb/valops.c:1190
 #12 0x0000558c681e3869 in expr::assign_operation::evaluate (this=0x558c6af8e150, expect_type=0x0, exp=0x558c6afcfe60, noside=EVAL_NORMAL) at ../../src/gdb/expop.h:1902
 #13 0x0000558c68450c89 in expr::logical_or_operation::evaluate (this=0x558c6afab060, expect_type=0x0, exp=0x558c6afcfe60, noside=EVAL_NORMAL) at ../../src/gdb/eval.c:2330
 #14 0x0000558c6844a896 in expression::evaluate (this=0x558c6afcfe60, expect_type=0x0, noside=EVAL_NORMAL) at ../../src/gdb/eval.c:110
 #15 0x0000558c6844a95e in evaluate_expression (exp=0x558c6afcfe60, expect_type=0x0) at ../../src/gdb/eval.c:124
 #16 0x0000558c682061ef in breakpoint_cond_eval (exp=0x558c6afcfe60) at ../../src/gdb/breakpoint.c:4971
 ...


The fix is to disable cooperative SIGINT handling while handling
inferior events, so that SIGINT is saved in the global quit flag, and
not in the extension language, while handling an event.

This commit augments the testcase added by the previous commit to test
this scenario as well.

Approved-By: Tom Tromey <[email protected]>
Change-Id: Idf8ab815774ee6f4b45ca2d0caaf30c9b9f127bb

gdb/extension.c

@@ -681,6 +681,35 @@ void (*hook_set_active_ext_lang) () = nullptr;
 }
 #endif
 
+/* True if cooperative SIGINT handling is disabled.  This is needed so
+   that calls to set_active_ext_lang do not re-enable cooperative
+   handling, which if enabled would make set_quit_flag store the
+   SIGINT in an extension language.  */
+static bool cooperative_sigint_handling_disabled = false;
+
+scoped_disable_cooperative_sigint_handling::scoped_disable_cooperative_sigint_handling ()
+{
+  /* Force the active extension language to the GDB scripting
+     language.  This ensures that a previously saved SIGINT is moved
+     to the quit_flag global, as well as ensures that future SIGINTs
+     are also saved in the global.  */
+  m_prev_active_ext_lang_state
+    = set_active_ext_lang (&extension_language_gdb);
+
+  /* Set the "cooperative SIGINT handling disabled" global flag, so
+     that a future call to set_active_ext_lang does not re-enable
+     cooperative mode.  */
+  m_prev_cooperative_sigint_handling_disabled
+    = cooperative_sigint_handling_disabled;
+  cooperative_sigint_handling_disabled = true;
+}
+
+scoped_disable_cooperative_sigint_handling::~scoped_disable_cooperative_sigint_handling ()
+{
+  cooperative_sigint_handling_disabled = m_prev_cooperative_sigint_handling_disabled;
+  restore_active_ext_lang (m_prev_active_ext_lang_state);
+}
+
 /* Set the currently active extension language to NOW_ACTIVE.
    The result is a pointer to a malloc'd block of memory to pass to
    restore_active_ext_lang.
@@ -702,7 +731,15 @@ void (*hook_set_active_ext_lang) () = nullptr;
    check_quit_flag is not called, the original SIGINT will be thrown.
    Non-cooperative extension languages are free to install their own SIGINT
    handler but the original must be restored upon return, either itself
-   or via restore_active_ext_lang.  */
+   or via restore_active_ext_lang.
+
+   If cooperative SIGINT handling is force-disabled (e.g., we're in
+   the middle of handling an inferior event), then we don't actually
+   record NOW_ACTIVE as the current active extension language, so that
+   set_quit_flag saves the SIGINT in the global quit flag instead of
+   in the extension language.  The caller does not need to concern
+   itself about this, though.  The currently active extension language
+   concept only exists for cooperative SIGINT handling.  */
 
 struct active_ext_lang_state *
 set_active_ext_lang (const struct extension_language_defn *now_active)
@@ -712,6 +749,22 @@ set_active_ext_lang (const struct extension_language_defn *now_active)
     selftests::hook_set_active_ext_lang ();
 #endif
 
+  /* If cooperative SIGINT handling was previously force-disabled,
+     make sure to not re-enable it (as NOW_ACTIVE could be a language
+     that supports cooperative SIGINT handling).  */
+  if (cooperative_sigint_handling_disabled)
+    {
+      /* Ensure set_quit_flag saves SIGINT in the quit_flag
+	 global.  */
+      gdb_assert (active_ext_lang->ops == nullptr
+		  || active_ext_lang->ops->check_quit_flag == nullptr);
+
+      /* The only thing the caller can do with the result is pass it
+	 to restore_active_ext_lang, which expects NULL when
+	 cooperative SIGINT handling is disabled.  */
+      return nullptr;
+    }
+
   struct active_ext_lang_state *previous
     = XCNEW (struct active_ext_lang_state);
 
@@ -743,6 +796,13 @@ set_active_ext_lang (const struct extension_language_defn *now_active)
 void
 restore_active_ext_lang (struct active_ext_lang_state *previous)
 {
+  if (cooperative_sigint_handling_disabled)
+    {
+      /* See set_active_ext_lang.  */
+      gdb_assert (previous == nullptr);
+      return;
+    }
+
   active_ext_lang = previous->ext_lang;
 
   if (target_terminal::is_ours ())

gdb/extension.h

@@ -343,4 +343,20 @@ extern void (*hook_set_active_ext_lang) ();
 }
 #endif
 
+/* Temporarily disable cooperative SIGINT handling.  Needed when we
+   don't want a SIGINT to interrupt the currently active extension
+   language.  */
+class scoped_disable_cooperative_sigint_handling
+{
+public:
+  scoped_disable_cooperative_sigint_handling ();
+  ~scoped_disable_cooperative_sigint_handling ();
+
+  DISABLE_COPY_AND_ASSIGN (scoped_disable_cooperative_sigint_handling);
+
+private:
+  struct active_ext_lang_state *m_prev_active_ext_lang_state;
+  bool m_prev_cooperative_sigint_handling_disabled;
+};
+
 #endif /* EXTENSION_H */

gdb/infrun.c

@@ -73,6 +73,7 @@
 #include "test-target.h"
 #include "gdbsupport/common-debug.h"
 #include "gdbsupport/buildargv.h"
+#include "extension.h"
 
 /* Prototypes for local functions */
 
@@ -4178,6 +4179,14 @@ fetch_inferior_event ()
   scoped_restore restore_quit_handler
     = make_scoped_restore (&quit_handler, infrun_quit_handler);
 
+  /* Make sure a SIGINT does not interrupt an extension language while
+     we're handling an event.  That could interrupt a Python unwinder
+     or a Python observer or some such.  A Ctrl-C should either be
+     forwarded to the inferior if the inferior has the terminal, or,
+     if GDB has the terminal, should interrupt the command the user is
+     typing in the CLI.  */
+  scoped_disable_cooperative_sigint_handling restore_coop_sigint;
+
   /* End up with readline processing input, if necessary.  */
   {
     SCOPE_EXIT { reinstall_readline_callback_handler_cleanup (); };

gdb/testsuite/gdb.base/bg-exec-sigint-bp-cond.c

@@ -15,6 +15,8 @@
    You should have received a copy of the GNU General Public License
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
+int global;
+
 int
 foo (void)
 {

gdb/testsuite/gdb.base/bg-exec-sigint-bp-cond.exp

@@ -26,7 +26,10 @@ if {[build_executable "failed to prepare" $testfile $srcfile debug]} {
 # SIGINT to GDB, and ensures that that doesn't make the breakpoint hit
 # cause a premature stop.  This emulates pressing Ctrl-C just while
 # GDB is evaluating the breakpoint condition.
-proc test {} {
+#
+# AFTER_KILL_COND is appended to the breakpoint condition, after "kill
+# -SIGINT $gdb_pid".
+proc test { {after_kill_cond ""} } {
     clean_restart $::binfile
 
     if {![runto_main]} {
@@ -48,7 +51,7 @@ proc test {} {
     # emulates pressing Ctrl-C just while GDB is evaluating the breakpoint
     # condition.
     gdb_test \
-	"break foo if \$hit_count\+\+ == $num_hits || \$_shell(\"kill -SIGINT $gdb_pid\") != 0" \
+	"break foo if \$hit_count\+\+ == $num_hits || \$_shell(\"kill -SIGINT $gdb_pid\") != 0 $after_kill_cond" \
 	"Breakpoint .*" \
 	"break foo if <condition>"
 
@@ -74,4 +77,22 @@ proc test {} {
     }
 }
 
-test
+# Test without writing to memory after killing GDB.  This does not
+# take any Python path at the time of writing.
+with_test_prefix "no force memory write" {
+    test
+}
+
+# Writing to memory from the condition makes GDB enter Python for
+# reporting a memory changed event.  Thus this tests that GDB doesn't
+# forward the SIGINT to Python, interrupting Python, causing the
+# breakpoint to prematurely stop like:
+#
+#  c&
+#  Continuing.
+#  (gdb) Error in testing breakpoint condition:
+#  Quit
+#
+with_test_prefix "force memory write" {
+    test " || (global = 0)"
+}