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

Clearer separation of WASI POSIX support with (or without) wasi-libc #20991

Closed
wants to merge 5 commits into from

Conversation

rootbeer
Copy link
Contributor

@rootbeer rootbeer commented Aug 8, 2024

Make a clearer separation between the Posix defines for WASI without a libc (the system defined in posix.zig) and the Posix defines for WASI with a libc (which should mirror the Musl-based wasi-libc implementation, and are defined in c.zig).

This PR is mostly focused on cleanly separating the std.posix and std.c WASI implementations, disentangling types and structures where that makes sense.

For context, here are the existing ways to access system services from Zig when compiling against a wasi target (e.g., -target wasm32-wasi or -target wasm32-wasi -lc):

  • The WASI APIs (in std.os.wasi). These are non-POSIX APIs that provide access to APIs outside the WASM sandbox. Zig provides the "preview 1" version of the WASI API.
  • The Zig std.posix API implemented directly on top of the WASI APIs. This is Zig's implementation of a subset of the POSIX API.
  • The Zig std.c API as implemented by wasi-libc (a fork of the Musl C Library for WASI), when built with -lc, this provides a richer implementation of the POSIX API implemented by wasi-libc (customized from musl libc) on the WASI API.
  • The Zig standard library (File.zig, Dir.zig, etc). These Zig-specific APIs are often built on std.os.wasi, but sometimes go through std.posix.

Summary of Changes

Make WASI posix.O look more POSIX-y when compiling without libc. These structures and constants do not need to mirror with with-libc Wasi API. For example, use of .ACCMODE. This removes a lot of Wasi-specific code in Dir.zig and posix.zig by making the structure use names consistent with other platforms. Define .S, .timespec, and .AT structs for wasi-without-libc in posix.zig. These are based on the wasi-with-libc structures, but cleaned up to remove cruft like padding or wholly unsupported fields.

Fix .S and .AT constants on wasi-with-libc (defined in c.zig) to match wasi-libc headers.

Define mode_t type as void for wasi targets (and Windows targets) because file permissions are not supported via the Posix APIS on these platforms. Use mode_t as the parameter type on Zig's mkdir* and openat* signatures (instead of u32).

Note mode_t is used both as a parameter for open/chmod/chown type functions (encoding user permissions) and also used as a property of struct stat results, which additionally encode file type (link, directory, regular, etc). These should probably be separate types?

std.c.makeOFlags() is an attempt to make initializing std.posix.O flags with or without the .ACCMODE simpler. The wasi-with-libc O flags cannot support ACCMODE due to the layout of the flags in wasi-libc.

The with-libc WASI AT struct needs to mirror the wasi-libc #defines. Fixes #20890

Enable a lot of tests that have accumulated stale not-on-wasi guards, and simplify some tests (e.g., dropping unnecessary absolute paths) so they can run on both of the wasi targets. Also, factors out supports_chmod and supports_absolute_paths, etc flags in tests to make the skip-this-test logic more readable.

Zig's Posix-on-Wasi wrapper defines the current working directory to be . (from the --dir=. argument to wasmtime). However, wasi-libc defines the current working directory to be /, which makes a bit more sense, as the wasi code is effectively chroot-ed into the given directory. Perhaps Zig should follow wasi-libc's lead here? (I'm not sure it makes any difference anywhere, yet.)

Clean up of test cases:

  • use realpathAlloc()to get the full path of tmpDir() instances
  • use testing.allocator where that simplifies things (vs. manual ArenaAllocator for 1 or 2 allocations)
  • Trust TmpDir.cleanup() to clean up sub-trees
  • Add unique_fname in posix.zig for creating unique-name files when testing in the default cwd (prefer tmpDir()). Fixes unit tests that write files to cwd have racy failures #14968
  • Remove some unnecessary absolute paths (to enable WASI tests)
  • Drop some no-longer necessary [_][]const u8 explicit types
  • Put cleanups into defer

@alexrp
Copy link
Member

alexrp commented Aug 8, 2024

The problem here is going to be the fact that we are no longer interested in maintaining a POSIX compatibility layer in the standard library - see #6600. This has 2 main implications for this PR:

  1. In general, making up types in std.posix that don't exist in the OS APIs is not allowed.
  2. I don't know that we actually want to expand the compatibility layer to cover even more configurations.
    • Instead, I would suggest that any standard library abstractions that depend on std.posix for WASI should be changed to have code paths for wasi-without-libc (std.os.wasi) and wasi-with-libc (std.c), with the latter hopefully just being "libc in general" in most cases.

See also #19614.

@rootbeer
Copy link
Contributor Author

rootbeer commented Aug 8, 2024

The problem here is going to be the fact that we are no longer interested in maintaining a POSIX compatibility layer in the standard library - see #6600. This has 2 main implications for this PR:

  1. In general, making up types in std.posix that don't exist in the OS APIs is not allowed.

Hmm, I can see the argument is for not making up a POSIX layer on target that don't have one. But given that WASI doesn't try to be POSIX (though they justify a lot of their APIs in terms of a POSIX relationship), that implies the Zig POSIX-via-WASI wrapper should completely go away. I was under the impression that the goal was to keep the Zig-impl-of-POSIX-on-WASI running (at least in the short term). Did I misinterpret #20845 (review)? (My interpretation is that std.posix.* should be a best-effort and implement POSIXy APIs for systems where that isn't too far out of whack, so for WASI without libc we get POSIX file open/read/write, but no chown/chmod/mmap, etc)

What you're suggesting, concretely, is that instead of making up a std.posix.system.O flags set for wasi-without-libc, I should have set that to void, too because POSIX open flags should only come from a libc? (In fact all the symbols in std.posix.system should be void in the wasi-without-libc case because there is no POSIX layer?) Or am I overstating your case?

  1. I don't know that we actually want to expand the compatibility layer to cover even more configurations.
    Instead, I would suggest that any standard library abstractions that depend on std.posix for WASI should be changed to have code paths for wasi-without-libc (std.os.wasi) and wasi-with-libc (std.c), with the latter hopefully just being "libc in general" in most cases.

I understand your suggestion to be removing all the if (native_os == .wasi and !builtin.link_libc) {...} blocks in posix.zig, then fixing anything in File.zig and Dir.zig that depended on those code paths in the WASI case to make direct, non-posix calls. E.g., Dir.openFile should only call posix.openat if libc is linked, and the current wasi-no-libc code should call directly to the std.os.wasi APIs (without going through std.posix.openat's wasi wrapper)? That seems like a really huge change ...

Can you maybe squint at this change and see it as a cleanup step in preparation for that deletion? (It helps clearly separate the existing Zig POSIX on wasi-without-libc?)

OTOH:

See also #19614.

The last comment from our BDFL is "man. wasi-libc kinda sucks. how about we provide libc via zig code instead?" Isn't a "libc via zig code" exactly what the std.posix implementation in the wasi-without-libc path is providing?

@alexrp
Copy link
Member

alexrp commented Aug 8, 2024

Hmm, I can see the argument is for not making up a POSIX layer on target that don't have one. But given that WASI doesn't try to be POSIX (though they justify a lot of their APIs in terms of a POSIX relationship), that implies the Zig POSIX-via-WASI wrapper should completely go away.

Well, that would indeed be my position. 🙂 I want std.posix moved out of std and into a community package. That said, I still need to type up a full proposal for this, so to be clear, this is not currently the Zig project's stance.

I was under the impression that the goal was to keep the Zig-impl-of-POSIX-on-WASI running (at least in the short term).

I do think that it should at least be kept to the same level of "running" as it is today, until we settle on a final decision regarding the future of std.posix. What I'm questioning is adding more on top of that since we do at least know that we don't want to be maintaining a POSIX compatibility layer in std anymore. (Other than outright removal, it has also been suggested to rename it to std.unix or similar to indicate that it is not, in fact, full POSIX.)

Did I misinterpret #20845 (review)? (My interpretation is that std.posix.* should be a best-effort and implement POSIXy APIs for systems where that isn't too far out of whack, so for WASI without libc we get POSIX file open/read/write, but no chown/chmod/mmap, etc)

Maybe I'm completely off, but I interpret Andrew's comment there to mean exactly what I'm saying: std.posix should expose what the system is actually capable of without making anything up (where "system" means either std.os.<name> or std.c). But rather than guess, let's just ask the man. 🙂 cc @andrewrk

What you're suggesting, concretely, is that instead of making up a std.posix.system.O flags set for wasi-without-libc, I should have set that to void, too because POSIX open flags should only come from a libc? (In fact all the symbols in std.posix.system should be void in the wasi-without-libc case because there is no POSIX layer?) Or am I overstating your case?

POSIX open flags can also come from the OS, if the OS defines them. For example, Linux does so in UAPI (distinct from glibc/musl values), as does FreeBSD (sort of; syscalls use libc types directly). But, bottom line, what I'm saying is that if the OS minus libc defines POSIX stuff, then it should be exposed in std.posix; if it does not, then the corresponding std.posix symbols should be void/{}.

But again, to be clear, I'm only saying this for new additions to std.posix; I don't personally see any reason to break what's already there, at least until we've decided on its future.

I understand your suggestion to be removing all the if (native_os == .wasi and !builtin.link_libc) {...} blocks in posix.zig

My suggestion is that any such blocks that already exist, and work, should stay. What I am suggesting is that I'm not sure we want to add more such blocks going forward if they require us to make stuff up about the underlying std.os.wasi capabilities/APIs.

then fixing anything in File.zig and Dir.zig that depended on those code paths in the WASI case to make direct, non-posix calls. E.g., Dir.openFile should only call posix.openat if libc is linked, and the current wasi-no-libc code should call directly to the std.os.wasi APIs (without going through std.posix.openat's wasi wrapper)? That seems like a really huge change ...

I'm not suggesting that you should do all that work for existing code paths. 🙂 The code paths that go through std.posix for wasi-without-libc today, and work, should just keep doing that until someone feels like switching them over to having separate paths for std.c and std.os.wasi. (Good chance that person will be me if my proposal ends up being accepted.)

What I am suggesting is that this should be done for any code paths that aren't currently working for wasi-without-libc.

Can you maybe squint at this change and see it as a cleanup step in preparation for that deletion? (It helps clearly separate the existing Zig POSIX on wasi-without-libc?)

That's up to Andrew. I personally wouldn't lose sleep over the changes here because my ideal world means pulling std.posix out of std entirely anyway, so I'm fine with viewing this as an incremental step on that path. I'm just echoing Andrew's prior opposition to making things up for std.posix.

The last comment from our BDFL is "man. wasi-libc kinda sucks. how about we provide libc via zig code instead?" Isn't a "libc via zig code" exactly what the std.posix implementation in the wasi-without-libc path is providing?

I can only guess here, but I took it to mean something like ziglibc, not necessarily part of std, but perhaps shipped with Zig.

Copy link
Member

@andrewrk andrewrk left a comment

Choose a reason for hiding this comment

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

sorry, I think this is taking things in the wrong direction

lib/std/fs/wasi.zig Outdated Show resolved Hide resolved
lib/std/c.zig Outdated Show resolved Hide resolved
lib/std/c.zig Outdated Show resolved Hide resolved
@rootbeer
Copy link
Contributor Author

rootbeer commented Aug 8, 2024

Thanks for taking the time and the detailed feedback, I think I understand where you are coming from, and I think that's actually pretty consistent with my understanding too.

What I'm questioning is adding more on top of that since we do at least know that we don't want to be maintaining a POSIX compatibility layer in std anymore.

I think I can make the case that, other than the types at the top (in the system definition), this PR adds no new code in posix.zig for implementing the if (native_os == .wasi and !builtin.link_libc) execution paths. The new makeOFlags() function is for the wasi-with-libc execution path (because the wasi-libc puts its RDONLY and WRONLY open flags in non-adjacent bits).

These types in posix.zig for the wasi-without-libc system are only those necessary to make the existing Zig-native std.posix implementation continue to work while also (mostly) disentangling it from the wasi-on-libc definitions in std.os. Separating these out made it clearer that bits in the wasi-on-libc AT enum were wrong (before it wasn't clear which properties in here were specific to wasi-libc, and which were part of the wasi-without-libc).

This PR also fixes a lot of tests to run on Wasi with or without libc, there are relatively few spots where tests are covering wasi-without-libc specifically.

So, I think you can look at this PR as improving the wasi-with-libc support in Zig by (1) improving the test coverage (2) fixing some bugs in the AT flags and (3) removing the distracting wasi-without-libc hacks from the wasi-with-libc implementation. The way we get that support is by isolating the pre-existing wasi-without-libc code more clearly in posix.zig.

Make a clearer separation between the Posix defines for WASI without a
libc (the `system` defined in posix.zig) and the Posix defines for WASI
with a libc (which should mirror the Musl-based wasi-libc implementation,
and are defined in c.zig).

Make WASI `posix.O` look more POSIX-y when compiling without libc.  These
structures and constants do not need to mirror the with-libc Wasi API
(e.g., can use `.ACCMODE`).  This removes some Wasi-specific code in
Dir.zig and posix.zig by making the structure use names consistent with
other platforms.  Define `.S`, `.timespec`, and `.AT` structs for
Wasi-without-libc in posix.zig.  These are based on the wasi-with-libc
structures, but cleaned up to remove cruft like padding or wholly
unsupported fields.

Fix `.S` and `.AT` constants on wasi-with-libc (defined in c.zig) to match
wasi-libc headers.

Define `mode_t` type as `void` for wasi targets because file permissions
are not supported via the Posix APIS on these platforms.  Use `mode_t` as
the parameter type in Zig's mkdir\* and openat\* wrappers.

Re-enable a lot of tests that have accumulated stale not-on-wasi guards,
and simplify some tests (e.g., dropping unnecessary absolute paths) so
they can run on wasi targets.
To be consistent with the Wasi mode_t, use 'void'.
Add `unique_fname` for creating unique-name files when testing in the
default CWD (though tests should still prefer tmpDir()).  Enable the tests
disabled because this is racy (ziglang#14968).

Add some WASI-specific behavior handling.

Other cleanups of the tests:
* use `realpathAlloc()`to get fullpath into tmpDir() instances
* use `testing.allocator` where that simplifies things (vs. manual ArenaAllocator for 1 or 2 allocs)
* Trust `TmpDir.cleanup()` to clean up sub-trees
* Remove some unnecessary absolute paths (to enable WASI tests)
* Drop some no-longer necessary `[_][]const u8` explicit types
* Put file cleanups into `defer`

Fixes ziglang#14968
Add a test for fstat{,at} with symlinks (for everyone, not just wasi)

Fix the wasi-libc Stat.mode field, and add the expected accessors to the S
struct.

Fixes ziglang#20890
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
Copy link
Contributor Author

rootbeer commented Oct 2, 2024

I'll send out the WASI bug fixes in separate, smaller changes.

@rootbeer rootbeer closed this Oct 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants