[PATCH 0/2] difftime isn't even pure

[PATCH 0/2] difftime isn't even pure

[Paul Eggert]

Here are two patches to fix the problem Vincent Lefèvre
recently noted in BZ 31808, which is that difftime can
throw an inexact exception, and the time_t range isn't
clearly documented.

Paul Eggert (2):
  difftime can throw exceptions
  Improve doc for time_t range (BZ 31808)

 manual/time.texi | 12 +++++++++++-
 time/time.h      |  5 ++---
 2 files changed, 13 insertions(+), 4 deletions(-)

--
2.45.1

[PATCH 1/2] difftime can throw exceptions

[Paul Eggert]

difftime can signal an inexact conversion when converting to double,
so it should not be marked as pure or nothrow (BZ 31808).

Although we could do something more complicated, in which difftime is
plain on modern platforms but const and nothrow on obsolescent
platforms with 32-bit time_t, it hardly seems worth the trouble.
difftime is used so rarely that it's not worth taking pains to
optimize calls to it on obsolescent platforms.
---
 time/time.h | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/time/time.h b/time/time.h
index 486367f7dd..b6b439a452 100644
--- a/time/time.h
+++ b/time/time.h
@@ -76,8 +76,7 @@ extern clock_t clock (void) __THROW;
 extern time_t time (time_t *__timer) __THROW;
 
 /* Return the difference between TIME1 and TIME0.  */
-extern double difftime (time_t __time1, time_t __time0)
-     __THROW __attribute_pure__;
+extern double difftime (time_t __time1, time_t __time0);
 
 /* Return the `time_t' representation of TP and normalize TP.  */
 extern time_t mktime (struct tm *__tp) __THROW;
@@ -85,7 +84,7 @@ extern time_t mktime (struct tm *__tp) __THROW;
 # ifdef __REDIRECT_NTH
 extern time_t __REDIRECT_NTH (time, (time_t *__timer), __time64);
 extern double __REDIRECT_NTH (difftime, (time_t __time1, time_t __time0),
-                              __difftime64) __attribute_pure__;
+                              __difftime64);
 extern time_t __REDIRECT_NTH (mktime, (struct tm *__tp), __mktime64);
 # else
 #  define time __time64
-- 
2.45.1

Re: [PATCH 1/2] difftime can throw exceptions

[DJ Delorie]

Paul Eggert <eggert@cs.ucla.edu> writes:
> Although we could do something more complicated, in which difftime is
> plain on modern platforms but const and nothrow on obsolescent
> platforms with 32-bit time_t, it hardly seems worth the trouble.
> difftime is used so rarely that it's not worth taking pains to
> optimize calls to it on obsolescent platforms.

So the rule is "function might throw an exception", ok...

>  /* Return the difference between TIME1 and TIME0.  */
> -extern double difftime (time_t __time1, time_t __time0)
> -     __THROW __attribute_pure__;
> +extern double difftime (time_t __time1, time_t __time0);

So now we (1) assume it can throw by not labelling it __THROW, and (2)
revoking its pure attribute because it might throw.

Ok.

(Who decided __THROW means "doesn't throw" ?)

>  extern double __REDIRECT_NTH (difftime, (time_t __time1, time_t __time0),
> -                              __difftime64) __attribute_pure__;
> +                              __difftime64);

Basically the same here, ok.

Neither of these changes could do anything other than reduce
optimization opportunities anyway, so I can't see how functionality
would be changed.

LGTM
Reviewed-by: DJ Delorie <dj@redhat.com>

Re: [PATCH 1/2] difftime can throw exceptions

[Adhemerval Zanella Netto]

On 03/06/24 19:01, DJ Delorie wrote:
> Paul Eggert <eggert@cs.ucla.edu> writes:
>> Although we could do something more complicated, in which difftime is
>> plain on modern platforms but const and nothrow on obsolescent
>> platforms with 32-bit time_t, it hardly seems worth the trouble.
>> difftime is used so rarely that it's not worth taking pains to
>> optimize calls to it on obsolescent platforms.
> 
> So the rule is "function might throw an exception", ok...
> 
>>  /* Return the difference between TIME1 and TIME0.  */
>> -extern double difftime (time_t __time1, time_t __time0)
>> -     __THROW __attribute_pure__;
>> +extern double difftime (time_t __time1, time_t __time0);
> 
> So now we (1) assume it can throw by not labelling it __THROW, and (2)
> revoking its pure attribute because it might throw.
> 
> Ok.
> 
> (Who decided __THROW means "doesn't throw" ?)

That is an excellent question...

