Page MenuHomePhabricator

chrono: check _POSIX_TIMERS before using clock_gettime
ClosedPublic

Authored by LittleFox94 on May 3 2020, 10:42 AM.

Details

Summary

clock_gettime is documented to be available when _POSIX_TIMERS is defined. Add a check for this

Diff Detail

Event Timeline

LittleFox94 created this revision.May 3 2020, 10:42 AM
Herald added a reviewer: Restricted Project. · View Herald TranscriptMay 3 2020, 10:42 AM
ldionne added a subscriber: ldionne.May 4 2020, 7:44 AM

LGTM generally speaking, but is this triggered by a specific failure you ran into? If so, on what configuration? We should have a way to test this change.

Also, next time, please include context in your Phabricator diff.

libcxx/src/chrono.cpp
15

From https://linux.die.net/man/3/clock_gettime, it says you need to include <unistd.h> to get the macro definition.

ldionne requested changes to this revision.May 4 2020, 7:44 AM
This revision now requires changes to proceed.May 4 2020, 7:44 AM
LittleFox94 added a comment.EditedMay 4 2020, 8:03 AM

LGTM generally speaking, but is this triggered by a specific failure you ran into? If so, on what configuration? We should have a way to test this change.

built libc++ for my hobby OS but this looked general enough to include upstream. The
__APPLE__ check is probably not required with the new one in place.

Also, next time, please include context in your Phabricator diff.

OK, will do. Phabricator was a bit confusing, first time usage ^^'

LittleFox94 updated this revision to Diff 261827.EditedMay 4 2020, 8:12 AM

Included unistd.h and removed the check for __APPLE__, since _POSIX_TIMERS should work there, too

ldionne requested changes to this revision.May 4 2020, 8:47 AM
ldionne added a subscriber: EricWF.

Please make a corresponding change at libcxx/src/filesystem/operations.cpp:40, where the #if has been copied.

@EricWF What would you say about having a header in src/ that contains these additional configuration checks only used for building the dylib? Either that, or we should move these checks to __config. The fact that we duplicated the check in src/chrono.cpp and src/filesystem/operations.cpp is bad.

libcxx/src/chrono.cpp
13

<unistd.h> is not supported on Windows AFAICT, so you'll need to guard this with an appropriate #if.

15–19

It turns out the __APPLE__ check is still required. See libcxx/src/include/apple_availability.h:33 & friends. Thus, I would use:

#if !defined(__APPLE__) && defined(_POSIX_TIMERS)
21

Please remove the comment at the closing #endif. For one-liner #ifs like that, it's just an occasion to get the actual condition and the comment out of sync. I know you were just being consistent with surrounding code, but it's bad that we've been doing it.

This revision now requires changes to proceed.May 4 2020, 8:47 AM
LittleFox94 marked 2 inline comments as done.

Do not include unistd.h on _WIN32, re-add __APPLE__ check and change _POSIX_TIMER check to check for > 0

LittleFox94 marked 3 inline comments as done.May 4 2020, 2:23 PM
LittleFox94 added inline comments.
libcxx/src/chrono.cpp
15–19

I extend the check with _POSIX_TIMERS > 0, I have seen a #define _POSIX_TIMERS (-1) in a apple unistd.h and doc says the symbol _POSIX_TIMERS is defined in <unistd.h> to a value greater than 0

LittleFox94 marked an inline comment as done.

Also fix duplicated check in filesystem/operations.cpp

ldionne accepted this revision.May 7 2020, 9:11 AM

LGTM. Do you need help committing? If so, please specify the author info to use in Git as "First Last <email@foo.com>" so I can give you proper credits. Thanks!

This revision is now accepted and ready to land.May 7 2020, 9:11 AM

Cannot commit myself

"Mara Sophie Grosch <littlefox@lf-net.org>"

Thank you very much for your time and comments :)

This revision was automatically updated to reflect the committed changes.

Thanks! Committed as

commit 59b3102739c5988c9fc7ea77888de1bc8ce9c8ab
Author: Mara Sophie Grosch <littlefox@lf-net.org>
Date:   Thu May 7 12:10:33 2020 -0400

    [libc++] chrono: check _POSIX_TIMERS before using clock_gettime

    clock_gettime is documented to be available when _POSIX_TIMERS is
    defined. Add a check for this.

    Differential Revision: https://reviews.llvm.org/D79305