PATCH: Re: Bug: time doesn't work on builtins

[Bart Schaefer <schaefer@xxxxxxxxxxxxxxxx>]

On Fri, Sep 6, 2024 at 12:13 PM Bart Schaefer <schaefer@xxxxxxxxxxxxxxxx> wrote:
>
> Remaining question is whether stats should be printed on error cases,
> e.g., "no such builtin":

I've gone ahead with this, it's a bit ugly because of all the early
"return;" but it works.

Tests included, though they only check whether `time' returns
something, not whether that something makes sense in terms of cpu
usage etc.  If anyone has any ideas how to make that predictable
enough to pattern-match the output, feel free to update the tests.

diff --git a/Src/exec.c b/Src/exec.c
index 00278ac50..8aa7466f5 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -881,6 +881,10 @@ execute(LinkList args, int flags, int defpath)
 	_realexit();
     else
 	zerr("command not found: %s", arg0);
+    /* This is bash behavior, but fails to restore interactive settings etc.
+    lastval = ((eno == EACCES || eno == ENOEXEC) ? 126 : 127);
+    return;
+    */
     _exit((eno == EACCES || eno == ENOEXEC) ? 126 : 127);
 }
 
@@ -1677,7 +1681,7 @@ execpline(Estate state, wordcode slcode, int how, int last1)
     wordcode code = *state->pc++;
     static int lastwj, lpforked;
 
-    if (wc_code(code) != WC_PIPE)
+    if (wc_code(code) != WC_PIPE && !(how & Z_TIMED))
 	return lastval = (slflags & WC_SUBLIST_NOT) != 0;
     else if (slflags & WC_SUBLIST_NOT)
 	last1 = 0;
@@ -2939,6 +2943,14 @@ execcmd_exec(Estate state, Execcmd_params eparams,
      */
     LinkList preargs;
 
+    /*
+     * for the "time" keyword
+     */
+    child_times_t shti, chti;
+    struct timeval then;
+    if (how & Z_TIMED)
+	shelltime(&shti, &chti, &then, 0);
+
     doneps4 = 0;
 
     /*
@@ -3071,6 +3083,8 @@ execcmd_exec(Estate state, Execcmd_params eparams,
 		if (!(hn = resolvebuiltin(cmdarg, hn))) {
 		    if (forked)
 			_realexit();
+		    if (how & Z_TIMED)
+			shelltime(&shti, &chti, &then, 1);
 		    return;
 		}
 		if (type != WC_TYPESET)
@@ -3252,6 +3266,8 @@ execcmd_exec(Estate state, Execcmd_params eparams,
 			    errflag |= ERRFLAG_ERROR;
 			    if (forked)
 				_realexit();
+			    if (how & Z_TIMED)
+				shelltime(&shti, &chti, &then, 1);
 			    return;
 			}
 		    }
@@ -3343,6 +3359,8 @@ execcmd_exec(Estate state, Execcmd_params eparams,
 			errflag |= ERRFLAG_ERROR;
 			if (forked)
 			    _realexit();
+			if (how & Z_TIMED)
+			    shelltime(&shti, &chti, &then, 1);
 			return;
 		    } else if (!nullcmd || !*nullcmd || opts[SHNULLCMD]) {
 			if (!args)
@@ -3363,6 +3381,8 @@ execcmd_exec(Estate state, Execcmd_params eparams,
 		    lastval = 0;
 		    if (forked)
 			_realexit();
+		    if (how & Z_TIMED)
+			shelltime(&shti, &chti, &then, 1);
 		    return;
 		} else {
 		    /*
@@ -3375,6 +3395,8 @@ execcmd_exec(Estate state, Execcmd_params eparams,
 			lastval = 1;
 			if (forked)
 			    _realexit();
+			if (how & Z_TIMED)
+			    shelltime(&shti, &chti, &then, 1);
 			return;
 		    }
 		    cmdoutval = use_cmdoutval ? lastval : 0;
@@ -3393,6 +3415,8 @@ execcmd_exec(Estate state, Execcmd_params eparams,
 		    }
 		    if (forked)
 			_realexit();
+		    if (how & Z_TIMED)
+			shelltime(&shti, &chti, &then, 1);
 		    return;
 		}
 	    } else if (isset(RESTRICTED) && (cflags & BINF_EXEC) && do_exec) {
@@ -3401,6 +3425,8 @@ execcmd_exec(Estate state, Execcmd_params eparams,
 		lastval = 1;
 		if (forked)
 		    _realexit();
+		if (how & Z_TIMED)
+		    shelltime(&shti, &chti, &then, 1);
 		return;
 	    }
 
@@ -3437,6 +3463,8 @@ execcmd_exec(Estate state, Execcmd_params eparams,
 			opts[AUTOCONTINUE] = oautocont;
 		    if (forked)
 			_realexit();
+		    if (how & Z_TIMED)
+			shelltime(&shti, &chti, &then, 1);
 		    return;
 		}
 		break;
@@ -3448,6 +3476,8 @@ execcmd_exec(Estate state, Execcmd_params eparams,
 		if (!(hn = resolvebuiltin(cmdarg, hn))) {
 		    if (forked)
 			_realexit();
+		    if (how & Z_TIMED)
+			shelltime(&shti, &chti, &then, 1);
 		    return;
 		}
 		break;
@@ -3466,6 +3496,8 @@ execcmd_exec(Estate state, Execcmd_params eparams,
 	    opts[AUTOCONTINUE] = oautocont;
 	if (forked)
 	    _realexit();
+	if (how & Z_TIMED)
+	    shelltime(&shti, &chti, &then, 1);
 	return;
     }
 
@@ -3545,6 +3577,8 @@ execcmd_exec(Estate state, Execcmd_params eparams,
 		    opts[AUTOCONTINUE] = oautocont;
 		if (forked)
 		    _realexit();
+		if (how & Z_TIMED)
+		    shelltime(&shti, &chti, &then, 1);
 		return;
 	    }
 	}
@@ -3575,6 +3609,8 @@ execcmd_exec(Estate state, Execcmd_params eparams,
 	    opts[AUTOCONTINUE] = oautocont;
 	if (forked)
 	    _realexit();
+	if (how & Z_TIMED)
+	    shelltime(&shti, &chti, &then, 1);
 	return;
     }
 
@@ -4377,6 +4413,8 @@ execcmd_exec(Estate state, Execcmd_params eparams,
 	    errflag |= ERRFLAG_ERROR;
 	}
     }
+    if ((is_cursh || do_exec) && (how & Z_TIMED))
+	shelltime(&shti, &chti, &then, 1);
     if (newxtrerr) {
 	int eno = errno;
 	fil = fileno(newxtrerr);
@@ -5265,7 +5303,7 @@ exectime(Estate state, UNUSED(int do_exec))
 
     jb = thisjob;
     if (WC_TIMED_TYPE(state->pc[-1]) == WC_TIMED_EMPTY) {
-	shelltime();
+	shelltime(NULL,NULL,NULL,0);
 	return 0;
     }
     execpline(state, *state->pc++, Z_TIMED|Z_SYNC, 0);
diff --git a/Src/jobs.c b/Src/jobs.c
index 07facc60b..39c664388 100644
--- a/Src/jobs.c
+++ b/Src/jobs.c
@@ -1894,7 +1894,7 @@ spawnjob(void)
 
 /**/
 void
-shelltime(void)
+shelltime(child_times_t *shell, child_times_t *kids, struct timeval *then, int delta)
 {
     struct timezone dummy_tz;
     struct timeval dtimeval, now;
@@ -1913,7 +1913,28 @@ shelltime(void)
     ti.ut = buf.tms_utime;
     ti.st = buf.tms_stime;
 #endif
-    printtime(dtime(&dtimeval, &shtimer, &now), &ti, "shell");
+    if (shell) {
+	if (delta) {
+#ifdef HAVE_GETRUSAGE
+	    dtime(&ti.ru_utime, &shell->ru_utime, &ti.ru_utime);
+	    dtime(&ti.ru_stime, &shell->ru_stime, &ti.ru_stime);
+#else
+	    ti.ut -= shell->ut;
+	    ti.st -= shell->st;
+#endif
+	} else
+	    *shell = ti;
+    }
+    if (delta)
+	dtime(&dtimeval, then, &now);
+    else {
+	if (then)
+	    *then = now;
+	dtime(&dtimeval, &shtimer, &now);
+    }
+
+    if (!delta == !shell)
+	printtime(&dtimeval, &ti, "shell");
 
 #ifdef HAVE_GETRUSAGE
     getrusage(RUSAGE_CHILDREN, &ti);
@@ -1921,8 +1942,20 @@ shelltime(void)
     ti.ut = buf.tms_cutime;
     ti.st = buf.tms_cstime;
 #endif
-    printtime(&dtimeval, &ti, "children");
-
+    if (kids) {
+	if (delta) {
+#ifdef HAVE_GETRUSAGE
+	    dtime(&ti.ru_utime, &kids->ru_utime, &ti.ru_utime);
+	    dtime(&ti.ru_stime, &kids->ru_stime, &ti.ru_stime);
+#else
+	    ti.ut -= shell->ut;
+	    ti.st -= shell->st;
+#endif
+	} else
+	    *kids = ti;
+    }
+    if (!delta == !kids)
+	printtime(&dtimeval, &ti, "children");
 }
 
 /* see if jobs need printing */
diff --git a/Test/A01grammar.ztst b/Test/A01grammar.ztst
index d57085798..660602caf 100644
--- a/Test/A01grammar.ztst
+++ b/Test/A01grammar.ztst
@@ -399,13 +399,6 @@
 >This is name2
 >This is still name2
 
-  (time cat) >&/dev/null
-0:`time' keyword (status only)
-
-  TIMEFMT='%E %mE %uE %* %m%mm %u%uu'; time (:)
-0:`time' keyword with custom TIMEFMT
-*?[0-9]##.[0-9](#c2)s [0-9]##ms [0-9]##us %\* %m%mm %u%uu
-
   if [[ -f foo && -d . && -n $ZTST_testdir ]]; then
     true
   else
diff --git a/Test/A08time.ztst b/Test/A08time.ztst
new file mode 100644
index 000000000..9fb1f3ebf
--- /dev/null
+++ b/Test/A08time.ztst
@@ -0,0 +1,64 @@
+#
+# This file contains tests for the "time" reserved word
+#
+
+%prep
+
+  unset TIMEFMT
+
+%test
+
+  (time cat) >&/dev/null
+0:`time' keyword (status only)
+
+  ( TIMEFMT='%E %mE %uE %* %m%mm %u%uu'; time (:) )
+0:`time' keyword with custom TIMEFMT
+*?[0-9]##.[0-9](#c2)s [0-9]##ms [0-9]##us %\* %m%mm %u%uu
+
+  time x=1
+0:`time' simple assignment
+*?shell*
+*?children*
+
+  time x=$(date)
+0:`time' assignment with external command
+*?shell*
+*?children*
+
+  x=0; time for ((i=1; i<=10000; ++i)); do ((x+=i)); done; echo $x
+0:`time' for-loop with arithmetic condition
+>50005000
+*?shell*
+*?children*
+
+  time echo $(x=0;for ((i=0; i<=100000; ++i)); do ((x+=i)); done; echo $x)
+0:`time' of a builtin with argument command substitution
+>5000050000
+*?shell*
+*?children*
+
+  time cat <(x=0;for ((i=0; i<=100000; ++i)); do ((x+=i)); done; echo $x)
+0:`time' of external command with process substitution
+>5000050000
+*?*user*system*cpu*total
+
+  print -u $ZTST_fd 'This test takes 2 seconds'
+  time builtin nonesuch $(sleep 2)
+1:`time' of nonexistent builtin with command substitution
+*?*: no such builtin: nonesuch
+*?shell*
+*?children*
+
+  time /no/such/commmand
+127:`time' of nonexistent external
+*?*no such file or directory: /no/such/commmand
+*?*user*system*cpu*total
+
+  ( setopt errexit; time false; print notreached )
+1:`time' of failed builtin with errexit
+*?shell*
+*?children*
+
+  ( setopt errexit; time =false; print notreached )
+1:`time' of failed external with errexit
+*?*user*system*cpu*total