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

threads: add function validation of atomics #1898

Merged
merged 1 commit into from
Jan 5, 2024

Conversation

anuraaga
Copy link
Contributor

@anuraaga anuraaga commented Jan 5, 2024

No description provided.

Copy link
Member

@mathetake mathetake left a comment

Choose a reason for hiding this comment

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

I slightly remember that memory must have min==max, but where is it validated?

@mathetake mathetake merged commit 6c9303e into tetratelabs:main Jan 5, 2024
56 checks passed
@anuraaga
Copy link
Contributor Author

anuraaga commented Jan 5, 2024

@mathetake Hmm, don't think I've seen that anywhere. For example in libpgquery wasm, memory looks like this

(memory (;0;) 15 65536 shared)

@mathetake
Copy link
Member

aha ok

@mathetake
Copy link
Member

then what happens when growing memory (changing the base address would be dangerous I guess when multiple threads are executing load/store?)

@anuraaga
Copy link
Contributor Author

anuraaga commented Jan 5, 2024

@mathetake Yup you're right. We discussed a bit but basically the only way to support it, without allocating tons of memory up front, is to use mmap. I think I need to clean up the implementation a bit before sending the PR for here, but currently it looks like this

https://github.com/wasilibs/wazerox/blob/main/internal/wasm/memory.go#L74

Will it be ok?

@mathetake
Copy link
Member

yeah looks good provided the behavior is well-documented on the API to toggle threads instructions. IIRC mmap itself doesn't immediately allocate physical pages immediately so it makes sense

@ncruces
Copy link
Collaborator

ncruces commented Jan 5, 2024

I wonder, do tests pass on 32-bit platforms, with mmaping 65536 pages (4GB)?

@anuraaga
Copy link
Contributor Author

anuraaga commented Jan 5, 2024

@ncruces Probably not, though I haven't tried it. To clarify it's not really engine or compiler related but a flag set by the user when building

https://github.com/wasilibs/go-pgquery/blob/main/buildtools/wasm/Dockerfile#L6

I copied that number from wasmer but I think I'll lower it to 2GB to best effort support 32-bit host.

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.

3 participants