[PATCH] Remove NOERREXIT_UNTIL_EXEC

[PATCH] Remove NOERREXIT_UNTIL_EXEC

[Philippe Altherr]

[-- Attachment #1: Type: text/plain, Size: 3604 bytes --]

The noerrexit NOERREXIT_UNTIL_EXEC bit is never set and can therefore be
removed. In other words the patch only removes dead code.

I have been intrigued by the NOERREXIT_UNTIL_EXEC bit for a while. I didn't
understand its purpose nor why the NOERREXIT_EXIT and NOERREXIT_RETURN bits
wouldn't be enough to solve the problem at hand. After finally having a
closer look, I must admit that I still don't understand. However, I
discovered that the bit is never set and can safely be removed. More on
that below.

The bit was introduced in commit d6a32dd
<https://github.com/zsh-users/zsh/commit/d6a32ddeed914434f5b56b013c9d03b28781d065>,
where the noerrexit value 2 corresponds to today's NOERREXIT_UNTIL_EXEC and
the value 1 to NOERREXIT_EXIT. There was no distinction yet between
NOERREXIT_EXIT and NOERREXIT_RETURN. At that time, there was also no
this_noerrexit variable. The distinction between NOERREXIT_EXIT and
NOERREXIT_RETURN was introduced in commit 97d4bdb
<https://github.com/zsh-users/zsh/commit/97d4bdbc7e86e6e8da0d4a059b118ffab289d3a9>,
which also introduced today's noerrexit bits.

One thing that distinguishes the NOERREXIT_UNTIL_EXEC bit from the other
noerrexit bits is that it seems intended for the caller(s) of the function
that sets it, while the other bits are intended for the callee(s). This is
similar to today's this_noerrexit variable. I wonder whether this bit was a
first attempt at solving the problem that is solved in today's code by the
this_noerrexit variable. This would explain why everything is still fine
even though the bit is no longer set.

In today's code, the NOERREXIT_UNTIL_EXEC bit is only ever set in loop.c at
line 578
<https://github.com/zsh-users/zsh/blob/41b402d36d0aeac594cf424a9e46b5edb20c815d/Src/loop.c#L578>.
This line can only be reached if run != 0 && run !=2 && olderrexit == 0 &&
lastval == 0. The loop above
<https://github.com/zsh-users/zsh/blob/41b402d36d0aeac594cf424a9e46b5edb20c815d/Src/loop.c#L551>
can
only be exited at one of the following lines:

- line 551
<https://github.com/zsh-users/zsh/blob/41b402d36d0aeac594cf424a9e46b5edb20c815d/Src/loop.c#L551>=>
run == 0
- line 557
<https://github.com/zsh-users/zsh/blob/41b402d36d0aeac594cf424a9e46b5edb20c815d/Src/loop.c#L557>
=> run == 0 || run == 2
- line 565
<https://github.com/zsh-users/zsh/blob/41b402d36d0aeac594cf424a9e46b5edb20c815d/Src/loop.c#L565>
=> run == 1 && lastval != 0
- line 568
<https://github.com/zsh-users/zsh/blob/41b402d36d0aeac594cf424a9e46b5edb20c815d/Src/loop.c#L568>
=> run == 0 && lastval == 0

None of these cases lead to line 578. Thus the NOERREXIT_UNTIL_EXEC bit is
never set and can safely be removed.

The code "noerrexit &= ~ (NOERREXIT_EXIT | NOERREXIT_RETURN)" at line 580
<https://github.com/zsh-users/zsh/blob/41b402d36d0aeac594cf424a9e46b5edb20c815d/Src/loop.c#L580>
can
be simplified into "noerrexit = olderrexit" because NOERREXIT_UNTIL_EXEC
was the only noerrexit bit that was flowing from callees to their caller.

Obviously all tests still pass. And I checked that the tests
introduced by commit
d6a32dd
<https://github.com/zsh-users/zsh/commit/d6a32ddeed914434f5b56b013c9d03b28781d065>
are
still present.

I have built this patch on top of my patch
patch-06-fix-eval-and-source-statements.txt but I don't think that it
depends on it nor on any of my previous patches (but I haven't tried to
confirm it).

QUESTION: Should NOERREXIT_SIGNAL's value
<https://github.com/zsh-users/zsh/blob/41b402d36d0aeac594cf424a9e46b5edb20c815d/Src/zsh.h#L2226>
be changed to 4 (from 8) since the value 4 is no longer used
by NOERREXIT_UNTIL_EXEC?

Philippe

[-- Attachment #2: Type: text/html, Size: 4201 bytes --]

Re: [PATCH] Remove NOERREXIT_UNTIL_EXEC

[Philippe Altherr]

[-- Attachment #1.1: Type: text/plain, Size: 3883 bytes --]

Stupid me. I forgot the patch! Here it is.

Philippe


On Sat, Dec 3, 2022 at 1:31 AM Philippe Altherr <philippe.altherr@gmail.com>
wrote:

> The noerrexit NOERREXIT_UNTIL_EXEC bit is never set and can therefore be
> removed. In other words the patch only removes dead code.
>
> I have been intrigued by the NOERREXIT_UNTIL_EXEC bit for a while. I
> didn't understand its purpose nor why the NOERREXIT_EXIT and
> NOERREXIT_RETURN bits wouldn't be enough to solve the problem at hand.
> After finally having a closer look, I must admit that I still don't
> understand. However, I discovered that the bit is never set and can safely
> be removed. More on that below.
>
> The bit was introduced in commit d6a32dd
> <https://github.com/zsh-users/zsh/commit/d6a32ddeed914434f5b56b013c9d03b28781d065>,
> where the noerrexit value 2 corresponds to today's NOERREXIT_UNTIL_EXEC and
> the value 1 to NOERREXIT_EXIT. There was no distinction yet between
> NOERREXIT_EXIT and NOERREXIT_RETURN. At that time, there was also no
> this_noerrexit variable. The distinction between NOERREXIT_EXIT and
> NOERREXIT_RETURN was introduced in commit 97d4bdb
> <https://github.com/zsh-users/zsh/commit/97d4bdbc7e86e6e8da0d4a059b118ffab289d3a9>,
> which also introduced today's noerrexit bits.
>
> One thing that distinguishes the NOERREXIT_UNTIL_EXEC bit from the other
> noerrexit bits is that it seems intended for the caller(s) of the function
> that sets it, while the other bits are intended for the callee(s). This is
> similar to today's this_noerrexit variable. I wonder whether this bit was a
> first attempt at solving the problem that is solved in today's code by the
> this_noerrexit variable. This would explain why everything is still fine
> even though the bit is no longer set.
>
> In today's code, the NOERREXIT_UNTIL_EXEC bit is only ever set in loop.c
> at line 578
> <https://github.com/zsh-users/zsh/blob/41b402d36d0aeac594cf424a9e46b5edb20c815d/Src/loop.c#L578>.
> This line can only be reached if run != 0 && run !=2 && olderrexit == 0 &&
> lastval == 0. The loop above
> <https://github.com/zsh-users/zsh/blob/41b402d36d0aeac594cf424a9e46b5edb20c815d/Src/loop.c#L551> can
> only be exited at one of the following lines:
>
> - line 551
> <https://github.com/zsh-users/zsh/blob/41b402d36d0aeac594cf424a9e46b5edb20c815d/Src/loop.c#L551>=>
> run == 0
> - line 557
> <https://github.com/zsh-users/zsh/blob/41b402d36d0aeac594cf424a9e46b5edb20c815d/Src/loop.c#L557>
> => run == 0 || run == 2
> - line 565
> <https://github.com/zsh-users/zsh/blob/41b402d36d0aeac594cf424a9e46b5edb20c815d/Src/loop.c#L565>
> => run == 1 && lastval != 0
> - line 568
> <https://github.com/zsh-users/zsh/blob/41b402d36d0aeac594cf424a9e46b5edb20c815d/Src/loop.c#L568>
> => run == 0 && lastval == 0
>
> None of these cases lead to line 578. Thus the NOERREXIT_UNTIL_EXEC bit is
> never set and can safely be removed.
>
> The code "noerrexit &= ~ (NOERREXIT_EXIT | NOERREXIT_RETURN)" at line 580
> <https://github.com/zsh-users/zsh/blob/41b402d36d0aeac594cf424a9e46b5edb20c815d/Src/loop.c#L580> can
> be simplified into "noerrexit = olderrexit" because NOERREXIT_UNTIL_EXEC
> was the only noerrexit bit that was flowing from callees to their caller.
>
> Obviously all tests still pass. And I checked that the tests introduced by commit
> d6a32dd
> <https://github.com/zsh-users/zsh/commit/d6a32ddeed914434f5b56b013c9d03b28781d065> are
> still present.
>
> I have built this patch on top of my patch
> patch-06-fix-eval-and-source-statements.txt but I don't think that it
> depends on it nor on any of my previous patches (but I haven't tried to
> confirm it).
>
> QUESTION: Should NOERREXIT_SIGNAL's value
> <https://github.com/zsh-users/zsh/blob/41b402d36d0aeac594cf424a9e46b5edb20c815d/Src/zsh.h#L2226>
> be changed to 4 (from 8) since the value 4 is no longer used
> by NOERREXIT_UNTIL_EXEC?
>
> Philippe
>
>

[-- Attachment #1.2: Type: text/html, Size: 4841 bytes --]

[-- Attachment #2: patch-07-remove-noerrexit-until-exec.txt --]
[-- Type: text/plain, Size: 2165 bytes --]

diff --git a/Src/exec.c b/Src/exec.c
index 8ff6489ec..1dd569019 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -1559,14 +1559,7 @@ execlist(Estate state, int dont_change_job, int exiting)
 	state->pc--;
 sublist_done:
 
-	/*
-	 * See hairy code near the end of execif() for the
-	 * following.  "noerrexit " only applies until
-	 * we hit execcmd on the way down.  We're now
-	 * on the way back up, so don't restore it.
-	 */
-	if (!(oldnoerrexit & NOERREXIT_UNTIL_EXEC))
-	    noerrexit = oldnoerrexit;
+	noerrexit = oldnoerrexit;
 
 	if (sigtrapped[SIGDEBUG] && !isset(DEBUGBEFORECMD) && !donedebug) {
 	    /*
@@ -3246,10 +3239,6 @@ execcmd_exec(Estate state, Execcmd_params eparams,
     } else
 	preargs = NULL;
 
-    /* if we get this far, it is OK to pay attention to lastval again */
-    if (noerrexit & NOERREXIT_UNTIL_EXEC)
-	noerrexit = 0;
-
     /* Do prefork substitutions.
      *
      * Decide if we need "magic" handling of ~'s etc. in
diff --git a/Src/loop.c b/Src/loop.c
index 7c3e04b8a..61543ed73 100644
--- a/Src/loop.c
+++ b/Src/loop.c
@@ -569,23 +569,14 @@ execif(Estate state, int do_exec)
 	s = 1;
 	state->pc = next;
     }
+    noerrexit = olderrexit;
 
     if (run) {
-	/* we need to ignore lastval until we reach execcmd() */
-	if (olderrexit || run == 2)
-	    noerrexit = olderrexit;
-	else if (lastval)
-	    noerrexit |= NOERREXIT_EXIT | NOERREXIT_RETURN | NOERREXIT_UNTIL_EXEC;
-	else
-	    noerrexit &= ~ (NOERREXIT_EXIT | NOERREXIT_RETURN);
 	cmdpush(run == 2 ? CS_ELSE : (s ? CS_ELIFTHEN : CS_IFTHEN));
 	execlist(state, 1, do_exec);
 	cmdpop();
-    } else {
-	noerrexit = olderrexit;
-	if (!retflag && !errflag)
-	    lastval = 0;
-    }
+    } else if (!retflag && !errflag)
+	lastval = 0;
     state->pc = end;
     this_noerrexit = 1;
 
diff --git a/Src/zsh.h b/Src/zsh.h
index 40f9ea537..703231ca2 100644
--- a/Src/zsh.h
+++ b/Src/zsh.h
@@ -2220,8 +2220,6 @@ enum noerrexit_bits {
     NOERREXIT_EXIT = 1,
     /* Suppress ERR_RETURN: per function call */
     NOERREXIT_RETURN = 2,
-    /* NOERREXIT only needed on way down */
-    NOERREXIT_UNTIL_EXEC = 4,
     /* Force exit on SIGINT */
     NOERREXIT_SIGNAL = 8
 };

Re: [PATCH] Remove NOERREXIT_UNTIL_EXEC

[Bart Schaefer]

On Fri, Dec 2, 2022 at 4:31 PM Philippe Altherr
<philippe.altherr@gmail.com> wrote:
>
> The noerrexit NOERREXIT_UNTIL_EXEC bit is never set and can therefore be removed. In other words the patch only removes dead code.

That explains MY confusion over the meaning of NOERREXIT_UNTIL_EXEC
when you first asked about it:  The use for which it was originally
intended had gone away.

> One thing that distinguishes the NOERREXIT_UNTIL_EXEC bit from the other noerrexit bits is that it seems intended for the caller(s) of the function that sets it, while the other bits are intended for the callee(s). This is similar to today's this_noerrexit variable.

Moot point now, but I believe it was still intended for the callees --
the original bug being fixed involved error state propagating down
into for/select loops.

> QUESTION: Should NOERREXIT_SIGNAL's value be changed to 4 (from 8) since the value 4 is no longer used by NOERREXIT_UNTIL_EXEC?

I don't think that's necessary.