-sWASM_BIGINT generates mutually incompatible setValue/getValue() impls

[sgbeal]

emcc (Emscripten gcc/clang-like replacement + linker emulating GNU ld) 3.1.13 (531257621816c200bc7c3be53129494afd029aec)
clang version 15.0.0 (https://github.com/llvm/llvm-project 5c6ed60c517c47b25b6b25d8ac3666d0e746b0c3)
Target: wasm32-unknown-emscripten
Thread model: posix

When building with -sWASM_BIGINT, the impls of getValue() and setValue() are inconsistent in how they handle i64 values:

  function getValue(ptr, type = 'i8') {
...
        case 'i64': return Number(HEAP64[((ptr)>>3)]);
...
    }

  function setValue(ptr, value, type = 'i8') {
...
        case 'i64': HEAP64[((ptr)>>3)] = BigInt(value); break;
...
    }

Note the Number(...) wrapper around getValue()'s impl.

That discrepancy leads to i64 return values working fine but i64 output pointer parameters (which are necessarily fetched via getValue()) to be silently mangled. That, in turn, leads to developers spending half a day scratching their head wondering why their output parameters behave differently than their return values ;). For example, from C:

int64_t foo( int64_t * arg ){
  *arg = 9007199254740991 /* == JS Number.MAX_SAFE_INTEGER */ * 2;
  return *arg;
}

will result, JS-side, in two different values for the return result and the output pointer.

The following workaround "works for me," but only because i use --post-js and thus have code-/scope-level access to overwrite getValue():

    if(Module['HEAP64']){
        const oldGetVal = getValue;
        getValue = Module['getValue'] = function(ptr,type='i8'){
            return ('i64'===type)
                ? Module['HEAP64'][ptr>>3]
                : oldGetVal(ptr,type);
        };
    }

That "works for me" with no issues so far, but having to overwrite getValue() that way is less than ideal and completely infeasible for clients who don't use --post-js to inject such a kludge.

Please consider removing the Number(...) wrapper from the i64 part of the getValue() impl. Whether or not it should be simply removed or replaced with BigInt(...) is unclear. Simply removing it works for me, but that admittedly somewhat surprises me.

[kripken]

Good catch. I agree, we should remove that Number cast. A PR would be welcome!

[sgbeal]

A PR would be welcome!

As a long-time developer for the Fossil SCM i avoid git like the plague, doing nothing with it more advanced than cloning, and wouldn't even know where to begin to create a PR ;).

[kripken]

Heh, ok @sgbeal 😄

I took a minute to look into this. Looks like getValue just uses {{{ makeGetValue }}}, so fixing that (removing the cast to Number there on a bigint) might fix a more general bug. That is, right now with WASM_BIGINT reading an i64 returns a JS number, silently. I pushed a job to CI to see if any tests break by changing that, and looks like other.test_memory64 fails, as well as many wasm64 tests like wasm64.test_zero_struct_varargs. So the fix isn't that simple, and someone would need to look into the details more - we seem to be depending on the automatic conversion to Number somewhere.

cc @sbc100

[sgbeal]

we seem to be depending on the automatic conversion to Number somewhere.

i was afraid that might be the case. Thank you for looking into, though! For the time being my local workaround is working just fine, but i don't have an existing code base which it risks breaking. That local fix is only possible, though, for people who use --post-js because that's the only way that they can overwrite the scope-local copy of getValue. Thus it "really should" be fixed upstream, even if doing so requires (due to backwards compatibility constraints) a compile-time flag or something similar to enable the bigint-friendly behavior.

[sbc100]

I think the correct fix is in #17341

[merceyz]

Seems like a duplicate of #16272

[sbc100]

Ah yes, thanks for pointing that out. Closing as dup of #16272.