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

Eagerly compute output_filenames #117584

Merged
merged 7 commits into from
Nov 27, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions compiler/rustc_codegen_ssa/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,7 @@ impl CodegenResults {
sess: &Session,
rlink_file: &Path,
codegen_results: &CodegenResults,
outputs: &OutputFilenames,
) -> Result<usize, io::Error> {
let mut encoder = FileEncoder::new(rlink_file)?;
encoder.emit_raw_bytes(RLINK_MAGIC);
Expand All @@ -224,10 +225,14 @@ impl CodegenResults {
encoder.emit_raw_bytes(&RLINK_VERSION.to_be_bytes());
encoder.emit_str(sess.cfg_version);
Encodable::encode(codegen_results, &mut encoder);
Encodable::encode(outputs, &mut encoder);
encoder.finish().map_err(|(_path, err)| err)
}

pub fn deserialize_rlink(sess: &Session, data: Vec<u8>) -> Result<Self, CodegenErrors> {
pub fn deserialize_rlink(
sess: &Session,
data: Vec<u8>,
) -> Result<(Self, OutputFilenames), CodegenErrors> {
// The Decodable machinery is not used here because it panics if the input data is invalid
// and because its internal representation may change.
if !data.starts_with(RLINK_MAGIC) {
Expand Down Expand Up @@ -256,6 +261,7 @@ impl CodegenResults {
}

let codegen_results = CodegenResults::decode(&mut decoder);
Ok(codegen_results)
let outputs = OutputFilenames::decode(&mut decoder);
Ok((codegen_results, outputs))
}
}
13 changes: 4 additions & 9 deletions compiler/rustc_driver_impl/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -401,9 +401,7 @@ fn run_compiler(
Ok(())
})?;

// Make sure the `output_filenames` query is run for its side
// effects of writing the dep-info and reporting errors.
queries.global_ctxt()?.enter(|tcx| tcx.output_filenames(()));
queries.write_dep_info()?;
} else {
let krate = queries.parse()?;
pretty::print(
Expand Down Expand Up @@ -431,9 +429,7 @@ fn run_compiler(
return early_exit();
}

// Make sure the `output_filenames` query is run for its side
// effects of writing the dep-info and reporting errors.
queries.global_ctxt()?.enter(|tcx| tcx.output_filenames(()));
queries.write_dep_info()?;

if sess.opts.output_types.contains_key(&OutputType::DepInfo)
&& sess.opts.output_types.len() == 1
Expand Down Expand Up @@ -648,12 +644,11 @@ fn show_md_content_with_pager(content: &str, color: ColorConfig) {
fn process_rlink(sess: &Session, compiler: &interface::Compiler) {
assert!(sess.opts.unstable_opts.link_only);
if let Input::File(file) = &sess.io.input {
let outputs = compiler.build_output_filenames(sess, &[]);
let rlink_data = fs::read(file).unwrap_or_else(|err| {
sess.emit_fatal(RlinkUnableToRead { err });
});
let codegen_results = match CodegenResults::deserialize_rlink(sess, rlink_data) {
Ok(codegen) => codegen,
let (codegen_results, outputs) = match CodegenResults::deserialize_rlink(sess, rlink_data) {
Ok((codegen, outputs)) => (codegen, outputs),
Err(err) => {
match err {
CodegenErrors::WrongFileType => sess.emit_fatal(RLinkWrongFileType),
Expand Down
16 changes: 2 additions & 14 deletions compiler/rustc_interface/src/interface.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use crate::util;

use rustc_ast::token;
use rustc_ast::{self as ast, LitKind, MetaItemKind};
use rustc_ast::{LitKind, MetaItemKind};
use rustc_codegen_ssa::traits::CodegenBackend;
use rustc_data_structures::defer;
use rustc_data_structures::fx::{FxHashMap, FxHashSet};
Expand All @@ -15,9 +15,7 @@ use rustc_middle::{bug, ty};
use rustc_parse::maybe_new_parser_from_source_str;
use rustc_query_impl::QueryCtxt;
use rustc_query_system::query::print_query_stack;
use rustc_session::config::{
self, Cfg, CheckCfg, ExpectedValues, Input, OutFileName, OutputFilenames,
};
use rustc_session::config::{self, Cfg, CheckCfg, ExpectedValues, Input, OutFileName};
use rustc_session::filesearch::sysroot_candidates;
use rustc_session::parse::ParseSess;
use rustc_session::{lint, CompilerIO, EarlyErrorHandler, Session};
Expand All @@ -43,16 +41,6 @@ pub struct Compiler {
pub(crate) override_queries: Option<fn(&Session, &mut Providers)>,
}

impl Compiler {
pub fn build_output_filenames(
&self,
sess: &Session,
attrs: &[ast::Attribute],
) -> OutputFilenames {
util::build_output_filenames(attrs, sess)
}
}

/// Converts strings provided as `--cfg [cfgspec]` into a `Cfg`.
pub(crate) fn parse_cfg(handler: &EarlyErrorHandler, cfgs: Vec<String>) -> Cfg {
cfgs.into_iter()
Expand Down
17 changes: 9 additions & 8 deletions compiler/rustc_interface/src/passes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ use std::any::Any;
use std::ffi::OsString;
use std::io::{self, BufWriter, Write};
use std::path::{Path, PathBuf};
use std::sync::{Arc, LazyLock};
use std::sync::LazyLock;
use std::{env, fs, iter};

pub fn parse<'a>(sess: &'a Session) -> PResult<'a, ast::Crate> {
Expand Down Expand Up @@ -553,13 +553,17 @@ fn resolver_for_lowering<'tcx>(
tcx.arena.alloc(Steal::new((untracked_resolver_for_lowering, Lrc::new(krate))))
}

fn output_filenames(tcx: TyCtxt<'_>, (): ()) -> Arc<OutputFilenames> {
pub(crate) fn write_dep_info(tcx: TyCtxt<'_>) {
// Make sure name resolution and macro expansion is run for
// the side-effect of providing a complete set of all
// accessed files and env vars.
let _ = tcx.resolver_for_lowering(());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should use ensure().

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If resolver_for_lowering weren't eval_always and it is green, using ensure() would mean that the side effects of resolver_for_lowering (adding the list of used files and env vars to the Session) are not replayed as far as I understand it. Now resolver_for_lowering is eval_always currently, so it shouldn't matter for now I guess. There shouldn't be any perf difference though as resolver_for_lowering returns a reference.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we maybe add a comment in the query definition explaining this, i.e. why this needs to be eval_always?


let sess = tcx.sess;
let _timer = sess.timer("prepare_outputs");
let (_, krate) = &*tcx.resolver_for_lowering(()).borrow();
let _timer = sess.timer("write_dep_info");
let crate_name = tcx.crate_name(LOCAL_CRATE);

let outputs = util::build_output_filenames(&krate.attrs, sess);
let outputs = tcx.output_filenames(());
let output_paths =
generated_output_paths(tcx, &outputs, sess.io.output_file.is_some(), crate_name);

Expand Down Expand Up @@ -596,15 +600,12 @@ fn output_filenames(tcx: TyCtxt<'_>, (): ()) -> Arc<OutputFilenames> {
}
}
}

outputs.into()
}

pub static DEFAULT_QUERY_PROVIDERS: LazyLock<Providers> = LazyLock::new(|| {
let providers = &mut Providers::default();
providers.analysis = analysis;
providers.hir_crate = rustc_ast_lowering::lower_to_hir;
providers.output_filenames = output_filenames;
providers.resolver_for_lowering = resolver_for_lowering;
providers.early_lint_checks = early_lint_checks;
proc_macro_decls::provide(providers);
Expand Down
37 changes: 20 additions & 17 deletions compiler/rustc_interface/src/queries.rs
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,6 @@ pub struct Queries<'tcx> {
hir_arena: WorkerLocal<rustc_hir::Arena<'tcx>>,

parse: Query<ast::Crate>,
pre_configure: Query<(ast::Crate, ast::AttrVec)>,
// This just points to what's in `gcx_cell`.
gcx: Query<&'tcx GlobalCtxt<'tcx>>,
}
Expand All @@ -98,7 +97,6 @@ impl<'tcx> Queries<'tcx> {
arena: WorkerLocal::new(|_| Arena::default()),
hir_arena: WorkerLocal::new(|_| rustc_hir::Arena::default()),
parse: Default::default(),
pre_configure: Default::default(),
gcx: Default::default(),
}
}
Expand All @@ -113,12 +111,12 @@ impl<'tcx> Queries<'tcx> {
})
}

#[deprecated = "pre_configure may be made private in the future. If you need it please open an issue with your use case."]
pub fn pre_configure(&self) -> Result<QueryResult<'_, (ast::Crate, ast::AttrVec)>> {
self.pre_configure.compute(|| {
pub fn global_ctxt(&'tcx self) -> Result<QueryResult<'_, &'tcx GlobalCtxt<'tcx>>> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

self.gcx.compute(|| {
let sess = &self.compiler.sess;

let mut krate = self.parse()?.steal();

let sess = &self.compiler.sess;
rustc_builtin_macros::cmdline_attrs::inject(
&mut krate,
&sess.parse_sess,
Expand All @@ -127,15 +125,6 @@ impl<'tcx> Queries<'tcx> {

let pre_configured_attrs =
rustc_expand::config::pre_configure_attrs(sess, &krate.attrs);
Ok((krate, pre_configured_attrs))
})
}

pub fn global_ctxt(&'tcx self) -> Result<QueryResult<'_, &'tcx GlobalCtxt<'tcx>>> {
self.gcx.compute(|| {
let sess = &self.compiler.sess;
#[allow(deprecated)]
let (krate, pre_configured_attrs) = self.pre_configure()?.steal();

// parse `#[crate_name]` even if `--crate-name` was passed, to make sure it matches.
let crate_name = find_crate_name(sess, &pre_configured_attrs);
Expand All @@ -146,6 +135,7 @@ impl<'tcx> Queries<'tcx> {
sess.opts.cg.metadata.clone(),
sess.cfg_version,
);
let outputs = util::build_output_filenames(&pre_configured_attrs, sess);
let dep_graph = setup_dep_graph(sess, crate_name, stable_crate_id)?;

let cstore = FreezeLock::new(Box::new(CStore::new(
Expand Down Expand Up @@ -180,11 +170,19 @@ impl<'tcx> Queries<'tcx> {
crate_name,
)));
feed.crate_for_resolver(tcx.arena.alloc(Steal::new((krate, pre_configured_attrs))));
feed.output_filenames(Arc::new(outputs));
});
Ok(qcx)
})
}

pub fn write_dep_info(&'tcx self) -> Result<()> {
self.global_ctxt()?.enter(|tcx| {
passes::write_dep_info(tcx);
});
Ok(())
}

/// Check for the `#[rustc_error]` annotation, which forces an error in codegen. This is used
/// to write UI tests that actually test that compilation succeeds without reporting
/// an error.
Expand Down Expand Up @@ -284,8 +282,13 @@ impl Linker {

if sess.opts.unstable_opts.no_link {
let rlink_file = self.output_filenames.with_extension(config::RLINK_EXT);
CodegenResults::serialize_rlink(sess, &rlink_file, &codegen_results)
.map_err(|error| sess.emit_fatal(FailedWritingFile { path: &rlink_file, error }))?;
CodegenResults::serialize_rlink(
sess,
&rlink_file,
&codegen_results,
&*self.output_filenames,
)
.map_err(|error| sess.emit_fatal(FailedWritingFile { path: &rlink_file, error }))?;
return Ok(());
}

Expand Down
6 changes: 3 additions & 3 deletions compiler/rustc_session/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -580,7 +580,7 @@ pub enum ResolveDocLinks {
/// *Do not* switch `BTreeMap` out for an unsorted container type! That would break
/// dependency tracking for command-line arguments. Also only hash keys, since tracking
/// should only depend on the output types, not the paths they're written to.
#[derive(Clone, Debug, Hash, HashStable_Generic)]
#[derive(Clone, Debug, Hash, HashStable_Generic, Encodable, Decodable)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this cause us to leak host info into build artifacts?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rlink file already contains absolute file paths. rustc -Zlink-only foo.rlink is supposed to be called immediately after all dependencies of a rustc -Zlink-only invocation are finished. This has to be done on the same machine and can only be done once because all temporary files that are shared between the two rustc invocations have unpredictable names and are removed by rustc -Zlink-only.

pub struct OutputTypes(BTreeMap<OutputType, Option<OutFileName>>);

impl OutputTypes {
Expand Down Expand Up @@ -818,7 +818,7 @@ impl Input {
}
}

#[derive(Clone, Hash, Debug, HashStable_Generic, PartialEq)]
#[derive(Clone, Hash, Debug, HashStable_Generic, PartialEq, Encodable, Decodable)]
pub enum OutFileName {
Real(PathBuf),
Stdout,
Expand Down Expand Up @@ -890,7 +890,7 @@ impl OutFileName {
}
}

#[derive(Clone, Hash, Debug, HashStable_Generic)]
#[derive(Clone, Hash, Debug, HashStable_Generic, Encodable, Decodable)]
pub struct OutputFilenames {
pub out_directory: PathBuf,
/// Crate name. Never contains '-'.
Expand Down