Fix dyncall() functions on i64 parameters in WASM_BIGINT mode #19689
Conversation
test/core/dyncall_specific.c
Outdated
| // Note that these would need to use BigInts if the file were built with | ||
| // -sWASM_BIGINT | ||
|
|
||
| #ifndef WASM_BIGINT |
There was a problem hiding this comment.
Can you flip the arms of the #if so this condition is in the positive?
test/test_core.py
Outdated
| # define DYNCALLS because this test does test calling them directly, and | ||
| # in WASM_BIGINT mode we do not enable them by default (since we can do | ||
| # more without them - we don't need to legalize) | ||
| args = list(args) + ['-sDYNCALLS=1', '-DWASM_BIGINT'] |
There was a problem hiding this comment.
I think you can just do args += here
There was a problem hiding this comment.
That modifies the input array, however, which can mean these accumulate, depending on how the test runner works? With the parallelism and all that I'd rather not make assumptions there.
There was a problem hiding this comment.
But you are using *arg .. which mean python is creating an array based on the inputs.. and it will be fresh each time I believe.
At the very least you can do args = args + ['-sDYNCALLS=1', '-DWASM_BIGINT'] since that also makes a copy.
There was a problem hiding this comment.
I checked now to make sure. It turns out that *args is always a tuple, not a list. So we do need the full syntax here, to turn the tuple into a list.
Instead, we could avoid *args, by replacing
'': [],
with
'': ([],),
and it is received as test_dyncall_specific(self, args), after which the code is simpler. Tradeoffs both ways, I don't feel strongly.
test/test_core.py
Outdated
| # define DYNCALLS because this test does test calling them directly, and | ||
| # in WASM_BIGINT mode we do not enable them by default (since we can do | ||
| # more without them - we don't need to legalize) | ||
| args = list(args) + ['-sDYNCALLS=1', '-DWASM_BIGINT'] |
With BigInt support we don't legalize, so a
'j'parameter is just a single value(a BigInt) and not two legalized i32s.
This is the last remaining useful fix in #19156 that is worth landing standalone.
The test now works in both BigInt and non-BigInt mode. To make that simple
I moved some of the expectations from the text output into assertions.