-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Object files can only be linked with legacy clang linker on Apple #8730
Comments
I've reproduced the issue with arm64. If I change to use x86_64 it works with either linker. The linker error is:
clang generates relocations like this:
cranelift generates:
If I hack cranelift to generate this:
then the linker error changes to:
If I hack cranelift to generate this:
then it links and runs successfully. Complete hack: diff --git a/cranelift/codegen/src/isa/aarch64/abi.rs b/cranelift/codegen/src/isa/aarch64/abi.rs
index 922e4ebfc..ea0bc22f3 100644
--- a/cranelift/codegen/src/isa/aarch64/abi.rs
+++ b/cranelift/codegen/src/isa/aarch64/abi.rs
@@ -1027,7 +1027,8 @@ impl ABIMachineSpec for AArch64MachineDeps {
) -> SmallVec<[Inst; 2]> {
let mut insts = SmallVec::new();
match &dest {
- &CallDest::ExtName(ref name, RelocDistance::Near) => insts.push(Inst::Call {
+ //&CallDest::ExtName(ref name, RelocDistance::Near) => insts.push(Inst::Call {
+ &CallDest::ExtName(ref name, _) => insts.push(Inst::Call {
info: Box::new(CallInfo {
dest: name.clone(),
uses,
@@ -1039,6 +1040,7 @@ impl ABIMachineSpec for AArch64MachineDeps {
callee_pop_size,
}),
}),
+/*
&CallDest::ExtName(ref name, RelocDistance::Far) => {
insts.push(Inst::LoadExtName {
rd: tmp,
@@ -1058,6 +1060,7 @@ impl ABIMachineSpec for AArch64MachineDeps {
}),
});
}
+*/
&CallDest::Reg(reg) => insts.push(Inst::CallInd {
info: Box::new(CallIndInfo {
rn: *reg,
diff --git a/cranelift/codegen/src/isa/aarch64/inst/emit.rs b/cranelift/codegen/src/isa/aarch64/inst/emit.rs
index f9df1a8d2..0f89903a4 100644
--- a/cranelift/codegen/src/isa/aarch64/inst/emit.rs
+++ b/cranelift/codegen/src/isa/aarch64/inst/emit.rs
@@ -3177,6 +3177,7 @@ impl MachInstEmit for Inst {
let inst = Inst::Adrp { rd, off: 0 };
inst.emit(sink, emit_info, state);
+/*
// ldr rd, [rd, :got_lo12:X]
sink.add_reloc(Reloc::Aarch64Ld64GotLo12Nc, &**name, 0);
let inst = Inst::ULoad64 {
@@ -3185,6 +3186,17 @@ impl MachInstEmit for Inst {
flags: MemFlags::trusted(),
};
inst.emit(sink, emit_info, state);
+*/
+ // add rd, rd, :lo12:X
+ sink.add_reloc(Reloc::Aarch64Ld64GotLo12Nc, &**name, 0);
+ Inst::AluRRImm12 {
+ alu_op: ALUOp::Add,
+ size: OperandSize::Size64,
+ rd,
+ rn: rd.to_reg(),
+ imm12: Imm12::maybe_from_u64(0).unwrap(),
+ }
+ .emit(sink, emit_info, state);
} else {
// With absolute offsets we set up a load from a preallocated space, and then jump
// over it.
diff --git a/cranelift/object/src/backend.rs b/cranelift/object/src/backend.rs
index 2d48e9b61..921c5273e 100644
--- a/cranelift/object/src/backend.rs
+++ b/cranelift/object/src/backend.rs
@@ -764,7 +764,8 @@ impl ObjectModule {
r_type: object::elf::R_AARCH64_ADR_GOT_PAGE,
},
object::BinaryFormat::MachO => RelocationFlags::MachO {
- r_type: object::macho::ARM64_RELOC_GOT_LOAD_PAGE21,
+ //r_type: object::macho::ARM64_RELOC_GOT_LOAD_PAGE21,
+ r_type: object::macho::ARM64_RELOC_PAGE21,
r_pcrel: true,
r_length: 2,
},
@@ -775,7 +776,8 @@ impl ObjectModule {
r_type: object::elf::R_AARCH64_LD64_GOT_LO12_NC,
},
object::BinaryFormat::MachO => RelocationFlags::MachO {
- r_type: object::macho::ARM64_RELOC_GOT_LOAD_PAGEOFF12,
+ //r_type: object::macho::ARM64_RELOC_GOT_LOAD_PAGEOFF12,
+ r_type: object::macho::ARM64_RELOC_PAGEOFF12,
r_pcrel: false,
r_length: 2,
}, So it appears that cranelift isn't generating the correct relocations for ARM64 on macOS. I'll let those who know more about this stuff fix it properly. |
A simple #[inline(never)]
pub unsafe fn foo() -> (unsafe extern "C" fn(), &'static u8) {
bar(); (bar, &FOO)
}
extern "C" {
fn bar();
static FOO: u8;
} produces
with the LLVM backend. If I try to link it I get a linker error as expected:
If I apply the following patch to Cranelift: diff --git a/cranelift/codegen/src/isa/aarch64/abi.rs b/cranelift/codegen/src/isa/aarch64/abi.rs
index 922e4ebfc..6d43c1db6 100644
--- a/cranelift/codegen/src/isa/aarch64/abi.rs
+++ b/cranelift/codegen/src/isa/aarch64/abi.rs
@@ -1027,7 +1027,7 @@ impl ABIMachineSpec for AArch64MachineDeps {
) -> SmallVec<[Inst; 2]> {
let mut insts = SmallVec::new();
match &dest {
- &CallDest::ExtName(ref name, RelocDistance::Near) => insts.push(Inst::Call {
+ &CallDest::ExtName(ref name, _/*RelocDistance::Near*/) => insts.push(Inst::Call {
info: Box::new(CallInfo {
dest: name.clone(),
uses, Relocations that print identical by
Yet the object file produced by LLVM gives a regular linker error due to undefined symbols, while the one produced by Cranelift crashes the linker:
This means there has to be a difference somewhere. If I run bingrep on both binaries, the relocations are shown in opposite order for both executable. For LLVM:
and for Cranelift:
As hack I tried to reverse the order of relocations emitted by Cranelift too, which worked. diff --git a/cranelift/codegen/src/isa/aarch64/abi.rs b/cranelift/codegen/src/isa/aarch64/abi.rs
index 922e4ebfc..6d43c1db6 100644
--- a/cranelift/codegen/src/isa/aarch64/abi.rs
+++ b/cranelift/codegen/src/isa/aarch64/abi.rs
@@ -1027,7 +1027,7 @@ impl ABIMachineSpec for AArch64MachineDeps {
) -> SmallVec<[Inst; 2]> {
let mut insts = SmallVec::new();
match &dest {
- &CallDest::ExtName(ref name, RelocDistance::Near) => insts.push(Inst::Call {
+ &CallDest::ExtName(ref name, _/*RelocDistance::Near*/) => insts.push(Inst::Call {
info: Box::new(CallInfo {
dest: name.clone(),
uses,
diff --git a/cranelift/object/src/backend.rs b/cranelift/object/src/backend.rs
index 2d48e9b61..c5a84f14c 100644
--- a/cranelift/object/src/backend.rs
+++ b/cranelift/object/src/backend.rs
@@ -501,7 +501,7 @@ impl ObjectModule {
ref name,
flags,
addend,
- } in &symbol.relocs
+ } in symbol.relocs.iter().rev()
{
let target_symbol = self.get_symbol(name);
self.object |
Looks like the change to cranelift/codegen/src/isa/aarch64/abi.rs isn't even necessary at all. Only the relocation order reversing is required. |
That sounds better. I was comparing clang output instead of looking at what rustc with llvm backend did, so you can probably ignore my results. |
What would be the best place to add this relocation reversing code? object or cranelift-object? On the one hand it is something that would affect other projects too, so object makes more sense, on the other hand reversing the relocation order right before writing would require anyone who wants to round trip an object file through the object crate to reverse relocation order again before the |
LLVM does the reverse in its object writer. I'm not certain which place is best, for the same reasons you give, but I'm inclined to fix it in |
Opened gimli-rs/object#702 |
This has been fixed in object 0.36.1. |
Thank you all who looked into this! |
This issue can be closed now, right? |
I'm now getting |
We will need to call set_macho_build_version in cranelift-object to emit the LC_BUILD_VERSION load command. |
It seems like rustc has implemented a fix for the same issue: rust-lang/rust#111384. |
It's unclear to me if cranelift-object should be exposing some function to set the macho build version or if it should handle it automatically. Do we have enough information in cranelift-object to target the exact version? |
I've looked into this recently, see rust-lang/rust#129432 for the broader Rust issue of deployment targets and SDKs. I'm pretty sure the correct behaviour for Cranelift would be to set the values on
Note that this will not support older tooling that uses |
So to answer this directly, Cranelift should have enough information to set this if it requires the deployment target / minimum OS version to be set in the target triple (e.g. that users pass |
Cranelift uses target-lexicon for target triples, which aims to math rustc. Rustc target triples do not allow passing in the OS version. Instead rustc has a default for each target and additionally allows setting the same env vars that clang reads. There is no way currently to pass this information to Cranelift. |
Hmm, isn't |
Yes, but as it turns out target-lexicon is just lenient with parsing to parse most LLVM triples fine too. I'm pretty sure it would parse aarch64-apple-macosx12.0.0 as having the OS field set to macosx12.0.1, which is not an OS known to rustc or target-lexicon and thus would fail parsing. |
Nope, they do parse the version number, see https://github.com/bytecodealliance/target-lexicon/blob/7c80d459a9fdd121e9f23feb680c3db13c1baa39/src/targets.rs#L1393-L1421. In any case, I have opened bytecodealliance/target-lexicon#111 for figuring out how to resolve the But if it is as you say, that Cranelift wants to accept A simple way to do this would be to add |
We are using target-lexicon 0.13 so we should now be able to call |
Hello, thank you very much for all of the incredible software y'all are writing. I'm currently using Cranelift to generate native binaries for my project, and I'm continually impressed at the level of detail and effort that goes into these things. Not to mention the super helpful Zulip chat.
In the aforementioned chat, I was receiving help from @bjorn3 and @cfallin on generating executables for an Apple silicon Mac. The following code does not successfully execute on an M-series processor Mac: https://gist.github.com/sezna/e358a9167d4ef3063f6cfe4ce4c2ad37
Using the legacy linker, that is, adding
-Wl -ld_classic
to the clang invocation, does work. However, as the legacy linker will likely be going away someday, I'm filing this issue in the hopes that we can figure out why it doesn't work with the current linker. Note thatcg_clif
also uses-Wl -ld_classic
, so when the legacy linker is dropped,cg_clif
could also have issues.The text was updated successfully, but these errors were encountered: