Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Aug 11, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 12 additions & 13 deletions compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 std::ffi::CString;
use rustc_span::Symbol;

/// Generates and exports the Coverage Map.
///
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -117,7 +119,7 @@ pub fn finalize(cx: &CodegenCx<'_, '_>) {
}

struct CoverageMapGenerator {
filenames: FxIndexSet<CString>,
filenames: FxIndexSet<Symbol>,
}

impl CoverageMapGenerator {
Expand All @@ -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 =
tcx.sess.opts.working_dir.remapped_path_if_available().to_string_lossy().to_string();
let c_filename =
CString::new(working_dir).expect("null error converting filename to C string");
filenames.insert(c_filename);
let working_dir = Symbol::intern(
&tcx.sess.opts.working_dir.remapped_path_if_available().to_string_lossy(),
);
filenames.insert(working_dir);
Self { filenames }
}

Expand Down Expand Up @@ -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())
.expect("null error converting filename to C string");
debug!(" file_id: {} = '{:?}'", current_file_id, c_filename);
let (filenames_index, _) = self.filenames.insert_full(c_filename);
Comment on lines -173 to -176
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Notice that the old code here needs to convert every filename (Symbol) into a CString to insert it into the set of filenames, even though there will usually already be a duplicate entry (created by another function in the same source file).

Symbols are already naturally good for checking whether two strings are identical, so it makes sense to use symbols as set entries, and then only resolve them to strings later when we actually need to pass those strings through FFI.

debug!(" file_id: {} = '{:?}'", current_file_id, file_name);
let (filenames_index, _) = self.filenames.insert_full(file_name);
virtual_file_mapping.push(filenames_index as u32);
}
debug!("Adding counter {:?} to map for {:?}", counter, region);
Expand Down
26 changes: 18 additions & 8 deletions compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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)
.expect("error converting function name to C string");
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_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(),
c_str_vec.len(),
pointers.as_ptr(),
pointers.len(),
lengths.as_ptr(),
lengths.len(),
Comment on lines +356 to +359
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a separate length parameter for the string lengths, so that on the Rust side it's clear that we aren't accidentally omitting the length part of a pointer/length argument pair.

The C++ code then verifies that the two lengths are the same, as they always should be.

buffer,
);
}
Expand Down
8 changes: 7 additions & 1 deletion compiler/rustc_codegen_llvm/src/llvm/ffi.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1704,6 +1704,8 @@ extern "C" {
pub fn LLVMRustCoverageWriteFilenamesSectionToBuffer(
Filenames: *const *const c_char,
FilenamesLen: size_t,
Lengths: *const size_t,
LengthsLen: size_t,
BufferOut: &RustString,
);

Expand All @@ -1718,7 +1720,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)]
Expand Down
19 changes: 15 additions & 4 deletions compiler/rustc_llvm/llvm-wrapper/CoverageMappingWrapper.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand Down Expand Up @@ -153,8 +161,11 @@ extern "C" void LLVMRustCoverageWriteMappingToBuffer(
CoverageMappingWriter.write(OS);
}

extern "C" LLVMValueRef LLVMRustCoverageCreatePGOFuncNameVar(LLVMValueRef F, const char *FuncName) {
StringRef FuncNameRef(FuncName);
extern "C" LLVMValueRef LLVMRustCoverageCreatePGOFuncNameVar(
LLVMValueRef F,
const char *FuncName,
size_t FuncNameLen) {
StringRef FuncNameRef(FuncName, FuncNameLen);
return wrap(createPGOFuncNameVar(*cast<Function>(unwrap(F)), FuncNameRef));
}

Expand Down