-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
ICE: failed to get layout #43132
Comments
Minimized a bit (52 lines): (expand)extern crate futures;
extern crate futures_mpsc;
extern crate tk_http;
extern crate tk_listen;
extern crate tokio_core;
use std::sync::Arc;
use futures::{Future, Sink, Stream};
use futures::future::FutureResult;
use futures::stream::empty as empty_stream;
use tokio_core::reactor::Core;
use tk_http::server::buffered::{Request, BufferedDispatcher};
use tk_http::server::{Encoder, EncoderDone, Config, Proto, Error};
use tk_listen::ListenExt;
use tokio_core::net::TcpStream;
use std::net::SocketAddr;
fn http<S>(_: Request, _: Encoder<S>)
-> FutureResult<EncoderDone<S>, Error>
{
unimplemented!()
}
fn main() {
let h = Core::new().unwrap().handle();
let (sender, receiver) = futures_mpsc::channel::<()>(1);
let sender = Arc::new(sender);
let _ = empty_stream::<(TcpStream, SocketAddr), ()>()
.map(|(socket, addr)| {
let sender = sender.clone();
Proto::new(socket, &Config::new().done(),
BufferedDispatcher::new_with_websockets(addr, &h,
http,
move |_, _| {
let sender = (*sender).clone();
futures::future::ok(()).and_then(move |_| {
empty_stream::<(), String>().fold(sender, |sender, _| {
sender.send(()).map_err(|_| "")
})
})
.then(|_| Ok(()))
}),
&h)
.then(|_| Ok(()))
})
.listen(1000)
.join(receiver.for_each(|_| Ok(())));
} |
This showed up in the |
A smaller version of what I believe is the same ICE: extern crate futures;
use futures::{Future, Stream, Sink};
use futures::stream::iter;
fn foo<T>() -> T {
loop {}
}
fn main() {
iter(foo::<std::iter::Cloned<std::slice::Iter<i32>>>().map(Ok))
.forward(
foo::<Box<Sink<SinkItem = i32, SinkError = ()>>>()
)
.join(
foo::<Box<Stream<Item = i32, Error = ()>>>()
.for_each(|_| Ok(()))
);
} |
@alexcrichton yeah now that I look at it is that on stable OP's example has a different ICE error than on nightly, very alike to #40274, while the one on nightly matches your example. |
Bisected the nightly ICE using bisect-rust to #42840 (9b85e1c) -- cc @arielb1 @nikomatsakis |
Looks like the projection cache is dropping normalization obligations on the floor at https://github.com/rust-lang/rust/blob/master/src/librustc/traits/project.rs#L1392: fn opt_normalize_projection_type<'a, 'b, 'gcx, 'tcx>( //
// ...
let result = // ...
Normalized {
value: normalized_ty,
obligations,
} // ...
infcx.projection_cache.borrow_mut()
.complete(projection_ty, &result, cacheable);
Some(result)
// ...
fn complete(&mut self,
key: ty::ProjectionTy<'tcx>,
value: &NormalizedTy<'tcx>,
cacheable: bool) {
let fresh_key = if cacheable {
debug!("ProjectionCacheEntry::complete: adding cache entry: key={:?}, value={:?}",
key, value);
// <-- WHERE DID value.obligations JUST DISAPPEAR TO?
self.map.insert(key, ProjectionCacheEntry::NormalizedTy(value.value))
} else {
debug!("ProjectionCacheEntry::complete: cannot cache: key={:?}, value={:?}",
key, value);
!self.map.remove(key)
};
assert!(!fresh_key, "never started projecting `{:?}`", key);
} This causes problems with my new evaluation code because it causes an earlier call into evaluation, which allows the cache pollution to proceed (I think). |
Actually, the problem is that the projection cache is incompatible with 1-pass evaluation. Not sure how to solve this. |
@arielb1 do you have further thoughts on this? The regression is now on beta :( |
cc @eddyb |
I don't have any specific knowledge of how the caches involved here interact. It seems like layout code triggers this but the problem could arise in a number of other ways. |
I have to talk to @nikomatsakis about this. |
I can look into this, but not until Saturday. |
triage: P-high |
So I have a fix pending in this branch but I'm not sure what to use for a regression test, since I don't want to add a dependency on futures. |
@nikomatsakis here's a "reasonably sized" test case which reproduces the error here. |
@nikomatsakis I've minimized @alexcrichton 's example to roughly the half here. |
save subobligations in the projection cache The projection cache explicitly chose not to "preserve" subobligations for projections, since the fulfillment context ought to have been doing so. But for the trait evaluation scheme that causes problems. This PR reproduces subobligations. This has the potential to slow down compilation, but minimal investigation suggests it does not do so. One hesitation about this PR: I could not find a way to make a standalone test case for #43132 (but admittedly I did not try very hard). Fixes #43132. r? @arielb1
The nightly is out and I no longer run into that ICE! On the other hand compiling this simple program now takes 8 minutes. The previous commit (before I introduced the ICE) is up from 6s to 1m51s. |
@remram44 I suggest you file a separate bug report for this. |
@remram44 hm I can't seem to get the commit linked above to compile? What compiler are you using to compile it "pre ICE"? |
I'm compiling using stable and the nightly from the 28th from rustup... not sure why this would fail for you? |
@remram44 sorry I still can't get the code to compile. Can you list the exact commands you're using, along with the exact toolchain in rustup-style syntax? |
The toolchains I tried are Whatever your environment you can reproduce this from scratch in Docker with:
|
Ok thanks for the repro @remram44! Want to open a new issue with that so we can be sure to track it? |
Will do! [edit: #43787] |
We want to retain obligations that *contain* inference variables, not obligations that *don't contain* them, in order to fix rust-lang#43132. Because of surrounding changes to inference, the ICE doesn't occur in its original case, but I believe it could still be made to occur on master. Maybe I should try to write a new test case? Certainly not right now (I'm mainly trying to get us a beta that we can ship) but maybe before we land this PR on nightly? This seems to cause a 10% performance regression in my imprecise attempt to benchmark item-body checking for rust-lang#43613, but it's better to be slow and right than fast and wrong. If we want to recover that, I think we can change the constrained-type-parameter code to actually give a list of projections that are important for resolving inference variables and filter everything else out.
We want to retain obligations that *contain* inference variables, not obligations that *don't contain* them, in order to fix rust-lang#43132. Because of surrounding changes to inference, the ICE doesn't occur in its original case, but I believe it could still be made to occur on master. Maybe I should try to write a new test case? Certainly not right now (I'm mainly trying to get us a beta that we can ship) but maybe before we land this PR on nightly? This seems to cause a 10% performance regression in my imprecise attempt to benchmark item-body checking for rust-lang#43613, but it's better to be slow and right than fast and wrong. If we want to recover that, I think we can change the constrained-type-parameter code to actually give a list of projections that are important for resolving inference variables and filter everything else out.
fix logic error in #44269's `prune_cache_value_obligations` We want to retain obligations that *contain* inference variables, not obligations that *don't contain* them, in order to fix #43132. Because of surrounding changes to inference, the ICE doesn't occur in its original case, but I believe it could still be made to occur on master. Maybe I should try to write a new test case? Certainly not right now (I'm mainly trying to get us a beta that we can ship) but maybe before we land this PR on nightly? This seems to cause a 10% performance regression in my imprecise attempt to benchmark item-body checking for #43613, but it's better to be slow and right than fast and wrong. If we want to recover that, I think we can change the constrained-type-parameter code to actually give a list of projections that are important for resolving inference variables and filter everything else out.
Code: https://gitlab.com/remram44/rs-web/blob/bf5066037ffb627c98b4988ff7d46b054ce33e40/src/main.rs#L92
Using versions (similar crash; logs):
rustc 1.20.0-nightly (720c596 2017-07-08)
rustc 1.18.0 (03fc9d6 2017-06-06)
I don't know too much about the compiler internals, but a bug search for "layout_of" didn't bring up anything that seemed relevant. So here you go.
Seems similar to #37096 but I'm not using impl Trait?
The text was updated successfully, but these errors were encountered: