Bug: time doesn't work on builtins

Bug: time doesn't work on builtins

[Mark J. Reed]

[-- Attachment #1: Type: text/plain, Size: 397 bytes --]

This was unexpected:

*zsh%*  time read
hello
*zsh%*


Any attempt to *time *a builtin generates no output whatsoever. No timings,
no error message.The timed command executes in the current shell and has
its usual side-effects, but the *time* itself does nothing.

Contrast to bash and ksh, in which time works on builtins the same as on
external commands.

-- 
Mark J. Reed <markjreed@gmail.com>

[-- Attachment #2: Type: text/html, Size: 925 bytes --]

Re: Bug: time doesn't work on builtins

[Bart Schaefer]

On Thu, Aug 15, 2024 at 6:15 AM Mark J. Reed <markjreed@gmail.com> wrote:
>
> Any attempt to time a builtin generates no output whatsoever. No timings, no error message.The timed command executes in the current shell and has its usual side-effects, but the time itself does nothing.

This was last discussed in users/28740 and subsequent thread ...

>> The OS facility that reports time statistics only works for a parent
>> process to read the stats of a child.  To work on builtins, we'd have
>> to rewrite that internally

That isn't quite accurate ... what we'd have to do is call RUSAGE_SELF
both before and after the builtin and subtract the former from the
latter, there's no (other) facility to get only the internal time
delta.

Re: Bug: time doesn't work on builtins

[Mark J. Reed]

[-- Attachment #1: Type: text/plain, Size: 1096 bytes --]

Absent a rewrite of the functionality to actually work on builtins, maybe
it could be modified to output a message about *time* only being supported
for external commands?

On Thu, Aug 15, 2024 at 2:52 PM Bart Schaefer <schaefer@brasslantern.com>
wrote:

> On Thu, Aug 15, 2024 at 6:15 AM Mark J. Reed <markjreed@gmail.com> wrote:
> >
> > Any attempt to time a builtin generates no output whatsoever. No
> timings, no error message.The timed command executes in the current shell
> and has its usual side-effects, but the time itself does nothing.
>
> This was last discussed in users/28740 and subsequent thread ...
>
> >> The OS facility that reports time statistics only works for a parent
> >> process to read the stats of a child.  To work on builtins, we'd have
> >> to rewrite that internally
>
> That isn't quite accurate ... what we'd have to do is call RUSAGE_SELF
> both before and after the builtin and subtract the former from the
> latter, there's no (other) facility to get only the internal time
> delta.
>


-- 
Mark J. Reed <markjreed@gmail.com>

[-- Attachment #2: Type: text/html, Size: 1706 bytes --]

Re: Bug: time doesn't work on builtins

[Bart Schaefer]

[-- Attachment #1: Type: text/plain, Size: 169 bytes --]

On Thu, Aug 15, 2024 at 3:08 PM Mark J. Reed <markjreed@gmail.com> wrote:
>
> Absent a rewrite of the functionality to actually work on builtins

Let's try this.

[-- Attachment #2: time-builtins.txt --]
[-- Type: text/plain, Size: 2956 bytes --]

diff --git a/Src/exec.c b/Src/exec.c
index 00278ac50..3c04c5efd 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -2938,6 +2938,11 @@ execcmd_exec(Estate state, Execcmd_params eparams,
      * in order to check for prefix commands.
      */
     LinkList preargs;
+    /*
+     * for the "time" builtin
+     */
+    child_times_t shti, chti;
+    struct timeval then;
 
     doneps4 = 0;
 
@@ -3621,6 +3626,8 @@ execcmd_exec(Estate state, Execcmd_params eparams,
 
     /* This is nonzero if the command is a current shell procedure? */
     is_cursh = (is_builtin || is_shfunc || nullexec || type >= WC_CURSH);
+    if (is_cursh && (how & Z_TIMED))
+	shelltime(&shti, &chti, &then, 0);
 
     /**************************************************************************
      * Do we need to fork?  We need to fork if:                               *
@@ -4377,6 +4384,8 @@ execcmd_exec(Estate state, Execcmd_params eparams,
 	    errflag |= ERRFLAG_ERROR;
 	}
     }
+    if (is_cursh && (how & Z_TIMED))
+	shelltime(&shti, is_builtin ? NULL : &chti, &then, 1);
     if (newxtrerr) {
 	int eno = errno;
 	fil = fileno(newxtrerr);
@@ -5265,7 +5274,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 */

Re: Bug: time doesn't work on builtins

[Bart Schaefer]

On Fri, Aug 16, 2024 at 12:20 PM Bart Schaefer
<schaefer@brasslantern.com> wrote:
>
> Let's try this.

Any comment on the patch?

Where would a test for this go?  The other "time" tests are in
A01grammar but only apply to whether it parses and produces TIMEFMT'ed
output correctly.  Perhaps A05execution?

Re: Bug: time doesn't work on builtins

[Jun. T]

> 2024/09/04 9:27, Bart Schaefer <schaefer@brasslantern.com> wrote:
> 
> On Fri, Aug 16, 2024 at 12:20 PM Bart Schaefer
> <schaefer@brasslantern.com> wrote:
>> 
>> Let's try this.
> 
> Any comment on the patch?

The followings still give no time statistics:

% time x=1
% time x=$(date)


The following works:

% x=0; time for ((i=1; i<=10000; ++i)); do let 'x+=i'; done; echo $x
shell  0.03s user 0.01s system 99% cpu 0.036 total
children  0.00s user 0.00s system 0% cpu 0.036 total
50005000

But if "let 'x+=i' is replaced by "((x+=i))"

% x=0; time for ((i=1; i<=10000; ++i)); do ((x+=i)); done; echo $x
0

The for loop is not executed, and the time statistics is not output.

This happens also without the patch time-builtins.txt.

Re: Bug: time doesn't work on builtins

[Jun. T]

> 2024/09/04 23:32, I wrote:
> 
> % x=0; time for ((i=1; i<=10000; ++i)); do ((x+=i)); done; echo $x
> 0
> 
> The for loop is not executed, and the time statistics is not output.

The reason that the for loop is skipped may be the following:

When exectime() calls execpline(,slcode=*state->pc++,) (exec.c:5280),
slcode is WC_SUBLIST with WC_SUBLIST_SIMPLE flag, and 'code' (the next
word code) obtained by (exec.c:1677)
    wordcode code = *state->pc++;
is just a line number, not WC_PIPE (see parse.c:757). Then the for
loop is skipped by (lines 1680,81)
    if (wc_code(code) != WC_PIPE)
        return lastval = (slflags & WC_SUBLIST_NOT) != 0;

Re: Bug: time doesn't work on builtins

[Bart Schaefer]

On Thu, Sep 5, 2024 at 10:29 AM Jun. T <takimoto-j@kba.biglobe.ne.jp> wrote:
>
> When exectime() calls execpline(,slcode=*state->pc++,) (exec.c:5280),
> slcode is WC_SUBLIST with WC_SUBLIST_SIMPLE flag, and 'code' (the next
> word code) obtained by (exec.c:1677)
>     wordcode code = *state->pc++;
> is just a line number, not WC_PIPE (see parse.c:757). Then the for
> loop is skipped by (lines 1680,81)
>     if (wc_code(code) != WC_PIPE)
>         return lastval = (slflags & WC_SUBLIST_NOT) != 0;

Is this enough?  All existing tests still pass, but I wonder if
there's another way to get there.

diff --git a/Src/exec.c b/Src/exec.c
index 00278ac50..057999844 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -1677,7 +1677,7 @@ execpline(Estate state, wordcode slcode, int how, int last
1)
     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;

Re: Bug: time doesn't work on builtins

[Jun T]

> 2024/09/06 8:37, Bart Schaefer <schaefer@brasslantern.com> wrote:
> 
> On Thu, Sep 5, 2024 at 10:29 AM Jun. T <takimoto-j@kba.biglobe.ne.jp> wrote:
>> 
>> loop is skipped by (lines 1680,81)
>>    if (wc_code(code) != WC_PIPE)
>>        return lastval = (slflags & WC_SUBLIST_NOT) != 0;
> 
> Is this enough?  All existing tests still pass, but I wonder if
> there's another way to get there.

Other calls of execpline() are all from execlist(), and it seems
the case with WC_SUBST_SIMPLE is already taken care of (for
example at around line 1492).

Re: Bug: time doesn't work on builtins

[Bart Schaefer]

On Wed, Sep 4, 2024 at 7:33 AM Jun. T <takimoto-j@kba.biglobe.ne.jp> wrote:
>
> The followings still give no time statistics:
>
> % time x=1
> % time x=$(date)

Hm, that's because we're taking the code path marked /* Empty command
*/ in execcmd_exec().

I think the only relevant case is around addvars() but can't be sure;
does anyone know a straightforward way to conditionally suppress the
child times in the case where the right-hand-side does not fork?

(Hand-fuzzed patch follows)

diff --git a/Src/exec.c b/Src/exec.c
index 00278ac50..c2f813078 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -3381,7 +3386,11 @@ execcmd_exec(Estate state, Execcmd_params eparams,
                    if (varspc) {
                        /* Make sure $? is still correct for assignment */
                        lastval = old_lastval;
+                       if (how & Z_TIMED)
+                           shelltime(&shti, &chti, &then, 0);
                        addvars(state, varspc, 0);
+                       if (how & Z_TIMED)
+                           shelltime(&shti, &chti, &then, 1);
                    }
                    if (errflag)
                        lastval = 1;

Re: Bug: time doesn't work on builtins

[Jun T]

> 2024/09/06 9:10, Bart Schaefer <schaefer@brasslantern.com> wrote:
> 
> On Wed, Sep 4, 2024 at 7:33 AM Jun. T <takimoto-j@kba.biglobe.ne.jp> wrote:
>> 
>> The followings still give no time statistics:
>> 
>> % time x=1
>> % time x=$(date)
> 
> (Hand-fuzzed patch follows)

The patch works for the above two cases, but I've fond there
are still more problems...

[1] In the patch file "time-builtin.txt":

@@ -4377,6 +4384,8 @@ execcmd_exec(Estate state, Execcmd_params eparams,
 	    errflag |= ERRFLAG_ERROR;
 	}
     }
+    if (is_cursh && (how & Z_TIMED))
+	shelltime(&shti, is_builtin ? NULL : &chti, &then, 1);

Due to "is_builtin ? NULL : &chti", child time is not reported for
the cases such as

% time eval 'external command'
% time . ./script	# script runs external command

I think it's better to pass &chti always.


[2] Time spent in prefork() is not included in the statistics.

% time echo $(x=0;for ((i=0; i<=100000; ++i)); do ((x+=i)); done; echo $x)  5000050000
shell  0.00s user 0.00s system 100% cpu 0.000 total

% time cat <(time-consuming-command)	# cat is not builtin
(the problem existed before applying "time-builtin.txt")

% time echo $(external command)
Even if the problem [1] is fixed (by always passing &chti)
the time reported would not include the time used for
the 'external command'.

Not sure this can be fixed easily.

Re: Bug: time doesn't work on builtins

[Bart Schaefer]

On Fri, Sep 6, 2024 at 2:52 AM Jun T <takimoto-j@kba.biglobe.ne.jp> wrote:
>
> % time eval 'external command'
> % time . ./script       # script runs external command
>
> I think it's better to pass &chti always.

OK.

> [2] Time spent in prefork() is not included in the statistics.

I believe that's fixable by always calling shelltime(...,0) when
Z_TIMED but then only calling shelltime(...,1) when is_cursh is
determined later.

Remaining question is whether stats should be printed on error cases,
e.g., "no such builtin":

time builtin nosuch $(long running command)

That means sticking in a few more calls to shelltime() in front of
early return statements.

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

[Bart Schaefer]

[-- Attachment #1: Type: text/plain, Size: 550 bytes --]

On Fri, Sep 6, 2024 at 12:13 PM Bart Schaefer <schaefer@brasslantern.com> 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.

[-- Attachment #2: time-builtins2.txt --]
[-- Type: text/plain, Size: 8351 bytes --]

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

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

[Oliver Kiddle]

On 13 Sep, Bart Schaefer wrote:
> +  ( setopt errexit; time =false; print notreached )
> +1:`time' of failed external with errexit
> +*?*user*system*cpu*total

This test fails on Solaris because the external false returns -1 instead
of 1.

Two alternatives that come to mind would be test which returns false if
given no arguments and expr 0 which has the advantage of not existing as
a builtin so no need for the =. Both of those return 1 on Solaris for
failure.

Oliver

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

[Bart Schaefer]

On Fri, Sep 20, 2024 at 12:46 AM Oliver Kiddle <opk@zsh.org> wrote:
>
> ... expr 0 which has the advantage of not existing as a builtin

I'll go with that.

diff --git a/Test/A08time.ztst b/Test/A08time.ztst
index 9fb1f3ebf..22a460f5e 100644
--- a/Test/A08time.ztst
+++ b/Test/A08time.ztst
@@ -59,6 +59,7 @@
 *?shell*
 *?children*

-  ( setopt errexit; time =false; print notreached )
+  ( setopt errexit; time expr 0; print notreached )
 1:`time' of failed external with errexit
+>0
 *?*user*system*cpu*total