Skip to content

Fix splitI64 so it can be used to send values from JS libraries into wasm #19575

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Jun 12, 2023
Merged

Fix splitI64 so it can be used to send values from JS libraries into wasm #19575

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
37d2a42
fix
kripken Jun 9, 2023
5f530ca
Merge remote-tracking branch 'origin/main' into js2wasm.i64_param
kripken Jun 12, 2023
561f155
signed
kripken Jun 12, 2023
9f67e91
test -42 as well
kripken Jun 12, 2023
0da5609
TODOs
kripken Jun 12, 2023
c979f28
test
kripken Jun 12, 2023
abac05f
undo and use splitI64
kripken Jun 12, 2023
d45e7ce
fix.bigint
kripken Jun 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions src/parseTools.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -885,11 +885,16 @@ function receiveI64ParamAsI53Unchecked(name) {
return `var ${name} = convertI32PairToI53(${name}_low, ${name}_high);`;
}

function sendI64Argument(low, high) {
// Given the name of a variable containing an unsigned 53-bit integer in a JS
// Number or BigInt, send it as a 64-bit argument in a call to wasm. This will
// legalize the argument if necessary (i.e., split it into two i32 arguments,
// if legalization is used).
function sendU53ToI64Param(name) {
// TODO: In ASSERTIONS mode add range checks here.
if (WASM_BIGINT) {
return 'BigInt(low) | (BigInt(high) << 32n)';
return `BigInt(${name})`;
}
return low + ', ' + high;
return `(${name} | 0), (${name} / (2**32))`;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like it should live in src/library_int53.js?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's such a tiny code fragment that I think it can stay in the helper function here? Pulling it out would then mean writing deps for it etc.

I do agree that if we add checking then that would be pulled into a library function, as that is no longer trivial.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a comment saying that this function logically is in the same family as the functions in src/library_int53.js

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code in library_int53.js uses expanded out numbers rather than 2**32.

You could do that here by putting the 2**32 part inside a ${} ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, why are we expanding them there? It's larger to expand. I think we should 2**32ify them?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well we should at least be consistent. I don't really care which form we use. If you want to use the expanded form in the PR and followup with change to the runtime multiple everywhere that is also fine be me.

}

// Any function called from wasm64 may have bigint args, this function takes
Expand Down
6 changes: 6 additions & 0 deletions test/core/js_library_i64_params.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
#include <assert.h>
#include <emscripten.h>
#include <stdio.h>

#define MAX_SAFE_INTEGER (1ll<<53)
Expand All @@ -8,6 +9,11 @@

int64_t jscall(int64_t arg);

EMSCRIPTEN_KEEPALIVE
void called_from_js(uint64_t arg) {
printf("called_from_js with: %lld\n", arg);
}

void check_ok(int64_t val) {
printf("checking: %lld\n", val);
int64_t rtn = jscall(val);
Expand Down
3 changes: 3 additions & 0 deletions test/core/js_library_i64_params.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@ TestLibrary = {
jscall: function({{{ defineI64Param('foo') }}}) {
{{{ receiveI64ParamAsI53('foo', `(err('overflow'), ${makeReturn64('42')})`) }}}
err('js:got: ' + foo);

_called_from_js({{{ sendU53ToI64Param("foo") }}});

if (foo < 0)
var rtn = Math.ceil(foo / 2);
else
Expand Down
5 changes: 5 additions & 0 deletions test/core/js_library_i64_params.out
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,25 +1,30 @@
checking: 42
js:got: 42
called_from_js with: 42
js:returning: 21
got: 21
expected: 21
checking: 4503599627370496
js:got: 4503599627370496
called_from_js with: 4503599627370496
js:returning: 2251799813685248
got: 2251799813685248
expected: 2251799813685248
checking: -4503599627370496
js:got: -4503599627370496
called_from_js with: -4503599627370496
js:returning: -2251799813685248
got: -2251799813685248
expected: -2251799813685248
checking: 9007199254740992
js:got: 9007199254740992
called_from_js with: 9007199254740992
js:returning: 4503599627370496
got: 4503599627370496
expected: 4503599627370496
checking: -9007199254740992
js:got: -9007199254740992
called_from_js with: -9007199254740992
js:returning: -4503599627370496
got: -4503599627370496
expected: -4503599627370496
Expand Down