-
Notifications
You must be signed in to change notification settings - Fork 997
WIP: Add initial syscall/js.copyBytesToJS support #702
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.
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
Conversation
It's working in initial testing, but could likely be improved somewhat. :)
|
Any chance of an automated test that exercises this? |
|
Good idea. Probably best to wait a few days, to see if it works ok with the further WebGL stuff. There's every chance it'll need tweaking/adjusting (etc). 😉 |
| // Originally copied from upstream Go project, then modified: | ||
| // https://github.com/golang/go/blob/3f995c3f3b43033013013e6c7ccc93a9b1411ca9/misc/wasm/wasm_exec.js#L404-L416 | ||
| // param4 and param5 (below) are likely "length" variables of some sort | ||
| "syscall/js.copyBytesToJS": (ret_addr, dest_addr, source_addr, param4, param5) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The calling convention changes have changed this function conceptually to the following:
func copyBytesToJS(returnValue *struct{int; bool}, dst *ref, srcPtr *byte, srcLen, srcCap uintptr)The following three rules apply to it:
- large return values are passed as a pointer-to-struct and prepended to the parameter list
int64/uint64(thereftype) are passed as a pointer-to-value- slices are split into 3 parts: the pointer to the underlying array, the length, and the capacity.
So yes, param4 is len and param5 is cap.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I'll update things accordingly over the next few days. 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any chance you can take a look at this PR? The next release is getting close.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, I can make time this weekend. 😄
|
Is this PR still valid? if so, any chance of it being able to bring it back to life? |
|
No idea, haven't touched Go at all in months. Might as well close it, and if there turns out to be need for it in future it can be re-opened or re-submitted. |
|
@deadprogram Looking back at this now, it turns out this PR is still needed after all. 😉 |
|
Wasn't able to re-open this PR for some unknown reason (GitHub technical issue?), so recreated it in #933. |
It's working in initial testing, but could likely be improved somewhat. 😄