search

Quuxplusone / LLVMBugzillaTest

0 stars 0 forks source link

UTIME_OMIT shouldn't be used to detect presence of utimensat() #32441

Closed Quuxplusone closed 6 years ago

Quuxplusone commented 7 years ago
Bugzilla Link PR33469
Status RESOLVED FIXED
Importance P normal
Reported by Jack Howarth (howarth.mailing.lists@gmail.com)
Reported on 2017-06-15 07:19:43 -0700
Last modified on 2018-02-06 11:19:12 -0800
Version 4.0
Hardware Macintosh MacOS X
CC dexonsmith@apple.com, jeremyhu@apple.com, llvm-bugs@lists.llvm.org, mclow.lists@gmail.com, nicolasweber@gmx.de
Fixed by commit(s)
Attachments
Blocks
Blocked by
See also 
The Xcode 9 beta release breaks the assumption...

    // We can use the presence of UTIME_OMIT to detect platforms that do not
    // provide utimensat.

made in src/experimental/filesystem/operations.cpp. The new Xcode 9 release has
a shared SDK for both 10.12/10.13 on 10.12...

% cd /Library/Developer/CommandLineTools/SDKs
[Mac-Pro:Developer/CommandLineTools/SDKs] howarth% ls -l
total 16
drwxr-xr-x  5 root  wheel  170 Jun 14 17:48 MacOSX.sdk
lrwxr-xr-x  1 root  wheel   10 Jun 14 09:11 MacOSX10.12.sdk -> MacOSX.sdk
lrwxr-xr-x  1 root  wheel   10 Jun 14 17:47 MacOSX10.13.sdk -> MacOSX.sdk

which results in a MacOSX.sdk/usr/include/sys/stat.h being used which
unconditionally contains...

#define UTIME_OMIT      -2

This produces a undefined _utimensat symbol when '-isysroot
/Library/Developer/CommandLineTools/SDKs/MacOSX10.12.sdk -mmacosx-version-
min=10.12' is used. This is because the the  MacOSX.sdk/usr/include/sys/stat.h
properly uses the __API_AVAILABLE() macro on the utimensat()...

int     utimensat(int __fd, const char *__path, const struct timespec
__times[2],
                int __flag) __API_AVAILABLE(macosx(10.13), ios(11.0), tvos(11.0), watchos(4.0));

which leaves that symbol undeclared on the 10.12 target. However
src/experimental/filesystem/operations.cpp errornously tries to use the
utimensat() call based on the definition of UTIME_OMIT.
    This should be replaced with an appropriate cmake module to check for utimensat() directly with the current compile flags rather than incorrectly depending on the definition of UTIME_OMIT.
Quuxplusone commented 7 years ago

Jack -- do you believe that this is a libc++ problem, or just a problem in the beta Xcode's headers?

Quuxplusone commented 7 years ago 
(In reply to Marshall Clow (home) from comment #1)
> Jack -- do you believe that this is a libc++ problem, or just a problem in
> the beta Xcode's headers?

I originally proposed wrapping the definition of

#define UTIME_OMIT      -2

in MacOSX.sdk/usr/include/sys/stat.h so that it doesn't get defined when
targeting earlier than 10.13...

radar://32783098, "MacOSX.sdk/usr/include/sys/stat.h needs to define UTIME_OMIT
only for 10.13 or later"

but Jeremy Huddleston Sequoia says that is unnecessary...

> On Jun 14, 2017, at 20:27, Jack Howarth <howarth.at.macports@gmail.com> wrote:
>
> Jeremy,
>       I assume that the define of  UTIME_NOW in
MacOSX.sdk/usr/include/sys/stat.h also needs to be conditional on the
deployment target of 10.13 as well.

No.  It needs to be defined unconditionally, so you can use it in your code
when running on 10.13.
Quuxplusone commented 7 years ago

I suspect that Jeremy misunderstood your question.

It seems to me that: UTIME_OMIT should be defined when compiling for 10.13. UTIME_OMIT should not be defined when compiling for 10.12.

Alternately, libc++ needs to figure out a different, portable way to determine when utimensat is available.

Quuxplusone commented 7 years ago

(In reply to Marshall Clow (home) from comment #3)

I suspect that Jeremy misunderstood your question.

It seems to me that: UTIME_OMIT should be defined when compiling for 10.13. UTIME_OMIT should not be defined when compiling for 10.12.

Alternately, libc++ needs to figure out a different, portable way to determine when utimensat is available.

Yes, my immediate reaction was that the Xcode header was missing...

--- stat.h.orig 2017-06-15 12:01:25.000000000 -0400 +++ stat.h 2017-06-15 12:03:32.000000000 -0400 @@ -367,8 +367,13 @@ int fstatat(int, const char , struct stat , int) DARWIN_INODE64(fstatat) OSX_AVAILABLE_STARTING(MAC_10_10, __IPHONE_8_0); int mkdirat(int, const char *, mode_t) OSX_AVAILABLE_STARTING(MAC_10_10, IPHONE_8_0);

+#if (defined(IPHONE_OS_VERSION_MIN_REQUIRED) && IPHONE_OS_VERSION_MIN_REQUIRED >= __IPHONE_11_0) \

Quuxplusone commented 7 years ago

(In reply to Jack Howarth from comment #4)

(In reply to Marshall Clow (home) from comment #3)

I suspect that Jeremy misunderstood your question.

It seems to me that: UTIME_OMIT should be defined when compiling for 10.13. UTIME_OMIT should not be defined when compiling for 10.12.

Alternately, libc++ needs to figure out a different, portable way to determine when utimensat is available.

Yes, my immediate reaction was that the Xcode header was missing...

--- stat.h.orig 2017-06-15 12:01:25.000000000 -0400 +++ stat.h 2017-06-15 12:03:32.000000000 -0400 @@ -367,8 +367,13 @@ int fstatat(int, const char , struct stat , int) DARWIN_INODE64(fstatat) OSX_AVAILABLE_STARTING(MAC_10_10, __IPHONE_8_0); int mkdirat(int, const char *, mode_t) OSX_AVAILABLE_STARTING(MAC_10_10, IPHONE_8_0);

+#if (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 UTIME_NOW -1

    define UTIME_OMIT -2

    +#endif

    int futimens(int fd, const struct timespec times[2]) __API_AVAILABLE(macosx(10.13), ios(11.0), tvos(11.0), watchos(4.0)); int utimensat(int fd, const char *path, const struct timespec __times[2],

On the other, you don't really see any instances of using these availability wrappers for simple defines. They seem to be reserved for defining ABI version levels rather than specific defines.

Quuxplusone commented 7 years ago

The header in the SDK is consistent with how Darwin decorates availability for C and Objective-C APIs. The expected user model is to check at runtime for availability of specific APIs when you're deploying back to a previous version, either using if(utimensat) or (new in Xcode 9) if(@available(macOS 10.13, *)).

This model is useful if you're building a single image, and deploying it to multiple OS versions.

I don't think it's particularly useful for cases like libc++, where it's likely being deploying to a specific OS version. We'd rather choose which API to use at compile-time. One option is to use __MAC_OS_X_VERSION_MIN_REQUIRED (etc.). Another option is to do some sort of CMake-based linker check.

Quuxplusone commented 7 years ago 
(In reply to Duncan from comment #6)
> (new in Xcode 9) `if(@available(macOS 10.13, *))`.

(This is also in Clang ToT, but as I mentioned, I don't think this model makes
sense for libc++ here.)
Quuxplusone commented 7 years ago

Yeah, what Duncan said matches exactly with what I was trying to convey to Jack in email.

The headers are correct. Unfortunately, we cannot add availability annotation to macros, but a runtime check on something equivalent (corresponding symbol, OS version) is appropriate.

This is an instance of a general class of problems that happens when trying to use newer SDKs than the deployment target with OSS software that doesn't follow that paradigm well (eg: cmake and autoconf rules frequently assume that the SDK represents the deployment target).

The workaround for this is using an SDK that matches the host OS. Jack should be able to get around this by installing the 10.12 SDK to /Library/Developer/CommandLineTools/SDKs, but that will require some manual cleanup of the bad symlink that is currently there. I've filed a radar about that.

Quuxplusone commented 7 years ago 
(In reply to Jeremy Huddleston Sequoia from comment #8)
> Yeah, what Duncan said matches exactly with what I was trying to convey to
> Jack in email.
>
> The headers are correct.  Unfortunately, we cannot add availability
> annotation to macros, but a runtime check on something equivalent
> (corresponding symbol, OS version) is appropriate.
>
> This is an instance of a general class of problems that happens when trying
> to use newer SDKs than the deployment target with OSS software that doesn't
> follow that paradigm well (eg: cmake and autoconf rules frequently assume
> that the SDK represents the deployment target).
>
> The workaround for this is using an SDK that matches the host OS.  Jack
> should be able to get around this by installing the 10.12 SDK to
> /Library/Developer/CommandLineTools/SDKs, but that will require some manual
> cleanup of the bad symlink that is currently there.  I've filed a radar
> about that.

Was the presence of the MacOSX10.12.sdk symlink intentional in the Xcode 9 beta
Command Line Tools for 10.12? The last time that Apple bumped the SDK for the
prior release was for Xcode 8.2. The Command Line Tools for Xcode 8.2 on 10.11
has...

% cd /Library/Developer/CommandLineTools/SDKs
% ls -l
total 8
drwxr-xr-x  5 root  wheel  170 Dec 19 17:38 MacOSX.sdk
lrwxr-xr-x  1 root  wheel   10 Dec 19 17:34 MacOSX10.12.sdk -> MacOSX.sdk

without a symlink for MacOSX10.11.sdk
Quuxplusone commented 7 years ago 
I suspect the 10.12 symlink is leftover from your install of Xcode 8 CLTools.

What's the output of:

pkgutil --file-info /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk
pkgutil --file-info /Library/Developer/CommandLineTools/SDKs/MacOSX10.12.sdk
pkgutil --file-info /Library/Developer/CommandLineTools/SDKs/MacOSX10.13.sdk
Quuxplusone commented 7 years ago 
(In reply to Jeremy Huddleston Sequoia from comment #10)
> I suspect the 10.12 symlink is leftover from your install of Xcode 8 CLTools.
>
> What's the output of:
>
> pkgutil --file-info /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk
> pkgutil --file-info /Library/Developer/CommandLineTools/SDKs/MacOSX10.12.sdk
> pkgutil --file-info /Library/Developer/CommandLineTools/SDKs/MacOSX10.13.sdk

% pkgutil --file-info /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk
volume: /
path: /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk

pkgid: com.apple.pkg.CLTools_SDK_macOS1013
pkg-version: 9.0.0.0.1.1496343141
install-time: 1497477032
uid: 0
gid: 0
mode: 755

pkgid: com.apple.pkg.CLTools_SDK_OSX1012
pkg-version: 8.3.2.0.1.1492020469
install-time: 1497446097
uid: 0
gid: 0
mode: 755

% pkgutil --file-info /Library/Developer/CommandLineTools/SDKs/MacOSX10.13.sdk
volume: /
path: /Library/Developer/CommandLineTools/SDKs/MacOSX10.13.sdk

pkgid: com.apple.pkg.CLTools_SDK_macOS1013
pkg-version: 9.0.0.0.1.1496343141
install-time: 1497477032
uid: 0
gid: 0
mode: 755

% pkgutil --file-info /Library/Developer/CommandLineTools/SDKs/MacOSX10.12.sdk
volume: /
path: /Library/Developer/CommandLineTools/SDKs/MacOSX10.12.sdk

pkgid: com.apple.pkg.CLTools_SDK_OSX1012
pkg-version: 8.3.2.0.1.1492020469
install-time: 1497446097
uid: 0
gid: 0
mode: 755

Apple might want to enhance the Xcode 9 beta Command Line Tools installer to
prune that symlink when upgrading from Xcode 8 to Xcode 9 on 10.12 (to insure a
more uniform installation).
Quuxplusone commented 7 years ago 
So, should I conclude from this that this is merely a beta Xcode install issue?

For 10.2, UTIME_OMIT is not defined, and utimensat is not implemented
For 10.3, UTIME_OMIT is     defined, and utimensat is     implemented

Is that correct?
Quuxplusone commented 7 years ago 
(In reply to Marshall Clow (home) from comment #12)
> So, should I conclude from this that this is merely a beta Xcode install
> issue?
>
> For 10.2, UTIME_OMIT is not defined, and utimensat is not implemented
> For 10.3, UTIME_OMIT is     defined, and utimensat is     implemented
>
> Is that correct?

Apple only ships the latest 'released' version of the SDK in both the Command
Line Tools and Xcode.app these days. So when a new OS release occurs, its
matching Xcode release runs on the prior OS release but only ships with the SDK
of the newest OS release. So, for example, Xcode 8.2 only has the 10.12 SDK
installed in the 10.11 Command Line Tools. The Xcode 9 Command Line Tools for
10.12 appears to be continuing this pattern by installing only the 10.13 SDK.

FYI, the DevSDK installed in / still is that from the actual OS X release
itself (i.e. 10.11 on 10.11 and 10.12 on 10.12) but this is different from the
buried MacOSX.sdk.
Quuxplusone commented 7 years ago 
(In reply to Marshall Clow (home) from comment #12)
> So, should I conclude from this that this is merely a beta Xcode install
> issue?
>
> For 10.2, UTIME_OMIT is not defined, and utimensat is not implemented
> For 10.3, UTIME_OMIT is     defined, and utimensat is     implemented
>
> Is that correct?

No.  This is not a beta SDK issues.  You're conflating SDK and runtime.

With the 10.13 SDK:
   With 10.12 deployment target, UTIME_OMIT is defined and utimensat is declared (weak).
   With 10.13 deployment target, UTIME_OMIT is defined and utimensat is declared.

With the 10.12 SDK:
   UTIME_OMIT is not defined and utimensat is not delcared.

On macOS 10.12, utimensat is not implemented.
On macOS 10.13, utimensat is implemented.

There is a bug with the CLTools package that Jack pointed out above (cross-
contamination between the 10.12 and 10.13 SDKs from Xcode 8 and Xcode 9 CLTools
packages), but it has nothing to do with this issue.
Quuxplusone commented 7 years ago 
Can we take the discussion of the SDK install issue elsewhere, and leave this
bug to deal with the libcxx bug?

Note that a fix for the latter is under review here:
https://reviews.llvm.org/D34249
Quuxplusone commented 7 years ago 
(In reply to Duncan from comment #15)
> Can we take the discussion of the SDK install issue elsewhere, and leave
> this bug to deal with the libcxx bug?
>
> Note that a fix for the latter is under review here:
> https://reviews.llvm.org/D34249

I'll test the fix as soon as a final form is settled on.

FYI, regarding the stale MacOSX10.12.sdk symlink left by Xcode 9 beta, I have
filed that issue as radar://32804690,
"Command_Line_Tools_macOS_10.12_for_Xcode_9_beta leaves previous
MacOSX10.12.sdk symlink from Xcode 8.3.2".
Quuxplusone commented 6 years ago

r324385