Skip to content
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

Merged
merged 2 commits into from
Nov 4, 2023
Merged

Conversation

QuLogic
Copy link
Contributor

@QuLogic QuLogic commented Apr 16, 2023

The version 17 SDK adds getpagesize, so use it instead of hardcoding a number (even if their implementation is also a hardcoded number.)

@QuLogic
Copy link
Contributor Author

QuLogic commented Apr 17, 2023

Hrm, bit confused since this test works for me locally just fine.

@aykevl
Copy link
Member

aykevl commented Apr 26, 2023

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 getpagesize in Linux for example, as the page size varies by architecture and sometimes by kernel build - Apple M1 has 16K pages for example. There it really does need to be a library call).

@dgryski
Copy link
Member

dgryski commented Jun 14, 2023

Aside from debating thegetpagesize change, seems like upgrading to the latest wasi-libc is a Good Thing?

@aykevl
Copy link
Member

aykevl commented Jun 17, 2023

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.

@deadprogram
Copy link
Member

@QuLogic could you please rebase this branch against the latest dev so we can verify it is passing the latest tests? I would like to merge.

@deadprogram deadprogram mentioned this pull request Sep 17, 2023
Comment on lines 326 to 402
func Getpagesize() int {
// per upstream
return 65536
return libc_getpagesize()
}
Copy link
Member

@aykevl aykevl Sep 18, 2023

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 🤷

Copy link
Contributor Author

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.

@QuLogic
Copy link
Contributor Author

QuLogic commented Sep 23, 2023

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.

@QuLogic
Copy link
Contributor Author

QuLogic commented Sep 23, 2023

I just realized there are some wasi-libc caches in CI, skipping their build if available, which is probably why CI is failing but not local.

The version 17 SDK adds `getpagesize`, so use it instead of hardcoding a
number (even if their implementation is _also_ a hardcoded number.)
@QuLogic QuLogic changed the title Update wasi-libc to SDK 19 Update wasi-libc to SDK 20 Sep 23, 2023
@QuLogic QuLogic linked an issue Sep 23, 2023 that may be closed by this pull request
@QuLogic
Copy link
Contributor Author

QuLogic commented Nov 2, 2023

This should be good to go now.

@deadprogram
Copy link
Member

Thanks @QuLogic now merging.

@deadprogram deadprogram merged commit ce25f00 into tinygo-org:dev Nov 4, 2023
13 checks passed
@QuLogic QuLogic deleted the update-wasi-libc branch November 4, 2023 23:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update wasi-libc
4 participants