Re: local_traps doesn't restore traps set from functions

[Daniel Shahaf <d.s@daniel.shahaf.name>]

Daniel Shahaf wrote on Sat, 09 May 2020 15:42 +0000:
> Peter Stephenson wrote on Sat, 09 May 2020 16:24 +0100:
> > > On 07 May 2020 at 22:21 Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> > > Roman Perepelitsa wrote on Wed, 06 May 2020 15:31 +0200:    
> > > > It appears that local_traps doesn't restore traps that were originally
> > > > set from functions.    
> > > 
> > > Precisely.
> > > 
> > > The following patch fixes it:    
> > 
> > OK, so the point here is that in this case the trap we're fiddling with
> > is not going out of scope, nor having something else restored over it,
> > it just happens to have been created inside a function.  At this
> > point, that fact becomes irrelevant, so we simply massage the information
> > to make it behave as if it had been created at the current (new) function
> > depth.
> > 
> > That seems fine --- my only comment would be it would probably be sensible
> > to insert some edited version of the previous paragraph as I for one
> > am almost certain to wonder what's going on if I encounter this code
> > in the future.  
> 
> Will do.  Thanks for the review!

Here's an interdiff, to be applied on top of 45790.

Test will follow separately.

diff --git a/Src/signals.c b/Src/signals.c
index 5d0bae7f5..56b3071de 100644
--- a/Src/signals.c
+++ b/Src/signals.c
@@ -30,8 +30,16 @@
 #include "zsh.mdh"
 #include "signals.pro"
  
-/* Array describing the state of each signal: an element contains *
- * 0 for the default action or some ZSIG_* flags ored together.   */
+/* 
+ * Array describing the state of each signal: an element contains
+ * 0 for the default action or some ZSIG_* flags ored together.
+ *
+ * The high bits (see ZSIG_SHIFT) identify the locallevel that owns the trap.
+ * Whenever execution returns to that locallevel (which is never larger/deeper
+ * than the current locallevel) from deeper levels, any function-scoped traps
+ * that shadow this trap will have gone out of scope.  See the LOCALTRAPS
+ * condition in removetrap().
+ */
 
 /**/
 mod_export int sigtrapped[VSIGCOUNT];
@@ -997,9 +1005,12 @@ removetrap(int sig)
     queue_signals();
     trapped = sigtrapped[sig];
     /*
+     * Save the trap if we should restore it at the end of the current function.
+     *
      * Note that we save the trap here even if there isn't an existing
-     * one, to aid in removing this one.  However, if there's
-     * already one at the current locallevel we just overwrite it.
+     * one, to aid in removing this one.  We also save the trap if it's owned
+     * by someone up the callstack.  However, if the trap is owned by the
+     * current locallevel we just overwrite it.
      *
      * Note we save EXIT traps based on the *current* setting of
      * POSIXTRAPS --- so if there is POSIX EXIT trap set but
@@ -1062,6 +1073,16 @@ removetrap(int sig)
     return NULL;
 }
 
+/*
+ * Return the value of ARG after setting all but the N least significant bits
+ * to zero.
+ */
+static unsigned int
+low_N_bits_of(unsigned int arg, unsigned char N)
+{
+    return arg & ((1u << N)-1u);
+}
+
 /**/
 void
 starttrapscope(void)
@@ -1114,6 +1135,9 @@ endtrapscope(void)
 	sigtrapped[SIGEXIT] = 0;
     }
 
+    /*
+     * Restore traps from savetraps to sigtrapped.
+     */
     if (savetraps) {
 	while ((ln = firstnode(savetraps)) &&
 	       (st = (struct savetrap *) ln->dat) &&
@@ -1122,6 +1146,8 @@ endtrapscope(void)
 
 	    remnode(savetraps, ln);
 
+	    /* ### This doesn't mask ZSIG_IGNORED out of the boolean check,
+	     * ### presumably because checking st->list is sufficient? */
 	    if (st->flags && (st->list != NULL)) {
 		/* prevent settrap from saving this */
 		dontsavetrap++;
@@ -1154,7 +1180,11 @@ endtrapscope(void)
 	}
     }
 
-    /* Fixup locallevel of signals. */
+    /*
+     * If the function that's in the process of returning set a global trap,
+     * that trap is now owned by that function's caller.  Update sigtrapped
+     * accordingly.
+     */
     {
 	int i;
 	for (i = 0; i < VSIGCOUNT; ++i) {
@@ -1170,7 +1200,7 @@ endtrapscope(void)
 		 * Keep the low ZSIG_SHIFT bits unchanged.
 		 */
 		sigtrapped[i] =
-		    (sigtrapped[i] & ((1u << ZSIG_SHIFT) - 1u))
+		    low_N_bits_of(sigtrapped[i], ZSIG_SHIFT)
 		    |
 		    (locallevel << ZSIG_SHIFT);
 	    }