Subject: Re: bug#77535: timeout treats very short durations as `0`

From: Pádraig Brady <[email-masked]>
Date: Sat, 5 Apr 2025 21:56:53 +0100

On 05/04/2025 12:03, Pádraig Brady wrote:
> On 05/04/2025 09:03, Paul Eggert wrote:
>> On 2025-04-04 12:27, Pádraig Brady wrote:
>>> +  /* Clamp underflow to 1ns, as 0 disables the timeout.  */
>>> +  if (duration == 0 && errno == ERANGE)
>>> +    duration = 1e-9;
>>
>> That isn't exactly right as the 1e-9 double-rounds to 2e-9 when we
>> compute the struct timespec. Also, even with the patch the code
>> mishandles 16777216.000000001 (2**24 + 10**-9) by treating it as if it
>> were just 16777216. These are tiny bugs, I know, but I took up the
>> challenge of doing this things more correctly by installing the attached.
> 
> This is the first time coreutils is linking with libm.
> So practically that does add measurable overhead:
> 
>     $ time seq 10000 | xargs -n1 src/timeout-old >/dev/null
>     real    0m7.139s
> 
>     $ time seq 10000 | xargs -n1 src/timeout >/dev/null
>     real    0m8.161s
> 
> So on my 2.6GHz i7-5600U we're adding 102,200 ns to the startup time,
> to address theoretical single nanosecond issues.
> Given the code is also more complex, I'm thinking using libm is not worth it?
> 
> BTW I'll apply the attached cleanups if we keep the libm dependency.

I'm going to apply the attached later to remove the libm dependency.
It's just too much of a dependency for minimal functionality.
All existing tests are maintained, and still pass.

cheers,
Pádraig

From 7b05df48a59963c31011b1891b63a99f9a89bbf1 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= <[email-masked]>
Date: Sat, 5 Apr 2025 21:48:05 +0100
Subject: [PATCH] timeout: remove dependence on libm

This was seen to add about 100,000 ns to the startup time,
on a 2.6 GHz i7-5600U with glibc 2.40.

* bootstrap.conf: Remove fenv-rounding and signbit deps.
* src/local.mk: Remove fenv lib dependency.
* src/timeout.c (is_negative): A new helper function to
be equivalent of signbit in the underflow case.
(parse_duration): Remove the rounding up logic,
as a nanosecond here or there has no significance.
---
 bootstrap.conf |  2 --
 src/local.mk   |  3 ---
 src/timeout.c  | 36 +++++++++++-------------------------
 3 files changed, 11 insertions(+), 30 deletions(-)

diff --git a/bootstrap.conf b/bootstrap.conf
index 3c133ef71..c99838e95 100644
--- a/bootstrap.conf
+++ b/bootstrap.conf
@@ -99,7 +99,6 @@ gnulib_modules="
   fdatasync
   fdopen
   fdutimensat
-  fenv-rounding
   file-has-acl
   file-type
   fileblocks
@@ -243,7 +242,6 @@ gnulib_modules="
   settime
   sig2str
   sigaction
-  signbit
   skipchars
   smack
   ssize_t
diff --git a/src/local.mk b/src/local.mk
index 0a4e3af43..fd9dc81c2 100644
--- a/src/local.mk
+++ b/src/local.mk
@@ -257,9 +257,6 @@ src_stat_LDADD += $(LIB_SELINUX)
 # for nvlist_lookup_uint64_array
 src_stat_LDADD += $(LIB_NVPAIR)
 
-# for fegetround, fesetround
-src_timeout_LDADD += $(FENV_ROUNDING_LIBM)
-
 # for gettime, settime, tempname, utimecmp, utimens
 copy_ldadd += $(CLOCK_TIME_LIB)
 src_date_LDADD += $(CLOCK_TIME_LIB)
diff --git a/src/timeout.c b/src/timeout.c
index 5947c1942..fe8eb4559 100644
--- a/src/timeout.c
+++ b/src/timeout.c
@@ -45,9 +45,8 @@
    Written by Pádraig Brady.  */
 
 #include <config.h>
-#include <fenv.h>
+#include <ctype.h>
 #include <getopt.h>
-#include <math.h>
 #include <stdio.h>
 #include <sys/types.h>
 #include <signal.h>
@@ -354,32 +353,25 @@ apply_time_suffix (double *x, char suffix_char)
   return true;
 }
 
+ATTRIBUTE_PURE static bool
+is_negative (char const *num)
+{
+  while (*num && isspace (to_uchar (*num)))
+    num++;
+  return *num == '-';
+}
+
 static double
 parse_duration (char const *str)
 {
-  /* If possible tell strtod to round up, so that we always wait at
-     least as long as STR specifies.  Although Standard C requires
-     FENV_ACCESS ON, don't bother if using GCC as it warns.  */
-#ifdef FE_UPWARD
-# if !defined __GNUC__ && 199901 <= __STDC_VERSION__
-#  pragma STDC FENV_ACCESS ON
-# endif
-  int round = fegetround ();
-  fesetround (FE_UPWARD);
-#endif
-
   char *ep;
   double duration = cl_strtod (str, &ep);
 
-#ifdef FE_UPWARD
-  fesetround (round);
-#endif
-
   if (ep == str
       /* Nonnegative interval.  */
       || ! (0 <= duration)
       /* The interval did not underflow to -0.  */
-      || (signbit (duration) && errno == ERANGE)
+      || (errno == ERANGE && is_negative (str))
       /* No extra chars after the number and an optional s,m,h,d char.  */
       || (*ep && *(ep + 1))
       /* Check any suffix char and update timeout based on the suffix.  */
@@ -391,16 +383,10 @@ parse_duration (char const *str)
 
   /* Do not let the duration underflow to 0, as 0 disables the timeout.
      Use 2**-30 instead of 0; settimeout will round it up to 1 ns,
-     whereas 1e-9 might double-round to 2 ns.
+     whereas 1e-9 might double-round to 2 ns.  */
 
-     If FE_UPWARD is defined, this code is not needed on glibc as its
-     strtod rounds upward correctly.  Although this code might not be
-     needed on non-glibc platforms too, it's too much trouble to
-     worry about that.  */
-#if !defined FE_UPWARD || !defined __GLIBC__
   if (duration == 0 && errno == ERANGE)
     duration = 9.313225746154785e-10;
-#endif
 
   return duration;
 }
-- 
2.48.1