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

ICE when using Self as uninitialized return type in trait method #57796

Closed
paulocsanz opened this issue Jan 21, 2019 · 9 comments
Closed

ICE when using Self as uninitialized return type in trait method #57796

paulocsanz opened this issue Jan 21, 2019 · 9 comments
Assignees
Labels
I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@paulocsanz
Copy link

paulocsanz commented Jan 21, 2019

trait FromUnchecked {
    unsafe fn from_unchecked(s: &str) -> Self;
}

impl FromUnchecked for [u8; 30] {
    unsafe fn from_unchecked(s: &str) -> Self {
        let mut array: Self = std::mem::uninitialized();
        let ptr = &mut array as *mut [u8] as *mut u8;
        std::ptr::copy_nonoverlapping(s.as_bytes().as_ptr(), ptr, s.len());
        std::ptr::write_bytes(ptr.add(s.len()), 0, 30 - s.len());
        array
    }
}

fn main() {
    let _: [u8; 30] = unsafe { FromUnchecked::from_unchecked("asdasd") };
}

Playground

To make it compile you can simply replace Self for [u8; 30] in the uninitialized call (Playground)

 Compiling playground v0.0.1 (/playground)
error: internal compiler error: src/librustc/ty/sty.rs:2107: expected constant usize, got Const {
    ty: usize,
    val: Unevaluated(
        DefId(0/1:9 ~ playground[ec94]::{{impl}}[0]::{{constant}}[0]),
        []
    )
}

thread 'main' panicked at 'Box<Any>', src/librustc_errors/lib.rs:600:9
stack backtrace:
   0: std::sys::unix::backtrace::tracing::imp::unwind_backtrace
             at src/libstd/sys/unix/backtrace/tracing/gcc_s.rs:49
   1: std::sys_common::backtrace::_print
             at src/libstd/sys_common/backtrace.rs:71
   2: std::panicking::default_hook::{{closure}}
             at src/libstd/sys_common/backtrace.rs:59
             at src/libstd/panicking.rs:211
   3: std::panicking::default_hook
             at src/libstd/panicking.rs:227
   4: rustc::util::common::panic_hook
   5: std::panicking::rust_panic_with_hook
             at src/libstd/panicking.rs:495
   6: std::panicking::begin_panic
   7: rustc_errors::Handler::bug
   8: rustc::util::bug::opt_span_bug_fmt::{{closure}}
   9: rustc::ty::context::tls::with_opt::{{closure}}
  10: rustc::ty::context::tls::with_context_opt
  11: rustc::ty::context::tls::with_opt
  12: rustc::util::bug::opt_span_bug_fmt
  13: rustc::util::bug::bug_fmt
  14: rustc::ty::sty::Const::unwrap_usize::{{closure}}
  15: <rustc_mir::transform::qualify_consts::Qualifier<'a, 'tcx, 'tcx> as rustc::mir::visit::Visitor<'tcx>>::visit_rvalue
  16: <rustc_mir::transform::qualify_consts::Qualifier<'a, 'tcx, 'tcx> as rustc::mir::visit::Visitor<'tcx>>::visit_assign
  17: <rustc_mir::transform::qualify_consts::QualifyAndPromoteConstants as rustc_mir::transform::MirPass>::run_pass
  18: rustc_mir::transform::run_passes::{{closure}}
  19: rustc_mir::transform::run_passes
  20: rustc_mir::transform::mir_validated
  21: rustc::ty::query::<impl rustc::ty::query::config::QueryAccessors<'tcx> for rustc::ty::query::queries::mir_validated<'tcx>>::compute
  22: rustc::dep_graph::graph::DepGraph::with_task_impl
  23: <rustc::ty::query::plumbing::JobOwner<'a, 'tcx, Q>>::start
  24: rustc::ty::query::plumbing::<impl rustc::ty::context::TyCtxt<'a, 'gcx, 'tcx>>::force_query_with_job
  25: rustc::ty::query::plumbing::<impl rustc::ty::context::TyCtxt<'a, 'gcx, 'tcx>>::get_query
  26: rustc_mir::borrow_check::mir_borrowck
  27: rustc::ty::query::__query_compute::mir_borrowck
  28: rustc::ty::query::<impl rustc::ty::query::config::QueryAccessors<'tcx> for rustc::ty::query::queries::mir_borrowck<'tcx>>::compute
  29: rustc::dep_graph::graph::DepGraph::with_task_impl
  30: <rustc::ty::query::plumbing::JobOwner<'a, 'tcx, Q>>::start
  31: rustc::ty::query::plumbing::<impl rustc::ty::context::TyCtxt<'a, 'gcx, 'tcx>>::force_query_with_job
  32: rustc::ty::query::plumbing::<impl rustc::ty::context::TyCtxt<'a, 'gcx, 'tcx>>::get_query
  33: rustc::ty::query::<impl rustc::ty::context::TyCtxt<'a, 'tcx, 'lcx>>::mir_borrowck
  34: rustc::ty::<impl rustc::ty::context::TyCtxt<'a, 'gcx, 'tcx>>::par_body_owners
  35: rustc::util::common::time
  36: rustc::ty::context::tls::enter_context
  37: <std::thread::local::LocalKey<T>>::with
  38: rustc::ty::context::TyCtxt::create_and_enter
  39: rustc_driver::driver::compile_input
  40: rustc_driver::run_compiler_with_pool
  41: <scoped_tls::ScopedKey<T>>::set
  42: rustc_driver::run_compiler
  43: rustc_driver::monitor::{{closure}}
  44: __rust_maybe_catch_panic
             at src/libpanic_unwind/lib.rs:102
  45: rustc_driver::run
  46: rustc_driver::main
  47: std::rt::lang_start::{{closure}}
  48: std::panicking::try::do_call
             at src/libstd/rt.rs:59
             at src/libstd/panicking.rs:310
  49: __rust_maybe_catch_panic
             at src/libpanic_unwind/lib.rs:102
  50: std::rt::lang_start_internal
             at src/libstd/panicking.rs:289
             at src/libstd/panic.rs:398
             at src/libstd/rt.rs:58
  51: main
  52: __libc_start_main
  53: <unknown>
