fix: relax unwind safety requirements #185
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
As far as I understand unwind safety in rust, this should be safe. First of all, because unwind "unsafety" in rust is already safe anyway, as shown by the absence of `unsafe` keyword. What it tries to prevent is witnessing a broken invariant in a capture, after a panic. Here, we replace `RefUnwindSafe` with an `AssertUnwindSafe` that is exclusively set on a value that will never be reused after the closure ends. Hence, to the best of my understanding this still keeps the informal contract of unwind safety in rust. In practice, it allows using wasm-smith’s Arbitrary implementation with `with_arbitrary`, without having to take bytes as arguments. Without that, wasm-smith’s ConfiguredModule contains elements that are not unwind safe, and would be rejected. I would add that the error message is particularly cryptic, and figuring out that unwind safety was the source of the issue required me to dig into bolero’s source code and use the fully-qualified method call syntax, because without that the full error would not be shown. It might be possible to hoist the remaining type checks to be earlier in the call chain (eg. at the with_arbitrary stage), but I haven’t investigated much deeper yet.
Ekleog-NEAR
force-pushed
the
relax-unwind-safety
branch
from
764f9dc
to
9931f5f
Compare
camshaft
changed the title
camshaft
approved these changes
Sep 27, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As far as I understand unwind safety in rust, this should be safe. First
of all, because unwind "unsafety" in rust is already safe anyway, as
shown by the absence of
unsafekeyword.What it tries to prevent is witnessing a broken invariant in a capture,
after a panic.
Here, we replace
RefUnwindSafewith anAssertUnwindSafethat isexclusively set on a value that will never be reused after the closure
ends.
Hence, to the best of my understanding this still keeps the informal
contract of unwind safety in rust.
In practice, it allows using wasm-smith’s Arbitrary implementation with
with_arbitrary, without having to take bytes as arguments. Withoutthat, wasm-smith’s ConfiguredModule contains elements that are not
unwind safe, and would be rejected.
I would add that the error message is particularly cryptic, and figuring
out that unwind safety was the source of the issue required me to dig
into bolero’s source code and use the fully-qualified method call
syntax, because without that the full error would not be shown.
It might be possible to hoist the remaining type checks to be earlier in
the call chain (eg. at the with_arbitrary stage), but I haven’t
investigated much deeper yet.