Make the success arms of if lhs || rhs meet up in a separate block
#121784
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
In the previous code, the success block of `lhs` would jump directly to the success block of `rhs`. However, `rhs_success_block` could already contain statements that are specific to the RHS, and the direct goto causes them to be executed in the LHS success path as well. This patch therefore creates a fresh block that the LHS and RHS success blocks can both jump to.
rustbot
added
S-waiting-on-review
|
Some changes occurred in match lowering cc @Nadrieril |
|
cc @dingxiangfei2009, do you know whether the current behaviour is intentional or not? |
|
Before and after MIR graphs for BEFORE
AFTER
Block 8 gets split into blocks 8 and 9, because the StorageDead statements are no longer on the LHS success path. |
|
Hey @Zalathar. That is indeed an oversight from my side and it was not intentional. Let's add the success pads as you have set up in this PR. |
|
LGTM r? @Nadrieril @bors r+ rollup=always |
bors
added
S-waiting-on-bors
workingjubilee
added a commit
to workingjubilee/rustc
that referenced
this pull request
Mar 1, 2024
Make the success arms of `if lhs || rhs` meet up in a separate block Extracted from rust-lang#118305, where this is necessary to avoid introducing a bug when injecting marker statements into the then/else arms. --- In the previous code (rust-lang#111752), the success block of `lhs` would jump directly to the success block of `rhs`. However, `rhs_success_block` could already contain statements that are specific to the RHS, and the direct goto causes them to be executed in the LHS success path as well. This patch therefore creates a fresh block that the LHS and RHS success blocks can both jump to. --- I think the reason we currently get away with this is that `rhs_success_block` usually doesn't contain anything other than StorageDead statements for locals used by the RHS, and those statements don't seem to cause problems in the LHS success path (which never makes those locals live). But if we start adding meaningful statements for branch coverage (or MC/DC coverage), it's important to keep the LHS and RHS blocks separate.
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Mar 1, 2024
…kingjubilee Rollup of 9 pull requests Successful merges: - rust-lang#117156 (Convert `Unix{Datagram,Stream}::{set_}passcred()` to per-OS traits) - rust-lang#119199 (Add arm64ec-pc-windows-msvc target) - rust-lang#120468 (Add a new `wasm32-wasip1` target to rustc) - rust-lang#120504 (Vec::try_with_capacity) - rust-lang#121416 (Improve error messages for generics with default parameters) - rust-lang#121475 (Add tidy check for .stderr/.stdout files for non-existent test revisions) - rust-lang#121736 (Remove `Mutex::unlock` Function) - rust-lang#121784 (Make the success arms of `if lhs || rhs` meet up in a separate block) - rust-lang#121818 (CFI: Remove unused `typeid_for_fnsig`) r? `@ghost` `@rustbot` modify labels: rollup
workingjubilee
added a commit
to workingjubilee/rustc
that referenced
this pull request
Mar 1, 2024
Make the success arms of `if lhs || rhs` meet up in a separate block Extracted from rust-lang#118305, where this is necessary to avoid introducing a bug when injecting marker statements into the then/else arms. --- In the previous code (rust-lang#111752), the success block of `lhs` would jump directly to the success block of `rhs`. However, `rhs_success_block` could already contain statements that are specific to the RHS, and the direct goto causes them to be executed in the LHS success path as well. This patch therefore creates a fresh block that the LHS and RHS success blocks can both jump to. --- I think the reason we currently get away with this is that `rhs_success_block` usually doesn't contain anything other than StorageDead statements for locals used by the RHS, and those statements don't seem to cause problems in the LHS success path (which never makes those locals live). But if we start adding meaningful statements for branch coverage (or MC/DC coverage), it's important to keep the LHS and RHS blocks separate.
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Mar 1, 2024
…kingjubilee Rollup of 8 pull requests Successful merges: - rust-lang#117156 (Convert `Unix{Datagram,Stream}::{set_}passcred()` to per-OS traits) - rust-lang#119199 (Add arm64ec-pc-windows-msvc target) - rust-lang#120468 (Add a new `wasm32-wasip1` target to rustc) - rust-lang#121416 (Improve error messages for generics with default parameters) - rust-lang#121475 (Add tidy check for .stderr/.stdout files for non-existent test revisions) - rust-lang#121736 (Remove `Mutex::unlock` Function) - rust-lang#121784 (Make the success arms of `if lhs || rhs` meet up in a separate block) - rust-lang#121818 (CFI: Remove unused `typeid_for_fnsig`) r? `@ghost` `@rustbot` modify labels: rollup
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
Mar 1, 2024
Make the success arms of `if lhs || rhs` meet up in a separate block Extracted from rust-lang#118305, where this is necessary to avoid introducing a bug when injecting marker statements into the then/else arms. --- In the previous code (rust-lang#111752), the success block of `lhs` would jump directly to the success block of `rhs`. However, `rhs_success_block` could already contain statements that are specific to the RHS, and the direct goto causes them to be executed in the LHS success path as well. This patch therefore creates a fresh block that the LHS and RHS success blocks can both jump to. --- I think the reason we currently get away with this is that `rhs_success_block` usually doesn't contain anything other than StorageDead statements for locals used by the RHS, and those statements don't seem to cause problems in the LHS success path (which never makes those locals live). But if we start adding meaningful statements for branch coverage (or MC/DC coverage), it's important to keep the LHS and RHS blocks separate.
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
Mar 1, 2024
Make the success arms of `if lhs || rhs` meet up in a separate block Extracted from rust-lang#118305, where this is necessary to avoid introducing a bug when injecting marker statements into the then/else arms. --- In the previous code (rust-lang#111752), the success block of `lhs` would jump directly to the success block of `rhs`. However, `rhs_success_block` could already contain statements that are specific to the RHS, and the direct goto causes them to be executed in the LHS success path as well. This patch therefore creates a fresh block that the LHS and RHS success blocks can both jump to. --- I think the reason we currently get away with this is that `rhs_success_block` usually doesn't contain anything other than StorageDead statements for locals used by the RHS, and those statements don't seem to cause problems in the LHS success path (which never makes those locals live). But if we start adding meaningful statements for branch coverage (or MC/DC coverage), it's important to keep the LHS and RHS blocks separate.
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Mar 1, 2024
…iaskrgr Rollup of 10 pull requests Successful merges: - rust-lang#119199 (Add arm64ec-pc-windows-msvc target) - rust-lang#121416 (Improve error messages for generics with default parameters) - rust-lang#121475 (Add tidy check for .stderr/.stdout files for non-existent test revisions) - rust-lang#121736 (Remove `Mutex::unlock` Function) - rust-lang#121744 (Stop using Bubble in coherence and instead emulate it with an intercrte check) - rust-lang#121784 (Make the success arms of `if lhs || rhs` meet up in a separate block) - rust-lang#121818 (CFI: Remove unused `typeid_for_fnsig`) - rust-lang#121819 (Handle stashing of delayed bugs) - rust-lang#121828 (Remove unused fluent messages) - rust-lang#121831 (Fix typo in comment) r? `@ghost` `@rustbot` modify labels: rollup
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
Mar 1, 2024
Make the success arms of `if lhs || rhs` meet up in a separate block Extracted from rust-lang#118305, where this is necessary to avoid introducing a bug when injecting marker statements into the then/else arms. --- In the previous code (rust-lang#111752), the success block of `lhs` would jump directly to the success block of `rhs`. However, `rhs_success_block` could already contain statements that are specific to the RHS, and the direct goto causes them to be executed in the LHS success path as well. This patch therefore creates a fresh block that the LHS and RHS success blocks can both jump to. --- I think the reason we currently get away with this is that `rhs_success_block` usually doesn't contain anything other than StorageDead statements for locals used by the RHS, and those statements don't seem to cause problems in the LHS success path (which never makes those locals live). But if we start adding meaningful statements for branch coverage (or MC/DC coverage), it's important to keep the LHS and RHS blocks separate.
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Mar 1, 2024
…iaskrgr Rollup of 9 pull requests Successful merges: - rust-lang#119199 (Add arm64ec-pc-windows-msvc target) - rust-lang#121416 (Improve error messages for generics with default parameters) - rust-lang#121475 (Add tidy check for .stderr/.stdout files for non-existent test revisions) - rust-lang#121736 (Remove `Mutex::unlock` Function) - rust-lang#121784 (Make the success arms of `if lhs || rhs` meet up in a separate block) - rust-lang#121818 (CFI: Remove unused `typeid_for_fnsig`) - rust-lang#121819 (Handle stashing of delayed bugs) - rust-lang#121828 (Remove unused fluent messages) - rust-lang#121831 (Fix typo in comment) r? `@ghost` `@rustbot` modify labels: rollup
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Mar 1, 2024
…iaskrgr Rollup of 12 pull requests Successful merges: - rust-lang#120646 (Fix incorrect suggestion for uninitialized binding in pattern) - rust-lang#121416 (Improve error messages for generics with default parameters) - rust-lang#121475 (Add tidy check for .stderr/.stdout files for non-existent test revisions) - rust-lang#121580 (make unused_imports less assertive in test modules) - rust-lang#121736 (Remove `Mutex::unlock` Function) - rust-lang#121784 (Make the success arms of `if lhs || rhs` meet up in a separate block) - rust-lang#121818 (CFI: Remove unused `typeid_for_fnsig`) - rust-lang#121819 (Handle stashing of delayed bugs) - rust-lang#121828 (Remove unused fluent messages) - rust-lang#121831 (Fix typo in comment) - rust-lang#121850 (Make `ZeroablePrimitive` trait unsafe.) - rust-lang#121853 (normalizes-to: handle negative impls) r? `@ghost` `@rustbot` modify labels: rollup
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Mar 1, 2024
Rollup merge of rust-lang#121784 - Zalathar:if-or-converge, r=Nadrieril Make the success arms of `if lhs || rhs` meet up in a separate block Extracted from rust-lang#118305, where this is necessary to avoid introducing a bug when injecting marker statements into the then/else arms. --- In the previous code (rust-lang#111752), the success block of `lhs` would jump directly to the success block of `rhs`. However, `rhs_success_block` could already contain statements that are specific to the RHS, and the direct goto causes them to be executed in the LHS success path as well. This patch therefore creates a fresh block that the LHS and RHS success blocks can both jump to. --- I think the reason we currently get away with this is that `rhs_success_block` usually doesn't contain anything other than StorageDead statements for locals used by the RHS, and those statements don't seem to cause problems in the LHS success path (which never makes those locals live). But if we start adding meaningful statements for branch coverage (or MC/DC coverage), it's important to keep the LHS and RHS blocks separate.
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.
Extracted from #118305, where this is necessary to avoid introducing a bug when injecting marker statements into the then/else arms.
In the previous code (#111752), the success block of
lhswould jump directly to the success block ofrhs. However,rhs_success_blockcould already contain statements that are specific to the RHS, and the direct goto causes them to be executed in the LHS success path as well.This patch therefore creates a fresh block that the LHS and RHS success blocks can both jump to.
I think the reason we currently get away with this is that
rhs_success_blockusually doesn't contain anything other than StorageDead statements for locals used by the RHS, and those statements don't seem to cause problems in the LHS success path (which never makes those locals live).But if we start adding meaningful statements for branch coverage (or MC/DC coverage), it's important to keep the LHS and RHS blocks separate.