This is an archive of the discontinued LLVM Phabricator instance.

This introduces gmtime to LLVM libc, based on C99/C2X/Single Unix Spec.
ClosedPublic

Authored by rtenneti on Mar 11 2021, 4:18 PM.

Details

Summary

This change doesn't handle TIMEZONE, tm_isdst and leap seconds.

Moved shared code between mktime and gmtime into time_utils.cpp.

Diff Detail

Event Timeline

rtenneti created this revision.Mar 11 2021, 4:18 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 11 2021, 4:18 PM
rtenneti requested review of this revision.Mar 11 2021, 4:18 PM
rtenneti updated this revision to Diff 330727.Mar 15 2021, 10:50 AM

Added missing OutOfRange test. Fixed few nits while reviewing.

sivachandra accepted this revision.Mar 16 2021, 12:10 AM

Couple of questions inline. Feel free to submit after answering/addressing them.

libc/src/time/CMakeLists.txt
8

Why do we need standalone_cpp?

libc/src/time/gmtime.cpp
18

Does any standard require thread safety? If yes, will making the below declaration thread-local solve the problem?

libc/test/src/time/gmtime_test.cpp
23

Why do we need the GmTimeTests prefix? Doesn't the suite name LlvmLibcGmTime already convey that it is a gmtime test?

libc/test/src/time/mktime_test.cpp
53

Ditto.

This revision is now accepted and ready to land.Mar 16 2021, 12:10 AM
jeffbailey added inline comments.Mar 16 2021, 3:27 PM
libc/src/time/gmtime.cpp
18

C20 introduces gmtime_r (and the family of _r time functions).

In 7.27.3:

" Execution of any of the functions that return a pointer to one of these static
objects may overwrite the information returned from any previous call to one of these functions that
uses the same object. "

But what's not clear to me is for the non-reentrant versions, is doing a thread-local OK? I'm not seeing anything in the standard discussing static buffers and the use of thread-locals to store them (it only's mentioned in the definition of errno)

sivachandra added inline comments.Mar 16 2021, 3:45 PM
libc/src/time/gmtime.cpp
18

Whether thread-local is OK or not depends on the supported uses cases. For example, if a change in value in one thread is to be visible from another thread, then making it a thread-local is not a good idea. If it is unclear from the standards, I would say just leave it as you have it here.

rtenneti updated this revision to Diff 331127.Mar 16 2021, 4:28 PM
rtenneti marked 6 inline comments as done.

Fixed Siva's code review comments.

Thanks Siva for the comments. PTAL.

libc/src/time/gmtime.cpp
18

I will implement gmtime_r in the next CL. Removed the TODO (it should have been a TODO in the description to implement gmtime_r).

This revision was landed with ongoing or failed builds.Mar 16 2021, 4:45 PM
This revision was automatically updated to reflect the committed changes.