Re: [PATCH glibc-2.44] Fix __pthread_timedblock for hurd

Hello,

Damien Zammit, le lun. 17 août 2026 23:49:24 +0000, a ecrit:
> When clock_id is CLOCK_MONOTONIC, there is an assumption
> that abstime parameter is actually a relative time

Where is such assumption?

pthread_cond_timedwait always take an absolute time.

> but the code was not making that assumption and getting the timeout
> wrong.
> 
> This is not tested, but according to OpenJDK 1.7.0 sources:
> 
>   if ((status = pthread_condattr_setclock(_condattr, CLOCK_MONOTONIC)) != 0) {
>     if (status == EINVAL) {
>       warning("Unable to use monotonic clock with relative timed-waits" \
>               " - changes to the time-of-day clock may have adverse affects");
> 
> So there are sources out there making this assumption.

I believe the "relative" word above is misunderstood. It doesn't mean
that the value is relative to "time now", but that it is relative to
e.g. the machine bootup, and not an absolute time which will be affected
by time-of-day changes. That's exactly what selecting CLOCK_MONOTONIC
means: get something that monitors actual time that passes, and not the
wall-clock time.

Even with CLOCK_MONOTONIC, pthread_cond_timedwait should be given values
akin to what clock_gettime() returns.

Samuel

> ---
>  .../hurd-pt-timedblock-relative.diff          | 54 +++++++++++++++++++
>  debian/patches/series                         |  1 +
>  2 files changed, 55 insertions(+)
>  create mode 100644 debian/patches/hurd-i386/hurd-pt-timedblock-relative.diff
> 
> diff --git a/debian/patches/hurd-i386/hurd-pt-timedblock-relative.diff b/debian/patches/hurd-i386/hurd-pt-timedblock-relative.diff
> new file mode 100644
> index 00000000..a7819ce3
> --- /dev/null
> +++ b/debian/patches/hurd-i386/hurd-pt-timedblock-relative.diff
> @@ -0,0 +1,54 @@
> +--- a/sysdeps/mach/htl/pt-timedblock.c
> ++++ b/sysdeps/mach/htl/pt-timedblock.c
> +@@ -40,23 +40,37 @@
> +   mach_msg_timeout_t timeout;
> +   struct timespec now;
> + 
> +-  /* We have an absolute time and now we have to convert it to a
> +-     relative time.  Arg.  */
> + 
> +-  err = __clock_gettime (clock_id, &now);
> +-  assert (!err);
> ++  if (clock_id == CLOCK_MONOTONIC)
> ++    {
> ++      /* abstime param should be a relative time */
> ++      if (abstime->tv_sec < 0
> ++          || (abstime->tv_sec == 0 && abstime->tv_nsec < 0))
> ++        return ETIMEDOUT;
> + 
> +-  if (now.tv_sec > abstime->tv_sec
> +-      || (now.tv_sec == abstime->tv_sec && now.tv_nsec > abstime->tv_nsec))
> +-    return ETIMEDOUT;
> +-
> +-  timeout = (abstime->tv_sec - now.tv_sec) * 1000;
> +-
> +-  if (abstime->tv_nsec >= now.tv_nsec)
> +-    timeout += (abstime->tv_nsec - now.tv_nsec + 999999) / 1000000;
> ++      timeout = abstime->tv_sec * 1000 + (abstime->tv_nsec + 999999) / 1000000;
> ++    }
> +   else
> +-    /* Need to do a carry.  */
> +-    timeout -= (now.tv_nsec - abstime->tv_nsec + 999999) / 1000000;
> ++    {
> ++      /* We have an absolute time and now we have to convert it to a
> ++         relative time.  This codepath could be avoided by using CLOCK_MONOTONIC
> ++         and passing a relative time for the abstime parameter. */
> ++
> ++      err = __clock_gettime (clock_id, &now);
> ++      assert (!err);
> ++
> ++      if (now.tv_sec > abstime->tv_sec
> ++          || (now.tv_sec == abstime->tv_sec && now.tv_nsec > abstime->tv_nsec))
> ++        return ETIMEDOUT;
> ++
> ++      timeout = (abstime->tv_sec - now.tv_sec) * 1000;
> ++
> ++      if (abstime->tv_nsec >= now.tv_nsec)
> ++        timeout += (abstime->tv_nsec - now.tv_nsec + 999999) / 1000000;
> ++      else
> ++        /* Need to do a carry.  */
> ++        timeout -= (now.tv_nsec - abstime->tv_nsec + 999999) / 1000000;
> ++    }
> + 
> +   err = __mach_msg (&msg, MACH_RCV_MSG | MACH_RCV_TIMEOUT | MSG_OPTIONS, 0,
> + 		    sizeof msg, thread->wakeupmsg.msgh_remote_port,
> diff --git a/debian/patches/series b/debian/patches/series
> index f4346bf6..4d9ea389 100644
> --- a/debian/patches/series
> +++ b/debian/patches/series
> @@ -41,6 +41,7 @@ hurd-i386/submitted-bind_umask2.diff
>  hurd-i386/tg-bootstrap.diff
>  hurd-i386/local-no_unsupported_ioctls.diff
>  hurd-i386/local-stack_chk_guard.diff
> +hurd-i386/hurd-pt-timedblock-relative.diff
>  
>  i386/local-biarch.diff
>  i386/unsubmitted-quiet-ldconfig.diff
> -- 
> 2.51.0
> 
> 
> 

-- 
Samuel
War doesn't prove who's right, just who's left.