* err_exit/err_return regression @ 2015-07-09 11:33 jsks 2015-07-09 13:38 ` Peter Stephenson 0 siblings, 1 reply; 9+ messages in thread From: jsks @ 2015-07-09 11:33 UTC (permalink / raw) To: zsh-workers 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: err_exit/err_return regression 2015-07-09 11:33 err_exit/err_return regression jsks @ 2015-07-09 13:38 ` Peter Stephenson 2015-09-29 15:43 ` Joshua Krusell 0 siblings, 1 reply; 9+ messages in thread From: Peter Stephenson @ 2015-07-09 13:38 UTC (permalink / raw) To: zsh-workers 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: err_exit/err_return regression 2015-07-09 13:38 ` Peter Stephenson @ 2015-09-29 15:43 ` Joshua Krusell 2015-09-29 18:54 ` Peter Stephenson 0 siblings, 1 reply; 9+ messages in thread From: Joshua Krusell @ 2015-09-29 15:43 UTC (permalink / raw) To: zsh-workers 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: err_exit/err_return regression 2015-09-29 15:43 ` Joshua Krusell @ 2015-09-29 18:54 ` Peter Stephenson 2015-09-29 20:52 ` Joshua Krusell 0 siblings, 1 reply; 9+ messages in thread From: Peter Stephenson @ 2015-09-29 18:54 UTC (permalink / raw) To: zsh-workers 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: err_exit/err_return regression 2015-09-29 18:54 ` Peter Stephenson @ 2015-09-29 20:52 ` Joshua Krusell 2015-09-30 1:09 ` Bart Schaefer 0 siblings, 1 reply; 9+ messages in thread From: Joshua Krusell @ 2015-09-29 20:52 UTC (permalink / raw) To: zsh-workers 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: err_exit/err_return regression 2015-09-29 20:52 ` Joshua Krusell @ 2015-09-30 1:09 ` Bart Schaefer 2015-10-01 20:32 ` Joshua Krusell 0 siblings, 1 reply; 9+ messages in thread From: Bart Schaefer @ 2015-09-30 1:09 UTC (permalink / raw) To: zsh-workers 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; } ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: err_exit/err_return regression 2015-09-30 1:09 ` Bart Schaefer @ 2015-10-01 20:32 ` Joshua Krusell 2015-10-04 0:26 ` Bart Schaefer 0 siblings, 1 reply; 9+ messages in thread From: Joshua Krusell @ 2015-10-01 20:32 UTC (permalink / raw) To: zsh-workers 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: err_exit/err_return regression 2015-10-01 20:32 ` Joshua Krusell @ 2015-10-04 0:26 ` Bart Schaefer 2015-10-04 2:58 ` Bart Schaefer 0 siblings, 1 reply; 9+ messages in thread From: Bart Schaefer @ 2015-10-04 0:26 UTC (permalink / raw) To: zsh-workers 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) { /* ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: err_exit/err_return regression 2015-10-04 0:26 ` Bart Schaefer @ 2015-10-04 2:58 ` Bart Schaefer 0 siblings, 0 replies; 9+ messages in thread From: Bart Schaefer @ 2015-10-04 2:58 UTC (permalink / raw) To: zsh-workers 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2015-10-04 2:58 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-07-09 11:33 err_exit/err_return regression jsks 2015-07-09 13:38 ` Peter Stephenson 2015-09-29 15:43 ` Joshua Krusell 2015-09-29 18:54 ` Peter Stephenson 2015-09-29 20:52 ` Joshua Krusell 2015-09-30 1:09 ` Bart Schaefer 2015-10-01 20:32 ` Joshua Krusell 2015-10-04 0:26 ` Bart Schaefer 2015-10-04 2:58 ` Bart Schaefer
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).