cg_llvm: Assorted cleanups to LLVM diagnostic handling

compiler/rustc_codegen_llvm/src/back/write.rs

@@ -2,7 +2,7 @@ use std::ffi::{CStr, CString};
 use std::io::{self, Write};
 use std::path::{Path, PathBuf};
 use std::sync::Arc;
-use std::{fs, slice, str};
+use std::{fs, ptr, slice, str};
 
 use libc::{c_char, c_int, c_void, size_t};
 use rustc_codegen_ssa::back::link::ensure_removed;
@@ -319,10 +319,22 @@ pub(crate) enum CodegenDiagnosticsStage {
     Codegen,
 }
 
+/// Context data that is passed to [`diagnostic_handler_callback`].
+#[derive(Clone, Copy)]
+struct DiagnosticHandlerCallbackData<'a> {
+    cgcx: &'a CodegenContext,
+    shared_emitter: &'a SharedEmitter,
+}
+
 pub(crate) struct DiagnosticHandlers<'a> {
-    data: *mut (&'a CodegenContext, &'a SharedEmitter),
     llcx: &'a llvm::Context,
-    old_handler: Option<&'a llvm::DiagnosticHandler>,
+    old_handler: Option<ptr::NonNull<llvm::DiagnosticHandler>>,
+
+    /// Ownership of the box-allocated data that was installed (as a pointer)
+    /// alongside [`diagnostic_handler_callback`].
+    ///
+    /// To be freed via [`Box::from_raw`] after the handler is removed.
+    diagnostic_handler_callback_data: *mut DiagnosticHandlerCallbackData<'a>,
 }
 
 impl<'a> DiagnosticHandlers<'a> {
@@ -363,13 +375,14 @@ impl<'a> DiagnosticHandlers<'a> {
             .and_then(|dir| dir.to_str().and_then(|p| CString::new(p).ok()));
 
         let pgo_available = cgcx.module_config.pgo_use.is_some();
-        let data = Box::into_raw(Box::new((cgcx, shared_emitter)));
+        let diagnostic_handler_callback_data =
+            Box::into_raw(Box::new(DiagnosticHandlerCallbackData { cgcx, shared_emitter }));
         unsafe {
-            let old_handler = llvm::LLVMRustContextGetDiagnosticHandler(llcx);
+            let old_handler = llvm::LLVMRustContextTakeDiagnosticHandler(llcx);
             llvm::LLVMRustContextConfigureDiagnosticHandler(
                 llcx,
-                diagnostic_handler,
-                data.cast(),
+                diagnostic_handler_callback,
+                diagnostic_handler_callback_data.cast::<c_void>(),
                 remark_passes_all,
                 remark_passes.as_ptr(),
                 remark_passes.len(),
@@ -378,7 +391,7 @@ impl<'a> DiagnosticHandlers<'a> {
                 remark_file.as_ref().map(|dir| dir.as_ptr()).unwrap_or(std::ptr::null()),
                 pgo_available,
             );
-            DiagnosticHandlers { data, llcx, old_handler }
+            DiagnosticHandlers { llcx, old_handler, diagnostic_handler_callback_data }
         }
     }
 }
@@ -387,7 +400,7 @@ impl<'a> Drop for DiagnosticHandlers<'a> {
     fn drop(&mut self) {
         unsafe {
             llvm::LLVMRustContextSetDiagnosticHandler(self.llcx, self.old_handler);
-            drop(Box::from_raw(self.data));
+            drop(Box::from_raw(self.diagnostic_handler_callback_data));
         }
     }
 }
@@ -418,11 +431,21 @@ fn report_inline_asm(
     InlineAsmError { lo, hi, msg, level, source }
 }
 
-unsafe extern "C" fn diagnostic_handler(info: &DiagnosticInfo, user: *mut c_void) {
-    if user.is_null() {
+unsafe extern "C" fn diagnostic_handler_callback(info: &DiagnosticInfo, data: *mut c_void) {
+    if data.is_null() {
         return;
     }
-    let (cgcx, shared_emitter) = unsafe { *(user as *const (&CodegenContext, &SharedEmitter)) };
+    // After casting the data pointer, pass it into an inner function to limit its lifetimes.
+    diagnostic_handler_callback_inner(info, unsafe {
+        &*data.cast::<DiagnosticHandlerCallbackData<'_>>()
+    });
+}
+
+fn diagnostic_handler_callback_inner(
+    info: &DiagnosticInfo,
+    data: &DiagnosticHandlerCallbackData<'_>,
+) {
+    let &DiagnosticHandlerCallbackData { cgcx, shared_emitter } = data;
 
     let dcx = DiagCtxt::new(Box::new(shared_emitter.clone()));
     let dcx = dcx.handle();

compiler/rustc_codegen_llvm/src/llvm/ffi.rs

@@ -731,10 +731,12 @@ pub(crate) struct OperandBundle<'a>(InvariantOpaque<'a>);
 pub(crate) struct Linker<'a>(InvariantOpaque<'a>);
 
 unsafe extern "C" {
+    /// Corresponds to LLVM's `DiagnosticHandler`.
     pub(crate) type DiagnosticHandler;
 }
 
-pub(crate) type DiagnosticHandlerTy = unsafe extern "C" fn(&DiagnosticInfo, *mut c_void);
+/// Must match the declaration of `LLVMRustDiagnosticHandlerCallback`.
+pub(crate) type DiagnosticHandlerCallback = unsafe extern "C" fn(&DiagnosticInfo, *mut c_void);
 
 pub(crate) mod debuginfo {
     use bitflags::bitflags;
@@ -2578,17 +2580,23 @@ unsafe extern "C" {
         data: &ThinLTOData,
     );
 
-    pub(crate) fn LLVMRustContextGetDiagnosticHandler(
+    /// Removes the currently-installed `DiagnosticHandler`, and returns an owning pointer to it.
+    pub(crate) fn LLVMRustContextTakeDiagnosticHandler(
         Context: &Context,
-    ) -> Option<&DiagnosticHandler>;
+    ) -> Option<ptr::NonNull<DiagnosticHandler>>;
+
+    /// Installs a diagnostic handler that was previously removed by
+    /// [`LLVMRustContextTakeDiagnosticHandler`].
     pub(crate) fn LLVMRustContextSetDiagnosticHandler(
         context: &Context,
-        diagnostic_handler: Option<&DiagnosticHandler>,
+        diagnostic_handler: Option<ptr::NonNull<DiagnosticHandler>>,
     );
+
+    /// Installs a custom diagnostic handler.
     pub(crate) fn LLVMRustContextConfigureDiagnosticHandler(
         context: &Context,
-        diagnostic_handler_callback: DiagnosticHandlerTy,
-        diagnostic_handler_context: *mut c_void,
+        diagnostic_handler_callback: DiagnosticHandlerCallback,
+        diagnostic_handler_callback_data: *mut c_void,
         remark_all_passes: bool,
         remark_passes: *const *const c_char,
         remark_passes_len: usize,

compiler/rustc_llvm/llvm-wrapper/RustWrapper.cpp

@@ -1495,7 +1495,7 @@ extern "C" uint64_t LLVMRustModuleInstructionStats(LLVMModuleRef M) {
 
 // Transfers ownership of DiagnosticHandler unique_ptr to the caller.
 extern "C" DiagnosticHandler *
-LLVMRustContextGetDiagnosticHandler(LLVMContextRef C) {
+LLVMRustContextTakeDiagnosticHandler(LLVMContextRef C) {
   std::unique_ptr<DiagnosticHandler> DH = unwrap(C)->getDiagnosticHandler();
   return DH.release();
 }
@@ -1507,7 +1507,9 @@ extern "C" void LLVMRustContextSetDiagnosticHandler(LLVMContextRef C,
   unwrap(C)->setDiagnosticHandler(std::unique_ptr<DiagnosticHandler>(DH));
 }
 
-using LLVMDiagnosticHandlerTy = DiagnosticHandler::DiagnosticHandlerTy;
+// Must match `rustc_codegen_llvm::llvm::ffi::DiagnosticHandlerCallback`.
+typedef void (*LLVMRustDiagnosticHandlerCallback)(LLVMDiagnosticInfoRef,
+                                                  void *);
 
 // Configures a diagnostic handler that invokes provided callback when a
 // backend needs to emit a diagnostic.
@@ -1519,22 +1521,23 @@ using LLVMDiagnosticHandlerTy = DiagnosticHandler::DiagnosticHandlerTy;
 // If RemarkFilePath is not NULL, optimization remarks will be streamed directly
 // into this file, bypassing the diagnostics handler.
 extern "C" void LLVMRustContextConfigureDiagnosticHandler(
-    LLVMContextRef C, LLVMDiagnosticHandlerTy DiagnosticHandlerCallback,
-    void *DiagnosticHandlerContext, bool RemarkAllPasses,
+    LLVMContextRef C,
+    LLVMRustDiagnosticHandlerCallback DiagnosticHandlerCallback,
+    void *DiagnosticHandlerCallbackData, bool RemarkAllPasses,
     const char *const *RemarkPasses, size_t RemarkPassesLen,
     const char *RemarkFilePath, bool PGOAvailable) {
 
   class RustDiagnosticHandler final : public DiagnosticHandler {
   public:
     RustDiagnosticHandler(
-        LLVMDiagnosticHandlerTy DiagnosticHandlerCallback,
-        void *DiagnosticHandlerContext, bool RemarkAllPasses,
+        LLVMRustDiagnosticHandlerCallback DiagnosticHandlerCallback,
+        void *DiagnosticHandlerCallbackData, bool RemarkAllPasses,
         std::vector<std::string> RemarkPasses,
         std::unique_ptr<ToolOutputFile> RemarksFile,
         std::unique_ptr<llvm::remarks::RemarkStreamer> RemarkStreamer,
         std::unique_ptr<LLVMRemarkStreamer> LlvmRemarkStreamer)
         : DiagnosticHandlerCallback(DiagnosticHandlerCallback),
-          DiagnosticHandlerContext(DiagnosticHandlerContext),
+          DiagnosticHandlerCallbackData(DiagnosticHandlerCallbackData),
           RemarkAllPasses(RemarkAllPasses),
           RemarkPasses(std::move(RemarkPasses)),
           RemarksFile(std::move(RemarksFile)),
@@ -1566,7 +1569,7 @@ extern "C" void LLVMRustContextConfigureDiagnosticHandler(
         }
       }
       if (DiagnosticHandlerCallback) {
-        DiagnosticHandlerCallback(&DI, DiagnosticHandlerContext);
+        DiagnosticHandlerCallback(wrap(&DI), DiagnosticHandlerCallbackData);
         return true;
       }
       return false;
@@ -1600,8 +1603,8 @@ extern "C" void LLVMRustContextConfigureDiagnosticHandler(
       return false;
     }
 
-    LLVMDiagnosticHandlerTy DiagnosticHandlerCallback = nullptr;
-    void *DiagnosticHandlerContext = nullptr;
+    LLVMRustDiagnosticHandlerCallback DiagnosticHandlerCallback = nullptr;
+    void *DiagnosticHandlerCallbackData = nullptr;
 
     bool RemarkAllPasses = false;
     std::vector<std::string> RemarkPasses;
@@ -1654,7 +1657,7 @@ extern "C" void LLVMRustContextConfigureDiagnosticHandler(
   }
 
   unwrap(C)->setDiagnosticHandler(std::make_unique<RustDiagnosticHandler>(
-      DiagnosticHandlerCallback, DiagnosticHandlerContext, RemarkAllPasses,
+      DiagnosticHandlerCallback, DiagnosticHandlerCallbackData, RemarkAllPasses,
       Passes, std::move(RemarkFile), std::move(RemarkStreamer),
       std::move(LlvmRemarkStreamer)));
 }