From 29c53d87481913511955c7bb33826899b964f2fe Mon Sep 17 00:00:00 2001 From: Zalathar Date: Fri, 7 Jul 2023 22:49:45 +1000 Subject: [PATCH 1/4] Don't clone symbol names for coverage hashing A symbol already contains a `&str`, and in this context there's no need to make an owned copy, so we can just use the original string reference. --- compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs index a1ff2aa662500..2b8f839134f2b 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs @@ -63,7 +63,7 @@ pub fn finalize(cx: &CodegenCx<'_, '_>) { let mut function_data = Vec::new(); for (instance, function_coverage) in function_coverage_map { debug!("Generate function coverage for {}, {:?}", cx.codegen_unit.name(), instance); - let mangled_function_name = tcx.symbol_name(instance).to_string(); + let mangled_function_name = tcx.symbol_name(instance).name; let source_hash = function_coverage.source_hash(); let is_used = function_coverage.is_used(); let (expressions, counter_regions) = @@ -228,7 +228,7 @@ impl CoverageMapGenerator { /// specific, well-known section and name. fn save_function_record( cx: &CodegenCx<'_, '_>, - mangled_function_name: String, + mangled_function_name: &str, source_hash: u64, filenames_ref: u64, coverage_mapping_buffer: Vec, @@ -238,7 +238,7 @@ fn save_function_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_str(&mangled_function_name); + let func_name_hash = coverageinfo::hash_str(mangled_function_name); 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); From 7a5ad35da4e5fd8cb4ec5bf181dbb75afd5dade9 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Thu, 13 Jul 2023 11:21:21 +1000 Subject: [PATCH 2/4] Pass a byte slice to `coverageinfo::hash_bytes` instead of an owned vector The function body immediately treats it as a slice anyway, so this just makes it possible to call the hash function with arbitrary read-only byte slices. --- compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs | 2 +- compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs index 2b8f839134f2b..c9321d63e934d 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs @@ -95,7 +95,7 @@ pub 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 = coverageinfo::hash_bytes(&filenames_buffer); // Generate the LLVM IR representation of the coverage map and store it in a well-known global let cov_data_val = mapgen.generate_coverage_map(cx, version, filenames_size, filenames_val); diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs index 42fdbd786185e..090e88003d616 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs @@ -378,7 +378,7 @@ pub(crate) fn hash_str(strval: &str) -> u64 { unsafe { llvm::LLVMRustCoverageHashCString(strval.as_ptr()) } } -pub(crate) fn hash_bytes(bytes: Vec) -> u64 { +pub(crate) fn hash_bytes(bytes: &[u8]) -> u64 { unsafe { llvm::LLVMRustCoverageHashByteArray(bytes.as_ptr().cast(), bytes.len()) } } From 7292608e213ddf64e2ecf69c6e074d7d67a2c9ec Mon Sep 17 00:00:00 2001 From: Zalathar Date: Fri, 7 Jul 2023 17:19:41 +1000 Subject: [PATCH 3/4] Fix the length parameter type of `LLVMRustCoverageHashByteArray` The Rust-side declaration uses `libc::size_t` for the number of bytes, but the C++ declaration was using `unsigned` instead of `size_t`. --- compiler/rustc_llvm/llvm-wrapper/CoverageMappingWrapper.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/rustc_llvm/llvm-wrapper/CoverageMappingWrapper.cpp b/compiler/rustc_llvm/llvm-wrapper/CoverageMappingWrapper.cpp index 87906dee4d348..35f83cdb4f123 100644 --- a/compiler/rustc_llvm/llvm-wrapper/CoverageMappingWrapper.cpp +++ b/compiler/rustc_llvm/llvm-wrapper/CoverageMappingWrapper.cpp @@ -165,7 +165,7 @@ extern "C" uint64_t LLVMRustCoverageHashCString(const char *StrVal) { extern "C" uint64_t LLVMRustCoverageHashByteArray( const char *Bytes, - unsigned NumBytes) { + size_t NumBytes) { StringRef StrRef(Bytes, NumBytes); return IndexedInstrProf::ComputeHash(StrRef); } From 352d0315998c3691e811d8406ae7931143cf7a16 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Fri, 7 Jul 2023 17:07:48 +1000 Subject: [PATCH 4/4] Remove `LLVMRustCoverageHashCString` Coverage has two FFI functions for computing the hash of a byte string. One takes a ptr/len pair, and the other takes a NUL-terminated C string. But on closer inspection, the C string version is unnecessary. The calling-side code converts a Rust `&str` into a C string, and the C++ code then immediately turns it back into a ptr/len string before actually hashing it. --- compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs | 2 +- compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs | 5 ----- compiler/rustc_codegen_llvm/src/llvm/ffi.rs | 1 - compiler/rustc_llvm/llvm-wrapper/CoverageMappingWrapper.cpp | 5 ----- 4 files changed, 1 insertion(+), 12 deletions(-) diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs index c9321d63e934d..6e815ab3ff511 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs @@ -238,7 +238,7 @@ fn save_function_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_str(mangled_function_name); + let func_name_hash = coverageinfo::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); diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs index 090e88003d616..1d4a3145cd4b6 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs @@ -373,11 +373,6 @@ pub(crate) fn write_mapping_to_buffer( } } -pub(crate) fn hash_str(strval: &str) -> u64 { - let strval = CString::new(strval).expect("null error converting hashable str to C string"); - unsafe { llvm::LLVMRustCoverageHashCString(strval.as_ptr()) } -} - pub(crate) fn hash_bytes(bytes: &[u8]) -> u64 { unsafe { llvm::LLVMRustCoverageHashByteArray(bytes.as_ptr().cast(), bytes.len()) } } diff --git a/compiler/rustc_codegen_llvm/src/llvm/ffi.rs b/compiler/rustc_codegen_llvm/src/llvm/ffi.rs index e1dfc1b2dd6fa..605f0154773a7 100644 --- a/compiler/rustc_codegen_llvm/src/llvm/ffi.rs +++ b/compiler/rustc_codegen_llvm/src/llvm/ffi.rs @@ -1916,7 +1916,6 @@ extern "C" { ); pub fn LLVMRustCoverageCreatePGOFuncNameVar(F: &Value, FuncName: *const c_char) -> &Value; - pub fn LLVMRustCoverageHashCString(StrVal: *const c_char) -> u64; pub fn LLVMRustCoverageHashByteArray(Bytes: *const c_char, NumBytes: size_t) -> u64; #[allow(improper_ctypes)] diff --git a/compiler/rustc_llvm/llvm-wrapper/CoverageMappingWrapper.cpp b/compiler/rustc_llvm/llvm-wrapper/CoverageMappingWrapper.cpp index 35f83cdb4f123..80b6c0fb43957 100644 --- a/compiler/rustc_llvm/llvm-wrapper/CoverageMappingWrapper.cpp +++ b/compiler/rustc_llvm/llvm-wrapper/CoverageMappingWrapper.cpp @@ -158,11 +158,6 @@ extern "C" LLVMValueRef LLVMRustCoverageCreatePGOFuncNameVar(LLVMValueRef F, con return wrap(createPGOFuncNameVar(*cast(unwrap(F)), FuncNameRef)); } -extern "C" uint64_t LLVMRustCoverageHashCString(const char *StrVal) { - StringRef StrRef(StrVal); - return IndexedInstrProf::ComputeHash(StrRef); -} - extern "C" uint64_t LLVMRustCoverageHashByteArray( const char *Bytes, size_t NumBytes) {