wasmtime / issue #2907 Cranelift: implement PIC relocatio... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / issue #2907 Cranelift: implement PIC relocatio...

Wasmtime GitHub notifications bot (May 15 2021 at 05:37):

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:
https://github.com/bytecodealliance/wasmtime/blob/207da989acf5c6d828dd5d1704c05c0d01b1dead/cranelift/codegen/src/isa/aarch64/inst/emit.rs#L2440-L2462

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

https://github.com/bytecodealliance/wasmtime/blob/e676589b0c6e8228c421e18249d4635eb6c4bbe4/cranelift/codegen/src/isa/x64/inst/emit.rs#L2350-L2387

Wasmtime GitHub notifications bot (May 15 2021 at 23:44):

syrusakbary commented on issue #2907:

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 which file is responsible for that logic?

Thanks!

Wasmtime GitHub notifications bot (May 16 2021 at 22:59):

syrusakbary edited a comment on issue #2907:

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!

Wasmtime GitHub notifications bot (May 17 2021 at 09:04):

bnjbvr commented on issue #2907:

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

Wasmtime GitHub notifications bot (May 17 2021 at 16:45):

akirilov-arm commented on issue #2907:

Issue #1657 seems to be related to this.

Wasmtime GitHub notifications bot (May 17 2021 at 16:45):

akirilov-arm labeled issue #2907:

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:
https://github.com/bytecodealliance/wasmtime/blob/207da989acf5c6d828dd5d1704c05c0d01b1dead/cranelift/codegen/src/isa/aarch64/inst/emit.rs#L2440-L2462

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

https://github.com/bytecodealliance/wasmtime/blob/e676589b0c6e8228c421e18249d4635eb6c4bbe4/cranelift/codegen/src/isa/x64/inst/emit.rs#L2350-L2387

Wasmtime GitHub notifications bot (May 21 2021 at 20:40):

cfallin commented on issue #2907:

@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.

Wasmtime GitHub notifications bot (May 21 2021 at 21:17):

syrusakbary commented on issue #2907:

@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).

Wasmtime GitHub notifications bot (May 21 2021 at 22:10):

cfallin commented on issue #2907:

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.

Wasmtime GitHub notifications bot (Dec 27 2021 at 20:56):

bjorn3 commented on issue #2907:

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.

Wasmtime GitHub notifications bot (Feb 15 2023 at 23:19):

jameysharp closed issue #2907:

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:
https://github.com/bytecodealliance/wasmtime/blob/207da989acf5c6d828dd5d1704c05c0d01b1dead/cranelift/codegen/src/isa/aarch64/inst/emit.rs#L2440-L2462

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

https://github.com/bytecodealliance/wasmtime/blob/e676589b0c6e8228c421e18249d4635eb6c4bbe4/cranelift/codegen/src/isa/x64/inst/emit.rs#L2350-L2387

Last updated: Nov 22 2024 at 17:03 UTC