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

Add wasi-emulated-mman cargo feature #3093

Closed
wants to merge 4 commits into from
Closed

Add wasi-emulated-mman cargo feature #3093

wants to merge 4 commits into from

Conversation

GregoryConrad
Copy link

@GregoryConrad GregoryConrad commented Jan 27, 2023

Fixes #3091 by adding a wasi-emulated-mman cargo feature that adds bindings for wasi-libc's emulated mman.

I am not sure if I implemented this completely correctly; feedback would be highly appreciated!

Some general comments/questions:

  1. I did not implement mmap64 here, as I was unsure how to handle C's #define mmap64 mmap. I only started learning Rust a couple months ago so I am not sure what would be analogous here.
  2. I am not 100% sure what is up with the _GNU_SOURCE and _BSD_SOURCE conditional includes in the C header, so I included the relevant bindings in the hopes that I could get feedback on that from someone here.
  3. The unix/linux-like MAP_FAILED is implemented as pub const MAP_FAILED: *mut ::c_void = !0 as *mut ::c_void;, which confused me. The mman.h in wasi-libc is #define MAP_FAILED ((void *) -1). What is the correct rust binding for this constant? I failed to realize the !0 is just the bitwise operator; all set here.
  4. There is no libc-test/semver/wasi.txt?
  5. And finally, a big newbie question: originally, I thought this library provided the target's libc (like wasi-libc), but that appears to be wrong as far as I can tell. How exactly does the target's libc get built & linked? Is this overridable using environment variables with cargo at build time (e.g. CC="clang ... --sysroot=..." cargo build --target="...")? Or through some different mechanism?

@rustbot
Copy link
Collaborator

rustbot commented Jan 27, 2023

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @JohnTitor (or someone else) soon.

Please see the contribution instructions for more information.

@GregoryConrad GregoryConrad marked this pull request as ready for review January 28, 2023 16:02
src/wasi.rs Outdated Show resolved Hide resolved
@JohnTitor
Copy link
Member

As mentioned in #3097 (comment), we don't accept any platform-specific feature.

@GregoryConrad
Copy link
Author

GregoryConrad commented Feb 5, 2023

@JohnTitor Ok, then what is the best course of action here? Remove the cargo feature (but keep my additions without the feature check), and if someone uses libc without the proper wasi-libc compilation flags enabled, they will just get linking/build failures?

Edit: I just removed the feature and added everything to the global namespace. If someone doesn't explicitly enable wasi-libc's emulated mman, they will just get compile/link build issues.

@JohnTitor
Copy link
Member

It should be separated into another crate as it may require additional setup as you said. The libc crate should be minimal to be usable on many hosts/targets.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add emulated features from wasi-libc (like mmap)
3 participants