[PATCH] Add timegm POSIX call

Post by Andrew Russell via newlib
From e182faa79c35984b667029ef7b6e4a8ce7329897 Mon Sep 17 00:00:00 2001

Date: Fri, 10 Aug 2018 12:14:18 -0700
Subject: [PATCH 1/4] Start of mktime.c copy to timegm.c
I am proposing to add the timegm POSIX call to
Newlib. Part of this refactors some of the code in libc/time/local.h and
https://sourceware.org/ml/newlib/2018/msg00186.html

I'm looking for comments from other (non-Cygwin) devs here.

Thanks,
Corinna

--
Corinna Vinschen
Cygwin Maintainer
Red Hat

Joel Sherrill

2018-08-14 17:49:20 UTC

Post by Andrew Russell via newlib
From e182faa79c35984b667029ef7b6e4a8ce7329897 Mon Sep 17 00:00:00 2001

I'm looking for comments from other (non-Cygwin) devs here.

From my perspective, I don't mind having common methods that
are not in libc or POSIX in newlib or RTEMS. Ultimately, the wider
set of methods makes packages easier to port.

I would ask that the method is documented using the newlib markup
and that its historical origin is noted. The Linux man page is clearly
discouraging:

CONFORMING TO
These functions are nonstandard GNU extensions that are also present
on
the BSDs. Avoid their use; see NOTES.

But overall, improving cross-platform compatibility is good even
when it means adding extensions that have no impact when not
used.

--joel

Post by Corinna Vinschen
Thanks,
Corinna
--
Corinna Vinschen
Cygwin Maintainer
Red Hat

Corinna Vinschen

2018-08-15 08:54:43 UTC

Post by Joel Sherrill

Post by Andrew Russell via newlib
From e182faa79c35984b667029ef7b6e4a8ce7329897 Mon Sep 17 00:00:00 2001

I'm looking for comments from other (non-Cygwin) devs here.
From my perspective, I don't mind having common methods that

are not in libc or POSIX in newlib or RTEMS. Ultimately, the wider
set of methods makes packages easier to port.
I would ask that the method is documented using the newlib markup
and that its historical origin is noted. The Linux man page is clearly
CONFORMING TO
These functions are nonstandard GNU extensions that are also present
on
the BSDs. Avoid their use; see NOTES.
But overall, improving cross-platform compatibility is good even
when it means adding extensions that have no impact when not
used.
--joel

The new functions are ok to go in, I was more interested in people
testing the code. Sorry if I was unclear!

Corinna

--
Corinna Vinschen
Cygwin Maintainer
Red Hat

Andreas Köpke

2018-08-15 07:52:59 UTC

Post by Andrew Russell via newlib
From e182faa79c35984b667029ef7b6e4a8ce7329897 Mon Sep 17 00:00:00 2001

I'm looking for comments from other (non-Cygwin) devs here.
Thanks,
Corinna

I did not test it yet but we need this function. cygwin has it, but uses a
completely different codebase (localtime.cc).

It is always needed if your default timezone is not GMT and you need to
exchange time information with e.g. sync mechanisms like NTP or GPS. You
usually get broken down time information referenced to GMT and need to re-
assemble using GMT as reference.

Using setenv/getenv to temporarily change the timezone does not work: there is
a memory leak in the implementation. Well, at least it looks like one.

Parsing the TZ information takes time (at least on 12MHz MCUs) and stack space
(~ 600 bytes) which is not always available. So having a working
implementation would be great.

Best regards, Andreas

Corinna Vinschen

2018-08-15 08:27:08 UTC

Post by Andreas KÃ¶pke

Post by Andrew Russell via newlib
From e182faa79c35984b667029ef7b6e4a8ce7329897 Mon Sep 17 00:00:00 2001

I'm looking for comments from other (non-Cygwin) devs here.
Thanks,
Corinna

I did not test it yet but we need this function. cygwin has it, but uses a
completely different codebase (localtime.cc).
It is always needed if your default timezone is not GMT and you need to
exchange time information with e.g. sync mechanisms like NTP or GPS. You
usually get broken down time information referenced to GMT and need to re-
assemble using GMT as reference.
Using setenv/getenv to temporarily change the timezone does not work: there is
a memory leak in the implementation. Well, at least it looks like one.

Patch?

Corinna

--
Corinna Vinschen
Cygwin Maintainer
Red Hat

Craig Howland

2018-08-15 15:01:34 UTC

