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 stubs for statvfs, chmod, etc. #463

Merged
merged 1 commit into from
Jan 11, 2024
Merged

Conversation

dicej
Copy link
Contributor

@dicej dicej commented Jan 10, 2024

Per WebAssembly/wasi-sdk#373, LLVM's libc++ no longer allows us to enable <fstream> and <filesystem> separately -- it's both or neither. Consequently, we either need to patch libc++ to not use statvfs, chmod, etc. or add stub functions for those features to wasi-libc. Since we're planning to eventually support those features with WASI Preview 2 and beyond, it makes sense to do the latter.

Note that since libc++ uses DT_SOCK, I've added a definition for it -- even though WASI Preview 1 does not define it. No Preview 1 file will ever have that type, so code that handles that type will never be reached, but defining it allows us to avoid WASI-specific patches to libc++.

Related to DT_SOCK, I had to change the S_IFIFO value so it does not conflict with S_IFSOCK, thereby avoiding ambiguity in __wasilibc_iftodt.

Per WebAssembly/wasi-sdk#373, LLVM's libc++ no longer
allows us to enable `<fstream>` and `<filesystem>` separately -- it's both or
neither.  Consequently, we either need to patch libc++ to not use `statvfs`,
`chmod`, etc. or add stub functions for those features to `wasi-libc`.  Since
we're planning to eventually support those features with WASI Preview 2 and
beyond, it makes sense to do the latter.

Note that since libc++ uses `DT_SOCK`, I've added a definition for it -- even
though WASI Preview 1 does not define it.  No Preview 1 file will ever have that
type, so code that handles that type will never be reached, but defining it
allows us to avoid WASI-specific patches to libc++.

Related to `DT_SOCK`, I had to change the `S_IFIFO` value so it does not
conflict with `S_IFSOCK`, thereby avoiding ambiguity in `__wasilibc_iftodt`.

Signed-off-by: Joel Dice <[email protected]>
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 cc62fa8 into WebAssembly:main Jan 11, 2024
8 checks passed
char *realpath (const char *__restrict, char *__restrict);
#endif
Copy link
Contributor

Choose a reason for hiding this comment

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

is this intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes -- realpath is one of the functions libcxx's <filesystem> relies on, although I just realized I neglected to add realpath.c to LIBC_TOP_HALF_MUSL_SOURCES in the Makefile, and I'm not sure if that implementation is what we want (vs. a stub), so I'll need to take a closer look.

Copy link
Contributor

Choose a reason for hiding this comment

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

it was intentionally disabled in the header. #82
if you are going to add a realpath implementation anytime soon, it's great. please go ahead.
otherwise, it's probably better to revert this part for now, until the implementation gets ready.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverting this part causes the libcxx build to fail, so I don't think that's an option (unless we want to patch libcxx). I've added a stub in #473, which I believe is the best we can do right now short of patching libcxx.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Update: the stub wasn't needed -- turns out musl's realpath implementation works fine.

dicej added a commit to dicej/wasi-libc that referenced this pull request Feb 13, 2024
In WebAssembly#463, I added stubs for
`statvfs`, `chmod`, etc. but forgot to add one for `realpath`, which is also
required by `libc++`'s `<filesystem>` implementation.

Signed-off-by: Joel Dice <[email protected]>
sunfishcode pushed a commit that referenced this pull request Feb 13, 2024
* provide a `realpath` stub

In #463, I added stubs for
`statvfs`, `chmod`, etc. but forgot to add one for `realpath`, which is also
required by `libc++`'s `<filesystem>` implementation.

Signed-off-by: Joel Dice <[email protected]>

* remove `realpath` stub and use musl's version instead

Signed-off-by: Joel Dice <[email protected]>

---------

Signed-off-by: Joel Dice <[email protected]>
rootbeer added a commit to rootbeer/zig that referenced this pull request Aug 16, 2024
Point to the correct header files where the wasi-libc constants are
defined that the Zig lib/std/c.zig is replicating.

Zig's current copy of wasi-libc has a bug in the file-mode constants.
Both S_IFIFO and S_IFSOCK are 0xc000, but S_IFIFO should be 0x1000.  See
WebAssembly/wasi-libc#463.
rootbeer added a commit to rootbeer/zig that referenced this pull request Aug 24, 2024
Point to the correct header files where the wasi-libc constants are
defined that the Zig lib/std/c.zig is replicating.

Zig's current copy of wasi-libc has a bug in the file-mode constants.
Both S_IFIFO and S_IFSOCK are 0xc000, but S_IFIFO should be 0x1000.  See
WebAssembly/wasi-libc#463.
kateinoigakukun added a commit to kateinoigakukun/swift that referenced this pull request Nov 20, 2024
As of a recent fix included in LLVM 17[1] and wasi-libc fix[2], we can
enable `LIBCXX_ENABLE_FILESYSTEM` in libcxx build for WebAssembly/WASI.
This allows us to use `<filesystem>`, `<fstream>`, etc in C++ code.

[1]: llvm/llvm-project@66a562d
[2]: WebAssembly/wasi-libc#463
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