query stack during panic:
#0 [mir_validated] processing `<[u8; _] as FromUnchecked>::from_unchecked`
#1 [mir_borrowck] processing `<[u8; _] as FromUnchecked>::from_unchecked`
end of query stack
error: aborting due to previous error


note: the compiler unexpectedly panicked. this is a bug.

note: we would appreciate a bug report: https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#bug-reports

note: rustc 1.32.0 (9fda7c223 2019-01-16) running on x86_64-unknown-linux-gnu

note: compiler flags: -C codegen-units=1 -C debuginfo=2 --crate-type bin

note: some of the compiler flags provided by cargo are hidden

error: Could not compile `playground`.

To learn more, run the command again with --verbose.
@paulocsanz paulocsanz changed the title ICE when using uninitialized in trait method ICE when using Self as uninitialized return type in trait method Jan 21, 2019
@estebank estebank added I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 21, 2019
@hellow554
Copy link
Contributor

hellow554 commented Jan 21, 2019

Wow... this bug is so old, I can reproduce it with nightly-2018-06-01... damn ^^

@oli-obk oli-obk self-assigned this Feb 5, 2019
@aoikonomopoulos
Copy link
Contributor

Took my first deep dive into the compiler trying to diagnose this. AFAICT, things first start going wrong when we resolve the Self path in AstConv::def_to_ty. From what I can tell by reading the code and capturing dynamic traces, we need to call normalize_associated_types_in on a type in order to evaluate the associated constant in [u8; 30].

The fix I've tried is:

