Manually test that panicking from C will abort the process#290
Merged
apoelstra merged 1 commit intorust-bitcoin:masterfrom Apr 7, 2021
Merged
Manually test that panicking from C will abort the process#290apoelstra merged 1 commit intorust-bitcoin:masterfrom
apoelstra merged 1 commit intorust-bitcoin:masterfrom
Conversation
real-or-random
force-pushed
the
202103_panic_from_c
branch
from
4095be0 to
e0b3145
Compare
elichai
requested changes
Mar 23, 2021
real-or-random
force-pushed
the
202103_panic_from_c
branch
from
e0b3145 to
b3055d0
Compare
real-or-random
force-pushed
the
202103_panic_from_c
branch
3 times, most recently
from
24df7ce to
d764a43
Compare
Collaborator
Author
|
Okay fixed... I have no idea about WASM builds but AFAIU this currently does not test if the panic works on WASM. |
elichai
reviewed
Mar 24, 2021
real-or-random
force-pushed
the
202103_panic_from_c
branch
2 times, most recently
from
9be4c4e to
b27f0fa
Compare
Panicking from C is not UB in newer rust versions and will reliably trigger an abort (without unwinding). In older rust versions, it is technically UB but empirically it seems to "just work" (and what should it realistically do except crashing, which is what we intent). Since there's potentially no unwinding, we can't test this behavior using [should_panic]. This PR will instead check the libtest output explicitly in our CI tests. Fixes rust-bitcoin#228.
real-or-random
force-pushed
the
202103_panic_from_c
branch
from
b27f0fa to
1eb2c32
Compare
Collaborator
Author
|
Ok this should be really ready for review now... |
elichai
approved these changes
Mar 26, 2021
Member
elichai
left a comment
elichai
left a comment
There was a problem hiding this comment.
ACK
Verified that if it panics for another reason it fails
Verified that if it doesn't panic it also fails
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Panicking from C is not UB in newer rust versions and will reliably
trigger an abort (without unwinding). In older rust versions, it is
technically UB but empirically it seems to "just work" (and what should
it realistically do except crashing, which is what we intent).
Since there's potentially no unwinding, we can't test this behavior
using [should_panic]. This PR will check the exit code instead in our
CI tests.
Fixes #228. This is an alternative to #288.