Auto merge of rust-lang#85698 - ehuss:incremental-session-panic, r=estebank

Don't panic when failing to initialize incremental directory.

This removes a panic when rustc fails to initialize the incremental directory. This can commonly happen on various filesystems that don't support locking (often various network filesystems). Panics can be confusing and scary, and there are already plenty of issues reporting this.

This has been panicking since 1.22 due to I think rust-lang#44502 which was a major rework of how things work. Previously, things were simpler and the [`load_dep_graph`](https://github.com/rust-lang/rust/blob/1.21.0/src/librustc_incremental/persist/load.rs#L43-L65) function would emit an error and then continue on without panicking. With 1.22, [`load_dep_graph`](https://github.com/rust-lang/rust/blob/1.22.0/src/librustc_incremental/persist/load.rs#L44) was changed so that it assumes it can load the data without errors. Today, the problem is that it calls [`prepare_session_directory`](https://github.com/rust-lang/rust/blob/fbf1b1a7193cda17008ab590e06ad28d9924023b/compiler/rustc_interface/src/passes.rs#L175-L179) and then immediately calls `garbage_collect_session_directories` which will panic since the session is `IncrCompSession::NotInitialized`.

The solution here is to have `prepare_session_directory` return an error that must be handled so that compilation stops if it fails.

Some other options:

* Ignore directory lock failures.
* Print a warning on directory lock failure, but otherwise continue with incremental enabled.
* Print a warning on directory lock failure, and disable incremental.
* Provide a different locking mechanism.

Cargo ignores lock errors if locking is not supported, so that would be a precedent for the first option. These options would require quite a bit more changes, but I'm happy to entertain any of them, as I think they all have valid justifications.

There is more discussion on the many issues where this is reported: rust-lang#49773, rust-lang#59224, rust-lang#66513, rust-lang#76251. I'm not sure if this can be considered closing any of those, though, since I think there is some value in discussing if there is a way to avoid the error altogether. But I think it would make sense to at least close all but one to consolidate them.

Author: bors

Cargo.lock

@@ -3876,6 +3876,7 @@ dependencies = [
  "rand 0.7.3",
  "rustc_ast",
  "rustc_data_structures",
+ "rustc_errors",
  "rustc_fs_util",
  "rustc_graphviz",
  "rustc_hir",

compiler/rustc_data_structures/Cargo.toml

@@ -34,7 +34,7 @@ tempfile = "3.2"
 version = "0.11"
 
 [target.'cfg(windows)'.dependencies]
-winapi = { version = "0.3", features = ["fileapi", "psapi"] }
+winapi = { version = "0.3", features = ["fileapi", "psapi", "winerror"] }
 
 [target.'cfg(not(target_arch = "wasm32"))'.dependencies]
 memmap2 = "0.2.1"

compiler/rustc_data_structures/src/flock.rs

@@ -54,6 +54,10 @@ cfg_if! {
                     Ok(Lock { _file: file })
                 }
             }
+
+            pub fn error_unsupported(err: &io::Error) -> bool {
+                matches!(err.raw_os_error(), Some(libc::ENOTSUP) | Some(libc::ENOSYS))
+            }
         }
 
         // Note that we don't need a Drop impl to execute `flock(fd, LOCK_UN)`. Lock acquired by
@@ -103,6 +107,10 @@ cfg_if! {
                     Ok(Lock { file })
                 }
             }
+
+            pub fn error_unsupported(err: &io::Error) -> bool {
+                matches!(err.raw_os_error(), Some(libc::ENOTSUP) | Some(libc::ENOSYS))
+            }
         }
 
         impl Drop for Lock {
@@ -122,6 +130,7 @@ cfg_if! {
         use std::mem;
         use std::os::windows::prelude::*;
 
+        use winapi::shared::winerror::ERROR_INVALID_FUNCTION;
         use winapi::um::minwinbase::{OVERLAPPED, LOCKFILE_FAIL_IMMEDIATELY, LOCKFILE_EXCLUSIVE_LOCK};
         use winapi::um::fileapi::LockFileEx;
         use winapi::um::winnt::{FILE_SHARE_DELETE, FILE_SHARE_READ, FILE_SHARE_WRITE};
@@ -194,6 +203,10 @@ cfg_if! {
                     Ok(Lock { _file: file })
                 }
             }
+
+            pub fn error_unsupported(err: &io::Error) -> bool {
+                err.raw_os_error() == Some(ERROR_INVALID_FUNCTION as i32)
+            }
         }
 
         // Note that we don't need a Drop impl on the Windows: The file is unlocked

