DEBUG trap: restore status 2 trigger to skip command (re: d00b4b39)

So now we know what that faulty check for shp->indebug in sh_trap()
was meant to do: it was meant to pass down the trap handler's exit
status, via sh_debug(), down to sh_exec() (xec.c) so that it could
then skip the execution of the next command if the trap's exit
status is 2, as documented in the manual page. As of d00b4b39, exit
status 2 was not passed down, so this stopped working.

This commit reinstates that functionality, but without the exit
status bug in command substitutions caused by the old way.

src/cmd/ksh93/sh/fault.c: sh_trap():
- Save the trap's exit status before restoring the parent
  envionment's exit status. Make this saved exit status the return
  value of the function. (This does not break anything, AFAICT; the
  majority of sh_trap() calls ignore the return value, and the few
  that don't ignore it seem to expect it to return exactly this.)

src/cmd/ksh93/sh/xec.c: sh_exec():
- The sh_trap() fix has one side effect: whereas the exit status of
  a skipped command was always 2 (as per the trap handler), now it
  is always 0, because it gets reset in sh_exec() but no command is
  executed. That is probably not a desirable change in behaviour,
  so let's fix that here instead: set sh.exitval to 2 when skipping
  commands.

src/cmd/ksh93/sh.1:
- Document that ${.sh.command} shell-quotes its arguments for use
  by 'eval' and such. This fact was not documented anywhere, AFAIK.

src/cmd/ksh93/shell.3:
- Document that $? (exit status) is made local to trap handlers.
- Document that sh_trap() returns the trap handler's exit status.

src/cmd/ksh93/tests/basic.sh:
- Add test for this bug.
- Add a missing test for the exit status 255 functionality (if a
  DEBUG trap handler yields this exit status and we're executing a
  function or dot script, a return is triggered).

Fixes: https://github.com/ksh93/ksh/issues/187

NEWS

@@ -3,6 +3,11 @@ For full details, see the git log at: https://github.com/ksh93/ksh
 
 Any uppercase BUG_* names are modernish shell bug IDs.
 
+2021-02-20:
+
+- Fixed a bug introduced on 2021-01-20: if a DEBUG trap action yielded exit
+  status 2, the execution of the next command was not skipped as documented.
+
 2021-02-18:
 
 - A bug was fixed in the 'read' builtin that caused it to fail to process

src/cmd/ksh93/sh.1

@@ -1561,6 +1561,8 @@ When processing a
 .B DEBUG
 trap, this variable contains the current command line
 that is about to run.
+Each argument is shell-quoted as necessary
+so that the value is safe for being evaluated by the shell.
 .TP
 .B .sh.edchar
 This variable contains the value of the keyboard character
@@ -7429,7 +7431,7 @@ then
 will be executed before each command.
 The variable
 .B .sh.command
-will contain the contents of the current command line
+will contain the shell-quoted arguments of the current command line
 when
 .I action\^
 is running.

src/cmd/ksh93/sh/fault.c

@@ -442,11 +442,12 @@ void	sh_chktrap(Shell_t* shp)
 /*
  * parse and execute the given trap string, stream or tree depending on mode
  * mode==0 for string, mode==1 for stream, mode==2 for parse tree
+ * The return value is the exit status of the trap action.
  */
 int sh_trap(const char *trap, int mode)
 {
 	Shell_t	*shp = sh_getinterp();
-	int	jmpval, savxit = shp->exitval;
+	int	jmpval, savxit = shp->exitval, savxit_return;
 	int	was_history = sh_isstate(SH_HISTORY);
 	int	was_verbose = sh_isstate(SH_VERBOSE);
 	int	staktop = staktell();
@@ -487,6 +488,7 @@ int sh_trap(const char *trap, int mode)
 	sh_popcontext(shp,&buff);
 	shp->intrap--;
 	sfsync(shp->outpool);
+	savxit_return = shp->exitval;
 	if(jmpval!=SH_JMPEXIT && jmpval!=SH_JMPFUN)
 		shp->exitval=savxit;
 	stakset(savptr,staktop);
@@ -498,7 +500,7 @@ int sh_trap(const char *trap, int mode)
 	exitset();
 	if(jmpval>SH_JMPTRAP && (((struct checkpt*)shp->jmpbuffer)->prev || ((struct checkpt*)shp->jmpbuffer)->mode==SH_JMPSCRIPT))
 		siglongjmp(*shp->jmplist,jmpval);
-	return(shp->exitval);
+	return(savxit_return);
 }
 
 /*

src/cmd/ksh93/sh/xec.c

@@ -1192,8 +1192,12 @@ int sh_exec(register const Shnode_t *t, int flags)
 						argp = 0;
 					}
 					else if(n==2)
+					{
+						/* Do not execute next command; keep exit status from trap handler */
+						shp->exitval = n;
 						break;
 					}
+				}
 				if(io)
 					sfsync(shp->outpool);
 				shp->lastpath = 0;

src/cmd/ksh93/shell.3

@@ -273,6 +273,10 @@ The complete file associated with the string or file
 is compiled and then executed so that aliases defined
 within the string or file will not take effect until
 the next command is executed.
+The shell's \f5$?\fP special parameter is made local to the string
+or file executed so that it is not affected for subsequent commands.
+The return value of \f5sh_trap()\fP is the exit status of
+the last command executed by the string or file.
 .PP
 The \f5sh_run()\fP function will run the command given by
 by the argument list \fIargv\fP containing \fIargc\fP elements. 

src/cmd/ksh93/tests/basic.sh

@@ -662,6 +662,7 @@ got=$(eval 'x=`for i in test; do case $i in test) true;; esac; done`' 2>&1) \
 
 # ======
 # Various DEBUG trap fixes: https://github.com/ksh93/ksh/issues/155
+#			    https://github.com/ksh93/ksh/issues/187
 
 # Redirecting disabled the DEBUG trap
 exp=$'LINENO: 4\nfoo\nLINENO: 5\nLINENO: 6\nbar\nLINENO: 7\nbaz'
@@ -713,6 +714,19 @@ r=`exit 123`
 (((e=$?)==123)) || err_exit "DEBUG trap run in \`comsub\` affects exit status (expected 123, got $e)"
 trap - DEBUG
 
+# After fixing the previous, the DEBUG trap stopped honouring exit status 2 to skip command execution -- whoops
+got=$(
+	myfn()
+	{
+		[[ ${.sh.command} == echo* ]] && return 2
+	}
+	trap 'myfn' DEBUG
+	print end
+	echo "Hi, I'm a bug"
+)
+(((e=$?)==2)) && [[ $got == end ]] || err_exit 'DEBUG trap: next command not skipped upon status 2' \
+	"(got status $e, $(printf %q "$got"))"
+
 # The DEBUG trap was incorrectly inherited by subshells
 exp=$'Subshell\nDebug 1\nParentshell'
 got=$(
@@ -754,5 +768,27 @@ trap - DEBUG  # bug compat
 [[ $got == "$exp" ]] || err_exit "DEBUG trap not inherited by POSIX function" \
 	"(expected $(printf %q "$exp"), got $(printf %q "$got"))"
 
+# Make sure the DEBUG trap still exits a POSIX function on exit status 255
+# TODO: same test for ksh function with -o functrace, once we add that option
+exp=$'one\ntwo'
+got=$(
+	myfn()
+	{
+		echo one
+		echo two
+		echo three
+		echo four
+	}
+	set255()
+	{
+		return 255
+	}
+	trap '[[ ${.sh.command} == *three ]] && set255' DEBUG
+	myfn
+)
+trap - DEBUG  # bug compat
+[[ $got == "$exp" ]] || err_exit "DEBUG trap did not trigger return from POSIX function on status 255" \
+	"(expected $(printf %q "$exp"), got $(printf %q "$got"))"
+
 # ======
 exit $((Errors<125?Errors:125))