Auto merge of #114005 - Zalathar:no-cstr, r=jackh726

coverage: Don't convert filename/symbol strings to `CString` for FFI

LLVM APIs are usually perfectly happy to accept pointer/length strings, as long as we supply a suitable length value when creating a `StringRef` or `std::string`.

This lets us avoid quite a few intermediate `CString` copies during coverage codegen. It also lets us use an `IndexSet<Symbol>` (instead of an `IndexSet<CString>`) when building the deduplicated filename table.

compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs

@@ -12,8 +12,7 @@ use rustc_middle::bug;
 use rustc_middle::middle::codegen_fn_attrs::CodegenFnAttrFlags;
 use rustc_middle::mir::coverage::CodeRegion;
 use rustc_middle::ty::TyCtxt;
-
+use rustc_span::Symbol;
-use std::ffi::CString;
 
 /// Generates and exports the Coverage Map.
 ///
@@ -89,7 +88,10 @@ pub fn finalize(cx: &CodegenCx<'_, '_>) {
 
     // Encode all filenames referenced by counters/expressions in this module
     let filenames_buffer = llvm::build_byte_buffer(|filenames_buffer| {
-        coverageinfo::write_filenames_section_to_buffer(&mapgen.filenames, filenames_buffer);
+        coverageinfo::write_filenames_section_to_buffer(
+            mapgen.filenames.iter().map(Symbol::as_str),
+            filenames_buffer,
+        );
     });
 
     let filenames_size = filenames_buffer.len();
@@ -117,7 +119,7 @@ pub fn finalize(cx: &CodegenCx<'_, '_>) {
 }
 
 struct CoverageMapGenerator {
-    filenames: FxIndexSet<CString>,
+    filenames: FxIndexSet<Symbol>,
 }
 
 impl CoverageMapGenerator {
@@ -128,11 +130,10 @@ impl CoverageMapGenerator {
         // Since rustc generates coverage maps with relative paths, the
         // compilation directory can be combined with the relative paths
         // to get absolute paths, if needed.
-        let working_dir =
+        let working_dir = Symbol::intern(
-            tcx.sess.opts.working_dir.remapped_path_if_available().to_string_lossy().to_string();
+            &tcx.sess.opts.working_dir.remapped_path_if_available().to_string_lossy(),
-        let c_filename =
+        );
-            CString::new(working_dir).expect("null error converting filename to C string");
+        filenames.insert(working_dir);
-        filenames.insert(c_filename);
         Self { filenames }
     }
 
@@ -170,10 +171,8 @@ impl CoverageMapGenerator {
                     current_file_id += 1;
                 }
                 current_file_name = Some(file_name);
-                let c_filename = CString::new(file_name.to_string())
+                debug!("  file_id: {} = '{:?}'", current_file_id, file_name);
-                    .expect("null error converting filename to C string");
+                let (filenames_index, _) = self.filenames.insert_full(file_name);
-                debug!("  file_id: {} = '{:?}'", current_file_id, c_filename);
-                let (filenames_index, _) = self.filenames.insert_full(c_filename);
                 virtual_file_mapping.push(filenames_index as u32);
             }
             debug!("Adding counter {:?} to map for {:?}", counter, region);

compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs

@@ -25,7 +25,6 @@ use rustc_middle::ty::Instance;
 use rustc_middle::ty::Ty;
 
 use std::cell::RefCell;
-use std::ffi::CString;
 
 pub(crate) mod ffi;
 pub(crate) mod map_data;
@@ -332,21 +331,32 @@ fn create_pgo_func_name_var<'ll, 'tcx>(
     cx: &CodegenCx<'ll, 'tcx>,
     instance: Instance<'tcx>,
 ) -> &'ll llvm::Value {
-    let mangled_fn_name = CString::new(cx.tcx.symbol_name(instance).name)
+    let mangled_fn_name: &str = cx.tcx.symbol_name(instance).name;
-        .expect("error converting function name to C string");
     let llfn = cx.get_fn(instance);
-    unsafe { llvm::LLVMRustCoverageCreatePGOFuncNameVar(llfn, mangled_fn_name.as_ptr()) }
+    unsafe {
+        llvm::LLVMRustCoverageCreatePGOFuncNameVar(
+            llfn,
+            mangled_fn_name.as_ptr().cast(),
+            mangled_fn_name.len(),
+        )
+    }
 }
 
 pub(crate) fn write_filenames_section_to_buffer<'a>(