compiler/rustc_incremental/Cargo.toml

@@ -20,3 +20,4 @@ rustc_macros = { path = "../rustc_macros" }
 rustc_span = { path = "../rustc_span" }
 rustc_fs_util = { path = "../rustc_fs_util" }
 rustc_session = { path = "../rustc_session" }
+rustc_errors = { path = "../rustc_errors" }

compiler/rustc_incremental/src/persist/fs.rs

@@ -106,6 +106,7 @@
 use rustc_data_structures::fx::{FxHashMap, FxHashSet};
 use rustc_data_structures::svh::Svh;
 use rustc_data_structures::{base_n, flock};
+use rustc_errors::ErrorReported;
 use rustc_fs_util::{link_or_copy, LinkOrCopy};
 use rustc_session::{CrateDisambiguator, Session};
 
@@ -189,9 +190,9 @@ pub fn prepare_session_directory(
     sess: &Session,
     crate_name: &str,
     crate_disambiguator: CrateDisambiguator,
-) {
+) -> Result<(), ErrorReported> {
     if sess.opts.incremental.is_none() {
-        return;
+        return Ok(());
     }
 
     let _timer = sess.timer("incr_comp_prepare_session_directory");
@@ -201,9 +202,7 @@ pub fn prepare_session_directory(
     // {incr-comp-dir}/{crate-name-and-disambiguator}
     let crate_dir = crate_path(sess, crate_name, crate_disambiguator);
     debug!("crate-dir: {}", crate_dir.display());
-    if create_dir(sess, &crate_dir, "crate").is_err() {
-        return;
-    }
+    create_dir(sess, &crate_dir, "crate")?;
 
     // Hack: canonicalize the path *after creating the directory*
     // because, on windows, long paths can cause problems;
@@ -217,7 +216,7 @@ pub fn prepare_session_directory(
                 crate_dir.display(),
                 err
             ));
-            return;
+            return Err(ErrorReported);
         }
     };
 
@@ -232,16 +231,11 @@ pub fn prepare_session_directory(
 
         // Lock the new session directory. If this fails, return an
         // error without retrying
-        let (directory_lock, lock_file_path) = match lock_directory(sess, &session_dir) {
-            Ok(e) => e,
-            Err(_) => return,
-        };
+        let (directory_lock, lock_file_path) = lock_directory(sess, &session_dir)?;
 
         // Now that we have the lock, we can actually create the session
         // directory
-        if create_dir(sess, &session_dir, "session").is_err() {
-            return;
-        }
+        create_dir(sess, &session_dir, "session")?;
 
         // Find a suitable source directory to copy from. Ignore those that we
         // have already tried before.
@@ -257,7 +251,7 @@ pub fn prepare_session_directory(
             );
 
             sess.init_incr_comp_session(session_dir, directory_lock, false);
-            return;
+            return Ok(());
         };
 
         debug!("attempting to copy data from source: {}", source_directory.display());
@@ -278,7 +272,7 @@ pub fn prepare_session_directory(
             }
 
             sess.init_incr_comp_session(session_dir, directory_lock, true);
