Skip to content

Include (potentially remapped) working dir in crate hash #87990

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

Merged
merged 4 commits into from
Aug 17, 2021
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
2 changes: 1 addition & 1 deletion compiler/rustc_codegen_cranelift/src/debuginfo/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ impl<'tcx> DebugContext<'tcx> {
rustc_interface::util::version_str().unwrap_or("unknown version"),
cranelift_codegen::VERSION,
);
let comp_dir = tcx.sess.working_dir.to_string_lossy(false).into_owned();
let comp_dir = tcx.sess.opts.working_dir.to_string_lossy(false).into_owned();
let (name, file_info) = match tcx.sess.local_crate_source_file.clone() {
Some(path) => {
let name = path.to_string_lossy().into_owned();
Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_codegen_llvm/src/debuginfo/metadata.rs
Original file line number Diff line number Diff line change
Expand Up @@ -771,7 +771,7 @@ pub fn file_metadata(cx: &CodegenCx<'ll, '_>, source_file: &SourceFile) -> &'ll
let hash = Some(&source_file.src_hash);
let file_name = Some(source_file.name.prefer_remapped().to_string());
let directory = if source_file.is_real_file() && !source_file.is_imported() {
Some(cx.sess().working_dir.to_string_lossy(false).to_string())
Some(cx.sess().opts.working_dir.to_string_lossy(false).to_string())
} else {
// If the path comes from an upstream crate we assume it has been made
// independent of the compiler's working directory one way or another.
Expand Down Expand Up @@ -999,7 +999,7 @@ pub fn compile_unit_metadata(
let producer = format!("clang LLVM ({})", rustc_producer);

let name_in_debuginfo = name_in_debuginfo.to_string_lossy();
let work_dir = tcx.sess.working_dir.to_string_lossy(false);
let work_dir = tcx.sess.opts.working_dir.to_string_lossy(false);
let flags = "\0";
let output_filenames = tcx.output_filenames(());
let out_dir = &output_filenames.out_directory;
Expand Down
5 changes: 4 additions & 1 deletion compiler/rustc_metadata/src/rmeta/encoder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -503,7 +503,10 @@ impl<'a, 'tcx> EncodeContext<'a, 'tcx> {
// Prepend path of working directory onto potentially
// relative paths, because they could become relative
// to a wrong directory.
let working_dir = &self.tcx.sess.working_dir;
// We include `working_dir` as part of the crate hash,
// so it's okay for us to use it as part of the encoded
// metadata.
let working_dir = &self.tcx.sess.opts.working_dir;
match working_dir {
RealFileName::LocalPath(absolute) => {
// If working_dir has not been remapped, then we emit a
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_save_analysis/src/dump_visitor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ impl<'tcx> DumpVisitor<'tcx> {
};

let data = CompilationOptions {
directory: self.tcx.sess.working_dir.remapped_path_if_available().into(),
directory: self.tcx.sess.opts.working_dir.remapped_path_if_available().into(),
program,
arguments,
output: self.save_ctxt.compilation_output(crate_name),
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_save_analysis/src/span_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ impl<'a> SpanUtils<'a> {
.to_string()
} else {
self.sess
.opts
.working_dir
.remapped_path_if_available()
.join(&path)
Expand Down
17 changes: 17 additions & 0 deletions compiler/rustc_session/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ use rustc_feature::UnstableFeatures;
use rustc_span::edition::{Edition, DEFAULT_EDITION, EDITION_NAME_LIST, LATEST_STABLE_EDITION};
use rustc_span::source_map::{FileName, FilePathMapping};
use rustc_span::symbol::{sym, Symbol};
use rustc_span::RealFileName;
use rustc_span::SourceFileHashAlgorithm;

use rustc_errors::emitter::HumanReadableErrorType;
Expand Down Expand Up @@ -707,6 +708,7 @@ impl Default for Options {
json_artifact_notifications: false,
json_unused_externs: false,
pretty: None,
working_dir: RealFileName::LocalPath(std::env::current_dir().unwrap()),
}
}
}
Expand Down Expand Up @@ -2132,6 +2134,18 @@ pub fn build_session_options(matches: &getopts::Matches) -> Options {
if candidate.join("library/std/src/lib.rs").is_file() { Some(candidate) } else { None }
};

let working_dir = std::env::current_dir().unwrap_or_else(|e| {
early_error(error_format, &format!("Current directory is invalid: {}", e));
});

let (path, remapped) =
FilePathMapping::new(remap_path_prefix.clone()).map_prefix(working_dir.clone());
let working_dir = if remapped {
RealFileName::Remapped { local_path: Some(working_dir), virtual_name: path }
} else {
RealFileName::LocalPath(path)
};

Options {
crate_types,
optimize: opt_level,
Expand Down Expand Up @@ -2167,6 +2181,7 @@ pub fn build_session_options(matches: &getopts::Matches) -> Options {
json_artifact_notifications,
json_unused_externs,
pretty,
working_dir,
}
}

Expand Down Expand Up @@ -2413,6 +2428,7 @@ crate mod dep_tracking {
use crate::utils::{NativeLib, NativeLibKind};
use rustc_feature::UnstableFeatures;
use rustc_span::edition::Edition;
use rustc_span::RealFileName;
use rustc_target::spec::{CodeModel, MergeFunctions, PanicStrategy, RelocModel};
use rustc_target::spec::{RelroLevel, SanitizerSet, SplitDebuginfo, TargetTriple, TlsModel};
use std::collections::hash_map::DefaultHasher;
Expand Down Expand Up @@ -2494,6 +2510,7 @@ crate mod dep_tracking {
TrimmedDefPaths,
Option<LdImpl>,
OutputType,
RealFileName,
);

impl<T1, T2> DepTrackingHash for (T1, T2)
Expand Down
4 changes: 4 additions & 0 deletions compiler/rustc_session/src/options.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ use rustc_target::spec::{RelocModel, RelroLevel, SplitDebuginfo, TargetTriple, T

use rustc_feature::UnstableFeatures;
use rustc_span::edition::Edition;
use rustc_span::RealFileName;
use rustc_span::SourceFileHashAlgorithm;

use std::collections::BTreeMap;
Expand Down Expand Up @@ -203,6 +204,9 @@ top_level_options!(
json_unused_externs: bool [UNTRACKED],

pretty: Option<PpMode> [UNTRACKED],

/// The (potentially remapped) working directory
working_dir: RealFileName [TRACKED],
Copy link
Member

Choose a reason for hiding this comment

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

This will result in a different hash for /wd/foo.rs vs /baz/foo.rs with /baz remapped to /wd despite the filename after any remapping being equal.

Copy link
Member Author

@Aaron1011 Aaron1011 Aug 15, 2021

Choose a reason for hiding this comment

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

I don't think so - this field is initialized using the remapped working directory.

Copy link
Member

Choose a reason for hiding this comment

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

RealFileName is an enum defined as

#[derive(HashStable_Generic)]
enum RealFileName {
    LocalPath(PathBuf),
    Remapped {
        local_path: Option<PathBuf>,
        virtual_name: PathBuf,
    },
}

while the Hash implementation only looks at the remapped path, the HashStable implementation is derived and looks at which enum variant it is and in case of a remapped path also at the local path before remapping. The dep tracking hash is calculated using HashStable and not Hash, so this will depend if the path is remapped or not and more importantly the local path before remapping, which is even worse that I thought as it breaks reproducible builds.

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 dep tracking hash is calculated using HashStable and not Hash

That's incorrect - I'm using impl_dep_tracking_hash_via_hash for RealFileName: https://github.com/Aaron1011/rust/blob/47daf829b1a0d2d499745a34cf4b1969929e5f7f/compiler/rustc_session/src/config.rs#L2513

It looks like the HashStable implementations for FileName and RealFileName are unused, and can be removed. Having Hash, HashStable,andDepTrackingHash` all implemented for a single type is pretty confusing - I wonder if there's any kind of compile-time check we can do to prevent that.

}
);

Expand Down
15 changes: 1 addition & 14 deletions compiler/rustc_session/src/session.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,8 @@ use rustc_errors::registry::Registry;
use rustc_errors::{DiagnosticBuilder, DiagnosticId, ErrorReported};
use rustc_macros::HashStable_Generic;
pub use rustc_span::def_id::StableCrateId;
use rustc_span::edition::Edition;
use rustc_span::source_map::{FileLoader, MultiSpan, RealFileLoader, SourceMap, Span};
use rustc_span::{edition::Edition, RealFileName};
use rustc_span::{sym, SourceFileHashAlgorithm, Symbol};
use rustc_target::asm::InlineAsmArch;
use rustc_target::spec::{CodeModel, PanicStrategy, RelocModel, RelroLevel};
Expand Down Expand Up @@ -139,8 +139,6 @@ pub struct Session {
/// The name of the root source file of the crate, in the local file system.
/// `None` means that there is no source file.
pub local_crate_source_file: Option<PathBuf>,
/// The directory the compiler has been executed in
pub working_dir: RealFileName,

/// Set of `(DiagnosticId, Option<Span>, message)` tuples tracking
/// (sub)diagnostics that have been set once, but should not be set again,
Expand Down Expand Up @@ -1304,16 +1302,6 @@ pub fn build_session(
let print_fuel_crate = sopts.debugging_opts.print_fuel.clone();
let print_fuel = AtomicU64::new(0);

let working_dir = env::current_dir().unwrap_or_else(|e| {
parse_sess.span_diagnostic.fatal(&format!("Current directory is invalid: {}", e)).raise()
});
let (path, remapped) = file_path_mapping.map_prefix(working_dir.clone());
let working_dir = if remapped {
RealFileName::Remapped { local_path: Some(working_dir), virtual_name: path }
} else {
RealFileName::LocalPath(path)
};

let cgu_reuse_tracker = if sopts.debugging_opts.query_dep_graph {
CguReuseTracker::new()
} else {
Expand Down Expand Up @@ -1344,7 +1332,6 @@ pub fn build_session(
parse_sess,
sysroot,
local_crate_source_file,
working_dir,
one_time_diagnostics: Default::default(),
crate_types: OnceCell::new(),
stable_crate_id: OnceCell::new(),
Expand Down
28 changes: 28 additions & 0 deletions src/test/run-make/issue-85019-moved-src-dir/Makefile
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
include ../../run-make-fulldeps/tools.mk

INCR=$(TMPDIR)/incr
FIRST_SRC=$(TMPDIR)/first_src
SECOND_SRC=$(TMPDIR)/second_src

# ignore-none no-std is not supported
# ignore-nvptx64-nvidia-cuda FIXME: can't find crate for 'std'

# Tests that we don't get an ICE when the working directory
# (but not the build directory!) changes between compilation
# sessions

all:
mkdir $(INCR)
# Build from 'FIRST_SRC'
mkdir $(FIRST_SRC)
cp my_lib.rs $(FIRST_SRC)/my_lib.rs
cp main.rs $(FIRST_SRC)/main.rs
cd $(FIRST_SRC) && \
$(RUSTC) -C incremental=$(INCR) --crate-type lib my_lib.rs --target $(TARGET) && \
$(RUSTC) -C incremental=$(INCR) --extern my_lib=$(TMPDIR)/libmy_lib.rlib main.rs --target $(TARGET)
# Build from 'SECOND_SRC', keeping the output directory and incremental directory
# the same
mv $(FIRST_SRC) $(SECOND_SRC)
cd $(SECOND_SRC) && \
$(RUSTC) -C incremental=$(INCR) --crate-type lib my_lib.rs --target $(TARGET) && \
$(RUSTC) -C incremental=$(INCR) --extern my_lib=$(TMPDIR)/libmy_lib.rlib main.rs --target $(TARGET)
5 changes: 5 additions & 0 deletions src/test/run-make/issue-85019-moved-src-dir/main.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
extern crate my_lib;

fn main() {
my_lib::my_fn("hi");
}
1 change: 1 addition & 0 deletions src/test/run-make/issue-85019-moved-src-dir/my_lib.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
pub fn my_fn<T: Copy>(_val: T) {}