zsh-workers
 help / color / mirror / code / Atom feed
* local_traps doesn't restore traps set from functions
@ 2020-05-06 13:31 Roman Perepelitsa
  2020-05-07 21:21 ` Daniel Shahaf
  0 siblings, 1 reply; 7+ messages in thread
From: Roman Perepelitsa @ 2020-05-06 13:31 UTC (permalink / raw)
  To: Zsh hackers list

Is this working as intended?

  ❯ zsh -f
  adam% () { trap '' INT }
  adam% () { emulate -L zsh; trap 'echo INT' INT }
  adam% kill -INT $$
  INT
  adam%

It appears that local_traps doesn't restore traps that were originally
set from functions.

If the first trap is set from global scope, it works as expected:

  ❯ zsh -f
  adam% trap '' INT
  adam% () { emulate -L zsh; trap 'echo INT' INT }
  adam% kill -INT $$
  adam%

If the first trap is not set at all, it also works as expected:

  ❯ zsh -f
  adam% () { emulate -L zsh; trap 'echo INT' INT }
  adam% kill -INT $$
  adam%

ZSH_PATCHLEVEL is zsh-5.8-126-gab835f0 (tip of master at the time of writing).

Roman.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: local_traps doesn't restore traps set from functions
  2020-05-06 13:31 local_traps doesn't restore traps set from functions Roman Perepelitsa
@ 2020-05-07 21:21 ` Daniel Shahaf
  2020-05-08  6:51   ` Roman Perepelitsa
  2020-05-09 15:24   ` Peter Stephenson
  0 siblings, 2 replies; 7+ messages in thread
From: Daniel Shahaf @ 2020-05-07 21:21 UTC (permalink / raw)
  To: Roman Perepelitsa; +Cc: Zsh hackers list

Roman Perepelitsa wrote on Wed, 06 May 2020 15:31 +0200:
> Is this working as intended?
> 
>   ❯ zsh -f
>   adam% () { trap '' INT }
>   adam% () { emulate -L zsh; trap 'echo INT' INT }
>   adam% kill -INT $$
>   INT
>   adam%
> 

I'd say it doesn't, since the trap _is_ set before the second function is called —
.
    % () { trap '' INT }
    % kill -INT $$
    % 
.
— and the documentation is quite clear about the semantics in this case:

       LOCAL_TRAPS <K>
              If this option is set when a signal trap is set inside a
              function, then the previous status of the trap for that signal
              will be restored when the function exits.

> It appears that local_traps doesn't restore traps that were originally
> set from functions.

Precisely.

The following patch fixes it:

