llvm@19: Incorrect use of deployment target to find SDK version

madsmtm commented 2 weeks ago

`brew gist-logs <formula>` link OR `brew config` AND `brew doctor` output

$ brew gist-logs llvm@19
Error: No logs.

$ brew config
HOMEBREW_VERSION: 4.4.5
ORIGIN: https://github.com/Homebrew/brew
HEAD: 254bf3fe9d8fa2e1b2fb55dbcf535b2d870180c4
Last commit: 2 days ago
Core tap JSON: 13 Nov 02:59 UTC
Core cask tap JSON: 13 Nov 02:59 UTC
HOMEBREW_PREFIX: /opt/homebrew
HOMEBREW_CASK_OPTS: []
HOMEBREW_MAKE_JOBS: 8
Homebrew Ruby: 3.3.6 => /opt/homebrew/Library/Homebrew/vendor/portable-ruby/3.3.6/bin/ruby
CPU: octa-core 64-bit dunno
Clang: 16.0.0 build 1600
Git: 2.39.5 => /Library/Developer/CommandLineTools/usr/bin/git
Curl: 8.7.1 => /usr/bin/curl
macOS: 15.1-arm64
CLT: 16.1.0.0.1.1729049160
Xcode: N/A
Rosetta 2: false

$ brew doctor
Your system is ready to brew.

Verification

[X] My brew doctor output says Your system is ready to brew. and am still able to reproduce my issue.
[X] I ran brew update and am still able to reproduce my issue.
[X] I have resolved all warnings from brew doctor and that did not fix my problem.
[X] I searched for recent similar issues at https://github.com/Homebrew/homebrew-core/issues?q=is%3Aissue and found no duplicates.

What were you trying to do (and why)?

The changes in https://github.com/Homebrew/homebrew-core/pull/196094 and https://github.com/Homebrew/homebrew-core/pull/197410 have an issue; they conflate the SDK version with the deployment target, but these two are not the same!

In almost all cases, you want to use the latest SDK available, since:

SDKs are backwards compatible, so you can still compile for a lower deployment target.
Newer SDKs can contain important ABI fixes and may show more compiler warnings.
The SDK version is embedded in the final binary (vtool -show a.out), and may have subtle effects at runtime.

In particular, the version in -target is the deployment target, and shouldn't be used to determine the SDK version, because a user might want to compile for a lower deployment target than the SDK that they have installed.

CC @carlocab @Bo98

What happened (include all command output)?

As an example, the following results in a link error if the macOS 11.0 SDK is not installed:

$ echo "int main() { return 0; }" > foo.c
$ /opt/homebrew/opt/llvm/bin/clang -target arm64-apple-macosx11.0 foo.c
ld: library 'System' not found
clang: error: linker command failed with exit code 1 (use -v to see invocation)

What did you expect to happen?

Linking succeeded.

Step-by-step reproduction instructions (by running `brew` commands)

brew install llvm@19
echo "int main() { return 0; }" > foo.c

# Any of these fail
/opt/homebrew/opt/llvm/bin/clang -target x86_64-apple-macosx10.14 foo.c
/opt/homebrew/opt/llvm/bin/clang -target x86_64-apple-darwin18.0 foo.c
/opt/homebrew/opt/llvm/bin/clang -target arm64-apple-macosx11.0 foo.c
/opt/homebrew/opt/llvm/bin/clang -target arm64-apple-darwin20.0 foo.c

madsmtm commented 2 weeks ago

If you really want to use configuration files, my suggested fix would just be to only have four of them: arm64-apple-darwin.cfg, x86_64-apple-darwin.cfg, arm64-apple-macosx.cfg and x86_64-apple-macosx.cfg. No versioning needed!

carlocab commented 2 weeks ago

I discussed this with @Bo98, and using a non-matching SDK only works if your source makes use of Apple's availability API -- a lot of software (e.g. git, curl) doesn't. This can lead to crashes at runtime.

This is also the reason why we didn't use the unversioned MacOSX.sdk for configuring DEFAULT_SYSROOT.

madsmtm commented 2 weeks ago

This can lead to crashes at runtime.

Huh, how so? I'd be very interested in examples of this!

Bo98 commented 2 weeks ago

You do raise a point that -target version indeed should be deployment target (though -mmacos-version-min is much more common on macOS instead of -target and should still work).

It's tricky to get the perfect solution here because ideally what we'd do what Apple does: have a wrapper around clang that determines the SDK automatically. Maybe we'll have to do that at some point, though Apple's solution is closed source so we'd need to make our own.

Huh, how so? I'd be very interested in examples of this!

I don't have a recent example as Apple recently backported a bunch of things in macOS 13.6.8 so curl versions are now aligned (which is rare), but this previously compiled fine on macOS 13 with the macOS 14 SDK with no warnings but crashed at runtime. There'll be another example with macOS 15 SDK on older macOS when 8.8.0+ arrives in the SDK.

#include <curl/multi.h>

int main() {
  CURLM *m = curl_multi_init();
  curl_multi_get_handles(m);
  return 0;
}

$ clang -mmacos-version-min=13 -lcurl foo.c
$ ./a.out

The way it works with Apple APIs is different:

#include <math.h>

int main() {
  __fabsf16(0);
  return 0;
}

foo.c:5:3: warning: '__fabsf16' is only available on macOS 15.0 or newer [-Wunguarded-availability-new]
    5 |   __fabsf16(0);
      |   ^~~~~~~~~
/Library/Developer/CommandLineTools/SDKs/MacOSX15.sdk/usr/include/math.h:614:17: note: '__fabsf16' has been marked as being introduced in macOS 15.0 here, but the deployment target is macOS 14.0.0
  614 | extern _Float16 __fabsf16(_Float16) __API_AVAILABLE(macos(15.0), ios(18.0), watchos(11.0), tvos(18.0));
      |                 ^
foo.c:5:3: note: enclose '__fabsf16' in a __builtin_available check to silence this warning
    5 |   __fabsf16(0);
      |   ^~~~~~~~~    
1 warning generated.

Which lead to a second problem: occasionally you would get configure scripts that detect the availability of a function by whether it compiles but not if it runs. This somewhat makes sense for the cross-compile case, so I guess that's why some did it. But a significant number of scripts did not set -Werror (or -Werror=unguarded-availability-new) so would not see the warning and take a successful compile as it working.

We previously had to do crazy workarounds for this every macOS version: https://github.com/Homebrew/brew/blob/bc31f731eaede602b3707fe2eadb61b4f5df4472/Library/Homebrew/extend/os/mac/extend/ENV/super.rb#L115-L131. Note how we don't need to do that anymore - we changed how the SDK selection to match the host OS around the time when 10.15 was current (at which point 10.12 and earlier wasn't in our CI anymore so we just kept those lines as a precaution).

madsmtm commented 2 weeks ago

Thanks for the detailed answer! Makes sense that this would fail with bad configure scripts, didn't consider that.

-mmacos-version-min is much more common on macOS instead of -target and should still work

Huh, indeed! Though it's odd that it does, since it translates the target internally to have the version before passing it to LLVM (can be seen with both -v and in the output from -S -emit-llvm).

I'll have to reconsider our method for passing the deployment target to Clang given that piece of information.

Homebrew / homebrew-core