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

Extend wasi-emulated-mman with mprotect #500

Merged
merged 1 commit into from
May 22, 2024

Conversation

whitequark
Copy link
Contributor

This implementation never changes actual memory protection, but it does verify that the address range and flags are valid. The direct motivation is fixing a linker error where LLVM links to mprotect in dead code.

@whitequark whitequark force-pushed the mprotect branch 3 times, most recently from b55e3e8 to 562e7cc Compare May 21, 2024 01:49
libc-bottom-half/mman/mman.c Outdated Show resolved Hide resolved
This implementation never changes actual memory protection, but it does
verify that the address range and flags are valid. The direct motivation
is fixing a linker error where LLVM links to `mprotect` in dead code.
@sbc100
Copy link
Member

sbc100 commented May 21, 2024

Would it make sense if patch LLVM to honor some kind of HAVE_MPROTECT instead/also?

@whitequark
Copy link
Contributor Author

whitequark commented May 21, 2024

Would it make sense if patch LLVM to honor some kind of HAVE_MPROTECT instead/also?

That's what I've been doing so far, but:

a) There is pushback against adding more compiletests, as they are slow and some find them hard to maintain.
b) A sensible implementation for wasi-emulated-mman is possible, so why not add it here? It may benefit other applications.

@sbc100
Copy link
Member

sbc100 commented May 21, 2024

Would it make sense if patch LLVM to honor some kind of HAVE_MPROTECT instead/also?

That's what I've been doing so far, but:

a) There is pushback against adding more compiletests, as they are slow and some find them hard to maintain.

Do they need to be compiletests? Can't it just be something that is hardcoded into the CMakeLists on the LLVM side? something like:

if (IS_WASM) set(HAVE_MMAP, 0)

b) A sensible implementation for wasi-emulated-mman is possible, so why not add it here? It may benefit other applications.

Fair enough, I agree it makes sense to emulate all these mman symbols.

However I do think it would be better for llvm to not include the dead code that calls mmap/mprotect at all. It seems like a good in general. Perhaps we can do both things?

@whitequark
Copy link
Contributor Author

whitequark commented May 21, 2024

Can't it just be something that is hardcoded into the CMakeLists on the LLVM side? something like:

Maybe, but I still don't understand exactly what changes to CMakeLists.txt for WASI will be upstreamable, and it seems there are objections to having those #defines in config.h.cmake too. So for now I'm avoiding doing any speculative work. If the current set of compiletests is accepted then one more for mprotect() is probably fine too (but I must point out that it will require a different approach because wasi-libc advertises support for mprotect() in the headers; it just doesn't have the symbol).

However I do think it would be better for llvm to not include the dead code that calls mmap/mprotect at all. It seems like a good in general. Perhaps we can do both things?

It looks like there's currently no way to disable the JIT (which is a little odd to me), which is what probably needs to be done for this. LTO builds don't suffer from this problem, so the code is already statically dead, just not dead enough for non-LTO builds.

Copy link
Member

@sunfishcode sunfishcode left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@sunfishcode sunfishcode merged commit 7528b13 into WebAssembly:main May 22, 2024
8 checks passed
@whitequark whitequark deleted the mprotect branch May 23, 2024 18:54
@whitequark
Copy link
Contributor Author

Thanks!

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.

4 participants