This fixes llvm.org/PR33469.
Details
Diff Detail
Event Timeline
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) |
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 |
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); |
src/experimental/filesystem/operations.cpp | ||
---|---|---|
22–24 | __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? |
src/experimental/filesystem/operations.cpp | ||
---|---|---|
22–24 | 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. |
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.
"""
I only just noticed you were including Availability.h. That shouldn't be necessary, since the macros should be defined by the compiler.