Auto merge of #117301 - saethlin:finish-rmeta-encoding, r=WaffleLapkin

Call FileEncoder::finish in rmeta encoding

Fixes https://github.com/rust-lang/rust/issues/117254

The bug here was that rmeta encoding never called FileEncoder::finish. Now it does. Most of the changes here are needed to support that, since rmeta encoding wants to finish _then_ access the File in the encoder, so finish can't move out.

I tried adding a `cfg(debug_assertions)` exploding Drop impl to FileEncoder that checked for finish being called before dropping, but fatal errors cause unwinding so this isn't really possible. If we encounter a fatal error with a dirty FileEncoder, the Drop impl ICEs even though the implementation is correct. If we try to paper over that by wrapping FileEncoder in ManuallyDrop then that just erases the fact that Drop automatically checks that we call finish on all paths.

I also changed the name of DepGraph::encode to DepGraph::finish_encoding, because that's what it does and it makes the fact that it is the path to FileEncoder::finish less confusing.

r? `@WaffleLapkin`

14 files changed, 82 insertions, 41 deletions

diff --git a/Cargo.lock b/Cargo.lock
index fa150b06b86..1296468b4e2 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -4038,6 +4038,7 @@ dependencies = [
  "rustc_query_impl",
  "rustc_query_system",
  "rustc_resolve",
+ "rustc_serialize",
  "rustc_session",
  "rustc_span",
  "rustc_symbol_mangling",
diff --git a/compiler/rustc_codegen_ssa/src/lib.rs b/compiler/rustc_codegen_ssa/src/lib.rs
index c65a0e968a5..a0d6e1885c2 100644
--- a/compiler/rustc_codegen_ssa/src/lib.rs
+++ b/compiler/rustc_codegen_ssa/src/lib.rs
@@ -224,7 +224,7 @@ impl CodegenResults {
         encoder.emit_raw_bytes(&RLINK_VERSION.to_be_bytes());
         encoder.emit_str(sess.cfg_version);
         Encodable::encode(codegen_results, &mut encoder);
-        encoder.finish()
+        encoder.finish().map_err(|(_path, err)| err)
     }
 
     pub fn deserialize_rlink(sess: &Session, data: Vec<u8>) -> Result<Self, CodegenErrors> {
diff --git a/compiler/rustc_incremental/src/persist/file_format.rs b/compiler/rustc_incremental/src/persist/file_format.rs
index 25bf83f64a0..b5742b97d02 100644
--- a/compiler/rustc_incremental/src/persist/file_format.rs
+++ b/compiler/rustc_incremental/src/persist/file_format.rs
@@ -80,8 +80,8 @@ where
             );
             debug!("save: data written to disk successfully");
         }
-        Err(err) => {
-            sess.emit_err(errors::WriteNew { name, path: path_buf, err });
+        Err((path, err)) => {
+            sess.emit_err(errors::WriteNew { name, path, err });
         }
     }
 }