diff --git a/src/librustc_typeck/astconv.rs b/src/librustc_typeck/astconv.rs
index cde6eb22bb..21e2466092 100644
--- a/src/librustc_typeck/astconv.rs
+++ b/src/librustc_typeck/astconv.rs
@@ -1701,7 +1701,7 @@ impl<'o, 'gcx: 'tcx, 'tcx> dyn AstConv<'gcx, 'tcx> + 'o {
                 // `Self` in impl (we know the concrete type).
                 assert_eq!(opt_self_ty, None);
                 self.prohibit_generics(&path.segments);
-                tcx.at(span).type_of(def_id)
+                self.normalize_ty(span, tcx.at(span).type_of(def_id))
             }
             Def::SelfTy(Some(_), None) => {
                 // `Self` in trait.

However, I'm not 100% sure that's the appropriate point to do normalization. Several other branches of the match statement do normalization of types at that point, yet some don't. So it would be great if someone with a tad more experience in the compiler can take a quick look and verify that (a) that's the correct fix (b) that's the only point in def_to_ty where normalization should be happening but isn't.

FWIW, I've introduced a run-pass testcase with the code snippet provided by @paulocsanz, which fails using master but passes with the above modification. Needless to say, the rest of the test suite(s) still pass with this patch.

@hellow554
Copy link
Contributor

@aoikonomopoulos can you confirm that your fix also fixed #58212 ?

@aoikonomopoulos
Copy link
Contributor

@hellow554 yah, it ICEs with nightly, but compiles with the above patch. Should I prepare a PR?

@hellow554
Copy link
Contributor

Sure, let's go 🎉

@hellow554
Copy link
Contributor

I think you can directly do r? @oli-obk in your PR.

@aoikonomopoulos
Copy link
Contributor

@hellow554 Ok, will do. Thanks for the guidance.
@paulocsanz Is it OK to include your testcase in src/test/run-pass/issues (i.e. distribute it under MIT/Apache2)?

@hellow554
Copy link
Contributor

hellow554 commented Feb 26, 2019

@aoikonomopoulos afaik you don't have to ask for taking testcases that are posted here, but IANAL!

You can also take the testcase from #58212 (comment) or this minified version

trait FromUnchecked {
    unsafe fn from_unchecked();
}

impl FromUnchecked for [u8; 1] {
    unsafe fn from_unchecked() {
        let mut array: Self = std::mem::uninitialized();
        let _ptr = &mut array as *mut [u8] as *mut u8;
    }
}

@aoikonomopoulos
Copy link
Contributor

Yah the minified version seems more appropriate too.

Centril added a commit to Centril/rust that referenced this issue Feb 27, 2019
Normalize the type Self resolves to in an impl

This is required at the very least in order to evaluate associated
constants for arrays.

Fixes rust-lang#57796
Fixes rust-lang#58212.

r? @oli-obk
cc @hellow554
Centril added a commit to Centril/rust that referenced this issue Feb 27, 2019
Normalize the type Self resolves to in an impl

This is required at the very least in order to evaluate associated
constants for arrays.

Fixes rust-lang#57796
Fixes rust-lang#58212.

r? @oli-obk
cc @hellow554
Centril added a commit to Centril/rust that referenced this issue Feb 27, 2019
Normalize the type Self resolves to in an impl

This is required at the very least in order to evaluate associated
constants for arrays.

Fixes rust-lang#57796
Fixes rust-lang#58212.

r? @oli-obk
cc @hellow554
Centril added a commit to Centril/rust that referenced this issue Feb 27, 2019
Normalize the type Self resolves to in an impl

This is required at the very least in order to evaluate associated
constants for arrays.

Fixes rust-lang#57796
Fixes rust-lang#58212.

r? @oli-obk
cc @hellow554
Centril added a commit to Centril/rust that referenced this issue Feb 27, 2019
Normalize the type Self resolves to in an impl

This is required at the very least in order to evaluate associated
constants for arrays.

Fixes rust-lang#57796
Fixes rust-lang#58212.

r? @oli-obk
cc @hellow554
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants