custom derive causing ICE on beta and nightly

[sgrif]

While working on a new feature for Diesel, one of the changes caused the compiler to start ICEing on beta and nightly. The code works fine on stable. The branch is https://github.com/diesel-rs/diesel/compare/sg-rust-bug

I'm still trying to pin down what specifically caused the ICE. The crash happens before it actually gets to the custom derive at all, so it seems that just adding #[derive(ValidGrouping)] to one of those types caused it. Here's the backtrace:

thread 'rustc' panicked at 'called `Option::unwrap()` on a `None` value', libcore/option.rs:335:21
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
stack backtrace:
   0: std::sys::unix::backtrace::tracing::imp::unwind_backtrace
   1: std::sys_common::backtrace::_print
   2: std::panicking::default_hook::{{closure}}
   3: std::panicking::default_hook
   4: std::panicking::rust_panic_with_hook
   5: std::panicking::begin_panic
   6: std::panicking::begin_panic_fmt
   7: rust_begin_unwind
   8: core::panicking::panic_fmt
   9: core::panicking::panic
  10: rustc_resolve::build_reduced_graph::<impl rustc_resolve::Resolver<'a>>::get_module
  11: rustc_resolve::macros::<impl syntax::ext::base::Resolver for rustc_resolve::Resolver<'a>>::resolve_invoc
  12: syntax::ext::expand::MacroExpander::expand
  13: syntax::ext::expand::MacroExpander::expand_crate
  14: rustc_driver::driver::phase_2_configure_and_expand_inner::{{closure}}
  15: rustc_driver::driver::phase_2_configure_and_expand_inner
  16: rustc_driver::driver::compile_input
  17: rustc_driver::run_compiler

error: internal compiler error: unexpected panic

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.25.0-beta.7 (da64ca931 2018-03-03) running on x86_64-apple-darwin

[sgrif]

I'm pushing to the branch linked above as I attempt to isolate the bug. Removing all instances of #[derive(ValidGrouping)] causes the ICE to stop occurring. The body of the derive is irrelevant (I have replaced it with "".parse().unwrap()). However, just having one invocation of the derive and the empty body is not enough to reproduce it.

[sgrif]

Specifically this line (or any usage of #[derive(ValidGrouping)]) causes the ICE. If you revert that one line, the ICE no longer occurs. I'm at a bit of a loss as to what else to try deleting here

[nikomatsakis]

cc @rust-lang/release -- it would be awesome to bisect this and try to find where the problem was introduced.

@sgrif the only working end point we have is that it works on current stable, right?

[nikomatsakis]

triage: P-high

[pietroalbini]

Running a bisect.

[pietroalbini]

@nikomatsakis the bisect run points at #47181 as the cause of the regression (merge commit ca09293).

[nikomatsakis]

cc @michaelwoerister -- this seems to be due to "Use DefIndex encoding that works better with on-disk variable length integer representations." (#47181)

[pnkfelix]

reassigning to @michaelwoerister under assumption that he is best person to investigate how #47181 caused this ICE

[nikomatsakis]

Think you'd be able to take a look?

[michaelwoerister]

OK, will take a look.

[michaelwoerister]

I think I know what's going on here: Proc-macros are handled weirdly in the CrateStore. In metadata they are stored under a different DefIndex than they actually have (and I have no idea why). At the same time, the DefPathTable for the proc-macro crate is stored in metadata verbatim. So we are trying to look up a DefKey with this weird proc-macro DefIndex in a table that expects the original DefIndex. It might well be that this was silently broken before, returning random names from the table.

Here is a stacktrace with line numbers (valid for rustc e65547d)

thread 'rustc' panicked at 'called `Option::unwrap()` on a `None` value', libcore/option.rs:335:21
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
stack backtrace:
   0: std::sys::unix::backtrace::tracing::imp::unwind_backtrace
             at libstd/sys/unix/backtrace/tracing/gcc_s.rs:49
   1: std::sys_common::backtrace::print
             at libstd/sys_common/backtrace.rs:71
             at libstd/sys_common/backtrace.rs:59
   2: std::panicking::default_hook::{{closure}}
             at libstd/panicking.rs:207
   3: std::panicking::default_hook
             at libstd/panicking.rs:223
   4: rustc::util::common::panic_hook
             at librustc/util/common.rs:51
   5: std::panicking::rust_panic_with_hook
             at libstd/panicking.rs:403
   6: std::panicking::begin_panic_fmt
             at libstd/panicking.rs:349
   7: rust_begin_unwind
             at libstd/panicking.rs:325
   8: core::panicking::panic_fmt
             at libcore/panicking.rs:72
   9: core::panicking::panic
             at libcore/panicking.rs:51
  10: rustc_resolve::build_reduced_graph::<impl rustc_resolve::Resolver<'a>>::get_module
             at /home/mw/3-rust/src/libcore/macros.rs:20
             at librustc_resolve/build_reduced_graph.rs:555
  11: rustc_resolve::build_reduced_graph::<impl rustc_resolve::Resolver<'a>>::macro_def_scope
             at librustc_resolve/build_reduced_graph.rs:574
  12: rustc_resolve::macros::<impl syntax::ext::base::Resolver for rustc_resolve::Resolver<'a>>::resolve_invoc
             at librustc_resolve/macros.rs:305
  13: syntax::ext::expand::MacroExpander::expand
             at libsyntax/ext/expand.rs:290
  14: syntax::ext::expand::MacroExpander::expand_crate
             at libsyntax/ext/expand.rs:245
  15: rustc_driver::driver::phase_2_configure_and_expand_inner::{{closure}}
             at librustc_driver/driver.rs:794
  16: rustc::util::common::time
             at /home/mw/3-rust/src/librustc/util/common.rs:139
             at /home/mw/3-rust/src/librustc/util/common.rs:133
  17: rustc_driver::driver::phase_2_configure_and_expand
             at librustc_driver/driver.rs:753
             at librustc_driver/driver.rs:605
  18: rustc_driver::driver::compile_input
             at librustc_driver/driver.rs:128
  19: rustc_driver::run_compiler
             at librustc_driver/lib.rs:520

I'm not sure how to solve this best. Probably by cleaning up proc-macro handling. @rust-lang/compiler, @alexcrichton, do any of you know why we don't use regular DefIds for proc macros in the first place?

[estebank]

Suspiciously similar to #48941

[alexcrichton]

@michaelwoerister I think long ago it was done as a way of easily assigning the registrar to have a known symbol (so it can be dlopen'd and dlsym'd by rustc itself), but nowadays I think there's much better mechanics for doing that. My guess is that we should probably rip out everything related to registrar symbol names and just slap a #[export_name = "..."] onto the generated function instead

[alexcrichton]

(I could also be totaly wrong, I barely remember what these are doing at this point)

[ExpHP]

@estebank while both are ICEs that occur around custom macros, #48941 is due to a very localized mistake (calling ok on PResult), while this seems to be a logic error occurring at a much wider scale.

If anything, I think it is just an indication that proc macros and especially plugins are undertested. (so refactorings are more likely to break them)

[michaelwoerister]

@alexcrichton I think symbol names of registrar functions don't depend on DefIds anymore at all:

rust/src/librustc/session/mod.rs

Lines 685 to 697 in 8c4ff22

	pub fn generate_plugin_registrar_symbol(&self, disambiguator: CrateDisambiguator) -> String {
	format!(
	"__rustc_plugin_registrar_{}__",
	disambiguator.to_fingerprint().to_hex()
	)
	}
	pub fn generate_derive_registrar_symbol(&self, disambiguator: CrateDisambiguator) -> String {
	format!(
	"__rustc_derive_registrar_{}__",
	disambiguator.to_fingerprint().to_hex()
	)
	}

I'll look into cleaning this up. I want to get acquainted with the crate store code anyway because I suspect that it will need refactoring for further querification/parallelization.

[sgrif]

If anything, I think it is just an indication that plugins and proc macros are undertested. (so refactorings are more likely to break them)

+100