Fix the max precision of the 'time' keyword

[JohnoKing]

This pull request makes the following changes:

• The time keyword now prioritizes getrusage to fix the bug reported at "time" builtin does not correctly report a third decimal digit ksh-community/ksh#7. Reproducer:
  TIMEFORMAT=$'\nreal\t%3R\tuser\t%3U\tsys\t%3S'; time sleep 1.235 # → real 1.240 user 0.000 sys 0.000
  This bugfix is based on the following ksh2020 commits:
  att/ast@48230e61
  att/ast@2e18c672
  att/ast@27777b3d (that commit breaks command substitutions, now reverted)
  getrusage, timeradd and timersub are detected with iffe feature tests. The times function is used as a fallback if getrusage isn't available.

• A single percent at the end of a format specifier is now treated as a literal '%' (like in Bash and ksh2020). The following command no longer causes a syntax error:
  TIMEFORMAT='%3S%'; time sleep .1

• The default output from time now zero-pads seconds. This change was made for consistency with the times builtin, which also zero-pads seconds as of commit 5c677a4.

• Fixed one instance of getrusage in libast that was missing an ifdef for _lib_getrusage.

This patch can be extended with support for microsecond precision, which I put in a separate gist edit: now in a downstream branch (since that isn't a bugfix):
JohnoKing@d97b64e

[hyenias]

@JohnoKing, wondering if you have taken into account the following existing code that pertains to $SECONDS:

ksh/src/cmd/ksh93/sh/init.c

Lines 571 to 628 in 84e2f6d

	/*
	* these functions are used to get and set the SECONDS variable
	*/
	#ifdef timeofday
	# define dtime(tp) ((double)((tp)->tv_sec)+1e-6*((double)((tp)->tv_usec)))
	# define tms timeval
	#else
	# define dtime(tp) (((double)times(tp))/shgd->lim.clk_tck)
	# define timeofday(a)
	#endif
	static void put_seconds(register Namval_t* np,const char *val,int flags,Namfun_t *fp)
	{
	double d;
	struct tms tp;
	if(!val)
	{
	nv_putv(np, val, flags, fp);
	fp = nv_stack(np, NIL(Namfun_t*));
	if(fp && !fp->nofree)
	free((void*)fp);
	return;
	}
	if(!np->nvalue.dp)
	{
	nv_setsize(np,3);
	nv_onattr(np,NV_DOUBLE);
	np->nvalue.dp = new_of(double,0);
	}
	nv_putv(np, val, flags, fp);
	d = *np->nvalue.dp;
	timeofday(&tp);
	*np->nvalue.dp = dtime(&tp)-d;
	}
	static char* get_seconds(register Namval_t* np, Namfun_t *fp)
	{
	Shell_t *shp = nv_shell(np);
	register int places = nv_size(np);
	struct tms tp;
	double d, offset = (np->nvalue.dp?*np->nvalue.dp:0);
	NOT_USED(fp);
	timeofday(&tp);
	d = dtime(&tp)- offset;
	sfprintf(shp->strbuf,"%.*f",places,d);
	return(sfstruse(shp->strbuf));
	}
	static Sfdouble_t nget_seconds(register Namval_t* np, Namfun_t *fp)
	{
	struct tms tp;
	double offset = (np->nvalue.dp?*np->nvalue.dp:0);
	NOT_USED(fp);
	timeofday(&tp);
	return(dtime(&tp)- offset);
	}

[JohnoKing]

The $SECONDS variable is something I overlooked, although from some testing it does work with millisecond precision when timeofday is defined (gettimeofday usually supports microsecond precision, although that is dependent on the OS):

$ echo $SECONDS
1547.796

If timeofday isn't defined (i.e. gettimeofday isn't supported), the precision is dependent on the times function, which on Linux is only up to a centisecond (so the third digit is always zero):

$ echo $SECONDS; sleep .002; echo $SECONDS
18.420
18.420

I'll update my gist to implement microsecond precision for $SECONDS when gettimeofday is present, although the fallback isn't going to be updated because the times function has undefined precision (limiting it will likely break compatibility if the OS implementation of times does support millisecond precision).

[hyenias]

I was not worried about the lesser precision (.01) when the OS cannot provide it. When I reviewed your code, you used names of variables and such that had already been defined elsewhere (might want to check out the alarm.c and timers.c as well).

As far as changing $SECONDS to implement microsecond (µsec aka .000001) precision when gettimeofday is present, no need as it already works with µsec precision on all of my systems I checked: Linux 32bit & 64bit, FreeBSD 32bit & 64bit, and macOS. While testing I found out that ksh's sleep builtin only supports millisecond (.001) and when given anything lower than that it appears to just exit without sleeping at all. As my machines are older, I utilized the following to determine the smallest time interval each system could support.

$ typeset -F a b
$ a=SECONDS; b=SECONDS; printf '%f\n0.123456\n' b-a
0.000003
0.123456
$ a=SECONDS; : ; b=SECONDS; printf '%f\n0.123456\n' b-a
0.000007
0.123456
$

ksh currently limits the output of variable $SECONDS to millisecond precision.

$ typeset -p SECONDS
typeset -F 3 SECONDS=1414.612

If one wishes the $SECONDS variable to expand to µsec precision then increase its float point precision from 3 to 6:

$ typeset -p SECONDS
typeset -F 6 SECONDS=1430.973949
$

[JohnoKing]

The sleep builtin in ksh93v- handles microseconds just fine, indicating a bugfix to backport. This can be demonstrated by running the following loop in ksh93u+ and ksh93v-:

$ cat ./loop.sh
for ((i = 0; i != 10000; i++)) do
    sleep PT100U
done
# The results below are inflated due to overhead from the for loop,
# but the time elapsed should be at least one second.  In ksh93u+ the
# time elapsed is much lower because microseconds are ignored.
$ time ksh93u+ ./loop.sh

real	0m0.06s
user	0m0.04s
sys	0m0.02s
$ time ksh93v- ./loop.sh

real	0m1.58s
user	0m0.07s
sys	0m0.00s

[hyenias]

Very nice find on the sleep builtin! Also, thank you for improving ksh93 as working on the time keyword was on my todo list. What's more, my systems are so much slower than yours.

$ time ksh93 ./loop.ksh
        1.32 real         0.00 user         1.32 sys
$ time ksh93v ./loop.ksh
       25.26 real         0.00 user         1.20 sys

[McDutchie]

Nice work.

However, after applying, ksh no longer uses the locale's radix point (. or ,) if TIMEFORMAT is set.

Test command:

$ LC_ALL=nl_NL.UTF-8 TIMEFORMAT=$'\nreal\t%3R\tuser\t%3U\tsys\t%3S' ksh -c 'time sleep 1.235'

Output before:

real	1,240	user	0,000	sys	0,000

Output after:

real	1.237	user	0.000	sys	0.000

Unsurprisingly, ksh2020 has this problem as well.

[JohnoKing]

The locale's radix point is now used when TIMEFORMAT is set. The fallback code for when getrusage and gettimeofday aren't available has also been updated to fix a bus error.

[JohnoKing]

The change from att/ast@27777b3d has been reverted because it caused a regression in command substitutions. The user and system time were intermittently returning negative numbers because getrusage was run before sh_subfork:

$ t=$(time sleep 0)

real	0m00.00s
user	0m-1.99s
sys	0m-1.99s

[McDutchie]

I'm getting this regression test failure:

$ bin/shtests -p basic
#### Regression-testing /usr/local/src/ksh93/ksh/arch/darwin.i386-64/bin/ksh ####
test basic begins at 2020-07-13+23:46:45
C-EU.UTF-8: unknown locale
	basic.sh[589]: The time keyword ignores the locale's radix point (both are .)
test basic failed at 2020-07-13+23:46:50 with exit code 1 [ 105 tests 1 error ]

You probably want C_EU.UTF-8 (note underscore). Apparently, libast supports this somewhere, but I can't find it in the code. It's mentioned and used in tests/locale.sh though.

[JohnoKing]

I have changed C-EU to C_EU, although I'm not sure why it was failing on your end. Setting LC_ALL to C-EU.UTF-8 works on my Linux and FreeBSD systems just fine (even though it isn't a system locale).

[McDutchie]

Yes, it's very strange. I've tried using variants of C-EU on an interactive ksh and that appears to work. I've tried to find out where the code is that supports these internal locales but I haven't found it so far.

[McDutchie]

Anyway, that's not directly relevant to this pull request. It looks good to me now. Are you still doing tests? Let me know when you consider it ready to merge.

[McDutchie]

Actually, what is your method of testing the fallback that uses times(3) (with neither timeofday() nor getrusage(3) available)? I can't seem to get there without triggering a segfault.

[JohnoKing]

Here is a debug patch:

--- a/src/cmd/ksh93/sh/xec.c
+++ b/src/cmd/ksh93/sh/xec.c
@@ -64,6 +64,16 @@
     static pid_t sh_ntfork(Shell_t*,const Shnode_t*,char*[],int*,int);
 #endif /* SHOPT_SPAWN */
 
+/* Note: If _lib_getrusage is defined then timeofday must be defined as well.
+   This forces the standalone times(3) fallback. */
+#if SHOPT_ONLY_TIMES
+#undef _lib_getrusage
+#undef timeofday
+#elif SHOPT_TIMES_WITH_GETTIMEOFDAY
+/* Fallback to times + gettimeofday without getrusage */
+#undef _lib_getrusage
+#endif
+
 static void	sh_funct(Shell_t *,Namval_t*, int, char*[], struct argnod*,int);
 static void	coproc_init(Shell_t*, int pipes[]);
 

[McDutchie]

Also, thanks for the patch for microsecond precision; it appears to be working nicely. I'll add that to my post-release todo list. :)