WebAssembly / memory64

Memory with 64-bit indexes
Other
220 stars 20 forks source link

LLVM: wasm64, atomics (exception handling) not really implemented #12

Closed ecs-deutschland-gmbh closed 3 years ago

ecs-deutschland-gmbh commented 3 years ago

Hi,

is anyone aware whether a working implementation of atomics instr. with target=wasm64 can be expected soon? EH depends on it and some more.

Just many thanks for any information regarding this. I could not quickly find a better place to ask.

FUNC __wasm_init_memory with target=wasm64: i32.const(67616) // of course NOT ;) i32.const(0) i32.const(1) i32.atomic.rmw.cmpxchg(0 align 2) if.op( extdatab @112)b1_len=11 b2_len=27 b1_stream_start=<4> b2_stream_start=11 stream_end=<26> depth=1 parent@0 i32.const(67616) i32.const(1) i64.const(-1) i32.atomic.wait(0 align 2) drop() else.op() i32.const(65536) i32.const(0) i32.const(8) memory.init( extdatab @48) i32.const(65544) i32.const(0) i32.const(16) memory.init( extdatab @80) i32.const(67616) i32.const(2) i32.atomic.store(0 align 2) i32.const(67616) i32.const(-1) atomic.notify(0 align 2) drop() end() data.drop() data.drop()

ecs-deutschland-gmbh commented 3 years ago

I just see this is related to void Writer::createInitMemoryFunction(), in wasm_ld. If anyone knows if/when this could theoretically get 'fixed', thanks for any info :;

tlively commented 3 years ago

cc @aardappel. Would you be able to update createInitMemoryFunction? Let me know if you have any questions about it.

aardappel commented 3 years ago

@tlively sure, I can have a look.

@ecs-commonA note that while wasm64 support in LLVM is intended to be fairly complete, it has not been tested extensively beyond the basic tests in LLVM (and now WABT, Binaryen etc). It still needs "end to end" toolchain tests of larger projects (which I am currently working on for Emscripten), and we don't even have an engine that can run it yet.

aardappel commented 3 years ago

Possible fix: https://reviews.llvm.org/D92348

ecs-deutschland-gmbh commented 3 years ago

Thanks to you guys for reacting so quickly and taking care of this. I can confirm wasm64 in LLVM is REALLY nice already (thx Wouter), exception handling does yet appear to have problems:

Compiling with -fwasm-exceptions and throwing/catching some works fine on wasm32 yet, but on wasm64 there is

fatal error: error in backend: Cannot select: intrinsic %llvm.wasm.extract.exception clang: error: clang frontend command failed with exit code 70 (use -v to see invocation) Ubuntu clang version 12.0.0-++20201103081716+a44b7322a27-1~exp1~20201103072359.373 Target: wasm64 Thread model: posix InstalledDir: /usr/bin clang: note: diagnostic msg:


PLEASE ATTACH THE FOLLOWING FILES TO THE BUG REPORT: Preprocessed source(s) and associated run script(s) are located at: clang: note: diagnostic msg: /tmp/6-3b31a5.cpp clang: note: diagnostic msg: /tmp/6-3b31a5.sh clang: note: diagnostic msg:


ecs-deutschland-gmbh commented 3 years ago

... I see, for wasm64 and EH to work also the $__wasm_init_tls func would need to be updated. This function also addresses a global (1) that most likely would need to be changed from i32 to i64 I think....

dschuff commented 3 years ago

-fwasm-exceptions should not be expected to work right now, as we are updating it (both in the toolchain and V8) to reflect changes in the wasm EH spec.

ecs-deutschland-gmbh commented 3 years ago

Hi,

re: Possible fix: https://reviews.llvm.org/D92348

I see the fix has landed but it appears incomplete to me, I think all instructions marked with '+' below shall be i64.const. The args for memory.init must all be i64 IMO (addr, start, size, all are 'memarg'). Please correct me if wrong.

+i32.const(67616) i32.const(0) i32.const(1) i32.atomic.rmw.cmpxchg(0 align 2) if.op( extdatab @112)b1_len=11 b2_len=27 b1_stream_start=<4> b2_stream_start=11 stream_end=<26> depth=1 parent@0

Regarding the new EH proposal (#3) I'm bit sad/disappointed, because I implemented #2 and found it's nice (LLVM win EH based, thx to Heejin). I really hope, #3 will be close to #2, at least for LLVM.

dschuff commented 3 years ago

+cc @aheejin If by "at least for LLVM" you mean will the LLVM IR be the same (i.e. using win EH), then I believe the answer is yes. It will just be translated to wasm differently.

aheejin commented 3 years ago

As @dschuff said, we are implementing recent changes (WebAssembly/exception-handling#137 and WebAssembly/exception-handling#143) in the spec, so please don't expect to work it in both 32 and 64 bits some months. The existing support was not tested for wasm64, but I think extending the support to wasm64 after finishing the new spec implementation shouldn't be hard.

aardappel commented 3 years ago

@ecs-commonA the patch has not been merged yet, and as you can see here https://github.com/WebAssembly/memory64/blob/master/proposals/memory64/Overview.md the memory.init only takes a single i64 arg.

ecs-deutschland-gmbh commented 3 years ago

Aardappel, thank you. I did not yet see this. I can confirm the wasm64 support is very good yet, even wasi-libc > dlmalloc etc. work perfectly after I removed some static asserts.

aardappel commented 3 years ago

@ecs-commonA https://reviews.llvm.org/D92348 has been merged, please let us know if its working for you once you get the chance.

ecs-deutschland-gmbh commented 3 years ago

Thanks Aardappel. Confirmed, function looks fine :)