Re: PATCH: skip command from debug trap

[Peter Stephenson <pws@xxxxxxx>]

On Wed, 06 Aug 2008 18:00:10 -0700
Bart Schaefer <schaefer@xxxxxxxxxxxxxxxx> wrote:
> Wow, longest thread in quite a while.
> 
> I'll preface this with a "gotcha" that I just noticed.  If you execute
> these commands:
> 
> 	TRAPDEBUG() { return 1 }
> 	setopt DEBUG_BEFORE_CMD IGNORE_EOF
> 
> You've just rendered your shell useless.  You can't even exit from it
> (except by way of the ten-EOFs failsafe we put in some while ago).

I suppose that's power vs. responsibility.

> Here's another possibility.  Presently it's fairly useless to combine
> "setopt ERR_EXIT" (set -x) with TRAPDEBUG, because any nonzero return
> from the trap will cause the entire shell to exit, rather than just the
> surrounding function.
> 
> I propose that ERR_EXIT be unset on entry to TRAPDEBUG, always.  Then at
> return from the function, if ERR_EXIT has become set, treat that as an
> indication to skip the command (and restore ERR_EXIT to whatever its
> pre-function state was).  If you setopt ERR_EXIT and return non-zero
> you still get what you always would (anyway, if you really wanted the
> shell to exit, you can just call "exit" from the trap).  I can imagine
> someone having done something arcane where they expect a failure of a
> command inside a debug trap to kill their shell only when ERR_EXIT has
> already been set globally, but it seems unlikely.

This seems quite neat, it even gets round the nastiness of working out
which level of the function call stack your at to fiddle with the return
value.  It seems perfectly reasonable to make non-use of ERR_EXIT within
DEBUG traps a documented feature:  we already do this during initialisation
scripts and can invoke the same mechanism trivially here.  That's not quite
what you said---you said let it behave as normal but with the additional
feature, but given we have the other mechanism to suppress its use, it
seems neater to apply that here when hijacking the option, right?

Here's the resulting code.  While looking at the other options I made the
trap_return stuff neater by adding a state variable and saving and
restoring properly in source() (the previous hack was what caused the
problems Rocky saw and this fixes the underlying issue much more neatly).

One minor point is that on digging further I found that trapreturn, now
trap_return, is already saved and restored by execsave() and execrestore()
so I didn't need to do that within the trap handler as I previously
claimed.

Index: README
===================================================================
RCS file: /cvsroot/zsh/zsh/README,v
retrieving revision 1.53
diff -u -r1.53 README
--- README	1 Jun 2008 18:35:50 -0000	1.53
+++ README	7 Aug 2008 10:07:08 -0000
@@ -56,6 +56,12 @@
 applies to expressions with forced splitting such as ${=1+"$@"}, but
 otherwise the case where SH_WORD_SPLIT is not set is unaffected.
 
+Debug traps (`trap ... DEBUG' or the function TRAPDEBUG) now run by default
+before the command to which they refer instead of after.  This is almost
+always the right behaviour for the intended purpose of debugging and is
+consistent with recent versions of other shells.  The option
+DEBUG_BEFORE_CMD can be unset to revert to the previous behaviour.
+
 In previous versions of the shell it was possible to use index 0 in an
 array or string subscript to refer to the same element as index 1 if the
 option KSH_ARRAYS was not in effect.  This was a limited approximation to
Index: Doc/Zsh/builtins.yo
===================================================================
RCS file: /cvsroot/zsh/zsh/Doc/Zsh/builtins.yo,v
retrieving revision 1.108
diff -u -r1.108 builtins.yo
--- Doc/Zsh/builtins.yo	10 Jun 2008 08:50:36 -0000	1.108
+++ Doc/Zsh/builtins.yo	7 Aug 2008 10:07:09 -0000
@@ -1301,8 +1301,15 @@
 after each command with a nonzero exit status.  tt(ERR) is an alias
 for tt(ZERR) on systems that have no tt(SIGERR) signal (this is the
 usual case).
+
 If var(sig) is tt(DEBUG) then var(arg) will be executed
-after each command.
+before each command if the option tt(DEBUG_BEFORE_CMD) is set
+(as it is by default), else after each command.  In the former
+case it is possible to skip the next command; see
+the description of the tt(ERR_EXIT) option in
+ifzman(zmanref(zshoptions))\
+ifnzman(noderef(Description of Options)).
+
 If var(sig) is tt(0) or tt(EXIT)
 and the tt(trap) statement is executed inside the body of a function,
 then the command var(arg) is executed after the function completes.
Index: Doc/Zsh/func.yo
===================================================================
RCS file: /cvsroot/zsh/zsh/Doc/Zsh/func.yo,v
retrieving revision 1.20
diff -u -r1.20 func.yo
--- Doc/Zsh/func.yo	30 Jul 2008 19:46:20 -0000	1.20
+++ Doc/Zsh/func.yo	7 Aug 2008 10:07:09 -0000
@@ -308,8 +308,12 @@
 )
 findex(TRAPDEBUG)
 item(tt(TRAPDEBUG))(
-Executed after each command.  If the option tt(DEBUG_BEFORE_CMD)
-is set, executed before each command instead.
+If the option tt(DEBUG_BEFORE_CMD) is set (as it is by default), executed
+before each command; otherwise executed after each command.  In the former
+case it is possible to skip the next command; see the description of the
+tt(ERR_EXIT) option in
+ifzman(zmanref(zshoptions))\
+ifnzman(noderef(Description of Options)).
 )
 findex(TRAPEXIT)
 item(tt(TRAPEXIT))(
Index: Doc/Zsh/options.yo
===================================================================
RCS file: /cvsroot/zsh/zsh/Doc/Zsh/options.yo,v
retrieving revision 1.61
diff -u -r1.61 options.yo
--- Doc/Zsh/options.yo	12 Jun 2008 13:45:05 -0000	1.61
+++ Doc/Zsh/options.yo	7 Aug 2008 10:07:09 -0000
@@ -1046,7 +1046,7 @@
 ifzman(the section ARITHMETIC EVALUATION in zmanref(zshmisc))
 has an explicit list.
 )
-pindex(DEBUG_BEFORE_CMD)
+pindex(DEBUG_BEFORE_CMD <D>)
 cindex(traps, DEBUG, before or after command)
 cindex(DEBUG trap, before or after command)
 item(tt(DEBUG_BEFORE_CMD))(
@@ -1060,6 +1060,13 @@
 If a command has a non-zero exit status, execute the tt(ZERR)
 trap, if set, and exit.  This is disabled while running initialization
 scripts.
+
+The behaviour is also disabled inside tt(DEBUG) traps.  In this
+case the option is handled specially: it is unset on entry to
+the trap.  If the option tt(DEBUG_BEFORE_CMD) is set,
+as it is by default, and the option tt(ERR_EXIT) is found to have been set
+on exit, then the command for which the tt(DEBUG) trap is being executed is
+skipped.  The option is restored after the trap exits.
 )
 pindex(ERR_RETURN)
 cindex(function return, on error)
Index: Src/builtin.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/builtin.c,v
retrieving revision 1.200
diff -u -r1.200 builtin.c
--- Src/builtin.c	31 Jul 2008 08:44:20 -0000	1.200
+++ Src/builtin.c	7 Aug 2008 10:07:10 -0000
@@ -4462,8 +4462,10 @@
 	    retflag = 1;
 	    breaks = loops;
 	    lastval = num;
-	    if (trapreturn == -2)
-		trapreturn = lastval;
+	    if (trap_state == TRAP_STATE_PRIMED && trap_return == -2) {
+		trap_state = TRAP_STATE_FORCE_RETURN;
+		trap_return = lastval;
+	    }
 	    return lastval;
 	}
 	zexit(num, 0);	/* else treat return as logout/exit */
Index: Src/exec.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/exec.c,v
retrieving revision 1.136
diff -u -r1.136 exec.c
--- Src/exec.c	1 Aug 2008 13:53:44 -0000	1.136
+++ Src/exec.c	7 Aug 2008 10:07:10 -0000
@@ -65,16 +65,23 @@
 mod_export int errflag;
  
 /*
- * Status of return from a trap.
+ * State of trap return value.  Value is from enum trap_state.
+ */
+
+/**/
+int trap_state;
+
+/*
+ * Value associated with return from a trap.
  * This is only active if we are inside a trap, else its value
  * is irrelevant.  It is initialised to -1 for a function trap and
  * -2 for a non-function trap and if negative is decremented as
  * we go deeper into functions and incremented as we come back up.
  * The value is used to decide if an explicit "return" should cause
  * a return from the caller of the trap; it does this by setting
- * trapreturn to a status (i.e. a non-negative value).
+ * trap_return to a status (i.e. a non-negative value).
  *
- * In summary, trapreturn is
+ * In summary, trap_return is
  * - zero unless we are in a trap
  * - negative in a trap unless it has triggered.  Code uses this
  *   to detect an active trap.
@@ -83,7 +90,7 @@
  */
  
 /**/
-int trapreturn;
+int trap_return;
  
 /* != 0 if this is a subshell */
  
@@ -1061,6 +1068,10 @@
 	}
 
 	if (sigtrapped[SIGDEBUG] && isset(DEBUGBEFORECMD)) {
+	    int oerrexit_opt = opts[ERREXIT];
+	    opts[ERREXIT] = 0;
+	    noerrexit = 1;
+
 	    exiting = donetrap;
 	    ret = lastval;
 	    dotrap(SIGDEBUG);
@@ -1071,7 +1082,8 @@
 	     * Only execute the trap once per sublist, even
 	     * if the DEBUGBEFORECMD option changes.
 	     */
-	    donedebug = 1;
+	    donedebug = isset(ERREXIT) ? 2 : 1;
+	    opts[ERREXIT] = oerrexit_opt;
 	} else
 	    donedebug = 0;
 
@@ -1087,6 +1099,18 @@
 
 	/* Loop through code followed by &&, ||, or end of sublist. */
 	code = *state->pc++;
+	if (donedebug == 2) {
+	    /* Skip sublist. */
+	    while (wc_code(code) == WC_SUBLIST) {
+		state->pc = state->pc + WC_SUBLIST_SKIP(code);
+		if (WC_SUBLIST_TYPE(code) == WC_SUBLIST_END)
+		    break;
+		code = *state->pc++;
+	    }
+	    donetrap = 1;
+	    /* yucky but consistent... */
+	    goto sublist_done;
+	}
 	while (wc_code(code) == WC_SUBLIST) {
 	    next = state->pc + WC_SUBLIST_SKIP(code);
 	    if (!oldnoerrexit)
@@ -1177,12 +1201,20 @@
 	noerrexit = oldnoerrexit;
 
 	if (sigtrapped[SIGDEBUG] && !isset(DEBUGBEFORECMD) && !donedebug) {
+	    /*
+	     * Save and restore ERREXIT for consistency with
+	     * DEBUGBEFORECMD, even though it's not used.
+	     */
+	    int oerrexit_opt = opts[ERREXIT];
+	    opts[ERREXIT] = 0;
+	    noerrexit = 1;
 	    exiting = donetrap;
 	    ret = lastval;
 	    dotrap(SIGDEBUG);
 	    lastval = ret;
 	    donetrap = exiting;
 	    noerrexit = oldnoerrexit;
+	    opts[ERREXIT] = oerrexit_opt;
 	}
 
 	cmdsp = csp;
@@ -4144,8 +4176,8 @@
 
     oargv0 = NULL;
     obreaks = breaks;;
-    if (trapreturn < 0)
-	trapreturn--;
+    if (trap_state == TRAP_STATE_PRIMED)
+	trap_return--;
     oldlastval = lastval;
     oldnumpipestats = numpipestats;
     if (noreturnval) {
@@ -4263,8 +4295,8 @@
 
     endtrapscope();
 
-    if (trapreturn < -1)
-	trapreturn++;
+    if (trap_state == TRAP_STATE_PRIMED)
+	trap_return++;
     ret = lastval;
     if (noreturnval) {
 	lastval = oldlastval;
@@ -4527,7 +4559,9 @@
     es->badcshglob = badcshglob;
     es->cmdoutpid = cmdoutpid;
     es->cmdoutval = cmdoutval;
-    es->trapreturn = trapreturn;
+    es->trap_return = trap_return;
+    es->trap_state = trap_state;
+    es->trapisfunc = trapisfunc;
     es->noerrs = noerrs;
     es->subsh_close = subsh_close;
     es->underscore = ztrdup(underscore);
@@ -4554,7 +4588,9 @@
     badcshglob = exstack->badcshglob;
     cmdoutpid = exstack->cmdoutpid;
     cmdoutval = exstack->cmdoutval;
-    trapreturn = exstack->trapreturn;
+    trap_return = exstack->trap_return;
+    trap_state = exstack->trap_state;
+    trapisfunc = exstack->trapisfunc;
     noerrs = exstack->noerrs;
     subsh_close = exstack->subsh_close;
     setunderscore(exstack->underscore);
Index: Src/init.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/init.c,v
retrieving revision 1.90
diff -u -r1.90 init.c
--- Src/init.c	4 Aug 2008 19:47:52 -0000	1.90
+++ Src/init.c	7 Aug 2008 10:07:10 -0000
@@ -191,10 +191,6 @@
 	    exit(lastval);
 	if (((!interact || sourcelevel) && errflag) || retflag)
 	    break;
-	if (intrap && trapreturn >= 0) {
-	    lastval = trapreturn;
-	    trapreturn = 0;
-	}
 	if (isset(SINGLECOMMAND) && toplevel) {
 	    if (sigtrapped[SIGEXIT])
 		dotrap(SIGEXIT);
@@ -880,7 +876,8 @@
     lastmailcheck = time(NULL);
     locallevel = sourcelevel = 0;
     sfcontext = SFC_NONE;
-    trapreturn = 0;
+    trap_return = 0;
+    trap_state = TRAP_STATE_INACTIVE;
     noerrexit = -1;
     nohistsave = 1;
     dirstack = znewlinklist();
@@ -1060,6 +1057,7 @@
     char *old_scriptname = scriptname, *us;
     unsigned char *ocs;
     int ocsp;
+    int otrap_return = trap_return, otrap_state = trap_state;
 
     if (!s || 
 	(!(prog = try_source_file((us = unmeta(s)))) &&
@@ -1090,6 +1088,13 @@
     dosetopt(SHINSTDIN, 0, 1);
     scriptname = s;
 
+    /*
+     * The special return behaviour of traps shouldn't
+     * trigger in files sourced from traps; the return
+     * is just a return from the file.
+     */
+    trap_state = TRAP_STATE_INACTIVE;
+
     sourcelevel++;
     if (prog) {
 	pushheap();
@@ -1100,6 +1105,9 @@
 	loop(0, 0);		     /* loop through the file to be sourced  */
     sourcelevel--;
 
+    trap_state = otrap_state;
+    trap_return = otrap_return;
+
     /* restore the current shell state */
     if (prog)
 	freeeprog(prog);
Index: Src/options.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/options.c,v
retrieving revision 1.43
diff -u -r1.43 options.c
--- Src/options.c	31 Jul 2008 08:44:21 -0000	1.43
+++ Src/options.c	7 Aug 2008 10:07:10 -0000
@@ -112,7 +112,7 @@
 {{NULL, "cshjunkiequotes",    OPT_EMULATE|OPT_CSH},	 CSHJUNKIEQUOTES},
 {{NULL, "cshnullcmd",	      OPT_EMULATE|OPT_CSH},	 CSHNULLCMD},
 {{NULL, "cshnullglob",	      OPT_EMULATE|OPT_CSH},	 CSHNULLGLOB},
-{{NULL, "debugbeforecmd",     OPT_EMULATE},		 DEBUGBEFORECMD},
+{{NULL, "debugbeforecmd",     OPT_ALL},			 DEBUGBEFORECMD},
 {{NULL, "emacs",	      0},			 EMACSMODE},
 {{NULL, "equals",	      OPT_EMULATE|OPT_ZSH},	 EQUALS},
 {{NULL, "errexit",	      OPT_EMULATE},		 ERREXIT},
Index: Src/signals.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/signals.c,v
retrieving revision 1.48
diff -u -r1.48 signals.c
--- Src/signals.c	1 Aug 2008 13:53:45 -0000	1.48
+++ Src/signals.c	7 Aug 2008 10:07:10 -0000
@@ -1082,12 +1082,10 @@
 {
     LinkList args;
     char *name, num[4];
-    int trapret = 0;
     int obreaks = breaks;
     int oretflag = retflag;
-    int otrapreturn = trapreturn;
     int isfunc;
-    int traperr;
+    int traperr, new_trap_state, new_trap_return;
 
     /* if signal is being ignored or the trap function		      *
      * is NULL, then return					      *
@@ -1123,6 +1121,7 @@
     *sigtr |= ZSIG_IGNORED;
 
     lexsave();
+    /* execsave will save the old trap_return and trap_state */
     execsave();
     breaks = retflag = 0;
     runhookdef(BEFORETRAPHOOK, NULL);
@@ -1148,7 +1147,8 @@
 	sprintf(num, "%d", sig);
 	zaddlinknode(args, num);
 
-	trapreturn = -1;	/* incremented by doshfunc */
+	trap_return = -1;	/* incremented by doshfunc */
+	trap_state = TRAP_STATE_PRIMED;
 	trapisfunc = isfunc = 1;
 
 	sfcontext = SFC_SIGNAL;
@@ -1156,46 +1156,32 @@
 	sfcontext = osc;
 	freelinklist(args, (FreeFunc) NULL);
 	zsfree(name);
-
     } else {
-	trapreturn = -2;	/* not incremented, used at current level */
+	trap_return = -2;	/* not incremented, used at current level */
+	trap_state = TRAP_STATE_PRIMED;
 	trapisfunc = isfunc = 0;
 
 	execode(sigfn, 1, 0);
     }
     runhookdef(AFTERTRAPHOOK, NULL);
 
-    if (trapreturn > 0 && isfunc) {
-	/*
-	 * Context was its own function.  We propagate the return
-	 * value specially.  Return value zero means don't do
-	 * anything special, so don't handle it.
-	 */
-	trapret = trapreturn;
-    } else if (trapreturn >= 0 && !isfunc) {
-	/*
-	 * Context was an inline trap.  If an explicit return value
-	 * was used, we need to set `lastval'.  Otherwise we use the
-	 * value restored by execrestore.  In this case, all return
-	 * values indicate an explicit return from the current function,
-	 * so always handle specially.  trapreturn is always restored by
-	 * execrestore.
-	 */
-	trapret = trapreturn + 1;
-    }
     traperr = errflag;