diff --git a/compiler/rustc_incremental/src/persist/save.rs b/compiler/rustc_incremental/src/persist/save.rs
index fa21320be26..d6320d680e7 100644
--- a/compiler/rustc_incremental/src/persist/save.rs
+++ b/compiler/rustc_incremental/src/persist/save.rs
@@ -50,9 +50,6 @@ pub fn save_dep_graph(tcx: TyCtxt<'_>) {
         join(
             move || {
                 sess.time("incr_comp_persist_dep_graph", || {
-                    if let Err(err) = tcx.dep_graph.encode(&tcx.sess.prof) {
-                        sess.emit_err(errors::WriteDepGraph { path: &staging_dep_graph_path, err });
-                    }
                     if let Err(err) = fs::rename(&staging_dep_graph_path, &dep_graph_path) {
                         sess.emit_err(errors::MoveDepGraph {
                             from: &staging_dep_graph_path,
diff --git a/compiler/rustc_interface/Cargo.toml b/compiler/rustc_interface/Cargo.toml
index fd587e53f91..319e8175809 100644
--- a/compiler/rustc_interface/Cargo.toml
+++ b/compiler/rustc_interface/Cargo.toml
@@ -40,6 +40,7 @@ rustc_privacy = { path = "../rustc_privacy" }
 rustc_query_impl = { path = "../rustc_query_impl" }
 rustc_query_system = { path = "../rustc_query_system" }
 rustc_resolve = { path = "../rustc_resolve" }
+rustc_serialize = { path = "../rustc_serialize" }
 rustc_session = { path = "../rustc_session" }
 rustc_span = { path = "../rustc_span" }
 rustc_symbol_mangling = { path = "../rustc_symbol_mangling" }
diff --git a/compiler/rustc_interface/src/queries.rs b/compiler/rustc_interface/src/queries.rs
index 1c65cf19cde..bee27dc2d69 100644
--- a/compiler/rustc_interface/src/queries.rs
+++ b/compiler/rustc_interface/src/queries.rs
@@ -1,6 +1,6 @@
 use crate::errors::{FailedWritingFile, RustcErrorFatal, RustcErrorUnexpectedAnnotation};
 use crate::interface::{Compiler, Result};
-use crate::{passes, util};
+use crate::{errors, passes, util};
 
 use rustc_ast as ast;
 use rustc_codegen_ssa::traits::CodegenBackend;
@@ -15,6 +15,7 @@ use rustc_metadata::creader::CStore;
 use rustc_middle::arena::Arena;
 use rustc_middle::dep_graph::DepGraph;
 use rustc_middle::ty::{GlobalCtxt, TyCtxt};
+use rustc_serialize::opaque::FileEncodeResult;
 use rustc_session::config::{self, CrateType, OutputFilenames, OutputType};
 use rustc_session::cstore::Untracked;
 use rustc_session::output::find_crate_name;
@@ -102,6 +103,10 @@ impl<'tcx> Queries<'tcx> {
         }
     }
 
+    pub fn finish(&self) -> FileEncodeResult {
+        if let Some(gcx) = self.gcx_cell.get() { gcx.finish() } else { Ok(0) }
+    }
+
     pub fn parse(&self) -> Result<QueryResult<'_, ast::Crate>> {
         self.parse.compute(|| {
             passes::parse(&self.compiler.sess).map_err(|mut parse_error| parse_error.emit())
@@ -317,6 +322,9 @@ impl Compiler {
         // The timer's lifetime spans the dropping of `queries`, which contains
         // the global context.
         _timer = Some(self.sess.timer("free_global_ctxt"));
+        if let Err((path, error)) = queries.finish() {
+            self.sess.emit_err(errors::FailedWritingFile { path: &path, error });
+        }
 
         ret
     }
diff --git a/compiler/rustc_metadata/messages.ftl b/compiler/rustc_metadata/messages.ftl
index d1815717e22..44b235a6d3d 100644
--- a/compiler/rustc_metadata/messages.ftl
+++ b/compiler/rustc_metadata/messages.ftl
@@ -63,11 +63,8 @@ metadata_extern_location_not_file =
 metadata_fail_create_file_encoder =
     failed to create file encoder: {$err}
 
-metadata_fail_seek_file =
-    failed to seek the file: {$err}
-
 metadata_fail_write_file =
-    failed to write to the file: {$err}
+    failed to write to `{$path}`: {$err}
 
 metadata_failed_copy_to_stdout =
     failed to copy {$filename} to stdout: {$err}
diff --git a/compiler/rustc_metadata/src/errors.rs b/compiler/rustc_metadata/src/errors.rs
index 70daee291e7..edc8d8532d3 100644
--- a/compiler/rustc_metadata/src/errors.rs
+++ b/compiler/rustc_metadata/src/errors.rs
@@ -308,14 +308,9 @@ pub struct FailCreateFileEncoder {
 }
 
 #[derive(Diagnostic)]
-#[diag(metadata_fail_seek_file)]
-pub struct FailSeekFile {
-    pub err: Error,
-}
-
-#[derive(Diagnostic)]
 #[diag(metadata_fail_write_file)]
-pub struct FailWriteFile {
+pub struct FailWriteFile<'a> {
+    pub path: &'a Path,
     pub err: Error,
 }
 
diff --git a/compiler/rustc_metadata/src/rmeta/encoder.rs b/compiler/rustc_metadata/src/rmeta/encoder.rs
index 7c5d0eaf927..d697f23d230 100644
--- a/compiler/rustc_metadata/src/rmeta/encoder.rs
+++ b/compiler/rustc_metadata/src/rmeta/encoder.rs
@@ -1,4 +1,4 @@
-use crate::errors::{FailCreateFileEncoder, FailSeekFile, FailWriteFile};
+use crate::errors::{FailCreateFileEncoder, FailWriteFile};
 use crate::rmeta::def_path_hash_map::DefPathHashMapRef;
 use crate::rmeta::table::TableBuilder;
 use crate::rmeta::*;
@@ -42,6 +42,7 @@ use rustc_span::symbol::{sym, Symbol};
 use rustc_span::{self, ExternalSource, FileName, SourceFile, Span, SpanData, SyntaxContext};
 use std::borrow::Borrow;
 use std::collections::hash_map::Entry;
+use std::fs::File;
 use std::hash::Hash;
 use std::io::{Read, Seek, Write};
 use std::num::NonZeroUsize;
@@ -2255,25 +2256,34 @@ fn encode_metadata_impl(tcx: TyCtxt<'_>, path: &Path) {
     // culminating in the `CrateRoot` which points to all of it.
     let root = ecx.encode_crate_root();
 
-    ecx.opaque.flush();
+    // Make sure we report any errors from writing to the file.
+    // If we forget this, compilation can succeed with an incomplete rmeta file,
+    // causing an ICE when the rmeta file is read by another compilation.
+    if let Err((path, err)) = ecx.opaque.finish() {
+        tcx.sess.emit_err(FailWriteFile { path: &path, err });
+    }
+
+    let file = ecx.opaque.file();
+    if let Err(err) = encode_root_position(file, root.position.get()) {
+        tcx.sess.emit_err(FailWriteFile { path: ecx.opaque.path(), err });
+    }
+
+    // Record metadata size for self-profiling
+    tcx.prof.artifact_size("crate_metadata", "crate_metadata", file.metadata().unwrap().len());
+}
 
-    let mut file = ecx.opaque.file();
+fn encode_root_position(mut file: &File, pos: usize) -> Result<(), std::io::Error> {
     // We will return to this position after writing the root position.
     let pos_before_seek = file.stream_position().unwrap();
 
     // Encode the root position.
     let header = METADATA_HEADER.len();
-    file.seek(std::io::SeekFrom::Start(header as u64))
-        .unwrap_or_else(|err| tcx.sess.emit_fatal(FailSeekFile { err }));
-    let pos = root.position.get();
-    file.write_all(&[(pos >> 24) as u8, (pos >> 16) as u8, (pos >> 8) as u8, (pos >> 0) as u8])
-        .unwrap_or_else(|err| tcx.sess.emit_fatal(FailWriteFile { err }));
+    file.seek(std::io::SeekFrom::Start(header as u64))?;
+    file.write_all(&[(pos >> 24) as u8, (pos >> 16) as u8, (pos >> 8) as u8, (pos >> 0) as u8])?;
 
     // Return to the position where we are before writing the root position.
-    file.seek(std::io::SeekFrom::Start(pos_before_seek)).unwrap();
-
-    // Record metadata size for self-profiling
-    tcx.prof.artifact_size("crate_metadata", "crate_metadata", file.metadata().unwrap().len());
+    file.seek(std::io::SeekFrom::Start(pos_before_seek))?;
+    Ok(())
 }
 
 pub fn provide(providers: &mut Providers) {
diff --git a/compiler/rustc_middle/src/query/on_disk_cache.rs b/compiler/rustc_middle/src/query/on_disk_cache.rs
index ee6567f6cc4..f37cfe8b0a1 100644
--- a/compiler/rustc_middle/src/query/on_disk_cache.rs
+++ b/compiler/rustc_middle/src/query/on_disk_cache.rs
@@ -25,7 +25,6 @@ use rustc_span::source_map::{SourceMap, StableSourceFileId};
 use rustc_span::{BytePos, ExpnData, ExpnHash, Pos, RelativeBytePos, SourceFile, Span};
 use rustc_span::{CachingSourceMapView, Symbol};
 use std::collections::hash_map::Entry;
-use std::io;
 use std::mem;
 
 const TAG_FILE_FOOTER: u128 = 0xC0FFEE_C0FFEE_C0FFEE_C0FFEE_C0FFEE;
@@ -862,7 +861,7 @@ impl<'a, 'tcx> CacheEncoder<'a, 'tcx> {
     }
 
     #[inline]
-    fn finish(self) -> Result<usize, io::Error> {
+    fn finish(mut self) -> FileEncodeResult {
         self.encoder.finish()
     }
 }
diff --git a/compiler/rustc_middle/src/ty/context.rs b/compiler/rustc_middle/src/ty/context.rs
index df5ac28130a..bb7f0315ebd 100644
--- a/compiler/rustc_middle/src/ty/context.rs
+++ b/compiler/rustc_middle/src/ty/context.rs
@@ -629,6 +629,10 @@ impl<'tcx> GlobalCtxt<'tcx> {
         let icx = tls::ImplicitCtxt::new(self);
         tls::enter_context(&icx, || f(icx.tcx))
     }
+
+    pub fn finish(&self) -> FileEncodeResult {
+        self.dep_graph.finish_encoding(&self.sess.prof)
+    }
 }
 
 impl<'tcx> TyCtxt<'tcx> {
diff --git a/compiler/rustc_query_system/src/dep_graph/graph.rs b/compiler/rustc_query_system/src/dep_graph/graph.rs
index d720744a7a7..5acd012ef04 100644
--- a/compiler/rustc_query_system/src/dep_graph/graph.rs
+++ b/compiler/rustc_query_system/src/dep_graph/graph.rs
@@ -982,7 +982,7 @@ impl<D: Deps> DepGraph<D> {
         }
     }
 
-    pub fn encode(&self, profiler: &SelfProfilerRef) -> FileEncodeResult {
+    pub fn finish_encoding(&self, profiler: &SelfProfilerRef) -> FileEncodeResult {
         if let Some(data) = &self.data {
             data.current.encoder.steal().finish(profiler)
         } else {
diff --git a/compiler/rustc_serialize/src/opaque.rs b/compiler/rustc_serialize/src/opaque.rs
index 55255439051..cc8d1c25092 100644
--- a/compiler/rustc_serialize/src/opaque.rs
+++ b/compiler/rustc_serialize/src/opaque.rs
@@ -5,12 +5,13 @@ use std::io::{self, Write};
 use std::marker::PhantomData;
 use std::ops::Range;
 use std::path::Path;
+use std::path::PathBuf;
 
 // -----------------------------------------------------------------------------
 // Encoder
 // -----------------------------------------------------------------------------
 
-pub type FileEncodeResult = Result<usize, io::Error>;
+pub type FileEncodeResult = Result<usize, (PathBuf, io::Error)>;
 
 /// The size of the buffer in `FileEncoder`.
 const BUF_SIZE: usize = 8192;
@@ -34,6 +35,9 @@ pub struct FileEncoder {
     // This is used to implement delayed error handling, as described in the
     // comment on `trait Encoder`.
     res: Result<(), io::Error>,
+    path: PathBuf,
+    #[cfg(debug_assertions)]
+    finished: bool,
 }
 
 impl FileEncoder {
@@ -41,14 +45,18 @@ impl FileEncoder {
         // File::create opens the file for writing only. When -Zmeta-stats is enabled, the metadata
         // encoder rewinds the file to inspect what was written. So we need to always open the file
         // for reading and writing.
-        let file = File::options().read(true).write(true).create(true).truncate(true).open(path)?;
+        let file =
+            File::options().read(true).write(true).create(true).truncate(true).open(&path)?;
 
         Ok(FileEncoder {
             buf: vec![0u8; BUF_SIZE].into_boxed_slice().try_into().unwrap(),
+            path: path.as_ref().into(),
             buffered: 0,
             flushed: 0,
             file,
             res: Ok(()),
+            #[cfg(debug_assertions)]
+            finished: false,
         })
     }
 
@@ -63,6 +71,10 @@ impl FileEncoder {
     #[cold]
     #[inline(never)]
     pub fn flush(&mut self) {
+        #[cfg(debug_assertions)]
+        {
+            self.finished = false;
+        }
         if self.res.is_ok() {
             self.res = self.file.write_all(&self.buf[..self.buffered]);
         }
@@ -74,6 +86,10 @@ impl FileEncoder {
         &self.file
     }
 
+    pub fn path(&self) -> &Path {
+        &self.path
+    }
+
     #[inline]
     fn buffer_empty(&mut self) -> &mut [u8] {
         // SAFETY: self.buffered is inbounds as an invariant of the type
@@ -97,6 +113,10 @@ impl FileEncoder {
 
     #[inline]
     fn write_all(&mut self, buf: &[u8]) {
+        #[cfg(debug_assertions)]
+        {
+            self.finished = false;
+        }
         if let Some(dest) = self.buffer_empty().get_mut(..buf.len()) {
             dest.copy_from_slice(buf);
             self.buffered += buf.len();
@@ -121,6 +141,10 @@ impl FileEncoder {
     /// with one instruction, so while this does in some sense do wasted work, we come out ahead.
     #[inline]
     pub fn write_with<const N: usize>(&mut self, visitor: impl FnOnce(&mut [u8; N]) -> usize) {
+        #[cfg(debug_assertions)]
+        {
+            self.finished = false;
+        }
         let flush_threshold = const { BUF_SIZE.checked_sub(N).unwrap() };
         if std::intrinsics::unlikely(self.buffered > flush_threshold) {
             self.flush();
@@ -152,20 +176,25 @@ impl FileEncoder {
         })
     }
 
-    pub fn finish(mut self) -> Result<usize, io::Error> {
+    pub fn finish(&mut self) -> FileEncodeResult {
         self.flush();
+        #[cfg(debug_assertions)]
+        {
+            self.finished = true;
+        }
         match std::mem::replace(&mut self.res, Ok(())) {
             Ok(()) => Ok(self.position()),
-            Err(e) => Err(e),
+            Err(e) => Err((self.path.clone(), e)),
         }
     }
 }
 
+#[cfg(debug_assertions)]
 impl Drop for FileEncoder {
     fn drop(&mut self) {
-        // Likely to be a no-op, because `finish` should have been called and
-        // it also flushes. But do it just in case.
-        self.flush();
+        if !std::thread::panicking() {
+            assert!(self.finished);
+        }
     }
 }
 
diff --git a/src/librustdoc/scrape_examples.rs b/src/librustdoc/scrape_examples.rs
index dd52deef672..14680fdb064 100644
--- a/src/librustdoc/scrape_examples.rs
+++ b/src/librustdoc/scrape_examples.rs
@@ -325,7 +325,7 @@ pub(crate) fn run(
         // Save output to provided path
         let mut encoder = FileEncoder::new(options.output_path).map_err(|e| e.to_string())?;
         calls.encode(&mut encoder);
-        encoder.finish().map_err(|e| e.to_string())?;
+        encoder.finish().map_err(|(_path, e)| e.to_string())?;
 
         Ok(())
     };