-    filenames: impl IntoIterator<Item = &'a CString>,
+    filenames: impl IntoIterator<Item = &'a str>,
     buffer: &RustString,
 ) {
-    let c_str_vec = filenames.into_iter().map(|cstring| cstring.as_ptr()).collect::<Vec<_>>();
+    let (pointers, lengths) = filenames
+        .into_iter()
+        .map(|s: &str| (s.as_ptr().cast(), s.len()))
+        .unzip::<_, _, Vec<_>, Vec<_>>();
+
     unsafe {
         llvm::LLVMRustCoverageWriteFilenamesSectionToBuffer(
-            c_str_vec.as_ptr(),
+            pointers.as_ptr(),
-            c_str_vec.len(),
+            pointers.len(),
+            lengths.as_ptr(),
+            lengths.len(),
             buffer,
         );
     }

compiler/rustc_codegen_llvm/src/llvm/ffi.rs

@@ -1707,6 +1707,8 @@ extern "C" {
     pub fn LLVMRustCoverageWriteFilenamesSectionToBuffer(
         Filenames: *const *const c_char,
         FilenamesLen: size_t,
+        Lengths: *const size_t,
+        LengthsLen: size_t,
         BufferOut: &RustString,
     );
 
@@ -1721,7 +1723,11 @@ extern "C" {
         BufferOut: &RustString,
     );
 
-    pub fn LLVMRustCoverageCreatePGOFuncNameVar(F: &Value, FuncName: *const c_char) -> &Value;
+    pub fn LLVMRustCoverageCreatePGOFuncNameVar(
+        F: &Value,
+        FuncName: *const c_char,
+        FuncNameLen: size_t,
+    ) -> &Value;
     pub fn LLVMRustCoverageHashByteArray(Bytes: *const c_char, NumBytes: size_t) -> u64;
 
     #[allow(improper_ctypes)]

compiler/rustc_llvm/llvm-wrapper/CoverageMappingWrapper.cpp

@@ -103,12 +103,20 @@ fromRust(LLVMRustCounterExprKind Kind) {
 }
 
 extern "C" void LLVMRustCoverageWriteFilenamesSectionToBuffer(
-    const char* const Filenames[],
+    const char *const Filenames[],
     size_t FilenamesLen,
+    const size_t *const Lengths,
+    size_t LengthsLen,
     RustStringRef BufferOut) {
+  if (FilenamesLen != LengthsLen) {
+    report_fatal_error(
+        "Mismatched lengths in LLVMRustCoverageWriteFilenamesSectionToBuffer");
+  }
+
   SmallVector<std::string,32> FilenameRefs;
+  FilenameRefs.reserve(FilenamesLen);
   for (size_t i = 0; i < FilenamesLen; i++) {
-    FilenameRefs.push_back(std::string(Filenames[i]));
+    FilenameRefs.emplace_back(Filenames[i], Lengths[i]);
   }
   auto FilenamesWriter =
       coverage::CoverageFilenamesSectionWriter(ArrayRef<std::string>(FilenameRefs));
@@ -153,8 +161,11 @@ extern "C" void LLVMRustCoverageWriteMappingToBuffer(
   CoverageMappingWriter.write(OS);
 }
 
-extern "C" LLVMValueRef LLVMRustCoverageCreatePGOFuncNameVar(LLVMValueRef F, const char *FuncName) {
+extern "C" LLVMValueRef LLVMRustCoverageCreatePGOFuncNameVar(
-  StringRef FuncNameRef(FuncName);
+    LLVMValueRef F,
+    const char *FuncName,
+    size_t FuncNameLen) {
+  StringRef FuncNameRef(FuncName, FuncNameLen);
   return wrap(createPGOFuncNameVar(*cast<Function>(unwrap(F)), FuncNameRef));
 }