Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

Fix miscompile in SimplifyBranchSame #77549

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
Collaborator

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
Contributor

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
Contributor

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
Contributor

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in 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