Address review comments

Author: Kobzol

compiler/rustc_codegen_llvm/src/back/lto.rs

@@ -1,4 +1,4 @@
-use crate::back::write::{self, save_temp_bitcode, CodegenDiagnosticsSection, DiagnosticHandlers};
+use crate::back::write::{self, save_temp_bitcode, CodegenDiagnosticsStage, DiagnosticHandlers};
 use crate::errors::{
     DynamicLinkingWithLTO, LlvmError, LtoBitcodeFromRlib, LtoDisallowed, LtoDylib,
 };
@@ -307,7 +307,7 @@ fn fat_lto(
             diag_handler,
             llcx,
             &module,
-            CodegenDiagnosticsSection::LTO,
+            CodegenDiagnosticsStage::LTO,
         );
 
         // For all other modules we codegened we'll need to link them into our own

compiler/rustc_codegen_llvm/src/back/write.rs

@@ -269,10 +269,13 @@ pub(crate) fn save_temp_bitcode(
 }
 
 /// In what context is a dignostic handler being attached to a codegen unit?
-pub enum CodegenDiagnosticsSection {
-    Codegen,
+pub enum CodegenDiagnosticsStage {
+    /// Prelink optimization stage.
     Opt,
+    /// LTO/ThinLTO postlink optimization stage.
     LTO,
+    /// Code generation.
+    Codegen,
 }
 
 pub struct DiagnosticHandlers<'a> {
@@ -287,7 +290,7 @@ impl<'a> DiagnosticHandlers<'a> {
         handler: &'a Handler,
         llcx: &'a llvm::Context,
         module: &ModuleCodegen<ModuleLlvm>,
-        section: CodegenDiagnosticsSection,
+        section: CodegenDiagnosticsStage,
     ) -> Self {
         let remark_passes_all: bool;
         let remark_passes: Vec<CString>;
@@ -307,17 +310,16 @@ impl<'a> DiagnosticHandlers<'a> {
         let remark_file = cgcx
             .remark_dir
             .as_ref()
-            .and_then(|dir| dir.to_str())
-            // Use the .opt.yaml file pattern, which is supported by LLVM's opt-viewer.
+            // Use the .opt.yaml file suffix, which is supported by LLVM's opt-viewer.
             .map(|dir| {
                 let section = match section {
-                    CodegenDiagnosticsSection::Codegen => "codegen",
-                    CodegenDiagnosticsSection::Opt => "opt",
-                    CodegenDiagnosticsSection::LTO => "lto",
+                    CodegenDiagnosticsStage::Codegen => "codegen",
+                    CodegenDiagnosticsStage::Opt => "opt",
+                    CodegenDiagnosticsStage::LTO => "lto",
                 };
-                format!("{dir}/{}.{section}.opt.yaml", module.name)
+                dir.join(format!("{}.{section}.opt.yaml", module.name))
             })
-            .map(|dir| CString::new(dir).unwrap());
+            .and_then(|dir| dir.to_str().and_then(|p| CString::new(p).ok()));
 
         let data = Box::into_raw(Box::new((cgcx, handler)));
         unsafe {
@@ -551,7 +553,7 @@ pub(crate) unsafe fn optimize(
     let llmod = module.module_llvm.llmod();
     let llcx = &*module.module_llvm.llcx;
     let _handlers =
-        DiagnosticHandlers::new(cgcx, diag_handler, llcx, module, CodegenDiagnosticsSection::Opt);
+        DiagnosticHandlers::new(cgcx, diag_handler, llcx, module, CodegenDiagnosticsStage::Opt);
 
     let module_name = module.name.clone();
     let module_name = Some(&module_name[..]);
@@ -615,7 +617,7 @@ pub(crate) unsafe fn codegen(
             diag_handler,
             llcx,
             &module,
-            CodegenDiagnosticsSection::Codegen,
+            CodegenDiagnosticsStage::Codegen,
         );
 
         if cgcx.msvc_imps_needed {

compiler/rustc_llvm/llvm-wrapper/RustWrapper.cpp

@@ -1878,21 +1878,24 @@ extern "C" void LLVMRustContextConfigureDiagnosticHandler(
       bool RemarkAllPasses,
       std::vector<std::string> RemarkPasses,
       std::unique_ptr<llvm::remarks::RemarkStreamer> RemarkStreamer,
+      std::unique_ptr<LLVMRemarkStreamer> LlvmRemarkStreamer,
       std::unique_ptr<ToolOutputFile> RemarksFile
     )
         : DiagnosticHandlerCallback(DiagnosticHandlerCallback),
           DiagnosticHandlerContext(DiagnosticHandlerContext),
           RemarkAllPasses(RemarkAllPasses),
           RemarkPasses(std::move(RemarkPasses)),
           RemarkStreamer(std::move(RemarkStreamer)),
+          LlvmRemarkStreamer(std::move(LlvmRemarkStreamer)),
           RemarksFile(std::move(RemarksFile)) {}
 
     virtual bool handleDiagnostics(const DiagnosticInfo &DI) override {
-      if (this->RemarkStreamer) {
+      if (this->LlvmRemarkStreamer) {
         if (auto *OptDiagBase = dyn_cast<DiagnosticInfoOptimizationBase>(&DI)) {
-          LLVMRemarkStreamer Streamer(*this->RemarkStreamer);
-          Streamer.emit(*OptDiagBase);
-          return true;
+          if (OptDiagBase->isEnabled()) {
+            this->LlvmRemarkStreamer->emit(*OptDiagBase);
+            return true;
+          }
         }
       }
       if (DiagnosticHandlerCallback) {
@@ -1935,7 +1938,11 @@ extern "C" void LLVMRustContextConfigureDiagnosticHandler(
 
     bool RemarkAllPasses = false;
     std::vector<std::string> RemarkPasses;
+
+    // Since LlvmRemarkStreamer contains a pointer to RemarkStreamer, the ordering of the two
+    // members below is important.
     std::unique_ptr<llvm::remarks::RemarkStreamer> RemarkStreamer;
+    std::unique_ptr<LLVMRemarkStreamer> LlvmRemarkStreamer;
     std::unique_ptr<ToolOutputFile> RemarksFile;
   };
 
@@ -1945,8 +1952,9 @@ extern "C" void LLVMRustContextConfigureDiagnosticHandler(
     Passes.push_back(RemarkPasses[I]);
   }
 
-  // We need to hold onto both the streamer and the opened file
+  // We need to hold onto both the streamers and the opened file
   std::unique_ptr<llvm::remarks::RemarkStreamer> RemarkStreamer;
+  std::unique_ptr<LLVMRemarkStreamer> LlvmRemarkStreamer;
   std::unique_ptr<ToolOutputFile> RemarkFile;
 
   if (RemarkFilePath != nullptr) {
@@ -1970,6 +1978,7 @@ extern "C" void LLVMRustContextConfigureDiagnosticHandler(
       report_fatal_error("Cannot create remark serializer");
     }
     RemarkStreamer = std::make_unique<llvm::remarks::RemarkStreamer>(std::move(*RemarkSerializer));
+    LlvmRemarkStreamer = std::make_unique<LLVMRemarkStreamer>(*RemarkStreamer);
   }
 
   unwrap(C)->setDiagnosticHandler(std::make_unique<RustDiagnosticHandler>(
@@ -1978,6 +1987,7 @@ extern "C" void LLVMRustContextConfigureDiagnosticHandler(
     RemarkAllPasses,
     Passes,
     std::move(RemarkStreamer),
+    std::move(LlvmRemarkStreamer),
     std::move(RemarkFile)
   ));
 }