> 
>>  extern double __REDIRECT_NTH (difftime, (time_t __time1, time_t __time0),
>> -                              __difftime64) __attribute_pure__;
>> +                              __difftime64);
> 
> Basically the same here, ok.
> 
> Neither of these changes could do anything other than reduce
> optimization opportunities anyway, so I can't see how functionality
> would be changed.
> 
> LGTM
> Reviewed-by: DJ Delorie <dj@redhat.com>
> 

Re: [PATCH 1/2] difftime can throw exceptions

[Florian Weimer]

* Adhemerval Zanella Netto:

>> (Who decided __THROW means "doesn't throw" ?)
>
> That is an excellent question...

It stands for throw(), the old version of the C++ noexcept specifier
which could also contain an explicit exception list.

Thanks,
Florian

__THROW → __NOEXCEPT (was: difftime can throw exceptions)

[Paul Eggert]

On 2024-06-04 05:39, Florian Weimer wrote:
> * Adhemerval Zanella Netto:
> 
>>> (Who decided __THROW means "doesn't throw" ?)
>> That is an excellent question...
> It stands for throw(), the old version of the C++ noexcept specifier
> which could also contain an explicit exception list.

Would anyone object to renaming __THROW to __NOEXCEPT? C++11 renamed 
'throw' to 'noexcept' and C++20 removed 'throw', so using __NOEXCEPT 
would make headers more readable to today's users.

Re: __THROW → __NOEXCEPT

[Florian Weimer]

* Paul Eggert:

> On 2024-06-04 05:39, Florian Weimer wrote:
>> * Adhemerval Zanella Netto:
>> 
>>>> (Who decided __THROW means "doesn't throw" ?)
>>> That is an excellent question...
>> It stands for throw(), the old version of the C++ noexcept specifier
>> which could also contain an explicit exception list.
>
> Would anyone object to renaming __THROW to __NOEXCEPT? C++11 renamed
> 'throw' to 'noexcept' and C++20 removed 'throw', so using __NOEXCEPT
> would make headers more readable to today's users.

I'm not sure if it's worth the update churn, but please make it
__NOEXCEPT_LEAF and use __NOEXCEPT for for __THROWNL.

And we really can't remove the old macros, they are widely used.

Thanks,
Florian

Re: [PATCH 1/2] difftime can throw exceptions

[Florian Weimer]

* Paul Eggert:

> difftime can signal an inexact conversion when converting to double,
> so it should not be marked as pure or nothrow (BZ 31808).

At the GCC level, throwing from a signal handler inside is only
supported if building with -fnon-call-exceptions.  Is anyone actually
doing that?

Thanks,
Florian

Re: [PATCH 1/2] difftime can throw exceptions

[Paul Eggert]

On 2024-06-04 09:42, Florian Weimer wrote:

> At the GCC level, throwing from a signal handler inside is only
> supported if building with -fnon-call-exceptions.  Is anyone actually
> doing that?

A quick search suggests that some people do it, yes. For example:

https://github.com/golang/gofrontend/blob/60f985a7852632834936b4b859aa75d9df88f038/libgo/Makefile.am#L49

https://github.com/LibreOffice/core/blob/d6607a9d9a3076c1d7c9a3e284631c5f57c24137/bridges/Library_cpp_uno.mk#L241

https://forums.zeroc.com/discussion/46747/problem-linking-to-debug-libraries-on-rh7

I didn't check whether any of these people also use difftime.

Re: [PATCH 1/2] difftime can throw exceptions

[Florian Weimer]

* Paul Eggert:

> On 2024-06-04 09:42, Florian Weimer wrote:
>
>> At the GCC level, throwing from a signal handler inside is only
>> supported if building with -fnon-call-exceptions.  Is anyone actually
>> doing that?
>
> A quick search suggests that some people do it, yes. For example:
>
> https://github.com/golang/gofrontend/blob/60f985a7852632834936b4b859aa75d9df88f038/libgo/Makefile.am#L49
>
> https://github.com/LibreOffice/core/blob/d6607a9d9a3076c1d7c9a3e284631c5f57c24137/bridges/Library_cpp_uno.mk#L241
>
> https://forums.zeroc.com/discussion/46747/problem-linking-to-debug-libraries-on-rh7

Yes, it's heavily exercised by the Ada front end.

But what I meant: The code that throws from a synchronously invoked
signal handler would have to be built with -fnon-call-exceptions, so in
our case, glibc.

Thanks,
Florian

Re: [PATCH 1/2] difftime can throw exceptions

[Paul Eggert]

