Re: [v2 PATCH] eval: Only restore exit status on exit/return
On 14/12/2018 10:07, Harald van Dijk wrote:
On 14/12/2018 09:47, Herbert Xu wrote:
On Fri, Dec 14, 2018 at 09:37:09AM +, Harald van Dijk wrote:
Still, that works as well, doesn't it? The control flow is basically the
same so the logic in my other message applies. returncmd() doesn't use
exceptions, so you get to dotrap() which already resets exitstatus if
necessary.
returncmd itself doesn't jump up but it may be part of a subshell
which would execute a jump to the top as part of the EV_EXIT
optimisation. That's where you'd need to restore the exit status.
Ah, okay, so for something like
trap 'false; (return); echo $?' EXIT
you want to remember that it's executed as part of a trap action and
print 0, not 1.
I actually want that to print 1, so for me it's not a problem. However,
my patch can be trivially modified to handle that: just have returncmd()
check savestatus the same way exitcmd() does. I think that also allows
merging SKIPFUNC and SKIPFUNCDEF, since you can then assume exitstatus
was always set appropriately, so it allows for some further reduction in
code.
I think this is needed regardless to fix a bug as well:
trap 'f() { false; return; }; f; echo $?' EXIT
Other shells are evenly divided, but POSIX clearly requires this to
print 0 (which is what bash/ksh/mksh/yash do): calling a function does
not reset any trap information, so this return statement must be
considered to execute in a trap action. It currently prints 1 (also seen
in bosh/pdksh/posh/zsh). With my suggested changes, it prints 0.
Re: [v2 PATCH] eval: Only restore exit status on exit/return
On 14/12/2018 09:47, Herbert Xu wrote: On Fri, Dec 14, 2018 at 09:37:09AM +, Harald van Dijk wrote: Still, that works as well, doesn't it? The control flow is basically the same so the logic in my other message applies. returncmd() doesn't use exceptions, so you get to dotrap() which already resets exitstatus if necessary. returncmd itself doesn't jump up but it may be part of a subshell which would execute a jump to the top as part of the EV_EXIT optimisation. That's where you'd need to restore the exit status. Ah, okay, so for something like trap 'false; (return); echo $?' EXIT you want to remember that it's executed as part of a trap action and print 0, not 1. I actually want that to print 1, so for me it's not a problem. However, my patch can be trivially modified to handle that: just have returncmd() check savestatus the same way exitcmd() does. I think that also allows merging SKIPFUNC and SKIPFUNCDEF, since you can then assume exitstatus was always set appropriately, so it allows for some further reduction in code.
Re: [v2 PATCH] eval: Only restore exit status on exit/return
On Fri, Dec 14, 2018 at 09:37:09AM +, Harald van Dijk wrote: > > Still, that works as well, doesn't it? The control flow is basically the > same so the logic in my other message applies. returncmd() doesn't use > exceptions, so you get to dotrap() which already resets exitstatus if > necessary. returncmd itself doesn't jump up but it may be part of a subshell which would execute a jump to the top as part of the EV_EXIT optimisation. That's where you'd need to restore the exit status. Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [v2 PATCH] eval: Only restore exit status on exit/return
On Fri, Dec 14, 2018 at 08:02:04AM +, Harald van Dijk wrote:
> On 14/12/2018 03:10, Herbert Xu wrote:
>
> > The reason this is needed is to support a naked return. With
> > your patch a naked return would either have to always return the
> > saved status or the new status. Neither of which is what we
> > want.
>
> I actually saw the commit you referenced already and tested that with my
> patch before sending. This is how I tested it:
>
> Returns 1:
>
> f() {
> false; return
> }
> f
That's not a naked return. A naked return is one used outside of
a function. I know this is unspecified under POSIX, but for dash
this has a specific meaning and that is the return is equivalent
to exit.
Cheers,
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [v2 PATCH] eval: Only restore exit status on exit/return
On 14/12/2018 08:02, Harald van Dijk wrote:
On 14/12/2018 03:10, Herbert Xu wrote:
On Sat, Dec 08, 2018 at 03:45:11PM +, Harald van Dijk wrote:
--- a/src/eval.c
+++ b/src/eval.c
@@ -116,10 +116,7 @@ INCLUDE "eval.h"
EXITRESET {
evalskip = 0;
loopnest = 0;
- if (savestatus >= 0) {
- exitstatus = savestatus;
- savestatus = -1;
- }
+ savestatus = -1;
}
#endif
The reason this is needed is to support a naked return. With
your patch a naked return would either have to always return the
saved status or the new status. Neither of which is what we
want.
I actually saw the commit you referenced already and tested that with my
patch before sending. This is how I tested it:
Returns 1:
f() {
false; return
}
f
Returns 0:
f() {
trap "false; return" USR1
kill -USR1 $$
}
f
The former picks up the status of the false command, the latter of the
kill command. This works because although returncmd() only looks at
exitstatus, so returns the wrong value, in the no-argument version it
sets skip to SKIPFUNCDEF, causing dotrap() to restore the original
exitstatus value, whereas in the version that does take arguments, skip
is set to SKIPFUNC, causing dotrap() to leave exitstatus alone.
Which is exactly how that particular commit was supposed to work in the
first place, so perhaps it's simpler to put it as "in these tests,
changes to exitreset() do not cause the test to break because
exitreset() is never called at the wrong time".
If there's some test that breaks, it would need to be one where the
original exitstatus needs to be restored by exitreset(), *and* the
original exitstatus is not already restored by dotrap().
The original exitstatus is not restored by dotrap() only if evalskip ==
SKIPFUNC (i.e. return with argument), in which case it shouldn't be
restored, or it exits with an exception. And exitreset() is only called
when control gets back to main() via an exception.
So you'd need an example where an exception is raised after the return
command successfully completes without arguments, yet before the
function the return command is in (if any) actually returns, where that
exception would then reach main(). I struggle to come up with such an
example.
Re: [v2 PATCH] eval: Only restore exit status on exit/return
On 14/12/2018 03:10, Herbert Xu wrote:
On Sat, Dec 08, 2018 at 03:45:11PM +, Harald van Dijk wrote:
--- a/src/eval.c
+++ b/src/eval.c
@@ -116,10 +116,7 @@ INCLUDE "eval.h"
EXITRESET {
evalskip = 0;
loopnest = 0;
- if (savestatus >= 0) {
- exitstatus = savestatus;
- savestatus = -1;
- }
+ savestatus = -1;
}
#endif
The reason this is needed is to support a naked return. With
your patch a naked return would either have to always return the
saved status or the new status. Neither of which is what we
want.
I actually saw the commit you referenced already and tested that with my
patch before sending. This is how I tested it:
Returns 1:
f() {
false; return
}
f
Returns 0:
f() {
trap "false; return" USR1
kill -USR1 $$
}
f
The former picks up the status of the false command, the latter of the
kill command. This works because although returncmd() only looks at
exitstatus, so returns the wrong value, in the no-argument version it
sets skip to SKIPFUNCDEF, causing dotrap() to restore the original
exitstatus value, whereas in the version that does take arguments, skip
is set to SKIPFUNC, causing dotrap() to leave exitstatus alone.
Re: [v2 PATCH] eval: Only restore exit status on exit/return
On Thu, Dec 06, 2018 at 09:35:47PM +, Harald van Dijk wrote: > On 04/12/2018 23:57, Harald van Dijk wrote: > > This has the benefit of fixing one other test case, a small modification > > from one of Martijn Dekker's: > > > > $SHELL -c 'trap "set -o bad@option" INT; kill -s INT $$' && echo BUG > > Another test case, one that still fails: > > trap exit INT > trap 'true; kill -s INT $$' EXIT > false > > Here, inside the EXIT handler, "the command that executed immediately > preceding the trap action" is `false`, but inside the INT handler, it's > either `true` or `kill -s INT $$` (I think the latter, but it doesn't > matter). dash treats it as if it were still `false`. I think this makes sense. The EXIT trap trumps whatever happens inside it. FWIW ksh/mksh both do the same thing. Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [v2 PATCH] eval: Only restore exit status on exit/return
On Sat, Dec 08, 2018 at 03:45:11PM +, Harald van Dijk wrote:
>
> --- a/src/eval.c
> +++ b/src/eval.c
> @@ -116,10 +116,7 @@ INCLUDE "eval.h"
> EXITRESET {
> evalskip = 0;
> loopnest = 0;
> - if (savestatus >= 0) {
> - exitstatus = savestatus;
> - savestatus = -1;
> - }
> + savestatus = -1;
> }
> #endif
The reason this is needed is to support a naked return. With
your patch a naked return would either have to always return the
saved status or the new status. Neither of which is what we
want.
Please refer to
commit 70c16dd30d4cf824aa895e9f6c095fec741c65a8
Author: Herbert Xu
Date: Mon Oct 6 21:51:26 2014 +0800
[BUILTIN] Return without arguments in a trap should use status outside traps
Thanks,
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [v2 PATCH] eval: Only restore exit status on exit/return
On 06/12/2018 21:35, Harald van Dijk wrote:
On 04/12/2018 23:57, Harald van Dijk wrote:
This has the benefit of fixing one other test case, a small
modification from one of Martijn Dekker's:
$SHELL -c 'trap "set -o bad@option" INT; kill -s INT $$' && echo BUG
Another test case, one that still fails:
trap exit INT
trap 'true; kill -s INT $$' EXIT
false
Here, inside the EXIT handler, "the command that executed immediately
preceding the trap action" is `false`, but inside the INT handler, it's
either `true` or `kill -s INT $$` (I think the latter, but it doesn't
matter). dash treats it as if it were still `false`.
The attached patch fixes this. In short, it assumes that if the EXIT
action raises an exception, exitstatus will have been set appropriately,
and modifies exitcmd() to set it. It also simplifies dotrap() by
removing the unnecessary special handling of recursive calls.
It handles the following test cases, from this thread:
exec 2>/dev/null
$SHELL -c 'trap "(false) && echo BUG1" INT; kill -s INT $$'
$SHELL -c 'trap "(false) && echo BUG2" EXIT'
$SHELL -c 'trap "(false); echo \$?" EXIT'
$SHELL -c 'trap "(true) || echo BUG3" INT; false; kill -s INT $$'
$SHELL -c 'trap "(true) || echo BUG4" EXIT; false'
$SHELL -c 'trap "(true); echo \$?" EXIT; false'
$SHELL -c 'trap "(! :) && echo BUG5" EXIT'
$SHELL -c 'trap "(false) && echo BUG6" EXIT'
$SHELL -c 'trap "readonly foo=bar; (foo=baz) && echo BUG7" EXIT'
$SHELL -c 'trap "(set -o bad@option) && echo BUG8" EXIT'
$SHELL -c 'trap "set -o bad@option" EXIT' && echo BUG9
$SHELL -c 'trap "set -o bad@option" INT; kill -s INT $$' && echo BUG10
$SHELL -c 'trap exit INT; trap "true; kill -s INT $$" EXIT; false' ||
echo BUG11
It does not change the behaviour for these test cases, also from this
thread:
$SHELL -c 'trap "(trap \"echo exit\" EXIT; :)" EXIT'
$SHELL -c 'trap "(:; exit) && echo exit" EXIT; false'
It can be combined with the other patches to also change the behaviour
of those two.
--- a/src/eval.c
+++ b/src/eval.c
@@ -116,10 +116,7 @@ INCLUDE "eval.h"
EXITRESET {
evalskip = 0;
loopnest = 0;
- if (savestatus >= 0) {
- exitstatus = savestatus;
- savestatus = -1;
- }
+ savestatus = -1;
}
#endif
--- a/src/main.c
+++ b/src/main.c
@@ -348,7 +348,9 @@ exitcmd(int argc, char **argv)
return 0;
if (argc > 1)
- savestatus = number(argv[1]);
+ exitstatus = number(argv[1]);
+ else if (savestatus >= 0)
+ exitstatus = savestatus;
exraise(EXEXIT);
/* NOTREACHED */
--- a/src/trap.c
+++ b/src/trap.c
@@ -320,17 +320,14 @@ void dotrap(void)
char *p;
char *q;
int i;
- int status, last_status;
+ int status;
if (!pending_sig)
return;
status = savestatus;
- last_status = status;
- if (likely(status < 0)) {
- status = exitstatus;
- savestatus = status;
- }
+ savestatus = exitstatus;
+
pending_sig = 0;
barrier();
@@ -350,10 +347,10 @@ void dotrap(void)
continue;
evalstring(p, 0);
if (evalskip != SKIPFUNC)
- exitstatus = status;
+ exitstatus = savestatus;
}
- savestatus = last_status;
+ savestatus = status;
}
@@ -390,8 +387,10 @@ exitshell(void)
savestatus = exitstatus;
TRACE(("pid %d, exitshell(%d)\n", getpid(), savestatus));
- if (setjmp(loc.loc))
+ if (setjmp(loc.loc)) {
+ savestatus = exitstatus;
goto out;
+ }
handler =
if ((p = trap[0])) {
trap[0] = NULL;
