Skip to content

Commit

Permalink
Pass SDK version to ld64 when linking
Browse files Browse the repository at this point in the history
This is an important property of the binary that we were previously
setting incorrectly, and is a big step towards making binaries linked
with cc and ld64 equal.
  • Loading branch information
madsmtm committed Oct 10, 2024
1 parent 7111bec commit 1960f3b
Show file tree
Hide file tree
Showing 5 changed files with 90 additions and 34 deletions.
8 changes: 8 additions & 0 deletions compiler/rustc_codegen_ssa/messages.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,11 @@ codegen_ssa_apple_sdk_error_missing_developer_dir =
- `{$sdkroot}`
- `{$sdkroot_bare}`
codegen_ssa_apple_sdk_error_missing_mac_catalyst_version =
failed to find {$version} in SDK version map key `macOS_iOSMac`.
This is probably a bug in your SDK.
codegen_ssa_apple_sdk_error_missing_sdk_settings =
failed finding `SDKSettings.json` or `SDKSettings.plist` in `{$sdkroot}`.
Expand All @@ -62,6 +67,9 @@ codegen_ssa_apple_sdk_error_missing_xcode_select =
Consider using `sudo xcode-select --switch path/to/Xcode.app` or `sudo xcode-select --reset` to select a valid path.
codegen_ssa_apple_sdk_error_must_have_when_using_ld64 =
must know the SDK when linking with ld64
codegen_ssa_apple_sdk_error_not_sdk_path =
failed parsing SDK at `{$path}`.
Expand Down
38 changes: 37 additions & 1 deletion compiler/rustc_codegen_ssa/src/apple.rs
Original file line number Diff line number Diff line change
Expand Up @@ -282,7 +282,6 @@ pub(crate) struct SDKSettings {
canonical_name: String,

#[serde(rename = "Version")]
#[allow(dead_code)]
version: AppleOSVersion,

#[serde(rename = "MaximumDeploymentTarget")]
Expand All @@ -294,6 +293,13 @@ pub(crate) struct SDKSettings {
/// Optional to support `SDKSettings.json` converted from an older `SDKSettings.plist`.
#[serde(rename = "SupportedTargets")]
supported_targets: Option<BTreeMap<String, SupportedTargets>>,

/// Optional to support `SDKSettings.json` converted from an older `SDKSettings.plist`.
///
/// Could in general be useful in the future for building "zippered" binaries, see:
/// <https://github.com/rust-lang/rust/issues/131216>
#[serde(rename = "VersionMap")]
version_map: Option<BTreeMap<String, BTreeMap<AppleOSVersion, AppleOSVersion>>>,
}

impl SDKSettings {
Expand Down Expand Up @@ -327,6 +333,7 @@ impl SDKSettings {
},
default_properties: DefaultProperties::default(),
supported_targets: None,
version_map: None,
})
}

Expand Down Expand Up @@ -372,6 +379,35 @@ impl SDKSettings {
.unwrap_or(Path::new("/System/iOSSupport"))
}

/// The version of the SDK.
///
/// This is needed by the linker.
///
/// settings["Version"] or settings["VersionMap"]["macOS_iOSMac"][settings["Version"]].
pub(crate) fn sdk_version(&self, target: &Target, sdkroot: &Path) -> Result<AppleOSVersion, AppleSdkError> {
if target.abi == "macabi" {
let map = self
.version_map
.as_ref()
.ok_or(AppleSdkError::SdkDoesNotSupportOS {
sdkroot: sdkroot.to_owned(),
os: target.os.clone(),
abi: target.abi.clone(),
})?
.get("macOS_iOSMac")
.ok_or(AppleSdkError::SdkDoesNotSupportOS {
sdkroot: sdkroot.to_owned(),
os: target.os.clone(),
abi: target.abi.clone(),
})?;
map.get(&self.version).cloned().ok_or(AppleSdkError::MissingMacCatalystVersion {
version: self.version.pretty().to_string(),
})
} else {
Ok(self.version)
}
}

fn supports_target(&self, target: &Target, sdkroot: &Path) -> Result<(), AppleSdkError> {
let arch = ld64_arch(target);

Expand Down
61 changes: 38 additions & 23 deletions compiler/rustc_codegen_ssa/src/back/link.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2410,10 +2410,10 @@ fn add_order_independent_options(
// Take care of the flavors and CLI options requesting the `lld` linker.
add_lld_args(cmd, sess, flavor, self_contained_components);

add_apple_link_args(cmd, sess, flavor);

let apple_sdk_data = add_apple_sdk(cmd, sess, crate_type, flavor);

add_apple_link_args(cmd, sess, flavor, &apple_sdk_data);

add_link_script(cmd, sess, tmpdir, crate_type);

if sess.target.os == "fuchsia"
Expand Down Expand Up @@ -2970,7 +2970,12 @@ pub(crate) fn are_upstream_rust_objects_already_included(sess: &Session) -> bool
/// - The environment / ABI.
/// - The deployment target.
/// - The SDK version.
fn add_apple_link_args(cmd: &mut dyn Linker, sess: &Session, flavor: LinkerFlavor) {
fn add_apple_link_args(
cmd: &mut dyn Linker,
sess: &Session,
flavor: LinkerFlavor,
settings: &Option<(PathBuf, SDKSettings)>,
) {
if !sess.target.is_like_osx {
return;
}
Expand Down Expand Up @@ -3030,31 +3035,41 @@ fn add_apple_link_args(cmd: &mut dyn Linker, sess: &Session, flavor: LinkerFlavo
// `-[NSView wantsBestResolutionOpenGLSurface]` is `YES` when the SDK version is >= 10.15.
// <https://developer.apple.com/documentation/appkit/nsview/1414938-wantsbestresolutionopenglsurface?language=objc>
//
// We do not currently know the actual SDK version though, so we have a few options:
// 1. Use the minimum version supported by rustc.
// 2. Use the same as the deployment target.
// 3. Use an arbitary recent version.
// 4. Omit the version.
// So it is important that we pass the correct version here.
//
// The first option is too low / too conservative, and means that users will not get the
// same behaviour from a binary compiled with rustc as with one compiled by clang.
//
// The second option is similarly conservative, and also wrong since if the user specified a
// higher deployment target than the SDK they're compiling/linking with, the runtime might
// make invalid assumptions about the capabilities of the binary.
// For posterity, insufficient alternatives are listed below:
//
// The third option requires that `rustc` is periodically kept up to date with Apple's SDK
// version, and is also wrong for similar reasons as above.
// 1. Use the minimum version supported by rustc.
// Too low / too conservative, and means that users will not get the same behaviour from
// a binary compiled with rustc as with one compiled by clang.
//
// The fourth option is bad because while `ld`, `otool`, `vtool` and such understand it to
// mean "absent" or `n/a`, dyld doesn't actually understand it, and will end up interpreting
// it as 0.0, which is again too low/conservative.
// 2. Use the same as the deployment target.
// Similarly conservative, and also wrong since if the user specified a higher deployment
// target than the SDK they're compiling/linking with, the runtime might make invalid
// assumptions about the capabilities of the binary.
//
// Currently, we lie about the SDK version, and choose the second option.
// 3. Use an arbitary recent version.
// Requires that `rustc` is periodically kept up to date with Apple's SDK version, and is
// also wrong for similar reasons as above.
//
// FIXME(madsmtm): Parse the SDK version from the SDK root instead.
// <https://github.com/rust-lang/rust/issues/129432>
let sdk_version = &*min_version;
// 4. Omit the version.
// Bad because while `ld`, `otool`, `vtool` and such understand it to mean "absent" or
// `n/a`, dyld doesn't actually understand it, and will end up interpreting it as 0.0,
// which is again too low/conservative.
let AppleOSVersion { major, minor, patch } = if let Some((sdkroot, settings)) = settings {
settings.sdk_version(&sess.target, &sdkroot).unwrap_or_else(|err| {
sess.dcx().emit_err(err);
AppleOSVersion::MAX
})
} else {
// If the SDK wasn't read properly, we may have errored already, but we may also only
// have given a warning to support `zig cc` on non-macOS hosts. `ld64` requires the SDK
// version though, so we must actually error here.
sess.dcx().emit_err(errors::AppleSdkError::MustHaveWhenUsingLd64);
AppleOSVersion::MAX
};
let sdk_version = format!("{major}.{minor}.{patch}");

// From the man page for ld64 (`man ld`):
// > This is set to indicate the platform, oldest supported version of
Expand All @@ -3064,7 +3079,7 @@ fn add_apple_link_args(cmd: &mut dyn Linker, sess: &Session, flavor: LinkerFlavo
// Like with `-arch`, the linker can figure out the platform versions
// itself from the binaries being linked, but to be safe, we specify
// the desired versions here explicitly.
cmd.link_args(&["-platform_version", platform_name, &*min_version, sdk_version]);
cmd.link_args(&["-platform_version", platform_name, &*min_version, &*sdk_version]);
} else {
// cc == Cc::Yes
//
Expand Down
6 changes: 6 additions & 0 deletions compiler/rustc_codegen_ssa/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -565,6 +565,9 @@ pub(crate) enum AppleSdkError {
#[diag(codegen_ssa_apple_sdk_error_missing_developer_dir)]
MissingDeveloperDir { dir: PathBuf, sdkroot: PathBuf, sdkroot_bare: PathBuf },

#[diag(codegen_ssa_apple_sdk_error_missing_mac_catalyst_version)]
MissingMacCatalystVersion { version: String },

#[diag(codegen_ssa_apple_sdk_error_missing_sdk_settings)]
MissingSDKSettings { sdkroot: PathBuf },

Expand All @@ -574,6 +577,9 @@ pub(crate) enum AppleSdkError {
#[diag(codegen_ssa_apple_sdk_error_missing_xcode_select)]
MissingXcodeSelect { dir: PathBuf, sdkroot: PathBuf, sdkroot_bare: PathBuf },

#[diag(codegen_ssa_apple_sdk_error_must_have_when_using_ld64)]
MustHaveWhenUsingLd64,

#[diag(codegen_ssa_apple_sdk_error_not_sdk_path)]
NotSdkPath { sdkroot: PathBuf },

Expand Down
11 changes: 1 addition & 10 deletions tests/run-make/apple-sdk-version/rmake.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,6 @@ fn has_sdk_version(file: &str, version: &str) {
}

fn main() {
// Fetch rustc's inferred deployment target.
let current_deployment_target =
rustc().target(target()).print("deployment-target").run().stdout_utf8();
let current_deployment_target =
current_deployment_target.strip_prefix("deployment_target=").unwrap().trim();

// Fetch current SDK version via. xcrun.
//
// Assumes a standard Xcode distribution, where e.g. the macOS SDK's Mac Catalyst
Expand Down Expand Up @@ -87,9 +81,6 @@ fn main() {
.input("foo.rs")
.output(&file_name)
.run();
// FIXME(madsmtm): This uses the current deployment target
// instead of the current SDK version like Clang does.
// https://github.com/rust-lang/rust/issues/129432
has_sdk_version(&file_name, current_deployment_target);
has_sdk_version(&file_name, current_sdk_version);
}
}

0 comments on commit 1960f3b

Please sign in to comment.