On 6/4/24 13:52, Florian Weimer wrote:
> But what I meant: The code that throws from a synchronously invoked
> signal handler would have to be built with -fnon-call-exceptions, so in
> our case, glibc.

Oh, in that case, no I don't know of anybody doing that.

We could document that one cannot build glibc with -fnon-call-exceptions 
(and similarly for -flto and other options you can't build glibc with), 
and then bring back __attribute__ ((pure)) for difftime. But we couldn't 
bring back __attribute__ ((const)) due to rounding mode changes, right?

Not sure it's worth the effort just for the rarely-used difftime, but it 
might be worth it if more commonly-used library calls benefit from these 
attributes.

[PATCH 2/2] Improve doc for time_t range (BZ 31808)

[Paul Eggert]

---
 manual/time.texi | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/manual/time.texi b/manual/time.texi
index dd77d3db04..2d3833755f 100644
--- a/manual/time.texi
+++ b/manual/time.texi
@@ -123,7 +123,7 @@ The number of clock ticks per second is system-specific.
 @code{time_t} is the simplest data type used to represent simple
 calendar time.
 
-In ISO C, @code{time_t} can be either an integer or a floating-point
+In ISO C, @code{time_t} can be either an integer or a real floating
 type, and the meaning of @code{time_t} values is not specified.  The
 only things a strictly conforming program can do with @code{time_t}
 values are: pass them to @code{difftime} to get the elapsed time
@@ -134,11 +134,21 @@ and pass them to the functions that convert them to broken-down time
 On POSIX-conformant systems, @code{time_t} is an integer type and its
 values represent the number of seconds elapsed since the @dfn{epoch},
 which is 00:00:00 on January 1, 1970, Coordinated Universal Time.
+The count of seconds ignores leap seconds.
 
 @Theglibc{} additionally guarantees that @code{time_t} is a signed
 type, and that all of its functions operate correctly on negative
 @code{time_t} values, which are interpreted as times before the epoch.
+Functions like @code{localtime} assume the Gregorian calendar even
+though this is historically inaccurate for timestamps before the
+calendar was introduced or after the calendar will become obsolete.
 @cindex epoch
+@Theglibc{} also supports leap seconds as an option, in which case
+@code{time_t} counts leap seconds instead of ignoring them.
+Currently the @code{time_t} type is 64 bits wide on all platforms
+supported by @theglibc{}, except that it is 32 bits wide on a few
+older platforms unless you define @code{_TIME_BITS} to 64.
+@xref{Feature Test Macros}.
 @end deftp
 
 @deftp {Data Type} {struct timespec}
-- 
2.45.1

Re: [PATCH 2/2] Improve doc for time_t range (BZ 31808)

[DJ Delorie]

Paul Eggert <eggert@cs.ucla.edu> writes:
> -In ISO C, @code{time_t} can be either an integer or a floating-point
> +In ISO C, @code{time_t} can be either an integer or a real floating

Ok.

>  On POSIX-conformant systems, @code{time_t} is an integer type and its
>  values represent the number of seconds elapsed since the @dfn{epoch},
>  which is 00:00:00 on January 1, 1970, Coordinated Universal Time.
> +The count of seconds ignores leap seconds.

Does this mean the value is not corrected for leap seconds, or that leap
seconds are part of the hh:mm:ss <-> time_t conversion so no further
worry about leap seconds exists?  I would think "elapsed seconds"
pre-determines whether artificial calendar/time adjustments are included
and/or applied.  I.e. I'm not sure what benefit to the reader this
statement is supposed to provide, perhaps adding ", so you don't have
to..." to clarify it?

>  @Theglibc{} additionally guarantees that @code{time_t} is a signed
>  type, and that all of its functions operate correctly on negative
>  @code{time_t} values, which are interpreted as times before the epoch.
> +Functions like @code{localtime} assume the Gregorian calendar even
> +though this is historically inaccurate for timestamps before the
> +calendar was introduced or after the calendar will become obsolete.

Ok.

>  @cindex epoch
> +@Theglibc{} also supports leap seconds as an option, in which case
> +@code{time_t} counts leap seconds instead of ignoring them.

This contradicts the addition above.  Perhaps that sentence belongs here
(or this, there) with a note about how this affects programs?

> +Currently the @code{time_t} type is 64 bits wide on all platforms
> +supported by @theglibc{}, except that it is 32 bits wide on a few
> +older platforms unless you define @code{_TIME_BITS} to 64.
> +@xref{Feature Test Macros}.
>  @end deftp

Ok.  It might flow better to swap this chunk with the calendar chunk,
though, and have the calendar chunk as a separate paragraph.