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

Fix ICE involving calling `Instance.ty` during const evaluation #67800

Merged
merged 7 commits into from Jan 6, 2020

Conversation

@Aaron1011
Copy link
Contributor

Aaron1011 commented Jan 2, 2020

Fixes #67639

Instance.ty assumes that we are in a fully monomorphic context (e.g.
codegen), and can therefore use an empty ParamEnv when performing
normalization. Howver, the MIR constant evaluator code ends up calling
Instance.ty as a result of us attemptign to 'speculatively'
const-evaluate generic functions during const propagation.

As a result,
we may end up with projections involving type parameters
(e.g. ::Bar>) in the type we are trying to normalize.
Normalization expects us to have proper predicates in the ParamEnv for
such projections, and will ICE if we don't.

This commit adds a new method Instance.ty_env, which takes a
ParamEnv for use during normalization. The MIR const-evaluator code is
changed to use this method, passing in the proper ParamEnv for the
context at hand.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Jan 2, 2020

r? @varkor

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

@Aaron1011 Aaron1011 changed the title Fix ICE involving calling `Instance.ty` durign const evaluation Fix ICE involving calling `Instance.ty` during const evaluation Jan 2, 2020
@Aaron1011 Aaron1011 force-pushed the Aaron1011:fix/mir-generic-instance branch from 51b9fa5 to 1e08f9e Jan 2, 2020
@Centril

This comment was marked as resolved.

Copy link
Member

Centril commented Jan 2, 2020

Could you add a regression test?

@Aaron1011 Aaron1011 force-pushed the Aaron1011:fix/mir-generic-instance branch from 1e08f9e to 7e86561 Jan 2, 2020
@Aaron1011

This comment was marked as resolved.

Copy link
Contributor Author

Aaron1011 commented Jan 2, 2020

@Centril: Oops, I forgot to commit it the first time.

src/librustc/ty/instance.rs Outdated Show resolved Hide resolved
@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Jan 3, 2020

The job x86_64-gnu-llvm-7 of your PR failed (pretty log, 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.
2020-01-03T01:39:32.1959938Z ##[command]git remote add origin https://github.com/rust-lang/rust
2020-01-03T01:39:32.7518807Z ##[command]git config gc.auto 0
2020-01-03T01:39:32.7520920Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2020-01-03T01:39:32.7524338Z ##[command]git config --get-all http.proxy
2020-01-03T01:39:32.7527028Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/67800/merge:refs/remotes/pull/67800/merge

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)

@wesleywiser

This comment has been minimized.

Copy link
Member

wesleywiser commented Jan 3, 2020

@bors try

@rust-timer queue

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Jan 3, 2020

Awaiting bors try build completion

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 3, 2020

⌛️ Trying commit b9dcd98 with merge c8783d8...

bors added a commit that referenced this pull request Jan 3, 2020
Fix ICE involving calling `Instance.ty` during const evaluation

Fixes #67639

`Instance.ty` assumes that we are in a fully monomorphic context (e.g.
codegen), and can therefore use an empty `ParamEnv` when performing
normalization. Howver, the MIR constant evaluator code ends up calling
`Instance.ty` as a result of us attemptign to 'speculatively'
const-evaluate generic functions during const propagation.

As a result,
we may end up with projections involving type parameters
(e.g. <T as MyTrait>::Bar>) in the type we are trying to normalize.
Normalization expects us to have proper predicates in the `ParamEnv` for
such projections, and will ICE if we don't.

This commit adds a new method `Instance.ty_env`, which takes a
`ParamEnv` for use during normalization. The MIR const-evaluator code is
changed to use this method, passing in the proper `ParamEnv` for the
context at hand.
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 3, 2020

☀️ Try build successful - checks-azure
Build commit: c8783d8 (c8783d8cf524e2ceaad6d300e62e58e68a6d22cf)

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Jan 3, 2020

Queued c8783d8 with parent 4877e16, future comparison URL.

@Aaron1011

This comment has been minimized.

Copy link
Contributor Author

Aaron1011 commented Jan 3, 2020

For some reason, the bot didn't comment saying that benchmarking completed.

It looks like this change had no perf impact above the noise floor, as I had hoped.

Copy link
Member

wesleywiser left a comment

Code and perf look good to me and the change seems reasonable enough but I don't think I know enough about the resolver to sign off on this myself. Perhaps @oli-obk or someone else could take a look as well?