-    trapreturn = otrapreturn;
+
+    /* Grab values before they are restored */
+    new_trap_state = trap_state;
+    new_trap_return = trap_return;
+
     execrestore();
     lexrestore();
 
-    if (trapret > 0) {
+    if (new_trap_state == TRAP_STATE_FORCE_RETURN &&
+	/* zero return from function isn't special */
+	!(isfunc && new_trap_return == 0)) {
 	if (isfunc) {
 	    breaks = loops;
 	    errflag = 1;
-	    lastval = trapret;
-	} else {
-	    lastval = trapret-1;
 	}
+	lastval = new_trap_return;
 	/* return triggered */
 	retflag = 1;
     } else {
Index: Src/zsh.h
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/zsh.h,v
retrieving revision 1.139
diff -u -r1.139 zsh.h
--- Src/zsh.h	31 Jul 2008 08:44:21 -0000	1.139
+++ Src/zsh.h	7 Aug 2008 10:07:10 -0000
@@ -921,7 +921,9 @@
     int badcshglob;
     pid_t cmdoutpid;
     int cmdoutval;
-    int trapreturn;
+    int trap_return;
+    int trap_state;
+    int trapisfunc;
     int noerrs;
     int subsh_close;
     char *underscore;
@@ -2225,6 +2227,24 @@
 #define ZSIG_ALIAS	(1<<3)  /* Trap is stored under an alias */
 #define ZSIG_SHIFT	4
 
+/*
+ * State of traps, stored in trap_state.
+ */
+enum trap_state {
+    /* Traps are not active; trap_return is not useful. */
+    TRAP_STATE_INACTIVE,
+    /*
+     * Traps are set but haven't triggered; trap_return gives
+     * minus function depth.
+     */
+    TRAP_STATE_PRIMED,
+    /*
+     * Trap has triggered to force a return; trap_return givens
+     * return value.
+     */
+    TRAP_STATE_FORCE_RETURN
+};
+
 /***********/
 /* Sorting */
 /***********/
Index: Test/A05execution.ztst
===================================================================
RCS file: /cvsroot/zsh/zsh/Test/A05execution.ztst,v
retrieving revision 1.5
diff -u -r1.5 A05execution.ztst
--- Test/A05execution.ztst	8 Nov 2006 10:38:07 -0000	1.5
+++ Test/A05execution.ztst	7 Aug 2008 10:07:10 -0000
@@ -124,6 +124,7 @@
 0:TRAPEXIT
 >Exit
 
+  unsetopt DEBUG_BEFORE_CMD
   unfunction fn
   print 'TRAPDEBUG() {
       print Line $LINENO
@@ -138,6 +139,7 @@
 >Line 1
 >Line 1
 
+  unsetopt DEBUG_BEFORE_CMD
   unfunction fn
   print 'trap '\''print Line $LINENO'\'' DEBUG
     :
Index: Test/C03traps.ztst
===================================================================
RCS file: /cvsroot/zsh/zsh/Test/C03traps.ztst,v
retrieving revision 1.13
diff -u -r1.13 C03traps.ztst
--- Test/C03traps.ztst	6 Aug 2008 08:54:23 -0000	1.13
+++ Test/C03traps.ztst	7 Aug 2008 10:07:10 -0000
@@ -402,6 +402,19 @@
 0: trapreturn handling bug is properly fixed
 ?./zsh-trapreturn-bug2:5: no such file or directory: ./fdasfsdafd
 
+  fn() {
+    setopt localtraps localoptions debugbeforecmd
+    trap '(( LINENO == 4 )) && setopt errexit' DEBUG
+    print $LINENO three
+    print $LINENO four
+    print $LINENO five
+    [[ -o errexit ]] && print "Hey, ERREXIT is set!"
+  }
+  fn
+1:Skip line from DEBUG trap
+>3 three
+>5 five
+
 %clean
 
   rm -f TRAPEXIT

-- 
Peter Stephenson <pws@xxxxxxx>                  Software Engineer
CSR PLC, Churchill House, Cambridge Business Park, Cowley Road
Cambridge, CB4 0WZ, UK                          Tel: +44 (0)1223 692070