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 support for WASI reactor in pure Zig-exe. #9178

Merged
merged 6 commits into from Jul 1, 2021

Conversation

mathetake
Copy link
Contributor

@mathetake mathetake commented Jun 20, 2021

This PR adds support for WASI reactor in pure Zig WASI targets. Notably this enables users to produce WASI reactor binary with build-exe command with -mexec-model=reactor just like zig cc, zig c++, and clang.

With -mexec-model=reactor flag, Zig 1) exports _initialize symbol which calls the user defined pub fn main, 2) does not define _start, and 3) does not execute proc_exit after main.

Please refer to https://github.com/WebAssembly/WASI/blob/main/design/application-abi.md for the ABI in detail.

Finally resolves #6757 together with #9052.

cc @kubkon

Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
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 @mathetake! I've got a few comments I'd like to see addressed before going forward with this PR.

src/main.zig Outdated Show resolved Hide resolved
src/wasi_libc.zig Outdated Show resolved Hide resolved
lib/std/start.zig Outdated Show resolved Hide resolved
Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
Comment on lines 3641 to 3648
const wasi_exec_model_fmt = if (comp.bin_file.options.wasi_exec_model) |model|
std.zig.fmtId(@tagName(model))
else
std.zig.fmtId(@tagName(std.builtin.WasiExecModel.command));
try buffer.writer().print(
\\pub const wasi_exec_model = std.builtin.WasiExecModel.{};
\\
, .{wasi_exec_model_fmt});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI: If I put this into a single statement,

    try buffer.writer().print(
        \\pub const wasi_exec_model = std.builtin.WasiExecModel.{};
        \\
    , .{if (comp.bin_file.options.wasi_exec_model) |model|
        std.zig.fmtId(@tagName(model))
    else
        std.zig.fmtId(@tagName(std.builtin.WasiExecModel.command))});

I got some weird error around LLVM:

make install
[ 77%] Built target embedded_softfloat
[ 78%] Built target opt_c_util
[ 83%] Built target zigcpp
[ 97%] Built target zigstage1
[ 98%] Built target zig0
[ 99%] Building self-hosted component /home/mathetake/zig/build/zig1.o
broken LLVM module found: Instruction does not dominate all uses!
  %369 = getelementptr inbounds %"Compilation.struct:3644:8", %"Compilation.struct:3644:8"* %39, i32 0, i32 0, !dbg !75434
  call fastcc void @std.zig.fmt.fmtId(%"std.fmt.Formatter(std.zig.fmt.formatId)"* sret(%"std.fmt.Formatter(std.zig.fmt.formatId)") %369, %"[]u8"* @13528), !dbg !75435
Instruction does not dominate all uses!
  %369 = getelementptr inbounds %"Compilation.struct:3644:8", %"Compilation.struct:3644:8"* %39, i32 0, i32 0, !dbg !75434
  %372 = bitcast %"std.fmt.Formatter(std.zig.fmt.formatId)"* %369 to i8*, !dbg !75435
Instruction does not dominate all uses!
  %369 = getelementptr inbounds %"Compilation.struct:3644:8", %"Compilation.struct:3644:8"* %39, i32 0, i32 0, !dbg !75434
  %373 = bitcast %"std.fmt.Formatter(std.zig.fmt.formatId)"* %369 to i8*, !dbg !75435

This is a bug in the Zig compiler.
Aborted (core dumped)
make[2]: *** [CMakeFiles/zig.dir/build.make:328: zig1.o] Error 134
make[1]: *** [CMakeFiles/Makefile2:148: CMakeFiles/zig.dir/all] Error 2
make: *** [Makefile:130: all] Error 2

Copy link
Member

Choose a reason for hiding this comment

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

I believe this is because it's not enough to just add an enum to std.builtin. You need to make more tweaks to stage1 in C++. For starters, we need to have the exact same enum shared between both stage1 and stage2 which means you need add a corresponding enum to src/stage1/stage1.h. Then, you'll need to tweak logic for actually marshalling the data between stage1 and stage2 - have a look how this is for CodeModel or some other builtin structs.

If you get stuck, I'll be happy to help you out but will probably take a look at this in more detail tomorrow. Alternatively, we can also get @andrewrk involved if he's got a minute.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I see. Thanks for clarifying!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so yeah I found that this also applies to other existing builtin types:

const std = @import("std");

pub fn main() void {
    std.debug.print("test {}", .{if (@rem(std.time.timestamp(), 2) == 0)
        std.zig.fmtId(@tagName(std.builtin.CodeModel.tiny))
    else
        std.zig.fmtId(@tagName(std.builtin.CodeModel.default))});
}

HEAD and 0.8.0 fails to compile this code with the same error like this:

zig build-exe main.zig
broken LLVM module found: Instruction does not dominate all uses!d.debug.ModuleDebug...
  %2 = getelementptr inbounds %"struct:4:33", %"struct:4:33"* %0, i32 0, i32 0, !dbg !16731
  call fastcc void @std.zig.fmt.fmtId(%"std.fmt.Formatter(std.zig.fmt.formatId)"* sret(%"std.fmt.Formatter(std.zig.fmt.formatId)") %2, %"[]u8.18"* @390), !dbg !16733
Instruction does not dominate all uses!
  %2 = getelementptr inbounds %"struct:4:33", %"struct:4:33"* %0, i32 0, i32 0, !dbg !16731
  %5 = bitcast %"std.fmt.Formatter(std.zig.fmt.formatId)"* %2 to i8*, !dbg !16733
Instruction does not dominate all uses!
  %2 = getelementptr inbounds %"struct:4:33", %"struct:4:33"* %0, i32 0, i32 0, !dbg !16731
  %6 = bitcast %"std.fmt.Formatter(std.zig.fmt.formatId)"* %2 to i8*, !dbg !16733

This is a bug in the Zig compiler.thread 3208559 panic:
Unable to dump stack trace: debug info stripped
zsh: abort (core dumped)  zig build-exe main.zig

and the same, if we split the line to two statements. the error disappears.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fyi: I tweaked the C++ and around stage1.zig and Compilation.zig following CodeModel, but the error has never gone.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Luckily the code with split statements just works and successfully produced Wasi Reactors!

@mathetake
Copy link
Contributor Author

Thanks @kubkon for a quick look:) I hope 6769659 addressed your comments! PTAL!

@mathetake mathetake requested a review from kubkon June 20, 2021 13:58
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.

Looks great to me, thanks @mathetake! I just need to double check with @andrewrk that we don't need to add matching logic to the C++ codebase for setting WasiExecModel builtin.

@mathetake
Copy link
Contributor Author

@andrewrk @kubkon I would appreciate it if you have any chance to take a look and upstream this! Thanks a lot:)

@andrewrk
Copy link
Member

Ah I'm sorry for taking so long to get to this! Looking now.

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 for the contribution!

I have some requested improvements.

lib/std/start.zig Outdated Show resolved Hide resolved
src/wasi_libc.zig Outdated Show resolved Hide resolved
src/wasi_libc.zig Outdated Show resolved Hide resolved
src/Compilation.zig Outdated Show resolved Hide resolved
src/link.zig Outdated Show resolved Hide resolved
src/main.zig Outdated Show resolved Hide resolved
src/main.zig Outdated Show resolved Hide resolved
.command => CRTFile.crt1_command_o,
}
else
CRTFile.crt1_o;
Copy link
Member

Choose a reason for hiding this comment

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

Based on my research it looks like every WASI program is either a command or a reactor. Is it meaningful for a program to be neither? Why is crt1_o even an option?

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 we should remove crt1_o in favor of crt1_command_o, where the latter lets wasm-ld call ctors rather than explicitly calling in crt.c. Also I believe this reduces the complexity here about null vs default stuff.

so I removed the support for crt1_o. cc @kubkon wdyt?

src/Compilation.zig Show resolved Hide resolved
also drop the support for the old crt1.o in favor of crt1-command.o

Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
@mathetake
Copy link
Contributor Author

Thank you so much @andrewrk, I hope I addressed your suggestion in 85b5503, and now it looks much better:)

PTAL when you have time, thanks!

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 for making the requested changes - just a couple small things left and then this looks ready to merge to me :)

lib/std/start.zig Outdated Show resolved Hide resolved
src/Compilation.zig Outdated Show resolved Hide resolved
src/wasi_libc.zig Outdated Show resolved Hide resolved
Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
@mathetake
Copy link
Contributor Author

thanks for the quick review! addressed 943f656 here :)

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.

Alright, this is ready to be merged, did you want to wait until @kubkon wakes up to discuss the question you asked? If not I'm happy to merge now.

@mathetake
Copy link
Contributor Author

I can discuss offline with @kubkon later, so I'm happy to merge now. I believe it's easy to bring crt1.o support even if we want later:)

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 reactors
3 participants