Uh oh!
There was an error while loading. Please reload this page.
Add support for WASI reactor in pure Zig-exe. - #9178
Conversation
Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
kubkon
left a comment
There was a problem hiding this comment.
Thanks @mathetake! I've got a few comments I'd like to see addressed before going forward with this PR.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
| 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}); |
There was a problem hiding this comment.
FYI: If I put this into a single statement,
trybuffer.writer().print(
\\pub const wasi_exec_model = std.builtin.WasiExecModel.{};\\
, .{if (comp.bin_file.options.wasi_exec_model) |model|std.zig.fmtId(@tagName(model))
elsestd.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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
fyi: I tweaked the C++ and around stage1.zig and Compilation.zig following CodeModel, but the error has never gone.
There was a problem hiding this comment.
Luckily the code with split statements just works and successfully produced Wasi Reactors!
mathetake
commented
Jun 20, 2021
kubkon
left a comment
There was a problem hiding this comment.
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
commented
Jun 28, 2021
andrewrk
commented
Jun 30, 2021
Ah I'm sorry for taking so long to get to this! Looking now. |
andrewrk
left a comment
There was a problem hiding this comment.
Thanks for the contribution!
I have some requested improvements.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| .command => CRTFile.crt1_command_o, | ||
| } | ||
| else | ||
| CRTFile.crt1_o; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
Uh oh!
There was an error while loading. Please reload this page.
also drop the support for the old crt1.o in favor of crt1-command.o Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
mathetake
commented
Jun 30, 2021
andrewrk
left a comment
There was a problem hiding this comment.
Thanks for making the requested changes - just a couple small things left and then this looks ready to merge to me :)
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
mathetake
commented
Jun 30, 2021
thanks for the quick review! addressed 943f656 here :) |
mathetake
commented
Jul 1, 2021
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:) |
This PR adds support for WASI reactor in pure Zig WASI targets. Notably this enables users to produce WASI reactor binary with
build-execommand with-mexec-model=reactorjust likezig cc,zig c++, and clang.With
-mexec-model=reactorflag, Zig 1) exports_initializesymbol which calls the user definedpub fn main, 2) does not define_start, and 3) does not executeproc_exitaftermain.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