Re: [v2 PATCH] eval: Only restore exit status on exit/return

[Harald van Dijk <harald@xxxxxxxxxxx>]

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 = &loc;
 	if ((p = trap[0])) {
 		trap[0] = NULL;