Skip to content

Commit

Permalink
Auto merge of #111329 - jyn514:metadata-ice, r=bjorn3
Browse files Browse the repository at this point in the history
Load only the crate header for `locator::crate_matches`

Previously, we used the following info to determine whether to load the crate:
1. The METADATA_HEADER, which includes a METADATA_VERSION constant
2. The embedded rustc version
3. Various metadata in the `CrateRoot`, including the SVH

This worked ok most of the time. Unfortunately, when building locally the rustc version is always
the same because `omit-git-hash` is on by default. That meant that we depended only on 1 and 3, and
we are not very good about bumping METADATA_VERSION (it's currently at 7) so in practice we were
only depending on 3. `CrateRoot` is a very large struct and changes somewhat regularly, so this led
to a steady stream of crashes from trying to load it.

Change the logic to add an intermediate step between 2 and 3: introduce a new `CrateHeader` struct
that contains only the minimum info needed to decide whether the crate should be loaded or not. That
avoids having to load all of `CrateRoot`, which in practice means we should crash much less often.

Note that this works because the SVH should be different between any two dependencies, even if the
compiler has changed, because we use `-Zbinary-dep-depinfo` in bootstrap. See
#111329 (comment) for more details about how the
original crash happened.
  • Loading branch information
bors committed May 29, 2023
2 parents 70e04bd + 60e95e7 commit 99ff5af
Show file tree
Hide file tree
Showing 6 changed files with 64 additions and 34 deletions.
17 changes: 8 additions & 9 deletions compiler/rustc_metadata/src/locator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -666,31 +666,30 @@ impl<'a> CrateLocator<'a> {
return None;
}

let root = metadata.get_root();
if root.is_proc_macro_crate() != self.is_proc_macro {
let header = metadata.get_header();
if header.is_proc_macro_crate != self.is_proc_macro {
info!(
"Rejecting via proc macro: expected {} got {}",
self.is_proc_macro,
root.is_proc_macro_crate(),
self.is_proc_macro, header.is_proc_macro_crate,
);
return None;
}

if self.exact_paths.is_empty() && self.crate_name != root.name() {
if self.exact_paths.is_empty() && self.crate_name != header.name {
info!("Rejecting via crate name");
return None;
}

if root.triple() != &self.triple {
info!("Rejecting via crate triple: expected {} got {}", self.triple, root.triple());
if header.triple != self.triple {
info!("Rejecting via crate triple: expected {} got {}", self.triple, header.triple);
self.crate_rejections.via_triple.push(CrateMismatch {
path: libpath.to_path_buf(),
got: root.triple().to_string(),
got: header.triple.to_string(),
});
return None;
}

let hash = root.hash();
let hash = header.hash;
if let Some(expected_hash) = self.hash {
if hash != expected_hash {
info!("Rejecting via hash: expected {} got {}", expected_hash, hash);
Expand Down
33 changes: 20 additions & 13 deletions compiler/rustc_metadata/src/rmeta/decoder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ pub(crate) struct CrateMetadata {
blob: MetadataBlob,

// --- Some data pre-decoded from the metadata blob, usually for performance ---
/// Data about the top-level items in a crate, as well as various crate-level metadata.
root: CrateRoot,
/// Trait impl data.
/// FIXME: Used only from queries and can use query cache,
Expand Down Expand Up @@ -449,7 +450,7 @@ impl<'a, 'tcx> Decodable<DecodeContext<'a, 'tcx>> for SyntaxContext {
You need to explicitly pass `(crate_metadata_ref, tcx)` to `decode` instead of just `crate_metadata_ref`.");
};

let cname = cdata.root.name;
let cname = cdata.root.name();
rustc_span::hygiene::decode_syntax_context(decoder, &cdata.hygiene_context, |_, id| {
debug!("SpecializedDecoder<SyntaxContext>: decoding {}", id);
cdata
Expand Down Expand Up @@ -564,7 +565,7 @@ impl<'a, 'tcx> Decodable<DecodeContext<'a, 'tcx>> for Span {
let cnum = u32::decode(decoder);
panic!(
"Decoding of crate {:?} tried to access proc-macro dep {:?}",
decoder.cdata().root.name,
decoder.cdata().root.header.name,
cnum
);
}
Expand Down Expand Up @@ -671,6 +672,16 @@ impl MetadataBlob {
.decode(self)
}

pub(crate) fn get_header(&self) -> CrateHeader {
let slice = &self.blob()[..];
let offset = METADATA_HEADER.len();

let pos_bytes = slice[offset..][..4].try_into().unwrap();
let pos = u32::from_be_bytes(pos_bytes) as usize;

LazyValue::<CrateHeader>::from_position(NonZeroUsize::new(pos).unwrap()).decode(self)
}

pub(crate) fn get_root(&self) -> CrateRoot {
let slice = &self.blob()[..];
let offset = METADATA_HEADER.len();
Expand All @@ -684,8 +695,8 @@ impl MetadataBlob {
pub(crate) fn list_crate_metadata(&self, out: &mut dyn io::Write) -> io::Result<()> {
let root = self.get_root();
writeln!(out, "Crate info:")?;
writeln!(out, "name {}{}", root.name, root.extra_filename)?;
writeln!(out, "hash {} stable_crate_id {:?}", root.hash, root.stable_crate_id)?;
writeln!(out, "name {}{}", root.name(), root.extra_filename)?;
writeln!(out, "hash {} stable_crate_id {:?}", root.hash(), root.stable_crate_id)?;
writeln!(out, "proc_macro {:?}", root.proc_macro_data.is_some())?;
writeln!(out, "=External Dependencies=")?;

Expand All @@ -709,21 +720,17 @@ impl CrateRoot {
}

pub(crate) fn name(&self) -> Symbol {
self.name
self.header.name
}

pub(crate) fn hash(&self) -> Svh {
self.hash
self.header.hash
}

pub(crate) fn stable_crate_id(&self) -> StableCrateId {
self.stable_crate_id
}

pub(crate) fn triple(&self) -> &TargetTriple {
&self.triple
}

pub(crate) fn decode_crate_deps<'a>(
&self,
metadata: &'a MetadataBlob,
Expand Down Expand Up @@ -794,7 +801,7 @@ impl<'a, 'tcx> CrateMetadataRef<'a> {
bug!(
"CrateMetadata::def_kind({:?}): id not found, in crate {:?} with number {}",
item_id,
self.root.name,
self.root.name(),
self.cnum,
)
})
Expand Down Expand Up @@ -1702,11 +1709,11 @@ impl CrateMetadata {
}

pub(crate) fn name(&self) -> Symbol {
self.root.name
self.root.header.name
}

pub(crate) fn hash(&self) -> Svh {
self.root.hash
self.root.header.hash
}

fn num_def_ids(&self) -> usize {
Expand Down
6 changes: 3 additions & 3 deletions compiler/rustc_metadata/src/rmeta/decoder/cstore_impl.rs
Original file line number Diff line number Diff line change
Expand Up @@ -317,9 +317,9 @@ provide! { tcx, def_id, other, cdata,
}
native_libraries => { cdata.get_native_libraries(tcx.sess).collect() }
foreign_modules => { cdata.get_foreign_modules(tcx.sess).map(|m| (m.def_id, m)).collect() }
crate_hash => { cdata.root.hash }
crate_hash => { cdata.root.header.hash }
crate_host_hash => { cdata.host_hash }
crate_name => { cdata.root.name }
crate_name => { cdata.root.header.name }

extra_filename => { cdata.root.extra_filename.clone() }

Expand Down Expand Up @@ -581,7 +581,7 @@ impl CrateStore for CStore {
}

fn crate_name(&self, cnum: CrateNum) -> Symbol {
self.get_crate_data(cnum).root.name
self.get_crate_data(cnum).root.header.name
}

fn stable_crate_id(&self, cnum: CrateNum) -> StableCrateId {
Expand Down
9 changes: 6 additions & 3 deletions compiler/rustc_metadata/src/rmeta/encoder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -662,10 +662,13 @@ impl<'a, 'tcx> EncodeContext<'a, 'tcx> {
let root = stat!("final", || {
let attrs = tcx.hir().krate_attrs();
self.lazy(CrateRoot {
name: tcx.crate_name(LOCAL_CRATE),
header: CrateHeader {
name: tcx.crate_name(LOCAL_CRATE),
triple: tcx.sess.opts.target_triple.clone(),
hash: tcx.crate_hash(LOCAL_CRATE),
is_proc_macro_crate: proc_macro_data.is_some(),
},
extra_filename: tcx.sess.opts.cg.extra_filename.clone(),
triple: tcx.sess.opts.target_triple.clone(),
hash: tcx.crate_hash(LOCAL_CRATE),
stable_crate_id: tcx.def_path_hash(LOCAL_CRATE.as_def_id()).stable_crate_id(),
required_panic_strategy: tcx.required_panic_strategy(LOCAL_CRATE),
panic_in_drop_strategy: tcx.sess.opts.unstable_opts.panic_in_drop,
Expand Down
31 changes: 26 additions & 5 deletions compiler/rustc_metadata/src/rmeta/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ pub(crate) fn rustc_version(cfg_version: &'static str) -> String {
/// Metadata encoding version.
/// N.B., increment this if you change the format of metadata such that
/// the rustc version can't be found to compare with `rustc_version()`.
const METADATA_VERSION: u8 = 7;
const METADATA_VERSION: u8 = 8;

/// Metadata header which includes `METADATA_VERSION`.
///
Expand Down Expand Up @@ -199,7 +199,27 @@ pub(crate) struct ProcMacroData {
macros: LazyArray<DefIndex>,
}

/// Serialized metadata for a crate.
/// Serialized crate metadata.
///
/// This contains just enough information to determine if we should load the `CrateRoot` or not.
/// Prefer [`CrateRoot`] whenever possible to avoid ICEs when using `omit-git-hash` locally.
/// See #76720 for more details.
///
/// If you do modify this struct, also bump the [`METADATA_VERSION`] constant.
#[derive(MetadataEncodable, MetadataDecodable)]
pub(crate) struct CrateHeader {
pub(crate) triple: TargetTriple,
pub(crate) hash: Svh,
pub(crate) name: Symbol,
/// Whether this is the header for a proc-macro crate.
///
/// This is separate from [`ProcMacroData`] to avoid having to update [`METADATA_VERSION`] every
/// time ProcMacroData changes.
pub(crate) is_proc_macro_crate: bool,
}

/// Serialized `.rmeta` data for a crate.
///
/// When compiling a proc-macro crate, we encode many of
/// the `LazyArray<T>` fields as `Lazy::empty()`. This serves two purposes:
///
Expand All @@ -217,10 +237,10 @@ pub(crate) struct ProcMacroData {
/// to being unused.
#[derive(MetadataEncodable, MetadataDecodable)]
pub(crate) struct CrateRoot {
name: Symbol,
triple: TargetTriple,
/// A header used to detect if this is the right crate to load.
header: CrateHeader,

extra_filename: String,
hash: Svh,
stable_crate_id: StableCrateId,
required_panic_strategy: Option<PanicStrategy>,
panic_in_drop_strategy: PanicStrategy,
Expand Down Expand Up @@ -465,6 +485,7 @@ trivially_parameterized_over_tcx! {
RawDefId,
TraitImpls,
IncoherentImpls,
CrateHeader,
CrateRoot,
CrateDep,
AttrFlags,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ pub fn read_version(dylib_path: &AbsPath) -> io::Result<String> {
// https://github.com/rust-lang/rust/commit/0696e79f2740ad89309269b460579e548a5cd632
let snappy_portion = match version {
5 | 6 => &dot_rustc[8..],
7 => {
7 | 8 => {
let len_bytes = &dot_rustc[8..12];
let data_len = u32::from_be_bytes(len_bytes.try_into().unwrap()) as usize;
&dot_rustc[12..data_len + 12]
Expand Down

0 comments on commit 99ff5af

Please sign in to comment.