Skip to content

Fix miscompile in SimplifyBranchSame #77549

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom
Merged
merged 2 commits into from
Oct 5, 2020
Merged

Fix miscompile in SimplifyBranchSame #77549

merged 2 commits into from
Oct 5, 2020

Conversation

tmiasko
Copy link
Contributor

@tmiasko tmiasko commented Oct 4, 2020

Cherry-picked from #77486, but with a different test case that used to be compiled incorrectly on both master & beta branches.

@rust-highfive
Copy link
Contributor

r? @davidtwco

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 4, 2020
@tmiasko tmiasko mentioned this pull request Oct 4, 2020
Closed
@Mark-Simulacrum
Copy link
Member

cc @rust-lang/wg-mir-opt, would be great to get a quick review here from someone familiar with the code and a consideration of whether it makes sense to backport this PR or something disabling the optimization entirely

@oli-obk
Copy link
Contributor

oli-obk commented Oct 5, 2020

@bors r+

I agree that the change fixes this bug.

Without digging into the MIR at various stages I cannot tell for sure why/how macro expansion influences this. I'm guessing that it's the Span in an Operand::Const (https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/mir/struct.Constant.html) which we then accidentally compare for equality.

I think we should just beta backport this as it is a clearly the right fix and a trivial change.

Since beta to stable promotion is happening very soon:

cc @rust-lang/compiler emergency backport approval needed here (alternative is to disable this optimization and take a perf hit somewhere, but we need some backport).

@bors
Copy link
Collaborator

bors commented Oct 5, 2020

📌 Commit f271957 has been approved by oli-obk

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 5, 2020
@oli-obk oli-obk added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Oct 5, 2020
@oli-obk
Copy link
Contributor

oli-obk commented Oct 5, 2020

@bors p=6

@lcnr
Copy link
Contributor

lcnr commented Oct 5, 2020

Without digging into the MIR at various stages I cannot tell for sure why/how macro expansion influences this. I'm guessing that it's the Span in an Operand::Const (https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/mir/struct.Constant.html) which we then accidentally compare for equality.

yeah, we compare the Span in the Constant here afaict

I think beta backporting this is fine

@bors
Copy link
Collaborator

bors commented Oct 5, 2020

⌛ Testing commit f271957 with merge d890e64...

@Mark-Simulacrum
Copy link
Member

Okay, beta-accepting. Will include in stable later today.

@Mark-Simulacrum Mark-Simulacrum added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Oct 5, 2020
@bors
Copy link
Collaborator

bors commented Oct 5, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: oli-obk
Pushing d890e64 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 5, 2020
@bors bors merged commit d890e64 into rust-lang:master Oct 5, 2020
@rustbot rustbot added this to the 1.49.0 milestone Oct 5, 2020
@tmiasko tmiasko deleted the simplify-branch-same-fix branch October 5, 2020 14:01
@Mark-Simulacrum Mark-Simulacrum mentioned this pull request Oct 5, 2020
Merged
@simonvandel simonvandel mentioned this pull request Oct 5, 2020
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 5, 2020
@bors
Auto merge of rust-lang#77577 - Mark-Simulacrum:stable-next, r=pietro…
90ed0a8 
…albini

[stable] 1.47 release

This PR includes backports of:

* Fix miscompile in SimplifyBranchSame rust-lang#77549
* Force posix-style quoting on lld, independent of host platform rust-lang#77543

Note that both are still beta-nominated/beta-accepted, as they need to be backported to 1.48 as well (future beta branch).
@oli-obk oli-obk mentioned this pull request Oct 6, 2020
Open
@Mark-Simulacrum Mark-Simulacrum removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Oct 8, 2020
@Mark-Simulacrum Mark-Simulacrum removed this from the 1.49.0 milestone Oct 8, 2020
@Mark-Simulacrum Mark-Simulacrum added this to the 1.48.0 milestone Oct 8, 2020
@Mark-Simulacrum Mark-Simulacrum mentioned this pull request Oct 8, 2020
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 8, 2020
@bors
Auto merge of rust-lang#77705 - Mark-Simulacrum:beta-next, r=Mark-Sim…
212e76c 
…ulacrum

[beta] backports

This backports the following:

* Improve build-manifest to work with the improved promote-release rust-lang#77407
* Force posix-style quoting on lld, independent of host platform rust-lang#77543
* Fix miscompile in SimplifyBranchSame rust-lang#77549
* Update RLS and Rustfmt rust-lang#77590
* Move `EarlyOtherwiseBranch` to mir-opt-level 2 rust-lang#77582
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@oli-obk oli-obk oli-obk left review comments

@Mark-Simulacrum Mark-Simulacrum Mark-Simulacrum left review comments

@lcnr lcnr lcnr left review comments

Assignees

@davidtwco davidtwco

Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.48.0
Development

Successfully merging this pull request may close these issues.
9 participants
@tmiasko @rust-highfive @Mark-Simulacrum @oli-obk @bors @lcnr @davidtwco @rustbot @simonvandel