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

Intercept panic calls and replace them with aborts. #305

Merged
merged 6 commits into from
Dec 3, 2020

Conversation

eddyb
Copy link
Contributor

@eddyb eddyb commented Dec 2, 2020

Where "abort" currently is equivalent to loop {}, but we could try to do something better in the future.

(Ideally we'd use a future variant of OpKill that works in all shaders, and ideally has the right semantics of leaving some kind of indication that there was an error in some shader invocation, but that might be asking for too much)


The reason this is "WIP", even if it does work (see the test changes), is because, in order to defer errors from creating unsupported constants (mostly &panic::Location) passed to panic entry-points, I had to turn them into zombies.
So the tests only work because the panic calls are replaced with "abort"s, and the zombie constants never used.

From my discussion with @khyperia, we should be able to use zombies here (and in other places), if we can properly track Spans for them, and I believe that's doable, but haven't gotten around to it.

EDIT: that's all done, thanks to #311!

@eddyb eddyb requested a review from khyperia December 2, 2020 22:09
@eddyb eddyb force-pushed the panic-at-the-spirsco branch from cafb25f to d2203a6 Compare December 3, 2020 17:41
@eddyb eddyb changed the title [WIP] Intercept panic calls and replace them with aborts. Intercept panic calls and replace them with aborts. Dec 3, 2020
@eddyb eddyb marked this pull request as ready for review December 3, 2020 17:42
@mergify mergify bot merged commit 6c7ca97 into EmbarkStudios:main Dec 3, 2020
@eddyb eddyb deleted the panic-at-the-spirsco branch December 4, 2020 05:02
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.

2 participants