Wasmtime GitHub notifications bot (Jun 23 2022 at 21:08): Wasmtime GitHub notifications bot (Jun 23 2022 at 21:08):alexcrichton commented on issue #4306:
I just went down a _really_ long rabbit hole to justify the addition of https://github.com/bytecodealliance/wasmtime/pull/4306/commits/6a02949a246558d9a4169b7c7e2d06da729ef27e here, namely adding
.cfi_def_cfa_offset 0to all thewasmtime_fiber_startfunctions. Long story short this assembly file:my_function: .cfi_startproc simple .cfi_rel_offset w29, -16 ret .cfi_endproc foo: .cfi_startproc .cfi_def_cfa_offset 16 ret .cfi_endprocis a massive simplification of the
wasmtime_fibercrate. Themy_functionhere is equivalent towasmtime_fiber_start. When compiling this with clang and looking at the unwind tables though:$ clang -c bad.s -o bad.o2 && objdump -W bad.o2 ... 0000003c 0000000000000010 00000018 FDE cie=00000028 pc=0000000000000000..0000000000000004 DW_CFA_offset: r29 (x29) at cfa-32 DW_CFA_nopHere the
cfa-32is unexpected, that's supposed to be-16. This appears to be affected by the.cfi_def_cfa_offset 16directive in thefoofunction. The reason this only showed up on CI is because oddly enough the bug only arose when incremental compliation was turned off. This shuffled around CGUs such that the global asm bits added here ended up getting shoved into a CGU with another function, which internally in LLVM had this.cfi_def_cfa_offsetdirective.Overall I don't know why there's this "global state" here or what this offset is. I suppose the word "global" in "global_asm" should have tipped me off. Anyway my hope here was that with the addition of
.cfi_def_cfa_offset 0directives we can work around this to "reset" the directive at the start of the function. Afterwards things appear to work.This is probably a bug in LLVM though. The same assembly file above going through gcc yields:
$ gcc -c bad.s -o bad.o2 && objdump -W bad.o2 bad.o2: file format elf64-littleaarch64 Contents of the .eh_frame section: 00000000 0000000000000010 00000000 CIE Version: 1 Augmentation: "zR" Code alignment factor: 4 Data alignment factor: -8 Return address column: 30 Augmentation data: 1b DW_CFA_offset: r29 (x29) at cfa-16 DW_CFA_nop 00000014 0000000000000010 00000018 FDE cie=00000000 pc=0000000000000000..0000000000000004 DW_CFA_nop DW_CFA_nop DW_CFA_nop ...Here it looks like gcc folded the FDE entry directly into the CIE because only one FDE references the CIE (I guess? unsure?). In any case the
cfa-16is what we want, unlike thecfa-32which is what clang produces. I don't really have the energy though to file an issue on LLVM at this time though.
Wasmtime GitHub notifications bot (Jun 27 2022 at 23:14):cfallin commented on issue #4306:
This unfortunately seems to have broken compilation on macOS/aarch64 with latest stable Rust (1.61.0):
Compiling num-rational v0.2.4 Compiling wasmtime-fiber v0.39.0 (/Users/cfallin/work/wasmtime2/crates/fiber) Compiling proptest v1.0.0 Compiling egg v0.6.0 Compiling capstone-sys v0.13.0 Compiling shuffling-allocator v1.1.2 Compiling cranelift-bforest v0.86.0 (/Users/cfallin/work/wasmtime2/cranelift/bforest) Compiling unicode-linebreak v0.1.2 error: macro expansion ends with an incomplete expression: expected expression --> crates/fiber/src/unix/aarch64.rs:41:5 | 25 | macro_rules! paciasp { () => (); } | -- in this macro arm ... 41 | paciasp!(), | ^^^^^^^^^^ expected expression | = note: the macro call doesn't expand to an expression, but it can expand to a statement help: add `;` to interpret the expansion as a statement | 41 | paciasp!();, | + error: macro expansion ends with an incomplete expression: expected expression --> crates/fiber/src/unix/aarch64.rs:42:5 | 23 | macro_rules! cfi_window_save { () => (); } | -- in this macro arm ... 42 | cfi_window_save!(), | ^^^^^^^^^^^^^^^^^^ expected expression | = note: the macro call doesn't expand to an expression, but it can expand to a statement help: add `;` to interpret the expansion as a statement | 42 | cfi_window_save!();, | + error: macro expansion ends with an incomplete expression: expected expression --> crates/fiber/src/unix/aarch64.rs:78:5 | 26 | macro_rules! autiasp { () => (); } | -- in this macro arm ... 78 | autiasp!(), | ^^^^^^^^^^ expected expression | = note: the macro call doesn't expand to an expression, but it can expand to a statement help: add `;` to interpret the expansion as a statement | 78 | autiasp!();, | + error: macro expansion ends with an incomplete expression: expected expression --> crates/fiber/src/unix/aarch64.rs:79:5 | 23 | macro_rules! cfi_window_save { () => (); } | -- in this macro arm ... 79 | cfi_window_save!(), | ^^^^^^^^^^^^^^^^^^ expected expression | = note: the macro call doesn't expand to an expression, but it can expand to a statement help: add `;` to interpret the expansion as a statement | 79 | cfi_window_save!();, | + error: macro expansion ends with an incomplete expression: expected expression --> crates/fiber/src/unix/aarch64.rs:117:5 | 24 | macro_rules! pacia1716 { () => (); } | -- in this macro arm ... 117 | pacia1716!(), | ^^^^^^^^^^^^ expected expression | = note: the macro call doesn't expand to an expression, but it can expand to a statement help: add `;` to interpret the expansion as a statement | 117 | pacia1716!();, | + error: macro expansion ends with an incomplete expression: expected expression --> crates/fiber/src/unix/aarch64.rs:148:5 | 23 | macro_rules! cfi_window_save { () => (); } | -- in this macro arm ... 148 | cfi_window_save!(), | ^^^^^^^^^^^^^^^^^^ expected expression | = note: the macro call doesn't expand to an expression, but it can expand to a statement help: add `;` to interpret the expansion as a statement | 148 | cfi_window_save!();, | + error: could not compile `wasmtime-fiber` due to 6 previous errors warning: build failed, waiting for other jobs to finish...I haven't dug into this at all but I can play with it more or test changes tomorrow if needed to help...
Wasmtime GitHub notifications bot (Jun 27 2022 at 23:20):cfallin commented on issue #4306:
(N.B.: all errors from
./ci/run-tests.sh; justcargo buildfor wasmtime-cli works fine still, I guess because it's not async?)The following patch seems to push it further:
diff --git a/crates/fiber/src/unix/aarch64.rs b/crates/fiber/src/unix/aarch64.rs index a0b4201b9..28b257974 100644 --- a/crates/fiber/src/unix/aarch64.rs +++ b/crates/fiber/src/unix/aarch64.rs @@ -20,10 +20,10 @@ cfg_if::cfg_if! { if #[cfg(target_os = "macos")] { - macro_rules! cfi_window_save { () => (); } - macro_rules! pacia1716 { () => (); } - macro_rules! paciasp { () => (); } - macro_rules! autiasp { () => (); } + macro_rules! cfi_window_save { () => (""); } + macro_rules! pacia1716 { () => (""); } + macro_rules! paciasp { () => (""); } + macro_rules! autiasp { () => (""); } } else { macro_rules! cfi_window_save { () => (".cfi_window_save\n"); } macro_rules! pacia1716 { () => ("pacia1716\n"); }unfortunately it then bottoms out at:
error: unknown AArch64 fixup kind! | note: instantiated into assembly here --> <inline asm>:53:9 | 53 | adr x17, _wasmtime_fiber_start |
Wasmtime GitHub notifications bot (Jun 27 2022 at 23:26):alexcrichton commented on issue #4306:
Oh oops this reminds me that I meant to poke you before I merged this and see if it compiles on your mac and completely forgot. I'll dig in tomorrow but feel free to revert in the meantime.
Wasmtime GitHub notifications bot (Jun 27 2022 at 23:35):cfallin commented on issue #4306:
No worries, as long as we dig in (I'm happy to help test) I don't think there's a need to revert immediately. Hopefully something simple to fix...
Wasmtime GitHub notifications bot (Jun 27 2022 at 23:46):cfallin commented on issue #4306:
OK, the following is sufficient to get everything to build and pass tests again (conditionalizing this just for macOS left as exercise for the reader):
diff --git a/crates/fiber/src/unix/aarch64.rs b/crates/fiber/src/unix/aarch64.rs index a0b4201b9..f242a0bc1 100644 --- a/crates/fiber/src/unix/aarch64.rs +++ b/crates/fiber/src/unix/aarch64.rs @@ -20,10 +20,10 @@ cfg_if::cfg_if! { if #[cfg(target_os = "macos")] { - macro_rules! cfi_window_save { () => (); } - macro_rules! pacia1716 { () => (); } - macro_rules! paciasp { () => (); } - macro_rules! autiasp { () => (); } + macro_rules! cfi_window_save { () => (""); } + macro_rules! pacia1716 { () => (""); } + macro_rules! paciasp { () => (""); } + macro_rules! autiasp { () => (""); } } else { macro_rules! cfi_window_save { () => (".cfi_window_save\n"); } macro_rules! pacia1716 { () => ("pacia1716\n"); } @@ -112,7 +112,8 @@ asm_func!( .cfi_startproc hint #34 // bti c sub x16, x0, #16 - adr x17, ", asm_sym!("wasmtime_fiber_start"), " + adrp x17, ", asm_sym!("wasmtime_fiber_start"), "@PAGE + add x17, x17, ", asm_sym!("wasmtime_fiber_start"), "@PAGEOFF ", pacia1716!(), "basically I figured that the linker is probably putting the functions in different sections now (whereas the
.Sfile put everything contiguously in one section) so we can't rely on the 21-bitadrrelocs now, we need the +/- 4GiB range ofadrp+ an imm12. I'd actually be surprised if this doesn't silently (or just in a delayed way) become an issue on aarch64-linux too, depending on the link order one happens to get...
Wasmtime GitHub notifications bot (Jun 28 2022 at 01:34):alexcrichton commented on issue #4306:
Ok I ended up cooking up https://github.com/bytecodealliance/wasmtime/pull/4341 which I'm hoping will work
Last updated: Dec 23 2024 at 13:07 UTC