Page MenuHomePhabricator

[libc++] Don't use UTIME_OMIT to detect utimensat on Apple
Needs ReviewPublic

Authored by EricWF on Jun 15 2017, 1:50 PM.

Details

Summary

This fixes llvm.org/PR33469.

Diff Detail

Event Timeline

EricWF created this revision.Jun 15 2017, 1:50 PM
dexonsmith edited edge metadata.Jun 15 2017, 3:13 PM

This is the right idea, although it only covers macOS.

Any ideas for how to test this?

src/experimental/filesystem/operations.cpp
23–28

Sadly this isn't quite sufficient. As per Jack's suggested SDK patch in the PR, we need to enumerate the platforms :/. I think this should be the right logic for the four Darwin platforms:

(defined(__IPHONE_OS_VERSION_MIN_REQUIRED) && __IPHONE_OS_VERSION_MIN_REQUIRED < __IPHONE_11_0) || \
(defined(__WATCH_OS_VERSION_MIN_REQUIRED) &&  __WATCH_OS_VERSION_MIN_REQUIRED < __WATCHOS_4_0)  || \
(defined(__TV_OS_VERSION_MIN_REQUIRED) && __TV_OS_VERSION_MIN_REQUIRED < __TVOS_11_0)           || \
(defined(__MAC_OS_X_VERSION_MIN_REQUIRED) && __MAC_OS_X_VERSION_MIN_REQUIRED < __MAC_10_13)
dexonsmith requested changes to this revision.Jun 15 2017, 3:14 PM
This revision now requires changes to proceed.Jun 15 2017, 3:14 PM
EricWF added inline comments.Jun 15 2017, 3:42 PM
src/experimental/filesystem/operations.cpp
23–28

Do we have to do the below dance for all of those macros?

#if !defined(__FOO_VERSION_MIN_REQUIRED) && defined(__ENVIROMENT_FOO_VERSION_MIN_REQUIRED)
#define __FOO_VERSION_MIN_REQUIRED __ENVIROMENT_FOO_VERSION_REQUIRED
#endif
EricWF updated this revision to Diff 102782.Jun 15 2017, 9:29 PM
EricWF updated this revision to Diff 102783.
EricWF edited edge metadata.
EricWF marked an inline comment as done.
  • Address inline comments.
dexonsmith added inline comments.Jun 15 2017, 9:34 PM
src/experimental/filesystem/operations.cpp
22–31

I only just noticed you were including Availability.h. That shouldn't be necessary, since the macros should be defined by the compiler.

23–28

Nope. I just advised you to use the wrong ones. Use the __ENVIRONMENT versions pre-defined by Clang.

dexonsmith added inline comments.Jun 15 2017, 9:36 PM
src/experimental/filesystem/operations.cpp
23–28

Specifically:

clang/lib/Basic/Targets.cpp:183:      Builder.defineMacro("__ENVIRONMENT_TV_OS_VERSION_MIN_REQUIRED__", Str);
clang/lib/Basic/Targets.cpp:185:      Builder.defineMacro("__ENVIRONMENT_IPHONE_OS_VERSION_MIN_REQUIRED__",
clang/lib/Basic/Targets.cpp:197:    Builder.defineMacro("__ENVIRONMENT_WATCH_OS_VERSION_MIN_REQUIRED__", Str);
clang/lib/Basic/Targets.cpp:221:    Builder.defineMacro("__ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__", Str);
EricWF added inline comments.Jun 15 2017, 9:38 PM
src/experimental/filesystem/operations.cpp
22–31

__MAC_10_13 et al are defined in Availability.h, and AvailabilityInternal.h seems to do the __ENV dance described above. Are you sure it's not needed?

dexonsmith added inline comments.Jun 15 2017, 9:45 PM
src/experimental/filesystem/operations.cpp
22–31

I don't think the dance is necessary, since libcxx won't be overriding those macros.

Also, we can skip the __MAC_10_13 macros, ala src/chrono.cpp.

EricWF added inline comments.Jun 16 2017, 4:39 PM
src/experimental/filesystem/operations.cpp
22–31

Using the __MAC_10_13 seems preferable, and since this is in a source file, and not a header, we lose nothing by including <Availability.h>.

23–28

Ack, I'll switch to using those.

mclow.lists added inline comments.
src/experimental/filesystem/operations.cpp
27

Would it be easier to read if we split this test into two halves - one for Apple platforms and the other for the rest of the world?

#if !defined(__APPLE__)
# if !defined(UTIME_OMIT)
# define _LIBCPP_HAS_NO_UTIMENSAT
# endif
#else
.....
#endif

The powers that be updated the SDK on the chromium clang bots, so we need some solution for this issue soon.

The approach in this patch should work, but it seems conservative. Now libc++ only uses utimesat() if it's built with a deployment target of macOS 10.13. But if we were to set a deployment target of 10.12 and then _run_ on 10.13, we could use utimesat() as well, even though this patch currently doesn't. (Apple bundles libc++ with the system, so it's not a meaningful difference for Apple-built libc++'s, but applications can build libc++ themselves and statically link to it, and for those it would make a difference.)

https://clang.llvm.org/docs/LanguageExtensions.html#objective-c-available has some words on it (except that @available is spelled __builtin_available in non-Objective-C code) -- thoughts on using runtime detection of utimes() instead? That's how things are supposed to work on the Apple platforms.

I went ahead and landed this with the comments requested by Duncan in r324385. (http://llvm.org/viewvc/llvm-project?view=revision&revision=324385)

I'm also quoting Duncan's email reply to my comment, since that's not showing up on phab:
"""
Using runtime availability checking doesn't make sense for a system Libc++, as you point out. If we add runtime checks they ought to be non-default, and hidden behind configuration flags.

Also, do I remember correctly that __builtin_available requires linking against Foundation (and thus the Obj-C runtime) for NSProcessInfo, or has that dependency been removed/avoided? It would be surprising for the C++ standard library to pull in the Objective-C runtime. A reasonable configuration option, but not a reasonable default behaviour IMO.
"""

dexonsmith resigned from this revision.Jun 28 2018, 8:56 PM