src/librustc/ty/instance.rs Outdated Show resolved Hide resolved
src/librustc/ty/instance.rs Outdated Show resolved Hide resolved
Aaron1011 and others added 5 commits Jan 2, 2020
Fixes #67639

`Instance.ty` assumes that we are in a fully monomorphic context (e.g.
codegen), and can therefore use an empty `ParamEnv` when performing
normalization. Howver, the MIR constant evaluator code ends up calling
`Instance.ty` as a result of us attemptign to 'speculatively'
const-evaluate generic functions during const propagation.

As a result,
we may end up with projections involving type parameters
(e.g. <T as MyTrait>::Bar>) in the type we are trying to normalize.
Normalization expects us to have proper predicates in the `ParamEnv` for
such projections, and will ICE if we don't.

This commit adds a new method `Instance.ty_env`, which takes a
`ParamEnv` for use during normalization. The MIR const-evaluator code is
changed to use this method, passing in the proper `ParamEnv` for the
context at hand.
Co-Authored-By: Wesley Wiser <wwiser@gmail.com>
@Aaron1011 Aaron1011 force-pushed the Aaron1011:fix/mir-generic-instance branch from b9dcd98 to 33caf0b Jan 5, 2020
@Aaron1011

This comment has been minimized.

Copy link
Contributor Author

Aaron1011 commented Jan 5, 2020

@oli-obk: Updated

@@ -2301,7 +2301,7 @@ impl<'tcx> ty::Instance<'tcx> {
// or should go through `FnAbi` instead, to avoid losing any
// adjustments `FnAbi::of_instance` might be performing.
fn fn_sig_for_fn_abi(&self, tcx: TyCtxt<'tcx>) -> ty::PolyFnSig<'tcx> {
let ty = self.ty(tcx);
let ty = self.monomorphic_ty(tcx);

This comment has been minimized.

Copy link
@oli-obk

oli-obk Jan 5, 2020

Contributor

It's not obvious to me that this is correct. Ca you go up the call chain and see if there are any noncodegen uses?

This comment has been minimized.

Copy link
@Aaron1011

Aaron1011 Jan 5, 2020

Author Contributor

The only caller is FnAbi:of_instance, which is only called from codegen.

@oli-obk

This comment has been minimized.

Copy link
Contributor

oli-obk commented Jan 5, 2020

r? @oli-obk

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 5, 2020

📌 Commit 336b902 has been approved by oli-obk

@rust-highfive rust-highfive assigned oli-obk and unassigned varkor Jan 5, 2020
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Jan 6, 2020
… r=oli-obk

Fix ICE involving calling `Instance.ty` during const evaluation

Fixes rust-lang#67639

`Instance.ty` assumes that we are in a fully monomorphic context (e.g.
codegen), and can therefore use an empty `ParamEnv` when performing
normalization. Howver, the MIR constant evaluator code ends up calling
`Instance.ty` as a result of us attemptign to 'speculatively'
const-evaluate generic functions during const propagation.

As a result,
we may end up with projections involving type parameters
(e.g. <T as MyTrait>::Bar>) in the type we are trying to normalize.
Normalization expects us to have proper predicates in the `ParamEnv` for
such projections, and will ICE if we don't.

This commit adds a new method `Instance.ty_env`, which takes a
`ParamEnv` for use during normalization. The MIR const-evaluator code is
changed to use this method, passing in the proper `ParamEnv` for the
context at hand.
bors added a commit that referenced this pull request Jan 6, 2020
Rollup of 6 pull requests

Successful merges:

 - #67800 (Fix ICE involving calling `Instance.ty` during const evaluation)
 - #67873 (change remove to have a PartialEq bound)
 - #67897 (Use `as_deref()` to replace `as_ref().map(...)`)
 - #67906 (Silence `TooGeneric` error)
 - #67912 (macros: typo fix)
 - #67915 (Use Self instead of $type)

Failed merges:

r? @ghost
@bors bors merged commit 336b902 into rust-lang:master Jan 6, 2020
4 checks passed
4 checks passed
pr Build #20200105.68 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-7) Linux x86_64-gnu-llvm-7 succeeded
Details
pr (Linux x86_64-gnu-tools) Linux x86_64-gnu-tools succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.