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.

Sign up for GitHub

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

Jump to bottom

Always add LC_BUILD_VERSION for metadata object files #114114

Merged
merged 3 commits into from
Aug 29, 2023
Merged

Always add LC_BUILD_VERSION for metadata object files #114114

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d37fdc9
Always add LC_BUILD_VERSION for metadata object files
keith Jul 27, 2023
f988cbb
Use target.abi instead of string matching llvm_target
keith Aug 21, 2023
2939e85
Add comment about unused sdk versions
keith Aug 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
29 changes: 12 additions & 17 deletions compiler/rustc_codegen_ssa/src/back/metadata.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,9 +226,7 @@ pub(crate) fn create_object_file(sess: &Session) -> Option<write::Object<'static

let mut file = write::Object::new(binary_format, architecture, endianness);
if sess.target.is_like_osx {
if let Some(build_version) = macho_object_build_version_for_target(&sess.target) {
file.set_macho_build_version(build_version)
}
file.set_macho_build_version(macho_object_build_version_for_target(&sess.target))
}
let e_flags = match architecture {
Architecture::Mips => {
Expand Down Expand Up @@ -334,31 +332,28 @@ pub(crate) fn create_object_file(sess: &Session) -> Option<write::Object<'static
Some(file)
}

/// Apple's LD, when linking for Mac Catalyst, requires object files to
/// contain information about what they were built for (LC_BUILD_VERSION):
/// the platform (macOS/watchOS etc), minimum OS version, and SDK version.
/// This returns a `MachOBuildVersion` if necessary for the target.
fn macho_object_build_version_for_target(
target: &Target,
) -> Option<object::write::MachOBuildVersion> {
if !target.llvm_target.ends_with("-macabi") {
return None;
}
/// Since Xcode 15 Apple's LD requires object files to contain information about what they were
/// built for (LC_BUILD_VERSION): the platform (macOS/watchOS etc), minimum OS version, and SDK
/// version. This returns a `MachOBuildVersion` for the target.
fn macho_object_build_version_for_target(target: &Target) -> object::write::MachOBuildVersion {
/// The `object` crate demands "X.Y.Z encoded in nibbles as xxxx.yy.zz"
/// e.g. minOS 14.0 = 0x000E0000, or SDK 16.2 = 0x00100200
fn pack_version((major, minor): (u32, u32)) -> u32 {
(major << 16) | (minor << 8)
}

let platform = object::macho::PLATFORM_MACCATALYST;
let min_os = (14, 0);
let sdk = (16, 2);
let platform =
rustc_target::spec::current_apple_platform(target).expect("unknown Apple target OS");
let min_os = rustc_target::spec::current_apple_deployment_target(target)
.expect("unknown Apple target OS");
let sdk =
rustc_target::spec::current_apple_sdk_version(platform).expect("unknown Apple target OS");

let mut build_version = object::write::MachOBuildVersion::default();
build_version.platform = platform;
build_version.minos = pack_version(min_os);
build_version.sdk = pack_version(sdk);
Some(build_version)
build_version
}

pub enum MetadataPosition {
Expand Down
8 changes: 3 additions & 5 deletions compiler/rustc_driver_impl/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -858,11 +858,9 @@ fn print_crate_info(
use rustc_target::spec::current_apple_deployment_target;

if sess.target.is_like_osx {
println_info!(
"deployment_target={}",
current_apple_deployment_target(&sess.target)
.expect("unknown Apple target OS")
)
let (major, minor) = current_apple_deployment_target(&sess.target)
.expect("unknown Apple target OS");
println_info!("deployment_target={}", format!("{major}.{minor}"))
} else {
handler
.early_error("only Apple targets currently support deployment version info")
Expand Down
43 changes: 40 additions & 3 deletions compiler/rustc_target/src/spec/apple_base.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,20 +179,52 @@ pub fn opts(os: &'static str, arch: Arch) -> TargetOptions {
}
}

pub fn deployment_target(target: &Target) -> Option<String> {
pub fn sdk_version(platform: u32) -> Option<(u32, u32)> {
// NOTE: These values are from an arbitrary point in time but shouldn't make it into the final
// binary since the final link command will have the current SDK version passed to it.
match platform {
object::macho::PLATFORM_MACOS => Some((13, 1)),
object::macho::PLATFORM_IOS
| object::macho::PLATFORM_IOSSIMULATOR
| object::macho::PLATFORM_TVOS
| object::macho::PLATFORM_TVOSSIMULATOR
| object::macho::PLATFORM_MACCATALYST => Some((16, 2)),
object::macho::PLATFORM_WATCHOS | object::macho::PLATFORM_WATCHOSSIMULATOR => Some((9, 1)),
_ => None,
}
}
keith marked this conversation as resolved.
Show resolved Hide resolved

pub fn platform(target: &Target) -> Option<u32> {
Some(match (&*target.os, &*target.abi) {
("macos", _) => object::macho::PLATFORM_MACOS,
("ios", "macabi") => object::macho::PLATFORM_MACCATALYST,
("ios", "sim") => object::macho::PLATFORM_IOSSIMULATOR,
("ios", _) => object::macho::PLATFORM_IOS,
("watchos", "sim") => object::macho::PLATFORM_WATCHOSSIMULATOR,
("watchos", _) => object::macho::PLATFORM_WATCHOS,
("tvos", "sim") => object::macho::PLATFORM_TVOSSIMULATOR,
("tvos", _) => object::macho::PLATFORM_TVOS,
_ => return None,
})
}

pub fn deployment_target(target: &Target) -> Option<(u32, u32)> {
let (major, minor) = match &*target.os {
"macos" => {
// This does not need to be specific. It just needs to handle x86 vs M1.
let arch = if target.arch == "x86" || target.arch == "x86_64" { X86_64 } else { Arm64 };
macos_deployment_target(arch)
}
"ios" => ios_deployment_target(),
"ios" => match &*target.abi {
"macabi" => mac_catalyst_deployment_target(),
_ => ios_deployment_target(),
},
"watchos" => watchos_deployment_target(),
"tvos" => tvos_deployment_target(),
_ => return None,
};

Some(format!("{major}.{minor}"))
Some((major, minor))
}

fn from_set_deployment_target(var_name: &str) -> Option<(u32, u32)> {
Expand Down Expand Up @@ -274,6 +306,11 @@ fn ios_deployment_target() -> (u32, u32) {
from_set_deployment_target("IPHONEOS_DEPLOYMENT_TARGET").unwrap_or((7, 0))
}

fn mac_catalyst_deployment_target() -> (u32, u32) {
// If you are looking for the default deployment target, prefer `rustc --print deployment-target`.
from_set_deployment_target("IPHONEOS_DEPLOYMENT_TARGET").unwrap_or((14, 0))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where did 14.0 come from here? Was it just because that's the current assumed target in the other spec files?

IMO (not the reviewer here) its fine to do this but in some followup the hardcoded versions in the current catalyst target specs should get nuked. This would match what users would expect now that were reading this variable. Definitely doesn't need to be done here though I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pulled it from the original implementation of this behavior that only applied to catalyst https://github.com/rust-lang/rust/pull/114114/files#diff-6bbf6edd930f26d3b180eb5c250b0e48d8f3c5eb474e3274909ef6ae6f0d1e61L343

I agree having it configurable makes sense, at least in this case if you set IPHONEOS_DEPLOYMENT_TARGET it would win though

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I missed the lines removed in the diff, oops. That answers the immediate question then.

I agree having it configurable makes sense, at least in this case if you set IPHONEOS_DEPLOYMENT_TARGET it would win though

Do you want to take that to a followup PR or shall I? Fixing the mess that's Catalyst has been on my list forever.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can send that one after!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah it looks like using the triple for this case 78bbe57#diff-f2aa7189f140501f7d0d283ef503e1980a9449ef0292b48b650f6e0c87bb8d6b complicates things slightly

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

iiuc it should just be making another of the <target_here>_lld_platform_version functions to use for this and other LLVM needs and have that source from the deployment target function. Might have missed a complication with the catalyst target though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok sounds good I can try it after this one

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like to make this work I would need to change add_pre_link_args to take a String or something else since ours would be dynamically computed here, would that be a reasonable change?

}

pub fn ios_llvm_target(arch: Arch) -> String {
// Modern iOS tooling extracts information about deployment target
// from LC_BUILD_VERSION. This load command will only be emitted when
Expand Down
2 changes: 2 additions & 0 deletions compiler/rustc_target/src/spec/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,8 @@ mod aix_base;
mod android_base;
mod apple_base;
pub use apple_base::deployment_target as current_apple_deployment_target;
pub use apple_base::platform as current_apple_platform;
pub use apple_base::sdk_version as current_apple_sdk_version;
mod avr_gnu_base;
pub use avr_gnu_base::ef_avr_arch;
mod bpf_base;
Expand Down