Auto merge of #115911 - nebulark:refactor_targetmachine, r=Nilstrieb

Add OwnedTargetMachine to manage llvm:TargetMachine

LLVMRustDisposeTargetMachine taking a &mut could be undefined behaviour.
Wrapping it with a struct and using pointers instead avoids this problem.
In addition the TargetMachine is now automatically freed via the Wrappers drop impl. This should fix some memory leaks when
create_informational_target_machine was used, e.g. 327e6cf55c/compiler/rustc_codegen_llvm/src/llvm_util.rs (L291-L314)

r? `@Nilstrieb`

compiler/rustc_codegen_llvm/src/back/owned_target_machine.rs

@@ -0,0 +1,101 @@
+use std::{
+    ffi::{c_char, CStr},
+    marker::PhantomData,
+    ops::Deref,
+    ptr::NonNull,
+};
+
+use rustc_data_structures::small_c_str::SmallCStr;
+
+use crate::{errors::LlvmError, llvm};
+
+/// Responsible for safely creating and disposing llvm::TargetMachine via ffi functions.
+/// Not cloneable as there is no clone function for llvm::TargetMachine.
+#[repr(transparent)]
+pub struct OwnedTargetMachine {
+    tm_unique: NonNull<llvm::TargetMachine>,
+    phantom: PhantomData<llvm::TargetMachine>,
+}
+
+impl OwnedTargetMachine {
+    pub fn new(
+        triple: &CStr,
+        cpu: &CStr,
+        features: &CStr,
+        abi: &CStr,
+        model: llvm::CodeModel,
+        reloc: llvm::RelocModel,
+        level: llvm::CodeGenOptLevel,
+        use_soft_fp: bool,
+        function_sections: bool,
+        data_sections: bool,
+        unique_section_names: bool,
+        trap_unreachable: bool,
+        singletree: bool,
+        asm_comments: bool,
+        emit_stack_size_section: bool,
+        relax_elf_relocations: bool,
+        use_init_array: bool,
+        split_dwarf_file: &CStr,
+        debug_info_compression: &CStr,
+        force_emulated_tls: bool,
+        args_cstr_buff: &[u8],
+    ) -> Result<Self, LlvmError<'static>> {
+        assert!(args_cstr_buff.len() > 0);
+        assert!(
+            *args_cstr_buff.last().unwrap() == 0,
+            "The last character must be a null terminator."
+        );
+
+        // SAFETY: llvm::LLVMRustCreateTargetMachine copies pointed to data
+        let tm_ptr = unsafe {
+            llvm::LLVMRustCreateTargetMachine(
+                triple.as_ptr(),
+                cpu.as_ptr(),
+                features.as_ptr(),
+                abi.as_ptr(),
+                model,
+                reloc,
+                level,
+                use_soft_fp,
+                function_sections,
+                data_sections,
+                unique_section_names,
+                trap_unreachable,
+                singletree,
+                asm_comments,
+                emit_stack_size_section,
+                relax_elf_relocations,
+                use_init_array,
+                split_dwarf_file.as_ptr(),
+                debug_info_compression.as_ptr(),
+                force_emulated_tls,
+                args_cstr_buff.as_ptr() as *const c_char,
+                args_cstr_buff.len(),
+            )
+        };
+
+        NonNull::new(tm_ptr)
+            .map(|tm_unique| Self { tm_unique, phantom: PhantomData })
+            .ok_or_else(|| LlvmError::CreateTargetMachine { triple: SmallCStr::from(triple) })
+    }
+}
+
+impl Deref for OwnedTargetMachine {
+    type Target = llvm::TargetMachine;
+
+    fn deref(&self) -> &Self::Target {
+        // SAFETY: constructing ensures we have a valid pointer created by llvm::LLVMRustCreateTargetMachine
+        unsafe { self.tm_unique.as_ref() }
+    }
+}
+
+impl Drop for OwnedTargetMachine {
+    fn drop(&mut self) {
+        // SAFETY: constructing ensures we have a valid pointer created by llvm::LLVMRustCreateTargetMachine
+        // OwnedTargetMachine is not copyable so there is no double free or use after free
+        unsafe {
+            llvm::LLVMRustDisposeTargetMachine(self.tm_unique.as_mut());
+        }
+    }
+}

compiler/rustc_codegen_llvm/src/back/write.rs

@@ -1,4 +1,5 @@
 use crate::back::lto::ThinBuffer;
