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

WASI,libc: fix libstd with wasi-libc linkage, and enable tests. #9227

Merged
merged 5 commits into from Aug 13, 2021

Conversation

mathetake
Copy link
Contributor

@mathetake mathetake commented Jun 24, 2021

This PR modifies libstd for WASI + wasi-libc target, and resolves #8845

@mathetake
Copy link
Contributor Author

mathetake commented Jun 26, 2021

finally resolved the symbol issue but weird.. I see "the number of locals exceeds the minimum".. look into this now though it looks like a calling convention mismatch..

Error: failed to run main module `/home/mathetake/zig/zig-cache/o/3f9eec2bc857dd8c7327f378ef491e47/test.wasm`

Caused by:
    0: WebAssembly failed to compile
    1: WebAssembly translation error
    2: Invalid input WebAssembly code at offset 14965884: locals exceed maximum
error: the following test command failed with exit code 1:
wasmtime --dir=. /home/mathetake/zig/zig-cache/o/3f9eec2bc857dd8c7327f378ef491e47/test.wasm /home/mathetake/zig/build/zig
The following command exited with error code 1:
/home/mathetake/zig/build/zig test /home/mathetake/zig/lib/std/std.zig -lc --test-name-prefix std-wasm32-wasi-Debug-c-single  --single-threaded --cache-dir /home/mathetake/zig/zig-cache --global-cache-dir /home/mathetake/.cache/zig --name test -target wasm32-wasi --test-cmd wasmtime --test-cmd --dir=. --test-cmd-bin -I /home/mathetake/zig/test --zig-lib-dir /home/mathetake/zig/lib 
error: the following build command failed with exit code 1:
/home/mathetake/zig/zig-cache/o/50a49ecd131d5c652f1182575d182f4d/build /home/mathetake/zig/build/zig /home/mathetake/zig /home/mathetake/zig/zig-cache /home/mathetake/.cache/zig test-std -Dtarget=wasm32-wasi -Denable-wasmtime

@mathetake
Copy link
Contributor Author

OK a bit of progress.. looks like linking with libc has not succeeded yet.

$ wasm-objdump /home/mathetake/zig/zig-cache/o/3f9eec2bc857dd8c7327f378ef491e47/test.wasm --section=import -x
...
 - func[6] sig=6 <exit|c> <- c.exit
 - func[7] sig=0 <malloc|c> <- c.malloc
 - func[8] sig=6 <free|c> <- c.free
 - func[9] sig=8 <write|c> <- c.write

@mathetake
Copy link
Contributor Author

looks like we have to remove "c" from here https://github.com/ziglang/zig/blob/master/lib/std/c.zig#L71-L86

@mathetake
Copy link
Contributor Author

OK now zig-bootstrap/out/host/bin/zig build-exe main.zig -lc --single-threaded -target wasm32-wasi has started working!

const std = @import("std");
pub fn main() void {
    std.debug.print("hello with libc!", .{});
}

lib/std/c.zig Outdated Show resolved Hide resolved
src/stage1/codegen.cpp Outdated Show resolved Hide resolved
@mathetake mathetake changed the title WASI,libc: enable libstd tests. WASI,libc: fix libstd with wasi-libc linkage, and enable tests. Jun 27, 2021
@mathetake mathetake marked this pull request as ready for review June 27, 2021 08:11
@mathetake
Copy link
Contributor Author

@kubkon I believe this is ready for review. I would appreciate it if you could have a chance to take a look! :)

lib/std/c.zig Outdated Show resolved Hide resolved
lib/std/os.zig Outdated Show resolved Hide resolved
@mathetake mathetake force-pushed the libc-wasi-test branch 2 times, most recently from 31f2b8f to 18557e8 Compare June 29, 2021 14:12
lib/std/fs.zig Outdated Show resolved Hide resolved
@mathetake
Copy link
Contributor Author

Almost there, but the fs tests haven't passed yet. I guess something to do with preopens, but couldn't figure out yet...

@mathetake
Copy link
Contributor Author

OK finally passes all the tests locally...🤞

@mathetake
Copy link
Contributor Author

OK I believe everything works now after rebasing 😄 hope all the tests would pass again 🤞

@mathetake
Copy link
Contributor Author

OK I did a bit of research on wasm-ld behavior on wasm-import-module and wasm-import-name attributes, and other frontend's behavior.. I found that the root cause of linktime symbol mismatch and maybe I could reduce the amount of change (mostly due to the workaround for it) here. Will work on a separate PR in next few days.

@mathetake
Copy link
Contributor Author

#9279 will reduce the fair amount of change in this PR.

@mathetake
Copy link
Contributor Author

rebased on #9279 .

@kubkon
Copy link
Member

kubkon commented Jul 2, 2021

If you don't mind, I'll convert this PR into a draft since it's blocked by #9279 so that we don't merge it prematurely.

@kubkon kubkon marked this pull request as draft July 2, 2021 16:36
@mathetake
Copy link
Contributor Author

Right, thanks for doing that for me!

@kubkon
Copy link
Member

kubkon commented Jul 26, 2021

@mathetake could you rebase/merge against master given that #9279 is now merged, and mark the PR as ready for review (if you still feel it's ready ofc!)?

Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
@mathetake mathetake marked this pull request as ready for review July 27, 2021 03:42
@mathetake
Copy link
Contributor Author

@kubkon I've just made this for ready again:) Thanks!

lib/std/os/bits/wasi.zig Outdated Show resolved Hide resolved
lib/std/os/bits/wasi.zig Outdated Show resolved Hide resolved
Copy link
Member

@kubkon kubkon left a comment

Choose a reason for hiding this comment

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

Thanks for addressing the previous set of suggestions, and I apologise for not spending more time on reviewing this PR earlier. Anyway, looks good! I've just got a final set of changes I'd like you to incorporate and we should be ready to merge!

lib/std/fs.zig Outdated Show resolved Hide resolved
Comment on lines +940 to +941
if (builtin.target.os.tag != .wasi) {
if (!has_flock_open_flags and flags.lock != .None) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (builtin.target.os.tag != .wasi) {
if (!has_flock_open_flags and flags.lock != .None) {
if (!has_flock_open_flags and flags.lock != .None and builtin.target.os.tag != .wasi) {

I think we can put all checks in one if.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah this is a little bit trick.. we need to remove out this if block since since the expression is not compiltime-known, and WASI doesn't have os.flock that's why I put the OS check prior to the if block. Added comment about this: 68617c9

Copy link
Member

Choose a reason for hiding this comment

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

Ah gotcha, makes sense!

lib/std/fs/wasi.zig Outdated Show resolved Hide resolved
lib/std/testing.zig Outdated Show resolved Hide resolved
lib/std/testing.zig Outdated Show resolved Hide resolved
Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
@mathetake
Copy link
Contributor Author

Thanks @kubkon always! I hope I addressed all your comments:)

@kubkon kubkon merged commit 1373df4 into ziglang:master Aug 13, 2021
@mathetake mathetake deleted the libc-wasi-test branch August 13, 2021 23:06
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.

wasi-libc: enable and pass libstd tests
4 participants