Subject: Re: bug#24047: Should 'signal' sometimes return?
From: Paul Eggert <[email-masked]>
To: Stefan Monnier <[email-masked]>
Cc: [email-masked]
Date: Mon, 25 Jul 2016 00:42:27 +0200
List-Id: <debbugs-submit.debbugs.gnu.org>

On 07/23/2016 08:37 PM, Stefan Monnier wrote:
> Haven't looked in detail, but it looks OK.

Thanks, I installed it in 'master' and am marking this as done.

>
> A further patch could change the new `quit' function so it checks
> debug-on-quit, and either calls the debugger or calls Fsignal.  This way
> we don't need the intermediate signal_or_quit function.

I tried something along those lines (see attached), but wasn't convinced 
that the result was correct (there are a lot of flags flying around), 
and my patch would cause quits to traverse the handler list twice, which 
seems inelegant.

diff --git a/src/eval.c b/src/eval.c
index 33b82f7..74e1409 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -1431,10 +1431,9 @@ struct handler *
 }
 
 
-static Lisp_Object signal_or_quit (Lisp_Object, Lisp_Object, bool);
-static Lisp_Object find_handler_clause (Lisp_Object, Lisp_Object);
-static bool maybe_call_debugger (Lisp_Object conditions, Lisp_Object sig,
-				 Lisp_Object data);
+static struct handler const return_nil_handler = { 0, };
+static struct handler *find_condition_handler (Lisp_Object, Lisp_Object,
+					       Lisp_Object);
 
 void
 process_quit_flag (void)
@@ -1465,25 +1464,6 @@ static bool maybe_call_debugger (Lisp_Object conditions, Lisp_Object sig,
        attributes: noreturn)
   (Lisp_Object error_symbol, Lisp_Object data)
 {
-  signal_or_quit (error_symbol, data, false);
-  eassume (false);
-}
-
-/* Quit, in response to a keyboard quit request.  */
-Lisp_Object
-quit (void)
-{
-  return signal_or_quit (Qquit, Qnil, true);
-}
-
-/* Signal an error, or quit.  ERROR_SYMBOL and DATA are as with Fsignal.
-   If KEYBOARD_QUIT, this is a quit; ERROR_SYMBOL should be
-   Qquit and DATA should be Qnil, and this function may return.
-   Otherwise this function is like Fsignal and does not return.  */
-
-static Lisp_Object
-signal_or_quit (Lisp_Object error_symbol, Lisp_Object data, bool keyboard_quit)
-{
   /* When memory is full, ERROR-SYMBOL is nil,
      and DATA is (REAL-ERROR-SYMBOL . REAL-DATA).
      That is a special case--don't do this in other situations.  */
@@ -1491,8 +1471,6 @@ static bool maybe_call_debugger (Lisp_Object conditions, Lisp_Object sig,
   Lisp_Object string;
   Lisp_Object real_error_symbol
     = (NILP (error_symbol) ? Fcar (data) : error_symbol);
-  register Lisp_Object clause = Qnil;
-  struct handler *h;
 
   immediate_quit = 0;
   abort_on_gc = 0;
@@ -1537,37 +1515,8 @@ static bool maybe_call_debugger (Lisp_Object conditions, Lisp_Object sig,
 	Vsignaling_function = backtrace_function (pdl);
     }
 
-  for (h = handlerlist; h; h = h->next)
-    {
-      if (h->type != CONDITION_CASE)
-	continue;
-      clause = find_handler_clause (h->tag_or_ch, conditions);
-      if (!NILP (clause))
-	break;
-    }
-
-  if (/* Don't run the debugger for a memory-full error.
-	 (There is no room in memory to do that!)  */
-      !NILP (error_symbol)
-      && (!NILP (Vdebug_on_signal)
-	  /* If no handler is present now, try to run the debugger.  */
-	  || NILP (clause)
-	  /* A `debug' symbol in the handler list disables the normal
-	     suppression of the debugger.  */
-	  || (CONSP (clause) && !NILP (Fmemq (Qdebug, clause)))
-	  /* Special handler that means "print a message and run debugger
-	     if requested".  */
-	  || EQ (h->tag_or_ch, Qerror)))
-    {
-      bool debugger_called
-	= maybe_call_debugger (conditions, error_symbol, data);
-      /* We can't return values to code which signaled an error, but we
-	 can continue code which has signaled a quit.  */
-      if (keyboard_quit && debugger_called && EQ (real_error_symbol, Qquit))
-	return Qnil;
-    }
-
-  if (!NILP (clause))
+  struct handler *h = find_condition_handler (conditions, error_symbol, data);
+  if (h)
     {
       Lisp_Object unwind_data
 	= (NILP (error_symbol) ? data : Fcons (error_symbol, data));
@@ -1762,6 +1711,57 @@ static bool maybe_call_debugger (Lisp_Object conditions, Lisp_Object sig,
   return Qnil;
 }
 
+struct handler *
+find_condition_handler (Lisp_Object conditions, Lisp_Object error_symbol,
+			Lisp_Object data)
+{
+  Lisp_Object clause = Qnil;
+  struct handler *h;
+
+  for (h = handlerlist; h; h = h->next)
+    {
+      if (h->type != CONDITION_CASE)
+	continue;
+      clause = find_handler_clause (h->tag_or_ch, conditions);
+      if (!NILP (clause))
+	break;
+    }
+
+  if (/* Don't run the debugger for a memory-full error.
+	 (There is no room in memory to do that!)  */
+      !NILP (error_symbol)
+      && (!NILP (Vdebug_on_signal)
+	  /* If no handler is present now, try to run the debugger.  */
+	  || NILP (clause)
+	  /* A `debug' symbol in the handler list disables the normal
+	     suppression of the debugger.  */
+	  || (CONSP (clause) && !NILP (Fmemq (Qdebug, clause)))
+	  /* Special handler that means "print a message and run debugger
+	     if requested".  */
+	  || EQ (h->tag_or_ch, Qerror)))
+    {
+      /* We can't return values to code which signaled an error, but we
+	 can continue code which has signaled a quit.  */
+      if (maybe_call_debugger (conditions, error_symbol, data)
+	  && EQ (NILP (error_symbol) ? Fcar (data) : error_symbol, Qquit))
+	return (struct handler *) &return_nil_handler;
+    }
+
+  return h;
+}
+
+/* Quit, in response to a keyboard quit request.
+   Normally this is like (signal 'quit nil) and does not return.
+   However, the debugger can cause this to return.  */
+
+Lisp_Object
+quit (void)
+{
+  AUTO_LIST1 (conditions, Qquit);
+  if (find_condition_handler (conditions, Qquit, Qnil) != &return_nil_handler)
+    Fsignal (Qquit, Qnil);
+  return Qnil;
+}
 
 /* Format and return a string; called like vprintf.  */
 Lisp_Object