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.

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

Metering does not work with LLVM backend on aarch64 #2227

Closed
bobrik opened this issue Apr 3, 2021 · 6 comments
Closed

Metering does not work with LLVM backend on aarch64 #2227

bobrik opened this issue Apr 3, 2021 · 6 comments
Assignees
Labels
bug Something isn't working 📦 lib-compiler-llvm About wasmer-compiler-llvm 📦 lib-middlewares About wasmer-middlewares 🕵️ needs investigation The issue/PR needs further investigation priority-high High priority issue
Milestone

Comments

@bobrik
Copy link

bobrik commented Apr 3, 2021

Describe the bug

Running with metering middleware on arm64 produces the following error:

Codegen("unknown ELF relocation 283")

Steps to reproduce

The simplest way to reproduce is to replace Cranelift with LLVM in tests:

$ git diff src/metering.rs
diff --git a/lib/middlewares/src/metering.rs b/lib/middlewares/src/metering.rs
index 4f968ed70..fd45faf13 100644
--- a/lib/middlewares/src/metering.rs
+++ b/lib/middlewares/src/metering.rs
@@ -283,7 +283,7 @@ mod tests {
     use super::*;

     use std::sync::Arc;
-    use wasmer::{imports, wat2wasm, CompilerConfig, Cranelift, Module, Store, JIT};
+    use wasmer::{imports, wat2wasm, CompilerConfig, Cranelift, Module, Store, JIT, LLVM};

     fn cost_function(operator: &Operator) -> u64 {
         match operator {
@@ -312,7 +312,7 @@ mod tests {
     #[test]
     fn get_remaining_points_works() {
         let metering = Arc::new(Metering::new(10, cost_function));
-        let mut compiler_config = Cranelift::default();
+        let mut compiler_config = LLVM::default();
         compiler_config.push_middleware(metering.clone());
         let store = Store::new(&JIT::new(compiler_config).engine());
         let module = Module::new(&store, bytecode()).unwrap();
@@ -357,7 +357,7 @@ mod tests {
     #[test]
     fn set_remaining_points_works() {
         let metering = Arc::new(Metering::new(10, cost_function));
-        let mut compiler_config = Cranelift::default();
+        let mut compiler_config = LLVM::default();
         compiler_config.push_middleware(metering.clone());
         let store = Store::new(&JIT::new(compiler_config).engine());
         let module = Module::new(&store, bytecode()).unwrap();

Test output is then:

running 2 tests
test metering::tests::set_remaining_points_works ... FAILED
test metering::tests::get_remaining_points_works ... FAILED

failures:

---- metering::tests::set_remaining_points_works stdout ----
thread 'metering::tests::set_remaining_points_works' panicked at 'called `Result::unwrap()` on an `Err` value: Codegen("unknown ELF relocation 283")', lib/middlewares/src/metering.rs:363:54

---- metering::tests::get_remaining_points_works stdout ----
thread 'metering::tests::get_remaining_points_works' panicked at 'called `Result::unwrap()` on an `Err` value: Codegen("unknown ELF relocation 283")', lib/middlewares/src/metering.rs:318:54
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    metering::tests::get_remaining_points_works
    metering::tests::set_remaining_points_works

test result: FAILED. 0 passed; 2 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.01s

Ideally we should probably run tests on all supported backends.

Expected behavior

Tests don't fail, no new errors on aarch64 appear.

Actual behavior

Tests fail, people question whether this is supposed to work to begin with.

@bobrik bobrik added the bug Something isn't working label Apr 3, 2021
@bobrik
Copy link
Author

bobrik commented Apr 3, 2021

The error comes from here: https://github.com/wasmerio/wasmer/blob/1.0.2/lib/compiler-llvm/src/object_file.rs#L170-L191.

ELF relocation 283 is R_AARCH64_CALL26, which is indeed not handled.

@bobrik
Copy link
Author

bobrik commented Apr 3, 2021

Looks like more complex apps fail with unknown relocation even without metering middleware enabled.

@nlewycky
Copy link
Contributor

nlewycky commented Apr 3, 2021

Aarch64 isn't supported with llvm + engine-jit, due to the lack of support for reading aarch64 relocations. Can you use the native engine?

@bobrik
Copy link
Author

bobrik commented Apr 3, 2021

Can you use the native engine?

Tried with #1928 workaround and it works with native engine, thanks for the pointer!

Should the docs be updated then? Currently there's no differentiation between native and JIT, and aarch64 is marked as ✅.

image

@Hywan Hywan added 📦 lib-compiler-llvm About wasmer-compiler-llvm 📦 lib-middlewares About wasmer-middlewares labels Jul 17, 2021
@Amanieu Amanieu added the priority-high High priority issue label Oct 20, 2021
@syrusakbary
Copy link
Member

I believe this might be solved by #2599.

@wchaudry wchaudry added this to the Wasmer Runtime 2.x milestone Oct 21, 2021
@syrusakbary syrusakbary added the 🕵️ needs investigation The issue/PR needs further investigation label Nov 3, 2021
@Amanieu Amanieu assigned ptitSeb and Amanieu and unassigned Amanieu and ptitSeb Nov 5, 2021
@Amanieu
Copy link
Contributor

Amanieu commented Nov 5, 2021

This is fixed.

@Amanieu Amanieu closed this as completed Nov 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working 📦 lib-compiler-llvm About wasmer-compiler-llvm 📦 lib-middlewares About wasmer-middlewares 🕵️ needs investigation The issue/PR needs further investigation priority-high High priority issue
Projects
None yet
Development

No branches or pull requests

7 participants