fathominfo / delphy-web

Other
5 stars 0 forks source link

step counter needs to be a long, not an int: #20

Closed mark-fathom closed 2 weeks ago

mark-fathom commented 3 weeks ago
Screen Shot 2024-10-31 at 12 01 23 PM
mark-fathom commented 3 weeks ago

@patvarilly I might your help solving this one. Looking into where this happens, it looks like the wasm file downcasts a 64bit counter for steps to 32 bit? Here's what the compiled wasm is doing in run.getStep() -> Module._delphy_run_get_step:

Screen Shot 2024-11-01 at 11 13 33 AM

Note last line: i32.wrap_i64 . That sounds like a conversion to someone who doesn't speak C++.

Looking in delphy core delphy_wasm.cpp, delphy_run_get_step explicitly returns int64_t, as does the step function in run.h , where the variable step_ is also an int64_t. So maybe this downcasting is some emscripten magic?

mark-fathom commented 3 weeks ago

Looking at some emscripten documentation, maybe we want to try MEMORY64=1?

MEMORY64
The “architecture” to compile for. 0 means the default wasm32, 
1 is the full end-to-end wasm64 mode, and 2 is wasm64 for 
clang/lld but lowered to wasm32 in Binaryen (such that it can 
run on wasm32 engines, while internally using i64 pointers). 
Assumes WASM_BIGINT.

Other pertinent options might be WASM_BIGINT and ERROR_ON_WASM_CHANGES_AFTER_LINK.

patvarilly commented 2 weeks ago

Ahhh!! Foiled by WebAssembly again. Compiling with -sWASM_BIGINT solves the issue. A good explanation of the problem is here. Basically, since WebAssembly doesn't want to automatically destroy 64-bit ints by putting them into doubles, the default way of handling int64's is crazy (it splits them into 32-bit high and low words, and the high word is returned out of band!). Since JS now has BigInts almost everywhere, WASM can use that instead, but you need to enable a flag (WASM_BIGINT) to do so. Long story short: a quick recompile with that flag (coming) should fix the problem, and the only quirk is that the stepsHists array (and any other place where 64-bit quantities were supposed to show up) will be JS BigInts.

mark-fathom commented 2 weeks ago

Thank you!