Skip to content

@trap fixups #14799

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.

Sign up for GitHub

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

Merged
andrewrk merged 6 commits into from
Mar 6, 2023
Merged

@trap fixups #14799

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
34a23db
zig.h: lower trap to SIGTRAP instead of SIGILL
andrewrk Mar 4, 2023
fb04ff4
langref: small clarification to `@trap`
andrewrk Mar 4, 2023
48e7296
llvm: fix lowering of `@trap`
andrewrk Mar 4, 2023
c839c18
stage2: add zig_backend to ZIR cache namespace
andrewrk Mar 5, 2023
cdb9cc8
update zig1.wasm
andrewrk Mar 4, 2023
8558983
std.os.abort: take advantage of `@trap`
andrewrk Mar 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion doc/langref.html.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9403,7 +9403,7 @@ fn List(comptime T: type) type {
Unlike for {#syntax#}@breakpoint(){#endsyntax#}, execution does not continue after this point.
</p>
<p>
This function is only valid within function scope.
Outside function scope, this builtin causes a compile error.
</p>
{#see_also|@breakpoint#}
{#header_close#}
Expand Down
16 changes: 3 additions & 13 deletions lib/std/os.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -575,22 +575,12 @@ pub fn abort() noreturn {
raise(SIG.KILL) catch {};
exit(127); // Pid 1 might not be signalled in some containers.
}
if (builtin.os.tag == .uefi) {
exit(0); // TODO choose appropriate exit code
}
if (builtin.os.tag == .wasi) {
exit(1);
}
if (builtin.os.tag == .cuda) {
// TODO: introduce `@trap` instead of abusing https://github.com/ziglang/zig/issues/2291
@"llvm.trap"();
switch (builtin.os.tag) {
.uefi, .wasi, .cuda => @trap(),
else => system.abort(),
}

system.abort();
}

extern fn @"llvm.trap"() noreturn;

pub const RaiseError = UnexpectedError;

pub fn raise(sig: u8) RaiseError!void {
Expand Down
2 changes: 1 addition & 1 deletion lib/zig.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@ typedef char bool;
#elif defined(__i386__) || defined(__x86_64__)
#define zig_trap() __asm__ volatile("ud2");
#else
#define zig_trap() raise(SIGILL)
#define zig_trap() raise(SIGTRAP)
Copy link

@ghost ghost Mar 4, 2023 •
edited by ghost
Loading

Choose a reason for hiding this comment

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

I've previously explained my reasoning for SIGILL but now I think SIGILL is not much better or as good as SIGTRAP, anyway.

After some more research, now I would like to give out my recommendation to maybe use SIGSTOP or SIGKILL here as they are AFAIK the only signals that cannot be caught which I believe would be more appropriate here? I was going to demonstrate how they cannot be caught but these two asserts are perhaps demonstrative enough:

zig/lib/std/os/linux.zig

Lines 1160 to 1161 in e7f128c

assert(sig != SIG.KILL);
assert(sig != SIG.STOP);

Raising signals that cannot be caught might be more appropriate as trap/@trap is not a debugging tool but rather something that should in fact terminate the program for sure, as far as I understand it.

The difference between SIGSTOP and SIGKILL seems to be that SIGSTOP actually stops the program for later resumption which seems inappropriate so perhaps SIGKILL is best and safest of all here.

Copy link
Member Author

@andrewrk andrewrk Mar 5, 2023

Choose a reason for hiding this comment

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

I think "trap" is the best signal because it's called "trap". If the world doesn't treat it that way, the world is broken.

#endif

#if zig_has_builtin(debugtrap)
Expand Down
1 change: 1 addition & 0 deletions src/Module.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3528,6 +3528,7 @@ pub fn astGenFile(mod: *Module, file: *File) !void {
const digest = hash: {
var path_hash: Cache.HashHelper = .{};
path_hash.addBytes(build_options.version);
path_hash.add(builtin.zig_backend);
if (!want_local_cache) {
path_hash.addOptionalBytes(file.pkg.root_src_directory.path);
}
Expand Down
1 change: 1 addition & 0 deletions src/codegen/llvm.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8261,6 +8261,7 @@ pub const FuncGen = struct {
_ = inst;
const llvm_fn = self.getIntrinsic("llvm.trap", &.{});
_ = self.builder.buildCall(llvm_fn.globalGetValueType(), llvm_fn, undefined, 0, .Cold, .Auto, "");
_ = self.builder.buildUnreachable();
Copy link

@ghost ghost Mar 4, 2023 •
edited by ghost
Loading

Choose a reason for hiding this comment

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

Oh, sorry about that! That's why I was seeing Basic Block in function 'trap.main' does not have terminator!--thought maybe that only happened because I compiled in debug mode. It still gave me the LLVM IR and all so I thought it would be fine.

BTW, the CI just failed and it's the exact same CI failure that I've been getting on my two PRs where I previously updated zig1.wasm (but later removed those changes) (#13998 and #14782). I'm guessing I did follow the procedure the intended way after all if you're getting the same failure.
Because I got the same failure after updating zig1.wasm on two entirely unrelated PRs, I believe it's some issues directly related to that whole stage1 stuff, unrelated to @trap or something.

Copy link
Member Author

@andrewrk andrewrk Mar 5, 2023

Choose a reason for hiding this comment

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

I opened #14802 to hopefully prevent this from happening again. I haven't diagnosed the failure yet but I suspect it is likely caused by some of the recent changes to the C backend, or perhaps due to a limitation of our C WASI shims.

I'm sorry that happened to you and others, that must have been very confusing.

return null;
}

Expand Down
Loading