Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
4 tasks done
madsmtm opened this issue Nov 13, 2024 · 5 comments
Open
4 tasks done

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

madsmtm opened this issue Nov 13, 2024 · 5 comments
Labels
bug Reproducible Homebrew/homebrew-core bug

Comments

@madsmtm
Copy link

madsmtm commented Nov 13, 2024

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

What were you trying to do (and why)?

The changes in #196094 and #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:

  1. SDKs are backwards compatible, so you can still compile for a lower deployment target.
  2. Newer SDKs can contain important ABI fixes and may show more compiler warnings.
  3. 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.

See also related #197278 and #197277.

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 madsmtm added the bug Reproducible Homebrew/homebrew-core bug label Nov 13, 2024
@madsmtm
Copy link
Author

madsmtm commented Nov 13, 2024

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
Copy link
Member

carlocab commented Nov 13, 2024

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
Copy link
Author

madsmtm commented Nov 13, 2024

This can lead to crashes at runtime.

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

@Bo98
Copy link
Member

Bo98 commented Nov 13, 2024

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
Copy link
Author

madsmtm commented Nov 13, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Reproducible Homebrew/homebrew-core bug
Projects
None yet
Development

No branches or pull requests

3 participants