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

debug.zig: Make it build with 'other' target OS #20845

Merged
merged 1 commit into from
Nov 29, 2024

Conversation

rootbeer
Copy link
Contributor

@rootbeer rootbeer commented Jul 28, 2024

Make std.debug.StackIterator compile on the other OS target, and port the "unwind_freestanding" test case to the other OS target.

Inspired by #20833 which reported the original problem and pin-pointed the issue.

[Edited summary and PR a couple times.]

@rootbeer
Copy link
Contributor Author

Looks like Wasi has an msync symbol visible, but depending on it in std.debug creates unwanted libc dependencies (?), so I have to prevent an msync dependency in std.debug when building against Wasi targets.

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.

Thanks.

This is the direction I would like to go instead:

diff --git a/lib/std/debug.zig b/lib/std/debug.zig
index 31321d62fd..77345821ed 100644
--- a/lib/std/debug.zig
+++ b/lib/std/debug.zig
@@ -842,10 +842,7 @@ pub const StackIterator = struct {
     }
 };
 
-const have_msync = switch (native_os) {
-    .wasi, .emscripten, .windows => false,
-    else => true,
-};
+const have_msync = @TypeOf(posix.system.msync) != void;
 
 pub fn writeCurrentStackTrace(
     out_stream: anytype,
diff --git a/lib/std/posix.zig b/lib/std/posix.zig
index 955987b036..d6c3746c4d 100644
--- a/lib/std/posix.zig
+++ b/lib/std/posix.zig
@@ -29,23 +29,34 @@ test {
     _ = @import("posix/test.zig");
 }
 
-/// Whether to use libc for the POSIX API layer.
-const use_libc = builtin.link_libc or switch (native_os) {
-    .windows, .wasi => true,
-    else => false,
-};
-
 const linux = std.os.linux;
 const windows = std.os.windows;
 const wasi = std.os.wasi;
 
 /// A libc-compatible API layer.
-pub const system = if (use_libc)
+pub const system = if (builtin.link_libc)
     std.c
 else switch (native_os) {
     .linux => linux,
     .plan9 => std.os.plan9,
+    .wasi => struct {
+        pub const E = wasi.errno_t;
+        pub const fd_t = wasi.fd_t;
+        pub const msync = {};
+        pub const pid_t = void;
+        pub const pollfd = void;
+        pub const rlimit_resource = void;
+        pub const ucontext_t = void;
+        pub const AT = void;
+        pub const ino_t = void;
+        pub const mode_t = void;
+    },
     else => struct {
+        pub const msync = {};
+        pub const pid_t = void;
+        pub const pollfd = void;
+        pub const rlimit_resource = void;
         pub const ucontext_t = void;
     },
 };
@@ -238,7 +249,7 @@ pub const socket_t = if (native_os == .windows) windows.ws2_32.SOCKET else fd_t;
 /// function only returns a well-defined value when it is called directly after
 /// the system function call whose errno value is intended to be observed.
 pub fn errno(rc: anytype) E {
-    if (use_libc) {
+    if (builtin.link_libc) {
         return if (rc == -1) @enumFromInt(std.c._errno().*) else .SUCCESS;
     }
     const signed: isize = @bitCast(rc);

It points out compilation errors at all the places that the standard library is incorrectly leaning on a POSIX API layer for libc-less WASI rather than lowering standard library API directly to the WASI API layer.

@rootbeer
Copy link
Contributor Author

rootbeer commented Jul 30, 2024

I am pushing this change in the direction you suggested. It will take me a bit of time (all I know of WASI is what I'm learning in the Zig source tree...). I believe the three existing commits in this PR are mostly independent of the WASI cleanup (other than forcing the issue to be a bit more apparent), so I don't think you need to hold up review/submit of those. But unless I hear otherwise, I'll assume you want the WASI fixes on this PR and will push a new version here once I get through the fallout ...

@rootbeer
Copy link
Contributor Author

rootbeer commented Aug 1, 2024

@andrewrk would you prefer the posix.mode_t resolve to void or u0 for Wasi-without-libc targets?

The "void" seems more correct to me, but the existing Windows targets in Zig implicitly map posix.system.mode_t to u0 (AFAICT, its the same underlying motivation -- POSIX file modes are not supported by the Windows target). Having both void for Wasi and u0 for Windows can work (its what I have currently in my tree), but it is a bit clumsy.

I recommend setting posix.system.mode_t to void for Wasi-without-libc, and also fixing the Windows implementation to match. But I wasn't sure if there was some reason the u0 might be preferred, or if the Windows case is different.

@andrewrk
Copy link
Member

andrewrk commented Aug 1, 2024

I recommend setting posix.system.mode_t to void for Wasi-without-libc, and also fixing the Windows implementation to match. But I wasn't sure if there was some reason the u0 might be preferred, or if the Windows case is different.

I think it's a good recommendation; let's do that.

@rootbeer rootbeer force-pushed the 20833-debug-msync-dependency branch from 81b4224 to c6995e6 Compare August 6, 2024 23:22
@rootbeer rootbeer marked this pull request as draft August 7, 2024 04:10
@rootbeer rootbeer force-pushed the 20833-debug-msync-dependency branch from c6995e6 to 8518ea2 Compare August 13, 2024 17:43
@rootbeer rootbeer marked this pull request as ready for review August 15, 2024 02:45
@rootbeer
Copy link
Contributor Author

I've forked the more general WASI POSIX cleanup changes to #20991, as they got large. I know a bit more about WASM now, so I've cleaned up this PR to make the msync symbol {} on WASI (so the MemoryAccessor.zig code can just check for msync-or-not without getting caught up in the platform details). I think this is ready for review and fixes the issue reported in #20833.

@rootbeer rootbeer force-pushed the 20833-debug-msync-dependency branch 2 times, most recently from 137b92a to 0167bc7 Compare September 17, 2024 16:08
@rootbeer rootbeer force-pushed the 20833-debug-msync-dependency branch from 0167bc7 to 513bb99 Compare September 30, 2024 21:31
@rootbeer
Copy link
Contributor Author

rootbeer commented Oct 1, 2024

I updated this PR for the recent panic changes. I simplified this PR a lot too. Now it just does the minimal necessary to get the "other" target OS to work as well as the "freestanding" target does for std.debug.StackIterator. No more mucking about with tangential improvements to msync detection. Please give this a fresh look.

@rootbeer rootbeer requested a review from andrewrk October 1, 2024 04:13
The freestanding and other OS targets by default need to just @trap in the
default Panic implementation.

And `isValidMemory` won't work with freestanding or other targets.

Update the unwind_freestanding.zig test case to also run on the 'other' OS
target, too.  This should keep the Zig's stacktrace generation from
regressing on the standalone targets.
@rootbeer rootbeer force-pushed the 20833-debug-msync-dependency branch from 513bb99 to 523f215 Compare November 25, 2024 04:51
@andrewrk andrewrk merged commit 5e1a83a into ziglang:master Nov 29, 2024
10 checks passed
@andrewrk
Copy link
Member

Thanks!

@rootbeer rootbeer deleted the 20833-debug-msync-dependency branch November 29, 2024 20:53
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