Rollup merge of #132452 - Zalathar:llvm-cov-wrappers, r=jieyouxu

coverage: Extract safe FFI wrapper functions to `llvm_cov`

This PR takes all of the inline `unsafe` calls in coverage codegen, and all the safe wrapper functions in `coverageinfo/mod.rs`, and moves them to a new `llvm_cov` submodule that is dedicated to safe FFI wrapper functions. This reduces the mixing of abstraction levels in the rest of coverage codegen.

As a follow-up, this PR also tidies up the names and signatures of several of the coverage FFI functions.

compiler/rustc_codegen_llvm/src/coverageinfo/llvm_cov.rs

@@ -0,0 +1,100 @@
+//! Safe wrappers for coverage-specific FFI functions.
+
+use std::ffi::CString;
+
+use crate::common::AsCCharPtr;
+use crate::coverageinfo::ffi;
+use crate::llvm;
+
+pub(crate) fn covmap_var_name() -> CString {
+    CString::new(llvm::build_byte_buffer(|s| unsafe {
+        llvm::LLVMRustCoverageWriteCovmapVarNameToString(s);
+    }))
+    .expect("covmap variable name should not contain NUL")
+}
+
+pub(crate) fn covmap_section_name(llmod: &llvm::Module) -> CString {
+    CString::new(llvm::build_byte_buffer(|s| unsafe {
+        llvm::LLVMRustCoverageWriteCovmapSectionNameToString(llmod, s);
+    }))
+    .expect("covmap section name should not contain NUL")
+}
+
+pub(crate) fn covfun_section_name(llmod: &llvm::Module) -> CString {
+    CString::new(llvm::build_byte_buffer(|s| unsafe {
+        llvm::LLVMRustCoverageWriteCovfunSectionNameToString(llmod, s);
+    }))
+    .expect("covfun section name should not contain NUL")
+}
+
+pub(crate) fn create_pgo_func_name_var<'ll>(
+    llfn: &'ll llvm::Value,
+    mangled_fn_name: &str,
+) -> &'ll llvm::Value {
+    unsafe {
+        llvm::LLVMRustCoverageCreatePGOFuncNameVar(
+            llfn,
+            mangled_fn_name.as_c_char_ptr(),
+            mangled_fn_name.len(),
+        )
+    }
+}
+
+pub(crate) fn write_filenames_to_buffer<'a>(
+    filenames: impl IntoIterator<Item = &'a str>,
+) -> Vec<u8> {
+    let (pointers, lengths) = filenames
+        .into_iter()
+        .map(|s: &str| (s.as_c_char_ptr(), s.len()))
+        .unzip::<_, _, Vec<_>, Vec<_>>();
+
+    llvm::build_byte_buffer(|buffer| unsafe {
+        llvm::LLVMRustCoverageWriteFilenamesToBuffer(
+            pointers.as_ptr(),
+            pointers.len(),
+            lengths.as_ptr(),
+            lengths.len(),
+            buffer,
+        );
+    })
+}
+
+pub(crate) fn write_function_mappings_to_buffer(
+    virtual_file_mapping: &[u32],
+    expressions: &[ffi::CounterExpression],
+    code_regions: &[ffi::CodeRegion],
+    branch_regions: &[ffi::BranchRegion],
+    mcdc_branch_regions: &[ffi::MCDCBranchRegion],
+    mcdc_decision_regions: &[ffi::MCDCDecisionRegion],
+) -> Vec<u8> {
+    llvm::build_byte_buffer(|buffer| unsafe {
+        llvm::LLVMRustCoverageWriteFunctionMappingsToBuffer(
+            virtual_file_mapping.as_ptr(),
+            virtual_file_mapping.len(),
+            expressions.as_ptr(),
+            expressions.len(),
+            code_regions.as_ptr(),
+            code_regions.len(),
+            branch_regions.as_ptr(),
+            branch_regions.len(),
+            mcdc_branch_regions.as_ptr(),
+            mcdc_branch_regions.len(),
+            mcdc_decision_regions.as_ptr(),
+            mcdc_decision_regions.len(),
+            buffer,
+        )
+    })
+}
+
+/// Hashes some bytes into a 64-bit hash, via LLVM's `IndexedInstrProf::ComputeHash`,
+/// as required for parts of the LLVM coverage mapping format.
+pub(crate) fn hash_bytes(bytes: &[u8]) -> u64 {
+    unsafe { llvm::LLVMRustCoverageHashBytes(bytes.as_c_char_ptr(), bytes.len()) }
+}
+
+/// Returns LLVM's `coverage::CovMapVersion::CurrentVersion` (CoverageMapping.h)
+/// as a raw numeric value. For historical reasons, the numeric value is 1 less
+/// than the number in the version's name, so `Version7` is actually `6u32`.
+pub(crate) fn mapping_version() -> u32 {
+    unsafe { llvm::LLVMRustCoverageMappingVersion() }
+}

compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs

@@ -1,4 +1,5 @@
 use std::ffi::CString;
+use std::iter;
 
 use itertools::Itertools as _;
 use rustc_abi::Align;
@@ -17,9 +18,9 @@ use rustc_target::spec::HasTargetSpec;
 use tracing::debug;
 
 use crate::common::CodegenCx;
-use crate::coverageinfo::ffi;
 use crate::coverageinfo::map_data::{FunctionCoverage, FunctionCoverageCollector};
-use crate::{coverageinfo, llvm};
+use crate::coverageinfo::{ffi, llvm_cov};
+use crate::llvm;
 
 /// Generates and exports the coverage map, which is embedded in special
 /// linker sections in the final binary.
@@ -33,7 +34,7 @@ pub(crate) fn finalize(cx: &CodegenCx<'_, '_>) {
     // agrees with our Rust-side code. Expected versions (encoded as n-1) are:
     // - `CovMapVersion::Version7` (6) used by LLVM 18-19
     let covmap_version = {
-        let llvm_covmap_version = coverageinfo::mapping_version();
+        let llvm_covmap_version = llvm_cov::mapping_version();
         let expected_versions = 6..=6;
         assert!(
             expected_versions.contains(&llvm_covmap_version),
@@ -78,7 +79,7 @@ pub(crate) fn finalize(cx: &CodegenCx<'_, '_>) {
 
     let filenames_size = filenames_buffer.len();
     let filenames_val = cx.const_bytes(&filenames_buffer);
-    let filenames_ref = coverageinfo::hash_bytes(&filenames_buffer);
+    let filenames_ref = llvm_cov::hash_bytes(&filenames_buffer);
 
     // Generate the coverage map header, which contains the filenames used by
     // this CGU's coverage mappings, and store it in a well-known global.
@@ -187,13 +188,10 @@ impl GlobalFileTable {
             .for_scope(tcx.sess, RemapPathScopeComponents::MACRO)
             .to_string_lossy();
 
-        llvm::build_byte_buffer(|buffer| {
-            coverageinfo::write_filenames_section_to_buffer(
         // Insert the working dir at index 0, before the other filenames.
-                std::iter::once(working_dir).chain(self.raw_file_table.iter().map(Symbol::as_str)),
+        let filenames =
-                buffer,
+            iter::once(working_dir).chain(self.raw_file_table.iter().map(Symbol::as_str));
-            );
+        llvm_cov::write_filenames_to_buffer(filenames)
-        })
     }
 }
 
@@ -296,17 +294,14 @@ fn encode_mappings_for_function(
     }
 
     // Encode the function's coverage mappings into a buffer.
-    llvm::build_byte_buffer(|buffer| {
+    llvm_cov::write_function_mappings_to_buffer(
-        coverageinfo::write_mapping_to_buffer(
+        &virtual_file_mapping.into_vec(),
-            virtual_file_mapping.into_vec(),
+        &expressions,
-            expressions,
         &code_regions,
         &branch_regions,
         &mcdc_branch_regions,
         &mcdc_decision_regions,
-            buffer,
+    )
-        );
-    })
 }
 
 /// Generates the contents of the covmap record for this CGU, which mostly
@@ -335,23 +330,11 @@ fn generate_covmap_record<'ll>(
     let covmap_data =
         cx.const_struct(&[cov_data_header_val, filenames_val], /*packed=*/ false);
 
-    let covmap_var_name = CString::new(llvm::build_byte_buffer(|s| unsafe {
+    let llglobal = llvm::add_global(cx.llmod, cx.val_ty(covmap_data), &llvm_cov::covmap_var_name());
-        llvm::LLVMRustCoverageWriteMappingVarNameToString(s);
-    }))
-    .unwrap();
-    debug!("covmap var name: {:?}", covmap_var_name);
-
-    let covmap_section_name = CString::new(llvm::build_byte_buffer(|s| unsafe {
-        llvm::LLVMRustCoverageWriteMapSectionNameToString(cx.llmod, s);
-    }))
-    .expect("covmap section name should not contain NUL");
-    debug!("covmap section name: {:?}", covmap_section_name);
-
-    let llglobal = llvm::add_global(cx.llmod, cx.val_ty(covmap_data), &covmap_var_name);
     llvm::set_initializer(llglobal, covmap_data);
     llvm::set_global_constant(llglobal, true);
     llvm::set_linkage(llglobal, llvm::Linkage::PrivateLinkage);
-    llvm::set_section(llglobal, &covmap_section_name);
+    llvm::set_section(llglobal, &llvm_cov::covmap_section_name(cx.llmod));
     // LLVM's coverage mapping format specifies 8-byte alignment for items in this section.
     // <https://llvm.org/docs/CoverageMappingFormat.html>
     llvm::set_alignment(llglobal, Align::EIGHT);
@@ -373,7 +356,7 @@ fn generate_covfun_record(
     let coverage_mapping_size = coverage_mapping_buffer.len();
     let coverage_mapping_val = cx.const_bytes(&coverage_mapping_buffer);
 
-    let func_name_hash = coverageinfo::hash_bytes(mangled_function_name.as_bytes());
+    let func_name_hash = llvm_cov::hash_bytes(mangled_function_name.as_bytes());
     let func_name_hash_val = cx.const_u64(func_name_hash);
     let coverage_mapping_size_val = cx.const_u32(coverage_mapping_size as u32);
     let source_hash_val = cx.const_u64(source_hash);

compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs

@@ -1,24 +1,23 @@
 use std::cell::{OnceCell, RefCell};
 use std::ffi::{CStr, CString};
 
-use libc::c_uint;
 use rustc_abi::Size;
 use rustc_codegen_ssa::traits::{
     BuilderMethods, ConstCodegenMethods, CoverageInfoBuilderMethods, MiscCodegenMethods,
 };
 use rustc_data_structures::fx::{FxHashMap, FxIndexMap};
-use rustc_llvm::RustString;
 use rustc_middle::mir::coverage::CoverageKind;
 use rustc_middle::ty::Instance;
 use rustc_middle::ty::layout::HasTyCtxt;
 use tracing::{debug, instrument};
 
 use crate::builder::Builder;
-use crate::common::{AsCCharPtr, CodegenCx};
+use crate::common::CodegenCx;
 use crate::coverageinfo::map_data::FunctionCoverageCollector;
 use crate::llvm;
 
 pub(crate) mod ffi;
+mod llvm_cov;
 pub(crate) mod map_data;
 mod mapgen;
 
@@ -80,12 +79,9 @@ impl<'ll, 'tcx> CodegenCx<'ll, 'tcx> {
     /// - `__LLVM_COV,__llvm_covfun` on macOS (includes `__LLVM_COV,` segment prefix)
     /// - `.lcovfun$M` on Windows (includes `$M` sorting suffix)
     fn covfun_section_name(&self) -> &CStr {
-        self.coverage_cx().covfun_section_name.get_or_init(|| {
+        self.coverage_cx()
-            CString::new(llvm::build_byte_buffer(|s| unsafe {
+            .covfun_section_name
-                llvm::LLVMRustCoverageWriteFuncSectionNameToString(self.llmod, s);
+            .get_or_init(|| llvm_cov::covfun_section_name(self.llmod))
-            }))
-            .expect("covfun section name should not contain NUL")
-        })
     }
 
     /// For LLVM codegen, returns a function-specific `Value` for a global
@@ -95,9 +91,11 @@ impl<'ll, 'tcx> CodegenCx<'ll, 'tcx> {
     fn get_pgo_func_name_var(&self, instance: Instance<'tcx>) -> &'ll llvm::Value {
         debug!("getting pgo_func_name_var for instance={:?}", instance);
         let mut pgo_func_name_var_map = self.coverage_cx().pgo_func_name_var_map.borrow_mut();
-        pgo_func_name_var_map
+        pgo_func_name_var_map.entry(instance).or_insert_with(|| {
-            .entry(instance)
+            let llfn = self.get_fn(instance);
-            .or_insert_with(|| create_pgo_func_name_var(self, instance))
+            let mangled_fn_name: &str = self.tcx.symbol_name(instance).name;
+            llvm_cov::create_pgo_func_name_var(llfn, mangled_fn_name)
+        })
     }
 }
 
@@ -225,80 +223,3 @@ impl<'tcx> CoverageInfoBuilderMethods<'tcx> for Builder<'_, '_, 'tcx> {
         }
     }
 }
-
-/// Calls llvm::createPGOFuncNameVar() with the given function instance's
-/// mangled function name. The LLVM API returns an llvm::GlobalVariable
-/// containing the function name, with the specific variable name and linkage
-/// required by LLVM InstrProf source-based coverage instrumentation. Use
-/// `bx.get_pgo_func_name_var()` to ensure the variable is only created once per
-/// `Instance`.
-fn create_pgo_func_name_var<'ll, 'tcx>(
-    cx: &CodegenCx<'ll, 'tcx>,
-    instance: Instance<'tcx>,
-) -> &'ll llvm::Value {
-    let mangled_fn_name: &str = cx.tcx.symbol_name(instance).name;
-    let llfn = cx.get_fn(instance);
-    unsafe {
-        llvm::LLVMRustCoverageCreatePGOFuncNameVar(
-            llfn,
-            mangled_fn_name.as_c_char_ptr(),
-            mangled_fn_name.len(),
-        )
-    }
-}
-
-pub(crate) fn write_filenames_section_to_buffer<'a>(
-    filenames: impl IntoIterator<Item = &'a str>,
-    buffer: &RustString,
-) {
-    let (pointers, lengths) = filenames
-        .into_iter()
-        .map(|s: &str| (s.as_c_char_ptr(), s.len()))
-        .unzip::<_, _, Vec<_>, Vec<_>>();
-
-    unsafe {
-        llvm::LLVMRustCoverageWriteFilenamesSectionToBuffer(
-            pointers.as_ptr(),
-            pointers.len(),
-            lengths.as_ptr(),
-            lengths.len(),
-            buffer,
-        );
-    }
-}
-
-pub(crate) fn write_mapping_to_buffer(
-    virtual_file_mapping: Vec<u32>,
-    expressions: Vec<ffi::CounterExpression>,
-    code_regions: &[ffi::CodeRegion],
-    branch_regions: &[ffi::BranchRegion],
-    mcdc_branch_regions: &[ffi::MCDCBranchRegion],
-    mcdc_decision_regions: &[ffi::MCDCDecisionRegion],
-    buffer: &RustString,
-) {
-    unsafe {
-        llvm::LLVMRustCoverageWriteMappingToBuffer(
-            virtual_file_mapping.as_ptr(),
-            virtual_file_mapping.len() as c_uint,
-            expressions.as_ptr(),
-            expressions.len() as c_uint,
-            code_regions.as_ptr(),
-            code_regions.len() as c_uint,
-            branch_regions.as_ptr(),
-            branch_regions.len() as c_uint,
-            mcdc_branch_regions.as_ptr(),
-            mcdc_branch_regions.len() as c_uint,
-            mcdc_decision_regions.as_ptr(),
-            mcdc_decision_regions.len() as c_uint,
-            buffer,
-        );
-    }
-}
-
-pub(crate) fn hash_bytes(bytes: &[u8]) -> u64 {
-    unsafe { llvm::LLVMRustCoverageHashByteArray(bytes.as_c_char_ptr(), bytes.len()) }
-}
-
-pub(crate) fn mapping_version() -> u32 {
-    unsafe { llvm::LLVMRustCoverageMappingVersion() }
-}

compiler/rustc_codegen_llvm/src/llvm/ffi.rs

@@ -1790,7 +1790,7 @@ unsafe extern "C" {
     ) -> bool;
 
     #[allow(improper_ctypes)]
-    pub(crate) fn LLVMRustCoverageWriteFilenamesSectionToBuffer(
+    pub(crate) fn LLVMRustCoverageWriteFilenamesToBuffer(
         Filenames: *const *const c_char,
         FilenamesLen: size_t,
         Lengths: *const size_t,
@@ -1799,19 +1799,19 @@ unsafe extern "C" {
     );
 
     #[allow(improper_ctypes)]
-    pub(crate) fn LLVMRustCoverageWriteMappingToBuffer(
+    pub(crate) fn LLVMRustCoverageWriteFunctionMappingsToBuffer(
         VirtualFileMappingIDs: *const c_uint,
-        NumVirtualFileMappingIDs: c_uint,
+        NumVirtualFileMappingIDs: size_t,
         Expressions: *const crate::coverageinfo::ffi::CounterExpression,
-        NumExpressions: c_uint,
+        NumExpressions: size_t,
         CodeRegions: *const crate::coverageinfo::ffi::CodeRegion,
-        NumCodeRegions: c_uint,
+        NumCodeRegions: size_t,
         BranchRegions: *const crate::coverageinfo::ffi::BranchRegion,
-        NumBranchRegions: c_uint,
+        NumBranchRegions: size_t,
         MCDCBranchRegions: *const crate::coverageinfo::ffi::MCDCBranchRegion,
-        NumMCDCBranchRegions: c_uint,
+        NumMCDCBranchRegions: size_t,
         MCDCDecisionRegions: *const crate::coverageinfo::ffi::MCDCDecisionRegion,
-        NumMCDCDecisionRegions: c_uint,
+        NumMCDCDecisionRegions: size_t,
         BufferOut: &RustString,
     );
 
@@ -1820,16 +1820,16 @@ unsafe extern "C" {
         FuncName: *const c_char,
         FuncNameLen: size_t,
     ) -> &Value;
-    pub(crate) fn LLVMRustCoverageHashByteArray(Bytes: *const c_char, NumBytes: size_t) -> u64;
+    pub(crate) fn LLVMRustCoverageHashBytes(Bytes: *const c_char, NumBytes: size_t) -> u64;
 
     #[allow(improper_ctypes)]
-    pub(crate) fn LLVMRustCoverageWriteMapSectionNameToString(M: &Module, Str: &RustString);
+    pub(crate) fn LLVMRustCoverageWriteCovmapSectionNameToString(M: &Module, OutStr: &RustString);
 
     #[allow(improper_ctypes)]
-    pub(crate) fn LLVMRustCoverageWriteFuncSectionNameToString(M: &Module, Str: &RustString);
+    pub(crate) fn LLVMRustCoverageWriteCovfunSectionNameToString(M: &Module, OutStr: &RustString);
 
     #[allow(improper_ctypes)]
-    pub(crate) fn LLVMRustCoverageWriteMappingVarNameToString(Str: &RustString);
+    pub(crate) fn LLVMRustCoverageWriteCovmapVarNameToString(OutStr: &RustString);
 
     pub(crate) fn LLVMRustCoverageMappingVersion() -> u32;
     pub fn LLVMRustDebugMetadataVersion() -> u32;

compiler/rustc_llvm/llvm-wrapper/CoverageMappingWrapper.cpp

@@ -123,13 +123,13 @@ fromRust(LLVMRustCounterExprKind Kind) {
   report_fatal_error("Bad LLVMRustCounterExprKind!");
 }
 
-extern "C" void LLVMRustCoverageWriteFilenamesSectionToBuffer(
+extern "C" void LLVMRustCoverageWriteFilenamesToBuffer(
     const char *const Filenames[], size_t FilenamesLen, // String start pointers
     const size_t *const Lengths, size_t LengthsLen,     // Corresponding lengths
     RustStringRef BufferOut) {
   if (FilenamesLen != LengthsLen) {
     report_fatal_error(
-        "Mismatched lengths in LLVMRustCoverageWriteFilenamesSectionToBuffer");
+        "Mismatched lengths in LLVMRustCoverageWriteFilenamesToBuffer");
   }
 
   SmallVector<std::string, 32> FilenameRefs;
@@ -143,16 +143,15 @@ extern "C" void LLVMRustCoverageWriteFilenamesSectionToBuffer(
   FilenamesWriter.write(OS);
 }
 
-extern "C" void LLVMRustCoverageWriteMappingToBuffer(
+extern "C" void LLVMRustCoverageWriteFunctionMappingsToBuffer(
-    const unsigned *VirtualFileMappingIDs, unsigned NumVirtualFileMappingIDs,
+    const unsigned *VirtualFileMappingIDs, size_t NumVirtualFileMappingIDs,
-    const LLVMRustCounterExpression *RustExpressions, unsigned NumExpressions,
+    const LLVMRustCounterExpression *RustExpressions, size_t NumExpressions,
-    const LLVMRustCoverageCodeRegion *CodeRegions, unsigned NumCodeRegions,
+    const LLVMRustCoverageCodeRegion *CodeRegions, size_t NumCodeRegions,
-    const LLVMRustCoverageBranchRegion *BranchRegions,
+    const LLVMRustCoverageBranchRegion *BranchRegions, size_t NumBranchRegions,
-    unsigned NumBranchRegions,
     const LLVMRustCoverageMCDCBranchRegion *MCDCBranchRegions,
-    unsigned NumMCDCBranchRegions,
+    size_t NumMCDCBranchRegions,
     const LLVMRustCoverageMCDCDecisionRegion *MCDCDecisionRegions,
-    unsigned NumMCDCDecisionRegions, RustStringRef BufferOut) {
+    size_t NumMCDCDecisionRegions, RustStringRef BufferOut) {
   // Convert from FFI representation to LLVM representation.
 
   // Expressions:
@@ -219,34 +218,37 @@ LLVMRustCoverageCreatePGOFuncNameVar(LLVMValueRef F, const char *FuncName,
   return wrap(createPGOFuncNameVar(*cast<Function>(unwrap(F)), FuncNameRef));
 }
 
-extern "C" uint64_t LLVMRustCoverageHashByteArray(const char *Bytes,
+extern "C" uint64_t LLVMRustCoverageHashBytes(const char *Bytes,
                                               size_t NumBytes) {
-  auto StrRef = StringRef(Bytes, NumBytes);
+  return IndexedInstrProf::ComputeHash(StringRef(Bytes, NumBytes));
-  return IndexedInstrProf::ComputeHash(StrRef);
 }
 
-static void WriteSectionNameToString(LLVMModuleRef M, InstrProfSectKind SK,
+// Private helper function for getting the covmap and covfun section names.
-                                     RustStringRef Str) {
+static void writeInstrProfSectionNameToString(LLVMModuleRef M,
+                                              InstrProfSectKind SectKind,
+                                              RustStringRef OutStr) {
   auto TargetTriple = Triple(unwrap(M)->getTargetTriple());
-  auto name = getInstrProfSectionName(SK, TargetTriple.getObjectFormat());
+  auto name = getInstrProfSectionName(SectKind, TargetTriple.getObjectFormat());
-  auto OS = RawRustStringOstream(Str);
+  auto OS = RawRustStringOstream(OutStr);
   OS << name;
 }
 
-extern "C" void LLVMRustCoverageWriteMapSectionNameToString(LLVMModuleRef M,
+extern "C" void
-                                                            RustStringRef Str) {
+LLVMRustCoverageWriteCovmapSectionNameToString(LLVMModuleRef M,
-  WriteSectionNameToString(M, IPSK_covmap, Str);
+                                               RustStringRef OutStr) {
+  writeInstrProfSectionNameToString(M, IPSK_covmap, OutStr);
 }
 
 extern "C" void
-LLVMRustCoverageWriteFuncSectionNameToString(LLVMModuleRef M,
+LLVMRustCoverageWriteCovfunSectionNameToString(LLVMModuleRef M,
-                                             RustStringRef Str) {
+                                               RustStringRef OutStr) {
-  WriteSectionNameToString(M, IPSK_covfun, Str);
+  writeInstrProfSectionNameToString(M, IPSK_covfun, OutStr);
 }
 
-extern "C" void LLVMRustCoverageWriteMappingVarNameToString(RustStringRef Str) {
+extern "C" void
+LLVMRustCoverageWriteCovmapVarNameToString(RustStringRef OutStr) {
   auto name = getCoverageMappingVarName();
-  auto OS = RawRustStringOstream(Str);
+  auto OS = RawRustStringOstream(OutStr);
   OS << name;
 }