coverage: Don't convert filename/symbol strings to CString for FFI
#114005
Conversation
|
r? @jackh726 (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
S-waiting-on-review
|
@rustbot label +A-code-coverage |
rustbot
added
the
A-code-coverage
|
My original goal was just to store symbols in the filenames set, and only convert to But after digging deeper, I found that the C++ code didn't really need nul-terminated strings at all. |
| pointers.as_ptr(), | ||
| pointers.len(), | ||
| lengths.as_ptr(), | ||
| lengths.len(), |
There was a problem hiding this comment.
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.
| 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); |
There was a problem hiding this comment.
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.
Zalathar
force-pushed
the
no-cstr
branch
5 times, most recently
from
318d478 to
a4e08cf
Compare
Zalathar
force-pushed
the
no-cstr
branch
3 times, most recently
from
c77ce64 to
df18f3d
Compare
|
@bors r+ |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (a9b2c6a): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)This benchmark run did not return any relevant results for this metric. CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 632.053s -> 631.684s (-0.06%) |
LLVM APIs are usually perfectly happy to accept pointer/length strings, as long as we supply a suitable length value when creating a
StringReforstd::string.This lets us avoid quite a few intermediate
CStringcopies during coverage codegen. It also lets us use anIndexSet<Symbol>(instead of anIndexSet<CString>) when building the deduplicated filename table.