Skip to content

trans: discrepancies between (MIR-based) collector and non-MIR translation. #34151

Closed

Description

@michaelwoerister

The translation item collector in trans::collector is supposed to find all monomorphizations and instantiations of local and extern functions and drop-glue that need to go into the binary currently being compiled. It does so by analyzing the MIR of every function, following calls, references, drops, etc.
In theory this is a sound strategy.

However, in combination with legacy-trans, which is not based on MIR, we are running into problems: Since the MIR is heavily optimized before being passed into the collector, it might not contain references to what really is dead code. HIR-based trans, however, does not have this knowledge and will have to translate everything. This leads to situations where HIR-based trans will insert references to what only MIR-based analysis knows to be dead code. Thus we end up with "dangling references" and, subsequently, linker errors.

In order to still support HIR-based trans for the time being, there's some code in trans that will instantiate functions and drop-glue on demand if the collector "missed" it. This code should be removed once HIR-based trans has been removed from the compiler.
Specifically, this concerns trans::monomorphize::monomorphic_fn() and trans::glue::get_drop_glue_core().

Activity

  1. ahmedcharles commented on Aug 27, 2016

    @ahmedcharles
    Contributor

    @michaelwoerister Now that MIR is the only option, is this still relevant? And if so, what needs to be done?

  2. eddyb commented on Aug 27, 2016

    @eddyb
    Contributor

    Drop glue might still be needed, but on-demand monomorphizations are gone.

    Somewhat relatedly, @arielb1 has expressed interest in working on making the collector generate MIR bodies for all the needed shims and drop glues.

  3. michaelwoerister commented on Aug 29, 2016

    @michaelwoerister
    MemberAuthor

    Drop glue might still be needed

    The code for instantiating it on demand is still there, but it should not be needed anymore.

  4. ahmedcharles commented on Aug 29, 2016

    @ahmedcharles
    Contributor

    Is there a pointer for what likely needs removing if I wanted to do this?

  5. michaelwoerister commented on Aug 29, 2016

    @michaelwoerister
    MemberAuthor

    @ahmedcharles The relevant code is here:

    // FIXME: #34151

    You can just delete anything below that line in that function. The trickier part is finding out if this causes any breakage. If make check passes with the code removed, that would be a good indication that everything still works as expected. If you get that far you could make a PR and someone will do a crater run that will test the changes against even more code.

  6. ahmedcharles commented on Aug 30, 2016

    @ahmedcharles
    Contributor

    I removed the code, changed the debug! to a panic! and got this:

    thread 'rustc' panicked at 'Could not find drop glue for Ty([closure@src/librustc_driver/lib.rs:494:59: 506:18 ppm:pretty::PpMode, opt_uii:std::option::Option<pretty::UserIdentifiedItem>]) -- DropGlue(Ty: 4560228208) -- rustc_driver.cgu-0.', src/librustc_trans/glue.rs:219
    stack backtrace:
       1:        0x10aa2d5b9 - std::sys::backtrace::tracing::imp::write::habd974eec51179ec
       2:        0x10aa3bfc0 - std::panicking::default_hook::{{closure}}::h723c2c1871f63513
       3:        0x10aa3a3d4 - std::panicking::default_hook::hd1cbe478d89db943
       4:        0x10aa3aac6 - std::panicking::rust_panic_with_hook::hb9e51cc78bf6920a
       5:        0x10aa3a914 - std::panicking::begin_panic::h1bc9c2b22b63e84a
       6:        0x10aa3a882 - std::panicking::begin_panic_fmt::ha843df1a651a1d04
       7:        0x1069c81cc - rustc_trans::glue::get_drop_glue_core::h7fe6df48d8013110
       8:        0x1069773f1 - rustc_trans::base::unsized_info::ha7784d67670ae524
       9:        0x106978218 - rustc_trans::base::unsize_thin_ptr::h722eb22b27e72d21
      10:        0x1069f7a0f - rustc_trans::mir::rvalue::<impl rustc_trans::mir::MirContext<'bcx, 'tcx>>::trans_rvalue_operand::h685ea1526e348d6b
      11:        0x1069e308b - rustc_trans::mir::block::<impl rustc_trans::mir::MirContext<'bcx, 'tcx>>::trans_block::hdeeee4856b5f0733
      12:        0x1069e0a76 - rustc_trans::mir::trans_mir::h0fc2d97a6125939e
      13:        0x10697d443 - rustc_trans::base::trans_closure::hc3c07ca31f9ed2e0
      14:        0x106a02cb9 - rustc_trans::trans_item::TransItem::define::hd2591d5f4d59e9a6
      15:        0x1069808be - rustc_trans::base::trans_crate::heae1d3de2b6c43f8
      16:        0x106497f33 - rustc_driver::driver::phase_4_translate_to_llvm::h9a6ace50899bdd34
      17:        0x1064d44ec - rustc_driver::driver::compile_input::{{closure}}::h0382874667e5d93d
      18:        0x1064bda9d - rustc_driver::driver::phase_3_run_analysis_passes::{{closure}}::h0de946251dacf778
      19:        0x10642d092 - rustc::ty::context::TyCtxt::create_and_enter::hd60a32a123ed477e
      20:        0x1064883c7 - rustc_driver::driver::compile_input::h4a9c4465bb71b3f3
      21:        0x1064ac1c6 - rustc_driver::run_compiler::he7e28d4ec9619a0f
      22:        0x1063fd3cf - std::panicking::try::do_call::hca6f65893d2350d0
      23:        0x10aa42c9a - __rust_maybe_catch_panic
      24:        0x10641d424 - <F as alloc::boxed::FnBox<A>>::call_box::h6ce4b753f356ad3c
      25:        0x10aa391b5 - std::sys::thread::Thread::new::thread_start::hf6ecc70f34c9b620
      26:     0x7fff8b15099c - _pthread_body
      27:     0x7fff8b150919 - _pthread_start
    
    error: Could not compile `rustc_driver`.
    
  7. michaelwoerister commented on Aug 30, 2016

    @michaelwoerister
    MemberAuthor

    That means that the collector did not anticipate that drop-glue for this type would be needed. It's not entirely surprising that this happens to be a closure type. Closures are special in a few ways. This looks like the collector is missing the drop glue because it is only accessed via the closures vtable.

  8. eddyb commented on Aug 30, 2016

    @eddyb
    Contributor

    Could be from rustc_trans::closure::trans_fn_once_adapter_shim scheduling a closure drop.

    This is where @arielb1's plans for generating shim/glue MIR would come in real handy.

  9. michaelwoerister commented on Aug 30, 2016

    @michaelwoerister
    MemberAuthor

    Yes, that would remove some special cases from the collector.

  10. ahmedcharles commented on Aug 30, 2016

    @ahmedcharles
    Contributor

    Any ideas on how to fix it? Well, given that I don't really know what drop glue is (I assume it's code that calls the drop function on things when they go out of scope), it seems hard to fix. :)

  11. eddyb commented on Aug 30, 2016

    @eddyb
    Contributor

    @arielb1 wants to fix this by generating MIR for those cases as-needed. Don't think there's a much simpler solution.

  12. michaelwoerister commented on Nov 9, 2016

    @michaelwoerister
    MemberAuthor

    old-trans is gone 🎉

  13. added
    T-compilerRelevant to the compiler team, which will review and decide on the PR/issue.
    on Mar 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    A-incr-compArea: Incremental compilationC-cleanupCategory: PRs that clean code up or issues documenting cleanup.T-compilerRelevant to the compiler team, which will review and decide on the PR/issue.

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions