Cranelift: implement PIC relocations on AArch64

[syrusakbary]

Cranelift emits AbsoluteRelocation Reloc::Abs8 when is_pic setting is enabled in architecture aarch64

Steps to Reproduce

(module
  ;; Recursive factorial
  (func (export "fac-rec") (param i64) (result i64)
    (if (result i64) (i64.eq (local.get 0) (i64.const 0))
      (then (i64.const 1))
      (else
        (i64.mul (local.get 0) (call 0 (i64.sub (local.get 0) (i64.const 1))))
      )
    )
  )
)

$ wasmtime wasm2obj fac.wat fac.o

Code emitted fac.o has absolute relocations.

Expected Results

Is expected for Cranelift to emit a relative relocation for aarch64 when is_pic is enabled.

Actual Results

Code emitted with an absolute relocation.

Versions and Environment

Cranelift version: cranelift-codegen = "0.73.0"
Operating system: Any
Architecture: Aarch64

Extra info

Here's where the wrong code is emitted:

wasmtime/cranelift/codegen/src/isa/aarch64/inst/emit.rs

Lines 2440 to 2462 in 207da98

	&Inst::LoadExtName {
	rd,
	ref name,
	offset,
	} => {
	let inst = Inst::ULoad64 {
	rd,
	mem: AMode::Label(MemLabel::PCRel(8)),
	flags: MemFlags::trusted(),
	};
	inst.emit(sink, emit_info, state);
	let inst = Inst::Jump {
	dest: BranchTarget::ResolvedOffset(12),
	};
	inst.emit(sink, emit_info, state);
	let srcloc = state.cur_srcloc();
	sink.add_reloc(srcloc, Reloc::Abs8, name, offset);
	if emit_info.flags().emit_all_ones_funcaddrs() {
	sink.put8(u64::max_value());
	} else {
	sink.put8(0);
	}
	}

In the new x86 backend however, the is_pic case is properly handled:

wasmtime/cranelift/codegen/src/isa/x64/inst/emit.rs

Lines 2350 to 2387 in e676589

	Inst::LoadExtName { dst, name, offset } => {
	if info.flags().is_pic() {
	// Generates: movq symbol@GOTPCREL(%rip), %dst
	let enc_dst = int_reg_enc(dst.to_reg());
	sink.put1(0x48 | ((enc_dst >> 3) & 1) << 2);
	sink.put1(0x8B);
	sink.put1(0x05 | ((enc_dst & 7) << 3));
	emit_reloc(sink, state, Reloc::X86GOTPCRel4, name, -4);
	sink.put4(0);
	// Offset in the relocation above applies to the address of the *GOT entry*, not
	// the loaded address; so we emit a separate add or sub instruction if needed.
	if *offset < 0 {
	assert!(*offset >= -i32::MAX as i64);
	sink.put1(0x48 | ((enc_dst >> 3) & 1));
	sink.put1(0x81);
	sink.put1(0xe8 | (enc_dst & 7));
	sink.put4((-*offset) as u32);
	} else if *offset > 0 {
	assert!(*offset <= i32::MAX as i64);
	sink.put1(0x48 | ((enc_dst >> 3) & 1));
	sink.put1(0x81);
	sink.put1(0xc0 | (enc_dst & 7));
	sink.put4(*offset as u32);
	}
	} else {
	// The full address can be encoded in the register, with a relocation.
	// Generates: movabsq $name, %dst
	let enc_dst = int_reg_enc(dst.to_reg());
	sink.put1(0x48 | ((enc_dst >> 3) & 1));
	sink.put1(0xB8 | (enc_dst & 7));
	emit_reloc(sink, state, Reloc::Abs8, name, *offset);
	if info.flags().emit_all_ones_funcaddrs() {
	sink.put8(u64::max_value());
	} else {
	sink.put8(0);
	}
	}
	}

[cfallin]

Thanks for the report @syrusakbary !

Indeed this is a missing feature in the current aarch64 backend; it hasn't been needed yet but it should certainly be implemented at some point. In the short term it could make sense to assert not-PIC, so at least the failure is correct; but I'd be happy to review a PR, if you would like to take a crack at this. (Otherwise I don't know if anyone currently has the bandwidth to do this offhand, to assign this issue, though @bnjbvr please correct me if you have any plans for PIC as part of the M1 focus!)

[syrusakbary]

I'd love to create a PR, but I will need a bit of guidance since I'm not very familiar with the new backend architecture.
I think asserting the non-PIC settings upon creation is a good idea. Could you point what (module/file) should be responsible to handle that logic?

Thanks!

[bnjbvr]

I don't have any plans to work on this as part of the M1 focus, so anyone feel free to take this!

[akirilov-arm]

Issue #1657 seems to be related to this.

[cfallin]

@syrusakbary a top-level check/assert in aarch64/mod.rs where the AArch64Backend is created (in new_with_flags) would be a good short-term bandaid to fail more cleanly.

The actual GOT-reference implementation is probably not too bad either -- the main thing is to match the relocations and instructions that the linker expects, which one could get by building e.g. some C code and looking at the resulting .o.

[syrusakbary]

@cfallin thanks for the insights, I just did a quick check on the code.
Would you be open on refactoring new_with_flags so it returns a Result? (probably this change will also spill on other structs)

The main reason for that is that setting up a Backend would never cause an abort/exit, even if it's provided with a flag that exists, but is not supported in a specific backend (in that case it will return an Err).

[cfallin]

Sure, that sounds reasonable -- it's plausible there may be other "unsupported configuration errors" in the future. This would propagate a Result out to the ISA builder API, I guess, and then we can fail on that in the toplevel CLI.

[bjorn3]

Someone on the zulip got hit by this. Android requires position independent code and the dynamic linker refuses to run any executables with relocations in the .text section.