Fix ld64 flags #90717
Fix ld64 flags #90717
Conversation
|
r? @davidtwco (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
Testing is awkward due to how Rust is bootstrapped. Currently, the Rust compiler will provide incorrect flags to the linker if one is explicitly defined in I handled this by writing a program to sit between rustc and ld64. This program would parse the malformed |
|
The bootstrap crate hard-codes flags that are not compatible with ld64. Some("-Wl,-rpath,@loader_path/../lib")From what I can the bootstrap crate does not have logic similar to I would like to merge changes to |
|
@kit-981 Linking directly with a linker (as opposed to a gcc/clang wrapper) is typically only supported for bare metal targets, which Apple targets are not. |
petrochenkov
added
S-waiting-on-author
if self.sess.target.is_like_osx {
if !self.is_ld {
arg.push("-Wl,")
}
arg.push("-exported_symbols_list,");
} else if self.sess.target.is_like_solaris {
if !self.is_ld {
arg.push("-Wl,")
}
arg.push("-M,");
} else {
if !self.is_ld {
arg.push("-Wl,")
}
// Both LD and LLD accept export list in *.def file form, there are no flags required
if !is_windows {
arg.push("--version-script=")
}
}
arg.push(&path);
self.cmd.arg(arg);Suppose you are using Clang to link for an osx-like target. The first condition Suppose you are using ld to link for an osx-like target. The first condition fn build_dylib(&mut self, out_filename: &Path) {
// On mac we need to tell the linker to let this library be rpathed
if self.sess.target.is_like_osx {
self.cmd.arg("-dynamiclib");As mentioned in #86064, /// Argument that must be passed *directly* to the linker
///
/// These arguments need to be prepended with `-Wl`, when a GCC-style linker is used.
fn linker_arg<S>(&mut self, arg: S) -> &mut Self
where
S: AsRef<OsStr>,
{
if !self.is_ld {
let mut os = OsString::from("-Wl,");
os.push(arg.as_ref());
self.cmd.arg(os);
} else {
self.cmd.arg(arg);
}
self
}Given that this logic exists, I can only assume that adding
This is not consistent with my experience. I have been very happily using a forked version of the Rust compiler with cctools to compile executables and run them on macOS. I have gone further and have run iOS executables using this toolchain as you can see in #88072 and #88077.
Sure but this doesn't change the fact that these are bugs that I have volunteered to fix and have fixed. I'm not introducing support. I'm fixing obviously broken code.
Clang will use the system linker by default on Linux which likely doesn't support mach-o or macOS/iOS targets. I assume I could use Philosophically, I don't think its sound for Rust to rely on Clang as a proxy to do linking. There is more control, less moving parts, and less room for error if Rust calls the linker directly. However, this is unrelated to this pull request. |
petrochenkov
added
S-waiting-on-review
apiraino
added
the
T-compiler
|
Sounds reasonable, I've left a couple of stylistic suggestions. |
petrochenkov
added
S-waiting-on-author
|
@petrochenkov Thanks for the feedback. I've updated my changes with your suggestions. |
Linker arguments must transformed when Rust is interacting with the linker through a compiler. This commit introduces a helper function that abstracts away details of this transformation.
This commit refactors linker argument generation to leverage a helper function that abstracts away details governing how these arguments are transformed and provided to the linker. This fixes the misuse of the `-exported_symbols_list` when an ld-like linker is used rather than a compiler. A compiler would expect `-Wl,-exported_symbols_list,path` but ld would expect `-exported_symbols_list` and `path` as two seperate arguments. Prior to this change, an ld-like linker was given `-exported_symbols_list,path`.
This is not a valid flag for ld64. When the ld64 linker is explicitly provided through `config.toml`, rustc will not successfully compile.
|
@rustbot label -S-waiting-on-author +S-waiting-on-review |
rustbot
added
S-waiting-on-review
|
@bors r+ |
|
📌 Commit f44fa63 has been approved by |
bors
added
S-waiting-on-bors
Fix ld64 flags - The `-exported_symbols_list` argument appears to be malformed for `ld64` (if you are not going through `clang`). - The `-dynamiclib` argument isn't support for `ld64`. It should be guarded behind a compiler flag. These problems are fixed by these changes. I have also refactored the way linker arguments are generated to be ld/compiler agnostic and therefore less error prone. These changes are necessary to support cross-compilation to darwin targets.
|
⌛ Testing commit f44fa63 with merge d1dfa755bbf9b466e32fe72790b68b6d80abecf4... |
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
@bors retry |
bors
added
S-waiting-on-bors
|
⌛ Testing commit f44fa63 with merge 9269d4fbb432e4cece5f54c2d4c1a5214398f13a... |
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
@bors retry |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (eab2d75): comparison url. Summary: This benchmark run did not return any relevant changes. If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. @rustbot label: -perf-regression |
-exported_symbols_listargument appears to be malformed forld64(if you are not going throughclang).-dynamiclibargument isn't support forld64. It should be guarded behind a compiler flag.These problems are fixed by these changes. I have also refactored the way linker arguments are generated to be ld/compiler agnostic and therefore less error prone.
These changes are necessary to support cross-compilation to darwin targets.