Re: [PATCH] Remove NOERREXIT_UNTIL_EXEC

[Philippe Altherr <philippe.altherr@xxxxxxxxx>]

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

Philippe

On Sat, Dec 3, 2022 at 1:31 AM Philippe Altherr <philippe.altherr@xxxxxxxxx> 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, 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, 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. This line can only be reached if run != 0 && run !=2 && olderrexit == 0 && lastval == 0. The loop above can only be exited at one of the following lines:

- line 551 => run == 0

- line 557 => run == 0 || run == 2

- line 565 => run == 1 && lastval != 0

- line 568 => 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 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 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 be changed to 4 (from 8) since the value 4 is no longer used by NOERREXIT_UNTIL_EXEC?

Philippe

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