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.

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

Function arguments should never get promoted #59724

Merged
merged 4 commits into from
Apr 8, 2019

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Apr 5, 2019

fixes #59469

@rust-highfive
Copy link
Collaborator

r? @matthewjasper

(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 Apr 5, 2019
@oli-obk
Copy link
Contributor Author

oli-obk commented Apr 5, 2019

r? @eddyb

@@ -639,7 +639,7 @@ impl<'a, 'tcx> Checker<'a, 'tcx> {
per_local.insert(local);
}
}
cx.per_local[IsNotPromotable].insert(local);
cx.per_local[IsNotConst].insert(local);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should do this for all locals where if !temps[local].is_promotable().

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I meant is that we should do this for all locals, not just temps.
That is, don't do it on a case-by-case basis in this match, but rather have this after it:

            if !temps[local].is_promotable() {
                cx.per_local[IsNotConst].insert(local);
            }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a change that I don't want to backport. are you alright with r+ing this PR and I'll address this immediately in a master-only PR?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a risk of there being more issues we haven't hit due to this not being general enough.
This is not just code cleanup, I think right now this is kind of broken.

@jonas-schievink jonas-schievink added beta-nominated Nominated for backporting to the compiler in the beta channel. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 5, 2019
@pnkfelix
Copy link
Member

pnkfelix commented Apr 5, 2019

As an experiment, I tried backporting this change to the beta branch, to double-check that it actually addresses the regression.

One potential issue is that the regression tests may not themselves be immediately backportable as written, because I do not think #59044 is in the beta branch. Just a warning.

@pnkfelix
Copy link
Member

pnkfelix commented Apr 5, 2019

(commit ccc0cd8 itself does seem to address the key problem in the beta branch.)

@oli-obk @eddyb Regarding the beta-nomination, I do not think we can afford to wait until the next rustc meeting. Do you think we should go ahead and beta-accept this now? Or wait until @nikomatsakis is back (on monday I think)?

(To be absolutely clear, I have no personal problem with backporting ccc0cd8 to beta. Though it might be good to explore whether the generalization suggested by @eddyb in their review is what really needs to be backported.)

@oli-obk
Copy link
Contributor Author

oli-obk commented Apr 5, 2019

I think we should beta accept this without a full compiler team consensus as it's small and important to get fixed. There seems no alternative to me.

I can create a beta backport that regenerates the ui output the way beta expects it.

@Centril
Copy link
Contributor

Centril commented Apr 6, 2019

There seems no alternative to me.

Yes; absolutely. I would really like to avoid a C-future-compatibility warning due to a failure to backport.

@@ -639,15 +639,15 @@ impl<'a, 'tcx> Checker<'a, 'tcx> {
per_local.insert(local);
}
}
cx.per_local[IsNotPromotable].insert(local);
cx.per_local[IsNotConst].insert(local);
}

LocalKind::Var if mode == Mode::Fn => {
cx.per_local[IsNotConst].insert(local);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like, does this mean we'll try to promote a Var in a const fn?!

@@ -817,15 +817,15 @@ impl<'a, 'tcx> Checker<'a, 'tcx> {
}
}

