-
Notifications
You must be signed in to change notification settings - Fork 917
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
Update wasi-libc to SDK 20 #3661
Conversation
Hrm, bit confused since this test works for me locally just fine. |
What's the benefit here? The page size is fixed for WebAssembly at 64kB, so I don't see why we need a library call. (This is different for |
Aside from debating the |
Yes, seems fine by me. I'm a bit careful with updating these libraries because they may introduce new bugs, but we have to do it from time to time. |
@QuLogic could you please rebase this branch against the latest |
func Getpagesize() int { | ||
// per upstream | ||
return 65536 | ||
return libc_getpagesize() | ||
} |
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.
EDIT: oops I see I already commented on this.
Probably unnecessary, the WebAssembly page size is fixed at 64kB. I don't really see a benefit of calling a libc function. (This is different on an architecture like arm64 which does in fact have multiple page sizes that can only be known at runtime).
On the other hand, it probably won't increase binary size by much either so 🤷
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.
Mostly for consistency, and to limit hard-coded numbers. I have no idea whether wasi will evolve in some direction as to not be 64KiB, but if it does, then that information will be encoded in the SDK instead of here.
068153c
to
23b6530
Compare
Gave it a rebase. I forget which build failed, and I think logs might have gone, so we'll see if it's okay now. |
I just realized there are some |
The version 17 SDK adds `getpagesize`, so use it instead of hardcoding a number (even if their implementation is _also_ a hardcoded number.)
23b6530
to
9076f97
Compare
This should be good to go now. |
Thanks @QuLogic now merging. |
The version 17 SDK adds
getpagesize
, so use it instead of hardcoding a number (even if their implementation is also a hardcoded number.)