Stream: git-wasmtime

Topic: wasmtime / issue #3026 aarch64: Implement TLS ELF GD Relo...

view this post on Zulip Wasmtime GitHub notifications bot (Jun 24 2021 at 10:22):

bjorn3 commented on issue #3026:

I suspect that this means that the first two relocations are silently not being done?

clif-util compile shows the relocations separate from the disassembly. I believe you need to use -p to show them at all though.

https://github.com/bytecodealliance/wasmtime/blob/03077e0de9bc5bb92623d58a1e5d78b828fd1634/cranelift/src/compile.rs#L71

view this post on Zulip Wasmtime GitHub notifications bot (Jun 24 2021 at 10:33):

afonso360 commented on issue #3026:

Oh, that's really nice, I didn't see that flag, here's what that outputs:

reloc_external: Aarch64TlsGdAdrPrel21 u1:0 0 at 36
reloc_external: Aarch64TlsGdAddLo12Nc u1:0 0 at 40
reloc_external: Call %ElfTlsGetAddr 0 at 44

So, it is doing the relocations, but i don't think those are the correct labels to be emitting?

view this post on Zulip Wasmtime GitHub notifications bot (Jun 24 2021 at 10:33):

afonso360 edited a comment on issue #3026:

Oh, that's really nice, I didn't see that flag, here's what that outputs:

reloc_external: Aarch64TlsGdAdrPrel21 u1:0 0 at 36
reloc_external: Aarch64TlsGdAddLo12Nc u1:0 0 at 40
reloc_external: Call %ElfTlsGetAddr 0 at 44

So, it is doing the relocations, but i don't think those are the correct offsets to be emitting?

view this post on Zulip Wasmtime GitHub notifications bot (Jun 24 2021 at 10:38):

bjorn3 commented on issue #3026:

Looks like it is off by 4 bytes if these offsets are in decimal.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 24 2021 at 10:39):

bjorn3 commented on issue #3026:

I think you need to move the relocs before the put4.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 24 2021 at 10:55):

afonso360 commented on issue #3026:

Yeah, that seems to be the right thing to do, we also put relocations before the call instruction and I didn't notice it before:

https://github.com/bytecodealliance/wasmtime/blob/81726207639cfb8ac2128135463291d7a55e2c9d/cranelift/codegen/src/isa/aarch64/inst/emit.rs#L2295-L2296

Unfortunately we get the same result, but I did another test, by compiling the following file with gcc and dumping relocations:

__thread int tl;

void bar(int b) {
  tl = b;
}

int foo() {
        return tl;
}

// aarch64-linux-gnu-gcc -O2 -fPIC -ftls-model=global-dynamic -mtls-dialect=trad -c ./reloc.c
// aarch64-linux-gnu-objdump -dr reloc.o

We get:

  10:   90000000        adrp    x0, 0 <bar>
                        10: R_AARCH64_TLSGD_ADR_PAGE21  tl
  14:   91000000        add     x0, x0, #0x0
                        14: R_AARCH64_TLSGD_ADD_LO12_NC tl
  18:   94000000        bl      0 <__tls_get_addr>
                        18: R_AARCH64_CALL26    __tls_get_addr
  1c:   d503201f        nop

Which is not the relocation we are emitting. I'm going to fix that now

view this post on Zulip Wasmtime GitHub notifications bot (Jun 24 2021 at 10:55):

bjorn3 commented on issue #3026:

Before linking there is:

    1730:       90000000        adrp    x0, 0 <_ZN67_$LT$mini_core..Option$LT$T$GT$$u20$as$u20$mini_core..PartialEq$GT$2ne17h6ea470a27df2424aE>
                        1730: R_AARCH64_TLSGD_ADR_PREL21        _ZN21mini_core_hello_world3TLS17hbf7bac7ea236bf4cE
    1734:       91000000        add     x0, x0, #0x0
                        1734: R_AARCH64_TLSGD_ADD_LO12_NC       _ZN21mini_core_hello_world3TLS17hbf7bac7ea236bf4cE
    1738:       94000000        bl      0 <__tls_get_addr>
                        1738: R_AARCH64_CALL26  __tls_get_addr
    173c:       d503201f        nop

which seems correct. However after linking it turns into:

    3f34:       580b0520        ldr     x0, 19fd8 <_GLOBAL_OFFSET_TABLE_+0x28>
    3f38:       d53bd041        mrs     x1, tpidr_el0
    3f3c:       8bfff9bd        .inst   0x8bfff9bd ; undefined
    3f40:       d503201f        nop

which has an undefined instruction.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 24 2021 at 10:56):

bjorn3 edited a comment on issue #3026:

Before linking there is:

    1730:       90000000        adrp    x0, 0 <_ZN67_$LT$mini_core..Option$LT$T$GT$$u20$as$u20$mini_core..PartialEq$GT$2ne17h6ea470a27df2424aE>
                        1730: R_AARCH64_TLSGD_ADR_PREL21        _ZN21mini_core_hello_world3TLS17hbf7bac7ea236bf4cE
    1734:       91000000        add     x0, x0, #0x0
                        1734: R_AARCH64_TLSGD_ADD_LO12_NC       _ZN21mini_core_hello_world3TLS17hbf7bac7ea236bf4cE
    1738:       94000000        bl      0 <__tls_get_addr>
                        1738: R_AARCH64_CALL26  __tls_get_addr
    173c:       d503201f        nop

which seems correct. However after linking it turns into:

    3f34:       580b0520        ldr     x0, 19fd8 <_GLOBAL_OFFSET_TABLE_+0x28>
    3f38:       d53bd041        mrs     x1, tpidr_el0
    3f3c:       8bfff9bd        .inst   0x8bfff9bd ; undefined
    3f40:       d503201f        nop

which has an undefined instruction.

Edit: missed your above comment.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 24 2021 at 11:05):

afonso360 commented on issue #3026:

I've updated it now, we should be emitting exactly what gcc is doing now.

By the way, can you tell me the steps to reproduce the test you are doing?

view this post on Zulip Wasmtime GitHub notifications bot (Jun 24 2021 at 11:13):

bjorn3 commented on issue #3026:

I am trying to run the test suite of cg_clif using TARGET_TRIPLE=aarch64-unknown-linux-gnu ./test.sh and the following patch:

diff --git a/Cargo.toml b/Cargo.toml
index ef68d7ee..f2bf9536 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -17,7 +17,7 @@ cranelift-jit = { git = "https://github.com/bytecodealliance/wasmtime.git", bran
 cranelift-object = { git = "https://github.com/bytecodealliance/wasmtime.git", branch = "main" }
 target-lexicon = "0.12.0"
 gimli = { version = "0.24.0", default-features = false, features = ["write"]}
-object = { version = "0.25.0", default-features = false, features = ["std", "read_core", "write", "archive", "coff", "elf", "macho", "pe"] }
+object = { version = "0.25.3", default-features = false, features = ["std", "read_core", "write", "archive", "coff", "elf", "macho", "pe"] }

 ar = { git = "https://github.com/bjorn3/rust-ar.git", branch = "do_not_remove_cg_clif_ranlib" }
 indexmap = "1.0.2"
@@ -25,13 +25,13 @@ libloading = { version = "0.6.0", optional = true }
 smallvec = "1.6.1"

 # Uncomment to use local checkout of cranelift
-#[patch."https://github.com/bytecodealliance/wasmtime.git"]
-#cranelift-codegen = { path = "../wasmtime/cranelift/codegen" }
-#cranelift-frontend = { path = "../wasmtime/cranelift/frontend" }
-#cranelift-module = { path = "../wasmtime/cranelift/module" }
-#cranelift-native = { path = "../wasmtime/cranelift/native" }
-#cranelift-jit = { path = "../wasmtime/cranelift/jit" }
-#cranelift-object = { path = "../wasmtime/cranelift/object" }
+[patch."https://github.com/bytecodealliance/wasmtime.git"]
+cranelift-codegen = { path = "../wasmtime/cranelift/codegen" }
+cranelift-frontend = { path = "../wasmtime/cranelift/frontend" }
+cranelift-module = { path = "../wasmtime/cranelift/module" }
+cranelift-native = { path = "../wasmtime/cranelift/native" }
+cranelift-jit = { path = "../wasmtime/cranelift/jit" }
+cranelift-object = { path = "../wasmtime/cranelift/object" }

 #[patch.crates-io]
 #gimli = { path = "../" }
diff --git a/example/mini_core_hello_world.rs b/example/mini_core_hello_world.rs
index 6570f2bf..3d2d0303 100644
--- a/example/mini_core_hello_world.rs
+++ b/example/mini_core_hello_world.rs
@@ -294,7 +294,7 @@ struct ExternTypeWrapper {

     #[cfg(all(not(jit), target_os = "linux"))]
     unsafe {
-        global_asm_test();
+        //global_asm_test();
     }

     // Both statics have a reference that points to the same anonymous allocation.
@@ -309,14 +309,14 @@ struct ExternTypeWrapper {
 }

 #[cfg(all(not(jit), target_os = "linux"))]
-global_asm! {
+/*global_asm! {
     "
     .global global_asm_test
     global_asm_test:
     // comment that would normally be removed by LLVM
     ret
     "
-}
+}*/

 #[repr(C)]
 enum c_void {
diff --git a/src/lib.rs b/src/lib.rs
index aed25a48..956113b2 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -289,7 +289,7 @@ fn build_isa(sess: &Session, backend_config: &BackendConfig) -> Box<dyn isa::Tar
                 cranelift_codegen::isa::lookup_variant(target_triple, variant).unwrap();
             // Don't use "haswell" as the default, as it implies `has_lzcnt`.
             // macOS CI is still at Ivy Bridge EP, so `lzcnt` is interpreted as `bsr`.
-            builder.enable("nehalem").unwrap();
+            //builder.enable("nehalem").unwrap();
             builder
         }
     };

view this post on Zulip Wasmtime GitHub notifications bot (Jun 24 2021 at 11:19):

bjorn3 commented on issue #3026:

mini_core_hello_world runs fine now!

view this post on Zulip Wasmtime GitHub notifications bot (Jun 24 2021 at 11:24):

bjorn3 commented on issue #3026:

libstd compiles fine too with a small patch to the build system to fix cross-compiling. (I recently rewrote it from shell scripts to rust) std_example doesn't compile yet. I think due to a select.i128.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 24 2021 at 11:25):

afonso360 commented on issue #3026:

mini_core_hello_world runs fine now!
That's awesome!

Although it would be nice if we had a more definitive test that would catch issues like this (Wrong relocations).
cc: @cfallin Any ideas on this?

@bjorn3 I'm having some issues running cg_clif's test suite, is there anywhere I can ask for a bit more guidance, so we don't pollute this PR?

view this post on Zulip Wasmtime GitHub notifications bot (Jun 24 2021 at 11:26):

afonso360 edited a comment on issue #3026:

mini_core_hello_world runs fine now!
That's awesome!

Although it would be nice if we had a more definitive test that would catch issues like this (Wrong relocations).
cc: @cfallin Any ideas on this?

@bjorn3 I'm having some issues running cg_clif's test suite, is there anywhere I can ask for a bit more guidance, so we don't pollute this PR?

I think due to a select.i128.

I guess that's next on the i128 impl list

view this post on Zulip Wasmtime GitHub notifications bot (Jun 24 2021 at 11:27):

afonso360 edited a comment on issue #3026:

mini_core_hello_world runs fine now!

That's awesome!

Although it would be nice if we had a more definitive test that would catch issues like this (Wrong relocations).
cc: @cfallin Any ideas on this?

@bjorn3 I'm having some issues running cg_clif's test suite, is there anywhere I can ask for a bit more guidance, so we don't pollute this PR?

I think due to a select.i128.

I guess that's next on the i128 impl list

view this post on Zulip Wasmtime GitHub notifications bot (Jun 24 2021 at 11:27):

bjorn3 commented on issue #3026:

When commenting out all i128 tests in std_example, it runs fine too.

@bjorn3 I'm having some issues running cg_clif's test suite, is there anywhere I can ask for a bit more guidance, so we don't pollute this PR?

Maybe zulip? You can PM me on either the rust-lang or bytecodealliance zulip.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 24 2021 at 18:22):

cfallin commented on issue #3026:

Although it would be nice if we had a more definitive test that would catch issues like this (Wrong relocations).
cc: @cfallin Any ideas on this?

Yeah, I agree this would be nice; we don't really have a good answer for this right now. Probably the best answer would be an end-to-end test of some sort that uses the relocation and executes on the target platform. In the absence of that, we could at least print relocations in the VCode output and check them (as a golden output) in compile filetests. I'd be happy to review something for that if you would like.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 24 2021 at 18:48):

afonso360 commented on issue #3026:

Oops, it was fixed, but i forgot to mark it as resolved. Thanks

Probably the best answer would be an end-to-end test of some sort that uses the relocation and executes on the target platform. In the absence of that, we could at least print relocations in the VCode output and check them (as a golden output) in compile filetests. I'd be happy to review something for that if you would like.

Hmm, its not something i want to pick up right now, but let me open an issue with this, and maybe someone does

Last updated: Dec 23 2024 at 12:05 UTC