codegen "unreachable" for invalid SetDiscriminant #67054
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
rust-highfive
added
the
S-waiting-on-review
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
Well that is curious: |
LLVM |
eddyb
reviewed
Dec 5, 2019
| @@ -333,6 +333,7 @@ impl<'a, 'tcx, V: CodegenObject> PlaceRef<'tcx, V> { | |||
| variant_index: VariantIdx | |||
| ) { | |||
| if self.layout.for_variant(bx.cx(), variant_index).abi.is_uninhabited() { | |||
| bx.unreachable(); | |||
There was a problem hiding this comment.
For now I made this an abort, also matching what we do for uninhabited returns. But it seems odd to me that we'd give those well-defined "trap" semantics as opposed to exploiting their UB.
That explains the first case (the |
RalfJung
commented
Dec 5, 2019
Comment on lines
264
to
266
| // We play it safe by using a well-defined `abort`, and not immediate UB. | ||
| bx.abort(); |
There was a problem hiding this comment.
I am documenting existing behavior here, but my own inclination would be to remove the abort here to give the optimizer some more information. Or should we wait until someone determines that this is actually costing us some optimizations?
RalfJung
force-pushed
the
set-discriminant-unreachable
branch
from
a744fca to
f5bd947
Compare
|
One difference between the two is that I don't know what difference that makes though. Technically it should suffice to just emit unreachables. |
|
The problem is that emitting |
|
Maybe emitting Alternatively we could add some scheme where an |
|
The builder has a notion of "current block", right? Maybe after calling But I don't know enough about our codegen APIs to implement any of these proposals. |
|
Let's get this merged and I'll push making all of the traps @bors r+ |
|
📌 Commit f5bd947 has been approved by |
bors
added
S-waiting-on-bors
|
Not all of them I hope. ;) The |
|
😂 right, only those that are unreachable at runtime |
|
Hang on, none of us know why the aborts are there!
I didn't check but maybe LLVM produces an assertion if they are enabled (do you have them on?). As for "poisoning the builder"... Yeah that massive complication which we had for a very long time and which was made unnecessary by MIR 🤣 |
We can also monomorphize the MIR, rerun optimizations and have some guaranteed optimizations that cause no such weird code to even be given to llvm. The monorphized MIR can then be thrown away right after codegen. No need to keep it around |
Fair. OTOH, given there is no comment in the code, if there was anything subtle going on then someone seriously screwed up by not documenting it right... |
JohnTitor
added a commit
to JohnTitor/rust
that referenced
this pull request
Dec 6, 2019
…le, r=oli-obk codegen "unreachable" for invalid SetDiscriminant Follow-up from rust-lang#66960. I also realized I don't understand our policy for using `abort` vs `unreachable`. AFAIK `abort` is safe to call and just aborts the process, while `unreachable` is UB. But sometimes we use both, like here https://github.com/rust-lang/rust/blob/d825e35ee8325146e6c175a4c61bcb645b347d5e/src/librustc_codegen_ssa/mir/block.rs#L827-L828 and here https://github.com/rust-lang/rust/blob/d825e35ee8325146e6c175a4c61bcb645b347d5e/src/librustc_codegen_ssa/mir/block.rs#L264-L265 The second case is even more confusing because that looks like an unreachable `return` to me, so why would we codegen a safe abort there? r? @eddyb Cc @oli-obk
bors
added a commit
that referenced
this pull request
Dec 6, 2019
Rollup of 11 pull requests Successful merges: - #66846 (Make try_mark_previous_green aware of cycles.) - #66959 (Remove potential cfgs duplicates) - #66988 (Fix angle bracket formatting when dumping MIR debug vars) - #66998 (Modified the testcases for VxWorks) - #67008 (rustdoc: Add test for fixed issue) - #67023 (SGX: Fix target linker used by bootstrap) - #67033 (Migrate to LLVM{Get,Set}ValueName2) - #67049 (Simplify {IoSlice, IoSliceMut}::advance examples and tests) - #67054 (codegen "unreachable" for invalid SetDiscriminant) - #67081 (Fix Query type docs) - #67085 (Remove boxed closures in address parser.) Failed merges: r? @ghost
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.
Follow-up from #66960. I also realized I don't understand our policy for using
abortvsunreachable. AFAIKabortis safe to call and just aborts the process, whileunreachableis UB. But sometimes we use both, like hererust/src/librustc_codegen_ssa/mir/block.rs
Lines 827 to 828 in d825e35
and here
rust/src/librustc_codegen_ssa/mir/block.rs
Lines 264 to 265 in d825e35
The second case is even more confusing because that looks like an unreachable
returnto me, so why would we codegen a safe abort there?r? @eddyb Cc @oli-obk