Post by Andrew Russell via newlib
Date: Fri, 10 Aug 2018 12:14:18 -0700
Subject: [PATCH 1/4] Start of mktime.c copy to timegm.c
I am proposing to add the timegm POSIX call to
Newlib. Part of this refactors some of the code in libc/time/local.h and
https://sourceware.org/ml/newlib/2018/msg00186.html
Thanks,
Andrew
...
diff --git a/newlib/libc/include/time.h b/newlib/libc/include/time.h
index a2efcc15e..8440ecb3c 100644
--- a/newlib/libc/include/time.h
+++ b/newlib/libc/include/time.h
@@ -56,6 +56,7 @@ struct tm
clock_t clock (void);
double difftime (time_t _time2, time_t _time1);
time_t mktime (struct tm *_timeptr);
+time_t timegm (struct tm *_timeptr);

This prototype should be gated by the appropriate-version GNU extension #if.
(According to the Linux timegm() man page the gate is _BSD_SOURCE ||
_SVID_SOURCE (although the Newlib in-header version might be different). But
given it is called a GNU extension, you'd think _GNU_SOURCE would also be there.)

Post by Andrew Russell via newlib
time_t time (time_t *_timer);
#ifndef _REENT_ONLY
char *asctime (const struct tm *_tblock);
...
diff --git a/newlib/libc/time/mktime.c b/newlib/libc/time/mktime.c
index 02032599a..ab47f8614 100644
--- a/newlib/libc/time/mktime.c
+++ b/newlib/libc/time/mktime.c
@@ -11,6 +11,8 @@
* represented, returns the value (time_t) -1.
*
* Modifications: Fixed tm_isdst usage - 27 August 2008 Craig Howland.
+ * Refactor code from mktime.c to shared internal
+ * functions. - 17 July 2018 Andrew Russell.
*/
/*
@@ -39,157 +41,22 @@ result is the time, converted to a <<time_t>> value.
PORTABILITY
ANSI C requires <<mktime>>.
-<<mktime>> requires no supporting OS subroutines.
+<<timegm>> requires no supporting OS subroutines.

Spurious change, as this file is still mktime.

Post by Andrew Russell via newlib
...
diff --git a/newlib/libc/time/timegm.c b/newlib/libc/time/timegm.c

(This diff as presented does not make sense from the point of view that this is
a new file.)

Post by Andrew Russell via newlib
index 02032599a..3299ce228 100644
--- a/newlib/libc/time/timegm.c
+++ b/newlib/libc/time/timegm.c
@@ -1,8 +1,8 @@
/*
- * mktime.c
+ * timegm.c
* Original Author: G. Haley
*
- * Converts the broken-down time, expressed as local time, in the structure
+ * Converts the broken-down time, expressed as UTC time, in the structure
* pointed to by tim_p into a calendar time value. The original values of the
* tm_wday and tm_yday fields of the structure are ignored, and the original
* values of the other fields have no restrictions. On successful completion
@@ -11,25 +11,27 @@
* represented, returns the value (time_t) -1.
*
* Modifications: Fixed tm_isdst usage - 27 August 2008 Craig Howland.

Can get rid of this fix tm_isdst comment, as not in timegm, but in mktime.
The comments that make the man page here should probably include the same "These
functions are nonstandard GNU extensions that are also present on the BSDs.
Avoid their use; see NOTES." statement that appears in the timegm/timelocal
Linux man page (with a small tweak for being only 1 function instead of two).

Post by Andrew Russell via newlib
...
#include <stdlib.h>
@@ -58,11 +60,8 @@ static const int DAYS_IN_MONTH[12] =
static const int _DAYS_BEFORE_MONTH[12] =
{0, 31, 59, 90, 120, 151, 181, 212, 243, 273, 304, 334};

Someone had suggested in the thread that DAYS_IN_MONTH be changed to char to
save space. How about int_least16_t to (possibly) save space?

Post by Andrew Russell via newlib
...
-time_t
-mktime (struct tm *tim_p)
+void
+__set_tm_wday (long days, struct tm *tim_p)
+{
+ if ((tim_p->tm_wday = (days + 4) % 7) < 0)
+ tim_p->tm_wday += 7;
+}
+
+/* returns either 0 or 1 */
+static
+int
+__is_leap_year (int year)
+{
+ return (year % 4) == 0 && ((year % 100) != 0 || (((year / 100) & 3)
== (-(YEAR_BASE / 100) & 3)));

How about "(year & 3) == 0" instead of the % to save time? (Should not matter
with a good optimizer, but it is not necessarily on.)

Post by Andrew Russell via newlib
...
+time_t
+timegm (struct tm *tim_p)
+{
+ time_t tim = __timegm_internal(tim_p);
+ long days = tim / SECSPERDAY;
- /* reset isdst flag to what we have calculated */
- tim_p->tm_isdst = isdst;
+ /* set isdst flag to 0 since we are in UTC */
+ tim_p->tm_isdst = 0;
/* compute day of the week */
- if ((tim_p->tm_wday = (days + 4) % 7) < 0)
- tim_p->tm_wday += 7;
-
+ __set_tm_wday(days, tim_p);
+
return tim;
}
+
--
2.18.0.597.ga71716f1ad-goog

I have not been able to try testing it, yet, but there are a few thoughts based
on examination. See inline above as well as some here.

I suggest that the new timegm.c file should contain only the new timegm()
function and that the supporting functions all belong in mktime.c. This comes
closest to preserving the same size on existing implementations, which otherwise
would then require the timegm object to be linked; the new timegm object will
only get linked for applications which call it. (Not that the timegm()
function, itself, is large, but it is the new extension.)

Craig

Joel Sherrill

2018-08-15 15:27:31 UTC

This prototype should be gated by the appropriate-version GNU extension
#if. (According to the Linux timegm() man page the gate is _BSD_SOURCE ||
_SVID_SOURCE (although the Newlib in-header version might be different).
But given it is called a GNU extension, you'd think _GNU_SOURCE would also
be there.)

Spurious change, as this file is still mktime.

Post by Andrew Russell via newlib
...
diff --git a/newlib/libc/time/timegm.c b/newlib/libc/time/timegm.c

(This diff as presented does not make sense from the point of view that
this is a new file.)

Can get rid of this fix tm_isdst comment, as not in timegm, but in mktime.
The comments that make the man page here should probably include the same
"These functions are nonstandard GNU extensions that are also present on
the BSDs. Avoid their use; see NOTES." statement that appears in the
timegm/timelocal Linux man page (with a small tweak for being only 1
function instead of two).

+1 Critical to have good documentation at the top.

Post by Craig Howland
...

Post by Andrew Russell via newlib
#include <stdlib.h>
@@ -58,11 +60,8 @@ static const int DAYS_IN_MONTH[12] =
static const int _DAYS_BEFORE_MONTH[12] =
{0, 31, 59, 90, 120, 151, 181, 212, 243, 273, 304, 334};

Someone had suggested in the thread that DAYS_IN_MONTH be changed to char
to save space. How about int_least16_t to (possibly) save space?

How about "(year & 3) == 0" instead of the % to save time? (Should not
matter with a good optimizer, but it is not necessarily on.)

I have not been able to try testing it, yet, but there are a few thoughts
based on examination. See inline above as well as some here.
I suggest that the new timegm.c file should contain only the new timegm()
function and that the supporting functions all belong in mktime.c. This
comes closest to preserving the same size on existing implementations,
which otherwise would then require the timegm object to be linked; the new
timegm object will only get linked for applications which call it. (Not
that the timegm() function, itself, is large, but it is the new extension.)
Craig

Freddie Chopin

2018-08-15 19:18:19 UTC

Post by Andrew Russell via newlib
+/* returns either 0 or 1 */
+static
+int
+__is_leap_year (int year)
+{
+ return (year % 4) == 0 && ((year % 100) != 0 || (((year / 100) & 3)
== (-(YEAR_BASE / 100) & 3)));

How about "(year & 3) == 0" instead of the % to save time? (Should not matter
with a good optimizer, but it is not necessarily on.)

On the other hand % is exactly 1:1 with the intent, while binary
operations are not.

godbolt suggests that for x86-64 "(x % 4) == 0" and "(x & 3) == 0" give
exactly the same code (binary operations) when optimization is
explicitly _disabled_ (-O0).

Also do note that year may be negative, and "& 3" is not the same as "%
4" for negative numbers. In this particular case this probably doesn't
matter anyway, but still (;

Regards,
FCh

Freddie Chopin

2018-08-15 19:27:28 UTC

Post by Freddie Chopin

How about "(year & 3) == 0" instead of the % to save time? (Should not matter
with a good optimizer, but it is not necessarily on.)

Same for arm-none-eabi (previously I could not find the right subpage
where arm-none-eabi) was available.

https://godbolt.org/g/MYFCkb

Regards,
FCh

Richard Earnshaw (lists)

2018-08-16 15:24:52 UTC

Post by Freddie Chopin