[[[
diff --git a/Src/signals.c b/Src/signals.c
index 4adf03202..3e4fdf370 100644
--- a/Src/signals.c
+++ b/Src/signals.c
@@ -1154,6 +1154,29 @@ endtrapscope(void)
 	}
     }
 
+    /* Fixup locallevel of signals. */
+    {
+	int i;
+	for (i = 0; i < VSIGCOUNT; ++i) {
+	    int locallevel_of_signal = (sigtrapped[i] >> ZSIG_SHIFT);
+	    if (locallevel_of_signal > locallevel) {
+		DPUTS3(locallevel_of_signal != locallevel + 1,
+		    "BUG: signal %s's locallevel unexpected: %d>>ZSIG_SHIFT seen, v. %d+1 expected",
+		    sigs[i], sigtrapped[i], locallevel);
+
+		/* 
+		 * Decrement the locallevel embedded in sigtrapped[i].
+		 *
+		 * Keep the low ZSIG_SHIFT bits unchanged.
+		 */
+		sigtrapped[i] =
+		    (sigtrapped[i] & ((1u << ZSIG_SHIFT) - 1u))
+		    |
+		    (locallevel << ZSIG_SHIFT);
+	    }
+	}
+    }
+
     if (exittr) {
 	/*
 	 * We already made sure this wasn't set as a POSIX exit trap.
diff --git a/Src/zsh.h b/Src/zsh.h
index 1f2d774a1..71ba6e6e0 100644
--- a/Src/zsh.h
+++ b/Src/zsh.h
@@ -2940,8 +2940,8 @@ struct heap {
 #define ZSIG_FUNC	(1<<2)	/* Trap is a function, not an eval list */
 /* Mask to get the above flags */
 #define ZSIG_MASK	(ZSIG_TRAPPED|ZSIG_IGNORED|ZSIG_FUNC)
-/* No. of bits to shift local level when storing in sigtrapped */
 #define ZSIG_ALIAS	(1<<3)  /* Trap is stored under an alias */
+/* No. of bits to shift locallevel when storing in sigtrapped */
 #define ZSIG_SHIFT	4
 
 /*
]]]

[[[
% () { trap '' INT }
% () { emulate -L zsh; trap 'echo INT' INT }
% kill -INT $$
% 
]]]

However, I don't know this area of the code well enough to be sure the
patch doesn't break anything.  Reviews would be appreciated.

Also, if someone could write a regression test for this, that'd be great.

Cheers,

Daniel

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: local_traps doesn't restore traps set from functions
  2020-05-07 21:21 ` Daniel Shahaf
@ 2020-05-08  6:51   ` Roman Perepelitsa
  2020-05-09 14:14     ` Daniel Shahaf
  2020-05-09 15:24   ` Peter Stephenson
  1 sibling, 1 reply; 7+ messages in thread
From: Roman Perepelitsa @ 2020-05-08  6:51 UTC (permalink / raw)
  To: Daniel Shahaf; +Cc: Zsh hackers list

On Thu, May 7, 2020 at 11:21 PM Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
>
> The following patch fixes it:

Thanks for the patch. I confirm that it works. I've tried a handful of
different combinations of traps and all work as expected.

One stylistic suggestion for the patch: replace sigtrapped[i] fixup with this:

    sigtrapped[i] -= 1 << ZSIG_SHIFT;

It'll align the code with the preceding comment to the point of making
the comment superfluous. I also find it easier to understand although
that may be subjective.

Roman.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: local_traps doesn't restore traps set from functions
  2020-05-08  6:51   ` Roman Perepelitsa
@ 2020-05-09 14:14     ` Daniel Shahaf
  0 siblings, 0 replies; 7+ messages in thread
From: Daniel Shahaf @ 2020-05-09 14:14 UTC (permalink / raw)
  To: Roman Perepelitsa; +Cc: Zsh hackers list

Roman Perepelitsa wrote on Fri, 08 May 2020 08:51 +0200:
> On Thu, May 7, 2020 at 11:21 PM Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> >
> > The following patch fixes it:  
> 
> Thanks for the patch. I confirm that it works. I've tried a handful of
> different combinations of traps and all work as expected.
> 

Thanks for testing.

> One stylistic suggestion for the patch: replace sigtrapped[i] fixup with this:
> 
>     sigtrapped[i] -= 1 << ZSIG_SHIFT;
> 
> It'll align the code with the preceding comment to the point of making
> the comment superfluous. I also find it easier to understand although
> that may be subjective.

Good point, thanks.  Perhaps it'd be clearer if I wrapped the first
disjunct by a macro, so it'd have a name and a doc string:

/* ... */
static unsigned int low_N_bits_of(unsigned arg, unsigned char N) { return arg & ((1u << N)-1u); }

(Function, rather than macro, for type safety in case the arguments are
passed in the wrong order.)

Cheers,

Daniel

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: local_traps doesn't restore traps set from functions
  2020-05-07 21:21 ` Daniel Shahaf
  2020-05-08  6:51   ` Roman Perepelitsa
@ 2020-05-09 15:24   ` Peter Stephenson
  2020-05-09 15:42     ` Daniel Shahaf
  1 sibling, 1 reply; 7+ messages in thread
From: Peter Stephenson @ 2020-05-09 15:24 UTC (permalink / raw)
  To: Zsh hackers list

> 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.

cheers
pws

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: local_traps doesn't restore traps set from functions
  2020-05-09 15:24   ` Peter Stephenson
@ 2020-05-09 15:42     ` Daniel Shahaf
  2020-05-09 19:51       ` Daniel Shahaf
  0 siblings, 1 reply; 7+ messages in thread
From: Daniel Shahaf @ 2020-05-09 15:42 UTC (permalink / raw)
  To: Peter Stephenson; +Cc: Zsh hackers list

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!

Daniel

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: local_traps doesn't restore traps set from functions
  2020-05-09 15:42     ` Daniel Shahaf
@ 2020-05-09 19:51       ` Daniel Shahaf
  0 siblings, 0 replies; 7+ messages in thread
From: Daniel Shahaf @ 2020-05-09 19:51 UTC (permalink / raw)
  To: Zsh hackers list

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);
 	    }

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2020-05-09 19:51 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-06 13:31 local_traps doesn't restore traps set from functions Roman Perepelitsa
2020-05-07 21:21 ` Daniel Shahaf
2020-05-08  6:51   ` Roman Perepelitsa
2020-05-09 14:14     ` Daniel Shahaf
2020-05-09 15:24   ` Peter Stephenson
2020-05-09 15:42     ` Daniel Shahaf
2020-05-09 19:51       ` Daniel Shahaf

Code repositories for project(s) associated with this public inbox

	https://git.vuxu.org/mirror/zsh/

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).