-            return;
+            return Ok(());
         } else {
             debug!("copying failed - trying next directory");
 
@@ -478,7 +472,7 @@ fn generate_session_dir_path(crate_dir: &Path) -> PathBuf {
     directory_path
 }
 
-fn create_dir(sess: &Session, path: &Path, dir_tag: &str) -> Result<(), ()> {
+fn create_dir(sess: &Session, path: &Path, dir_tag: &str) -> Result<(), ErrorReported> {
     match std_fs::create_dir_all(path) {
         Ok(()) => {
             debug!("{} directory created successfully", dir_tag);
@@ -492,13 +486,16 @@ fn create_dir(sess: &Session, path: &Path, dir_tag: &str) -> Result<(), ()> {
                 path.display(),
                 err
             ));
-            Err(())
+            Err(ErrorReported)
         }
     }
 }
 
 /// Allocate the lock-file and lock it.
-fn lock_directory(sess: &Session, session_dir: &Path) -> Result<(flock::Lock, PathBuf), ()> {
+fn lock_directory(
+    sess: &Session,
+    session_dir: &Path,
+) -> Result<(flock::Lock, PathBuf), ErrorReported> {
     let lock_file_path = lock_file_path(session_dir);
     debug!("lock_directory() - lock_file: {}", lock_file_path.display());
 
@@ -510,13 +507,36 @@ fn lock_directory(sess: &Session, session_dir: &Path) -> Result<(flock::Lock, Pa
     ) {
         // the lock should be exclusive
         Ok(lock) => Ok((lock, lock_file_path)),
-        Err(err) => {
-            sess.err(&format!(
+        Err(lock_err) => {
+            let mut err = sess.struct_err(&format!(
                 "incremental compilation: could not create \
-                               session directory lock file: {}",
-                err
+                 session directory lock file: {}",
+                lock_err
             ));
-            Err(())
+            if flock::Lock::error_unsupported(&lock_err) {
+                err.note(&format!(
+                    "the filesystem for the incremental path at {} \
+                     does not appear to support locking, consider changing the \
+                     incremental path to a filesystem that supports locking \
+                     or disable incremental compilation",
+                    session_dir.display()
+                ));
+                if std::env::var_os("CARGO").is_some() {
+                    err.help(
+                        "incremental compilation can be disabled by setting the \
+                         environment variable CARGO_INCREMENTAL=0 (see \
+                         https://doc.rust-lang.org/cargo/reference/profiles.html#incremental)",
+                    );
+                    err.help(
+                        "the entire build directory can be changed to a different \
+                        filesystem by setting the environment variable CARGO_TARGET_DIR \
+                        to a different path (see \
+                        https://doc.rust-lang.org/cargo/reference/config.html#buildtarget-dir)",
+                    );
+                }
+            }
+            err.emit();
+            Err(ErrorReported)
         }
     }
 }

compiler/rustc_interface/src/passes.rs

@@ -172,7 +172,7 @@ pub fn register_plugins<'a>(
 
     let disambiguator = util::compute_crate_disambiguator(sess);
     sess.crate_disambiguator.set(disambiguator).expect("not yet initialized");
-    rustc_incremental::prepare_session_directory(sess, &crate_name, disambiguator);
+    rustc_incremental::prepare_session_directory(sess, &crate_name, disambiguator)?;
 
     if sess.opts.incremental.is_some() {
         sess.time("incr_comp_garbage_collect_session_directories", || {

compiler/rustc_interface/src/queries.rs

@@ -148,7 +148,7 @@ impl<'tcx> Queries<'tcx> {
                 self.compiler.register_lints.as_deref().unwrap_or_else(|| empty),
                 krate,
                 &crate_name,
-            );
+            )?;
 
             // Compute the dependency graph (in the background). We want to do
             // this as early as possible, to give the DepGraph maximum time to
@@ -157,7 +157,7 @@ impl<'tcx> Queries<'tcx> {
             // called, which happens within passes::register_plugins().
             self.dep_graph_future().ok();
 
-            result
+            Ok(result)
         })
     }
 

src/test/run-make/incremental-session-fail/Makefile

@@ -0,0 +1,14 @@
+include ../../run-make-fulldeps/tools.mk
+
+SESSION_DIR := $(TMPDIR)/session
+OUTPUT_FILE := $(TMPDIR)/build-output
+
+all:
+	echo $(TMPDIR)
+	# Make it so that rustc will fail to create a session directory.
+	touch $(SESSION_DIR)
+	# Check exit code is 1 for an error, and not 101 for ICE.
+	$(RUSTC) foo.rs --crate-type=rlib -C incremental=$(SESSION_DIR) > $(OUTPUT_FILE) 2>&1; [ $$? -eq 1 ]
+	$(CGREP) "Could not create incremental compilation crate directory" < $(OUTPUT_FILE)
+	# -v tests are fragile, hopefully this text won't change
+	$(CGREP) -v "internal compiler error" < $(OUTPUT_FILE)

src/test/run-make/incremental-session-fail/foo.rs

@@ -0,0 +1 @@
+// intentionally empty