Re: [BUG] queueing_enabled grows infinitely when in .recursive-edit

[Bart Schaefer <schaefer@xxxxxxxxxxxxxxxx>]

On Oct 2,  9:00pm, Sebastian Gniazdowski wrote:
}
} ...
} 
} This causes Ctrl-C signal to be queued when in .recursive-edit, what
} results in need of multiple Ctrl-C presses (errflag is set in middle
} of sequence of checks of it's value and raw_getbytes() executes in
} weird way). Also, errflag is set at random place after select() in
} raw_getbyte(), and as Bart says, this prevents scheduled function to
} execute at all, thus chain of rescheduling breaks.

There seem to be a bunch of inter-related things going on here.

The first is that recursiveedit() calls zlecore() which calls
getkeycmd() which cascades into raw_getbyte() with do_keytmout = 0
which in some circumstances means that raw_getbyte() effectively
does a blocking read on its first call and only runs the sched
after a key is pressed.  I haven't figured out what's wrong with
the calc_timeout() logic that makes this possible, but it's a
race -- it happens only once in a while.

The second is that zlecore() expects to be entered with the signal
queue disabled, but zle widgets are called with queueing enabled, so
recursiveedit() needs save/zero/restore the queue level around the
call to zlecore().

The third is that somewhere below execstring() from checksched(),
the signal queueing level is being incremented but not decremented.
The problem is that the execution code calls itself recursively so
deeply that I can't pinpoint the place this occurs.  Even with a
watchpoint on queueing_enabled, it looks as though we should be
fine -- it's as if the recursive calls never quite unwind all the
way back to the top, but printing stack traces in gdb doesn't show
that happening at any place where queueing_enabled changes.

Usually the third effect is hidden by the restore_queue_signals()
in getbyte(), but when in recursiveedit() the queue_signal_level()
in raw_getbyte() starts out > 0, and never goes all the way back
down again.  Or something like that.  I expected fixing the second
problem to again mask the third, but it does not.

It's almost like there's a tail-call optimization occuring that is
causing an unqueue_signals() to be skipped.  And in fact there are
cases where the queueing_enabled is decremented but the watchpoint
mysteriously does not trigger -- I see the smaller "old" value the
next time the watchpoint triggers on the *increment*, but I never
see the assignment that reduces the value.  I think this is because
gdb actually stops on the NEXT instruction AFTER the watched location,
and there are some places where there is no breakable next line (the
unqueue_signals() is the last line of a function).

It's quite normal for queueing_enabled to run up to 12 or so on a
normal execution stack and still decrement all the way back to zero,
so watching for a high-water mark is nearly useless.

Here's a patch (not to be committed) that causes a DPUTS() when
the condition is detected, but that doesn't help with tracking down
what causes it in the first place.

diff --git a/Src/Zle/zle_main.c b/Src/Zle/zle_main.c
index 0bdd82b..ba7ef90 100644
--- a/Src/Zle/zle_main.c
+++ b/Src/Zle/zle_main.c
@@ -1867,11 +1867,17 @@ int
 recursiveedit(UNUSED(char **args))
 {
     int locerror;
+    int q = queue_signal_level();
+
+    /* zlecore() expects to be entered with signal queue disabled */
+    dont_queue_signals();
 
     redrawhook();
     zrefresh();
     zlecore();
 
+    restore_queue_signals(q);
+
     locerror = errflag ? 1 : 0;
     errflag = done = eofsent = 0;
 
diff --git a/Src/signals.c b/Src/signals.c
index e2587dc..9b22dcd 100644
--- a/Src/signals.c
+++ b/Src/signals.c
@@ -67,7 +67,7 @@ static int exit_trap_posix;
 /* Variables used by signal queueing */
 
 /**/
-mod_export int queueing_enabled, queue_front, queue_rear;
+mod_export int queueing_enabled, queue_front, queue_rear, queue_in;
 /**/
 mod_export int signal_queue[MAX_QUEUE_SIZE];
 /**/
diff --git a/Src/signals.h b/Src/signals.h
index d680968..cb6a171 100644
--- a/Src/signals.h
+++ b/Src/signals.h
@@ -82,7 +82,7 @@
 
 #define MAX_QUEUE_SIZE 128
 
-#define queue_signals()    (queueing_enabled++)
+#define queue_signals()    (queue_in++, queueing_enabled++)
 
 #define run_queued_signals() do { \
     while (queue_front != queue_rear) {      /* while signals in queue */ \
@@ -96,17 +96,23 @@
 
 #define unqueue_signals()  do { \
     DPUTS(!queueing_enabled, "BUG: unqueue_signals called but not queueing"); \
+    --queue_in; \
     if (!--queueing_enabled) run_queued_signals(); \
 } while (0)
 
 #define queue_signal_level() queueing_enabled
 
 #define dont_queue_signals() do { \
+    queue_in = queueing_enabled; \
     queueing_enabled = 0; \
     run_queued_signals(); \
 } while (0)
 
-#define restore_queue_signals(q) (queueing_enabled = (q))
+#define restore_queue_signals(q) do { \
+    DPUTS2(queueing_enabled && queue_in != q, \
+         "BUG: q = %d != queue_in = %d", q, queue_in); \
+    queue_in = (queueing_enabled = (q)); \
+} while (0)
 
 #ifdef BSD_SIGNALS
 #define signal_block(S) sigblock(S)