Skip to content

@trap fixups#14799

Merged
andrewrk merged 6 commits intomasterfrom
update-zig1
Mar 6, 2023
Merged

@trap fixups#14799
andrewrk merged 6 commits intomasterfrom
update-zig1

Conversation

@andrewrk
Copy link
Member

@andrewrk andrewrk commented Mar 4, 2023

follow-up to #14782

@andrewrk andrewrk enabled auto-merge March 4, 2023 21:50
_ = 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

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

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.

#define zig_trap() __asm__ volatile("ud2");
#else
#define zig_trap() raise(SIGILL)
#define zig_trap() raise(SIGTRAP)
Copy link

@ghost ghost Mar 4, 2023

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

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.

andrewrk added a commit that referenced this pull request Mar 5, 2023
Perhaps this would have caught the problem we are seeing in #14799.
andrewrk added a commit that referenced this pull request Mar 5, 2023
Perhaps this would have caught the problem we are seeing in #14799.
andrewrk added a commit that referenced this pull request Mar 6, 2023
Perhaps this would have caught the problem we are seeing in #14799.
@andrewrk andrewrk merged commit ac1b0e8 into master Mar 6, 2023
@andrewrk andrewrk deleted the update-zig1 branch March 6, 2023 06:29
andrewrk added a commit that referenced this pull request Mar 6, 2023
Perhaps this would have caught the problem we are seeing in #14799.
andrewrk added a commit that referenced this pull request Mar 8, 2023
This would have caught the problem we are seeing in #14799.
andrewrk added a commit that referenced this pull request Mar 8, 2023
This would have caught the problem we are seeing in #14799.
andrewrk added a commit that referenced this pull request Mar 9, 2023
This would have caught the problem we are seeing in #14799.
truemedian pushed a commit to truemedian/zig that referenced this pull request Mar 30, 2023
This would have caught the problem we are seeing in ziglang#14799.
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.

1 participant