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

Call FileEncoder::finish in rmeta encoding

Fixes #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`

Author: bors

Diff for: 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 for: 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 for: 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 for: 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 for: 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 for: 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 for: 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 for: compiler/rustc_metadata/src/errors.rs

@@ -307,15 +307,10 @@ pub struct FailCreateFileEncoder {
     pub err: Error,
 }
 
-#[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 for: 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 for: 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 for: 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 for: 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 for: 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,21 +35,28 @@ 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 {
     pub fn new<P: AsRef<Path>>(path: P) -> io::Result<Self> {
         // 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 for: 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(())
     };