err_exit/err_return regression

err_exit/err_return regression

[jsks]

Since patch 34065, with either err_exit or err_return set, zsh does
not exit/return given a nonzero exist status of a command following
'else'.

Example:

$ setopt err_return; if false; then :; else false; echo foo; fi
foo

However, command lists following 'if' and 'elif' work as expected.

$ setopt err_return; if true; then false; echo foo; else :; fi

In the execif function of loop.c, when executing the command list of
an else statement, given that lastval from the evaluation of the
previous if/elif is nonzero, the flag noerrexit is never reset to its
original value.

noerrexit = olderrexit ? olderrexit : lastval ? 2 : 0;

Unless I misunderstood the intended behaviour, didn't know how to
handle this with regards to the patch fixing for example return values
and for loops, so simply reporting.

/jsks

Re: err_exit/err_return regression

[Peter Stephenson]

On Thu, 9 Jul 2015 13:33:09 +0200
jsks <js.shirin@gmail.com> wrote:
> Since patch 34065, with either err_exit or err_return set, zsh does
> not exit/return given a nonzero exist status of a command following
> 'else'.

Hmm, this code is quite hairy and is crying out to be a bit more
structured (it's not lonely in that respect).  But the test suite says
the following is good enough for now.

diff --git a/Src/exec.c b/Src/exec.c
index 960601f..4eee82b 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -1351,7 +1351,13 @@ execlist(Estate state, int dont_change_job, int exiting)
 	state->pc--;
 sublist_done:
 
-	noerrexit = oldnoerrexit;
+	/*
+	 * See hairy code near the end of execif() for the
+	 * following.  "noerrexit == 2" only applies until
+	 * we hit execcmd on the way down.  We're now
+	 * on the way back up, so don't restore it.
+	 */
+	noerrexit = (oldnoerrexit == 2) ? 0 : oldnoerrexit;
 
 	if (sigtrapped[SIGDEBUG] && !isset(DEBUGBEFORECMD) && !donedebug) {
 	    /*
diff --git a/Test/C03traps.ztst b/Test/C03traps.ztst
index 757f75c..4e23388 100644
--- a/Test/C03traps.ztst
+++ b/Test/C03traps.ztst
@@ -399,6 +399,46 @@
    )
 1:ERREXIT in loop with simple commands
 
+  fn() {
+    emulate -L zsh
+    setopt errreturn
+    if false; then
+      false
+      print No.
+    else
+      print Oh, yes
+    fi
+  }
+  fn
+0:ERRRETURN not triggered in if condition
+>Oh, yes
+
+  fn() {
+    emulate -L zsh
+    setopt errreturn
+    if true; then
+      false
+      print No.
+    else
+      print No, no.
+    fi
+  }
+  fn
+1:ERRRETURN in "if"
+
+  fn() {
+    emulate -L zsh
+    setopt errreturn
+    if false; then
+      print No.
+    else
+      false
+      print No, no.
+    fi
+  }
+  fn
+1:ERRRETURN in "else" branch (regression test)
+
 %clean
 
   rm -f TRAPEXIT

Re: err_exit/err_return regression

[Joshua Krusell]

Heads up, found another edge case probably related to this.

The following snippet will never reach the print statement. I can only
reproduce this with a nested [[ followed by a var assignment.

set -x
setopt err_return

if false; then
    :
else
    if [[ -n '' ]]; then
        a=2
    fi

    print foo
fi

Re: err_exit/err_return regression

[Peter Stephenson]

On Tue, 29 Sep 2015 17:43:27 +0200
Joshua Krusell <js.shirin@gmail.com> wrote:
> Heads up, found another edge case probably related to this.
> 
> The following snippet will never reach the print statement. I can only
> reproduce this with a nested [[ followed by a var assignment.

Looks like there's another ingredient here since I don't see this with
the current contents of the master branch.

pws

Re: err_exit/err_return regression

[Joshua Krusell]

On 29/09/15 at 07:54P, Peter Stephenson wrote:
> On Tue, 29 Sep 2015 17:43:27 +0200
> Joshua Krusell <js.shirin@gmail.com> wrote:
> > Heads up, found another edge case probably related to this.
> > 
> > The following snippet will never reach the print statement. I can only
> > reproduce this with a nested [[ followed by a var assignment.
> 
> Looks like there's another ingredient here since I don't see this with
> the current contents of the master branch.
> 
> pws

Was in a hurry and didn't provide much context, but that was with zsh
5.1.1. I just checked out master and I'm still getting the same results
on both OSX and Linux. Can provide output of `./configure` but suffice
to say just doing a default build (`reporter` is also pretty boring
http://pastebin.com/QaCh869w).

$ cat ex.sh
setopt err_return

print $ZSH_VERSION
setopt

if false; then
    :
else
    if [[ -n '' ]]; then
        a=2
    fi

    print foo
fi
$ env -i Src/zsh -f ex.sh
5.1.1-dev-0
errreturn
nohashdirs
norcs

Any other info I can provide?

/jsks

Re: err_exit/err_return regression

[Bart Schaefer]

On Sep 29, 10:52pm, Joshua Krusell wrote:
}
} Was in a hurry and didn't provide much context, but that was with zsh
} 5.1.1. I just checked out master and I'm still getting the same results
} on both OSX and Linux.

I can confirm.  If you also "setopt xtrace" you can see that it returns
at [[ -n '' ]].  If there is anything other than an assignment in the
"then" part of that "if", the bug does not occur, so I think this may
be in part a parsing/wordcode problem.

In the case with the assignment, when we hit line 1209 of exec.c [in
execlist()] the test (ltype & Z_ZIMPLE) is true, and eventually we
hit the same test at line 1252 and "goto sublist_done" which [here I
begin to lose track of the exact sequence of events] skips over the
execpline() call on line 1284 and therefore never resets lastval = 0
at line 569 of execif(), so we obey errreturn.

If it's a command rather than an assignment, we go through the
WC_SUBLIST branch instead, and everything works out.

I think what this boils down to is, "retflag" needs two values to
distinguish an actual return from an ERR_RETURN, the same way that
we distinguish interrupts from actual errors.  Patch below seems to
work for the "if" case, there may be other such cases.  Might want
to use bitflags and/or #defines instead of just 1 and 2.

Or maybe there's some other way to untangle the Z_SIMPLE case in the
exec*() chain and the patch below isn't needed at all.

Here's a cut-and-paste-able version of the failing example (before
the patch).  Replace "a=2" with e.g. "a=2 :" to compare.

source =(<<\EOF
setopt err_return xtrace

print $ZSH_VERSION
setopt

if false; then
    :
else
    if [[ -n '' ]]; then
        a=2  
    fi

    print foo
fi
EOF
)


diff --git a/Doc/Zsh/builtins.yo b/Doc/Zsh/builtins.yo
index 97c3370..691aaeb 100644
--- a/Doc/Zsh/builtins.yo
+++ b/Doc/Zsh/builtins.yo
@@ -175,7 +175,7 @@ pattern are loaded.
 With the tt(-w) flag, the var(name)s are taken as names of files compiled
 with the tt(zcompile) builtin, and all functions defined in them are
 marked for autoloading.  Note this does not otherwise change the search
-order for 
+order via tt(fpath) when the function is first called.
 
 The flags tt(-z) and tt(-k) mark the function to be autoloaded using the
 zsh or ksh style, as if the option tt(KSH_AUTOLOAD) were unset or were
diff --git a/Src/exec.c b/Src/exec.c
index da808d6..154bbb8 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -1408,7 +1408,7 @@ sublist_done:
 			exit(lastval);
 		}
 		if (errreturn) {
-		    retflag = 1;
+		    retflag = 2;
 		    breaks = loops;
 		}
 	    }
diff --git a/Src/loop.c b/Src/loop.c
index 4def9b6..7d1528e 100644
--- a/Src/loop.c
+++ b/Src/loop.c
@@ -552,8 +552,12 @@ execif(Estate state, int do_exec)
 	    run = 1;
 	    break;
 	}
-	if (retflag)
-	    break;
+	if (retflag) {
+	    if (retflag == 2)
+		retflag = 0; /* Never ERR_RETURN here */
+	    else
+		break;
+	}
 	s = 1;
 	state->pc = next;
     }

Re: err_exit/err_return regression

[Joshua Krusell]

On 29/09/15 at 06:09P, Bart Schaefer wrote:
> I think what this boils down to is, "retflag" needs two values to
> distinguish an actual return from an ERR_RETURN, the same way that
> we distinguish interrupts from actual errors.  Patch below seems to
> work for the "if" case, there may be other such cases.  Might want
> to use bitflags and/or #defines instead of just 1 and 2.

With this patch it's still failing for me, but only when running the
snippet I posted as a script. Seems to magically work with the sourced
'cut-and-paste' version.

/jsks

Re: err_exit/err_return regression

[Bart Schaefer]

On Oct 1, 10:32pm, Joshua Krusell wrote:
} Subject: Re: err_exit/err_return regression
}
} On 29/09/15 at 06:09P, Bart Schaefer wrote:
} > I think what this boils down to is, "retflag" needs two values to
} > distinguish an actual return from an ERR_RETURN

I'm still not entirely happy with that patch, in case anyone has any
better ideas.  I'm tempted to check cmdstack[cmdsp-1] to see whether
it is one of CS_IF or CS_ELIF, but that's abusing the prompt mechanism
for program control, which seems wrong.

} With this patch it's still failing for me, but only when running the
} snippet I posted as a script. Seems to magically work with the sourced
} 'cut-and-paste' version.

The following seems like an obvious former thinko, in retrospect.  All
the regular tests still pass.  Anyone see a problem?

diff --git a/Src/exec.c b/Src/exec.c
index 154bbb8..235faf3 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -1364,7 +1364,8 @@ sublist_done:
 	 * we hit execcmd on the way down.  We're now
 	 * on the way back up, so don't restore it.
 	 */
-	noerrexit = (oldnoerrexit == 2) ? 0 : oldnoerrexit;
+	if (oldnoerrexit != 2)
+	    noerrexit = oldnoerrexit;
 
 	if (sigtrapped[SIGDEBUG] && !isset(DEBUGBEFORECMD) && !donedebug) {
 	    /*

Re: err_exit/err_return regression

[Bart Schaefer]

On Oct 3,  5:26pm, Bart Schaefer wrote:
}
} I'm still not entirely happy with [36707], in case anyone has any
} better ideas.
} 
} [36766] seems [to fix] an obvious former thinko, in retrospect.  All
} the regular tests still pass.  Anyone see a problem?

In fact with 36766 in place, 36707 isn't necessary any longer, so that
seems to have been the root of the problem all along.

I'll back out 36707.