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

[EricWF]

Summary

This fixes llvm.org/PR33469.

[dexonsmith]

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)

[EricWF]

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]

• Address inline comments.

[dexonsmith]

src/experimental/filesystem/operations.cpp

22–24

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]

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]

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?

[dexonsmith]

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.

[EricWF]

src/experimental/filesystem/operations.cpp

22–24

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]

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

[thakis]

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.

[thakis]

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.
"""

src/experimental/filesystem/operations.cpp

 #include "random"  /* for unique_path */
 #include "cstdlib"
 #include "climits"
 
 #include <unistd.h>
 #include <sys/stat.h>
 #include <sys/statvfs.h>
 #include <fcntl.h>  /* values for fchmodat */
-#if !defined(UTIME_OMIT)
+#if defined(__APPLE__)
+#include <Availability.h>
+#endif
+
+// We can use the presence of UTIME_OMIT to detect platforms that do not
+// provide utimensat, with some exceptions on OS X.
+#if !defined(UTIME_OMIT) ||                                                    \
+    (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)
+#define _LIBCPP_HAS_NO_UTIMENSAT
+#endif
+
+#if defined(_LIBCPP_HAS_NO_UTIMENSAT)
 #include <sys/time.h> // for ::utimes as used in __last_write_time
 #endif
 
 _LIBCPP_BEGIN_NAMESPACE_EXPERIMENTAL_FILESYSTEM
 
 filesystem_error::~filesystem_error() {}
 
 

 }
 
 void __last_write_time(const path& p, file_time_type new_time,
                        std::error_code *ec)
 {
     using namespace std::chrono;
     std::error_code m_ec;
 
-    // We can use the presence of UTIME_OMIT to detect platforms that do not
-    // provide utimensat.
-#if !defined(UTIME_OMIT)
+#if defined(_LIBCPP_HAS_NO_UTIMENSAT)
     // This implementation has a race condition between determining the
     // last access time and attempting to set it to the same value using
     // ::utimes
     struct ::stat st;
     file_status fst = detail::posix_stat(p, st, &m_ec);
     if (m_ec && !status_known(fst)) {
         set_or_throw(m_ec, ec, "last_write_time", p);
         return;