+use crate::back::owned_target_machine::OwnedTargetMachine;
 use crate::back::profiling::{
     selfprofile_after_pass_callback, selfprofile_before_pass_callback, LlvmSelfProfiler,
 };
@@ -98,7 +99,7 @@ pub fn write_output_file<'ll>(
     }
 }
 
-pub fn create_informational_target_machine(sess: &Session) -> &'static mut llvm::TargetMachine {
+pub fn create_informational_target_machine(sess: &Session) -> OwnedTargetMachine {
     let config = TargetMachineFactoryConfig { split_dwarf_file: None };
     // Can't use query system here quite yet because this function is invoked before the query
     // system/tcx is set up.
@@ -107,7 +108,7 @@ pub fn create_informational_target_machine(sess: &Session) -> &'static mut llvm:
         .unwrap_or_else(|err| llvm_err(sess.diagnostic(), err).raise())
 }
 
-pub fn create_target_machine(tcx: TyCtxt<'_>, mod_name: &str) -> &'static mut llvm::TargetMachine {
+pub fn create_target_machine(tcx: TyCtxt<'_>, mod_name: &str) -> OwnedTargetMachine {
     let split_dwarf_file = if tcx.sess.target_can_use_split_dwarf() {
         tcx.output_filenames(()).split_dwarf_path(
             tcx.sess.split_debuginfo(),
@@ -259,34 +260,29 @@ pub fn target_machine_factory(
             path_mapping.map_prefix(config.split_dwarf_file.unwrap_or_default()).0;
         let split_dwarf_file = CString::new(split_dwarf_file.to_str().unwrap()).unwrap();
 
-        let tm = unsafe {
+        OwnedTargetMachine::new(
-            llvm::LLVMRustCreateTargetMachine(
+            &triple,
-                triple.as_ptr(),
+            &cpu,
-                cpu.as_ptr(),
+            &features,
-                features.as_ptr(),
+            &abi,
-                abi.as_ptr(),
+            code_model,
-                code_model,
+            reloc_model,
-                reloc_model,
+            opt_level,
-                opt_level,
+            use_softfp,
-                use_softfp,
+            ffunction_sections,
-                ffunction_sections,
+            fdata_sections,
-                fdata_sections,
+            funique_section_names,
-                funique_section_names,
+            trap_unreachable,
-                trap_unreachable,
+            singlethread,
-                singlethread,
+            asm_comments,
-                asm_comments,
+            emit_stack_size_section,
-                emit_stack_size_section,
+            relax_elf_relocations,
-                relax_elf_relocations,
+            use_init_array,
-                use_init_array,
+            &split_dwarf_file,
-                split_dwarf_file.as_ptr(),
+            &debuginfo_compression,
-                debuginfo_compression.as_ptr(),
+            force_emulated_tls,
-                force_emulated_tls,
+            &args_cstr_buff,
-                args_cstr_buff.as_ptr() as *const c_char,
+        )
-                args_cstr_buff.len(),
-            )
-        };
-
-        tm.ok_or_else(|| LlvmError::CreateTargetMachine { triple: triple.clone() })
     })
 }
 

compiler/rustc_codegen_llvm/src/context.rs

@@ -160,9 +160,9 @@ pub unsafe fn create_module<'ll>(
 
     // Ensure the data-layout values hardcoded remain the defaults.
     if sess.target.is_builtin {
+        // tm is disposed by its drop impl
         let tm = crate::back::write::create_informational_target_machine(tcx.sess);
-        llvm::LLVMRustSetDataLayoutFromTargetMachine(llmod, tm);
+        llvm::LLVMRustSetDataLayoutFromTargetMachine(llmod, &tm);
-        llvm::LLVMRustDisposeTargetMachine(tm);
 
         let llvm_data_layout = llvm::LLVMGetDataLayoutStr(llmod);
         let llvm_data_layout = str::from_utf8(CStr::from_ptr(llvm_data_layout).to_bytes())

compiler/rustc_codegen_llvm/src/lib.rs

@@ -22,6 +22,7 @@ extern crate rustc_macros;
 #[macro_use]
 extern crate tracing;
 
+use back::owned_target_machine::OwnedTargetMachine;
 use back::write::{create_informational_target_machine, create_target_machine};
 
 use errors::ParseTargetMachineConfig;
@@ -52,6 +53,7 @@ use std::io::Write;
 mod back {
     pub mod archive;
     pub mod lto;
+    pub mod owned_target_machine;
     mod profiling;
     pub mod write;
 }
@@ -162,7 +164,7 @@ impl ExtraBackendMethods for LlvmCodegenBackend {
 impl WriteBackendMethods for LlvmCodegenBackend {
     type Module = ModuleLlvm;
     type ModuleBuffer = back::lto::ModuleBuffer;
-    type TargetMachine = &'static mut llvm::TargetMachine;
+    type TargetMachine = OwnedTargetMachine;
     type TargetMachineError = crate::errors::LlvmError<'static>;
     type ThinData = back::lto::ThinData;
     type ThinBuffer = back::lto::ThinBuffer;
@@ -401,7 +403,9 @@ impl CodegenBackend for LlvmCodegenBackend {
 pub struct ModuleLlvm {
     llcx: &'static mut llvm::Context,
     llmod_raw: *const llvm::Module,
-    tm: &'static mut llvm::TargetMachine,
+
+    // independent from llcx and llmod_raw, resources get disposed by drop impl
+    tm: OwnedTargetMachine,
 }
 
 unsafe impl Send for ModuleLlvm {}
@@ -453,7 +457,6 @@ impl ModuleLlvm {
 impl Drop for ModuleLlvm {
     fn drop(&mut self) {
         unsafe {
-            llvm::LLVMRustDisposeTargetMachine(&mut *(self.tm as *mut _));
             llvm::LLVMContextDispose(&mut *(self.llcx as *mut _));
         }
     }

compiler/rustc_codegen_llvm/src/llvm/ffi.rs

@@ -2112,6 +2112,8 @@ extern "C" {
     );
 
     pub fn LLVMRustGetHostCPUName(len: *mut usize) -> *const c_char;
+
+    // This function makes copies of pointed to data, so the data's lifetime may end after this function returns
     pub fn LLVMRustCreateTargetMachine(
         Triple: *const c_char,
         CPU: *const c_char,
@@ -2135,9 +2137,9 @@ extern "C" {
         ForceEmulatedTls: bool,
         ArgsCstrBuff: *const c_char,
         ArgsCstrBuffLen: usize,
-    ) -> Option<&'static mut TargetMachine>;
+    ) -> *mut TargetMachine;
 
-    pub fn LLVMRustDisposeTargetMachine(T: &'static mut TargetMachine);
+    pub fn LLVMRustDisposeTargetMachine(T: *mut TargetMachine);
     pub fn LLVMRustAddLibraryInfo<'a>(
         PM: &PassManager<'a>,
         M: &'a Module,

compiler/rustc_codegen_llvm/src/llvm_util.rs

@@ -303,7 +303,7 @@ pub fn target_features(sess: &Session, allow_unstable: bool) -> Vec<Symbol> {
             // check that all features in a given smallvec are enabled
             for llvm_feature in to_llvm_features(sess, feature) {
                 let cstr = SmallCStr::new(llvm_feature);
-                if !unsafe { llvm::LLVMRustHasFeature(target_machine, cstr.as_ptr()) } {
+                if !unsafe { llvm::LLVMRustHasFeature(&target_machine, cstr.as_ptr()) } {
                     return false;
                 }
             }
@@ -422,14 +422,14 @@ pub(crate) fn print(req: &PrintRequest, mut out: &mut dyn PrintBackendInfo, sess
             }
             unsafe {
                 llvm::LLVMRustPrintTargetCPUs(
-                    tm,
+                    &tm,
                     cpu_cstring.as_ptr(),
                     callback,
                     &mut out as *mut &mut dyn PrintBackendInfo as *mut c_void,
                 );
             }
         }
-        PrintKind::TargetFeatures => print_target_features(out, sess, tm),
+        PrintKind::TargetFeatures => print_target_features(out, sess, &tm),
         _ => bug!("rustc_codegen_llvm can't handle print request: {:?}", req),
     }
 }

compiler/rustc_data_structures/src/small_c_str.rs

@@ -79,3 +79,9 @@ impl<'a> FromIterator<&'a str> for SmallCStr {
         Self { data }
     }
 }
+
+impl From<&ffi::CStr> for SmallCStr {
+    fn from(s: &ffi::CStr) -> Self {
+        Self { data: SmallVec::from_slice(s.to_bytes()) }
+    }
+}