// Ensure the `IsNotPromotable` qualification is preserved.
// Ensure the `IsNotConst` qualification is preserved.
// NOTE(eddyb) this is actually unnecessary right now, as
// we never replace the local's qualif, but we might in
// the future, and so it serves to catch changes that unset
// important bits (in which case, asserting `contains` could
// be replaced with calling `insert` to re-set the bit).
if kind == LocalKind::Temp {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, here, this seems risky, why don't we do this for all locals?
I think relying on the Var / Temp distinction was a bit of a mistake... I mean, here, as opposed to in the analysis in promote_consts.

@eddyb
Copy link
Member

eddyb commented Apr 6, 2019

r=me with or without #59724 (comment) / #59724 (comment) resolved

For runtime code, this PR is probably fine (it marks all args and vars as "not const" - not the return place, but that tends not to ever get borrowed or passed to a function), but for const fn some of this pass might still be bogus.

@pnkfelix
Copy link
Member

pnkfelix commented Apr 6, 2019

I think we should beta accept this without a full compiler team consensus as it's small and important to get fixed. There seems no alternative to me.

I agree with this and am going to mark the PR as beta-accepted accordingly

@pnkfelix pnkfelix added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Apr 6, 2019
@eddyb
Copy link
Member

eddyb commented Apr 6, 2019

I guess I shouldn't delay this needlessly:
@bors r+ p=10 (needs backport soon)

@bors
Copy link
Contributor

bors commented Apr 6, 2019

📌 Commit cc66b17 has been approved by eddyb

@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 Apr 6, 2019
@bors
Copy link
Contributor

bors commented Apr 6, 2019

⌛ Testing commit cc66b17 with merge f46819ad4835ed97d28d4f2b276314d63551fff7...

@bors
Copy link
Contributor

bors commented Apr 6, 2019

💔 Test failed - checks-travis

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 6, 2019
@bors
Copy link
Contributor

bors commented Apr 7, 2019

📌 Commit 01e8394 has been approved by eddyb

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 7, 2019
@pietroalbini
Copy link
Member

@bors p=1 (needs to be backported)

@pietroalbini pietroalbini removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Apr 7, 2019
@bors
Copy link
Contributor

bors commented Apr 7, 2019

⌛ Testing commit 01e8394 with merge 4976e83b11d48330e77f7aef8c79666fbfffdb4f...

@pietroalbini
Copy link
Member

@bors retry

Yielding priority to the beta backports.

@bors
Copy link
Contributor

bors commented Apr 7, 2019

⌛ Testing commit 01e8394 with merge b6978e58ef44ca7c3b33ccaff8185111416e30ad...

@pietroalbini
Copy link
Member

@bors retry

Yielding priority again to the beta backports.

bors added a commit that referenced this pull request Apr 7, 2019
[beta] Rollup backports

Cherry-picked:

* #58021: Fix fallout from #57667
* #59599: Updated RELEASES.md for 1.34.0
* #59587: Remove #[doc(hidden)] from Error::type_id
* #58994: Hide deprecation warnings inside derive expansions
* #58015: Expand docs for `TryFrom` and `TryInto`.
* #59770: ci: pin android emulator to 28.0.23
* #59704: ci: Update FreeBSD tarball downloads
* #59257: Update CI configuration for building Redox libraries
* #59724: Function arguments should never get promoted

r? @ghost
@Centril
Copy link
Contributor

Centril commented Apr 7, 2019

Moving before the clippy PR, @bors p=15

@bors
Copy link
Contributor

bors commented Apr 7, 2019

⌛ Testing commit 01e8394 with merge 330db3bec5731d012cdcf05ddfe577bd0b828a65...

@Centril
Copy link
Contributor

Centril commented Apr 7, 2019

@bors retry

@rust-highfive
Copy link
Collaborator

Your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
Checking out files: 100% (18109/18109), done.
travis_time:end:18f31c75:start=1554679821140769000,finish=1554679830524436000,duration=9383667000
$ cd rust-lang/rust
$ git checkout -qf 330db3bec5731d012cdcf05ddfe577bd0b828a65
fatal: reference is not a tree: 330db3bec5731d012cdcf05ddfe577bd0b828a65
The command "git checkout -qf 330db3bec5731d012cdcf05ddfe577bd0b828a65" failed and exited with 128 during .
Your build has been stopped.

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 @TimNN. (Feature Requests)

@bors
Copy link
Contributor

bors commented Apr 8, 2019

⌛ Testing commit 01e8394 with merge 3750348...

bors added a commit that referenced this pull request Apr 8, 2019
Function arguments should never get promoted

fixes #59469
@bors
Copy link
Contributor

bors commented Apr 8, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: eddyb
Pushing 3750348 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 8, 2019
@bors bors merged commit 01e8394 into rust-lang:master Apr 8, 2019
bors added a commit that referenced this pull request Apr 8, 2019
[beta] Rollup backports

Cherry-picked:

* #58021: Fix fallout from #57667
* #59599: Updated RELEASES.md for 1.34.0
* #59587: Remove #[doc(hidden)] from Error::type_id
* #58994: Hide deprecation warnings inside derive expansions
* #58015: Expand docs for `TryFrom` and `TryInto`.
* #59770: ci: pin android emulator to 28.0.23
* #59704: ci: Update FreeBSD tarball downloads
* #59257: Update CI configuration for building Redox libraries
* #59724: Function arguments should never get promoted
* #59499: Fix broken download link in the armhf-gnu image
* #58330: Add rustdoc JS non-std tests
* #58848: Prevent cache issues on version updates

r? @ghost
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrapping simd call results in compiler panic (nightly)
10 participants