Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Remove LLVMRustCoverageHashCString #113430

Merged
merged 4 commits into from
Jul 16, 2023
Merged

Remove LLVMRustCoverageHashCString #113430

merged 4 commits into from
Jul 16, 2023

Conversation

Zalathar
Copy link
Contributor

@Zalathar Zalathar commented Jul 7, 2023

Coverage has two FFI functions for computing the hash of a byte string. One takes a ptr/len pair (LLVMRustCoverageHashByteArray), and the other takes a NUL-terminated C string (LLVMRustCoverageHashCString).

But on closer inspection, the C string version is unnecessary. The calling-side code converts a Rust &str into a CString, and the C++ code then immediately turns it back into a ptr/len string before actually hashing it. So we can just call the ptr/len version directly instead.

This PR also fixes a bug in the C++ declaration of LLVMRustCoverageHashByteArray. It should be size_t, since that's what is declared and passed on the Rust side, and it's what StrRef's constructor expects to receive on the callee side.

@rustbot
Copy link
Collaborator

rustbot commented Jul 7, 2023

r? @b-naber

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 7, 2023
@rustbot
Copy link
Collaborator

rustbot commented Jul 7, 2023

This PR changes how LLVM is built. Consider updating src/bootstrap/download-ci-llvm-stamp.

@Zalathar
Copy link
Contributor Author

Zalathar commented Jul 7, 2023

@rustbot label +A-code-coverage

@rustbot rustbot added the A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) label Jul 7, 2023
@Zalathar
Copy link
Contributor Author

Zalathar commented Jul 7, 2023

I've tested this locally in two ways:

  • Before removing the function, I added an assert to verify that both functions give the same result, then ran x test run-coverage.
  • After switching to just one function, I deliberately corrupted the hash result (via a bitwise not), and ran x test run-coverage again to verify that this causes the tests to fail (since llvm-cov complains about malformed profile data).

@Zalathar
Copy link
Contributor Author

Zalathar commented Jul 7, 2023

I noticed that the string being hashed also doesn't need to be cloned with to_string, so I've fixed that as well (since it doesn't commute with the rest of this PR).

@Zalathar Zalathar force-pushed the hash branch 2 times, most recently from e9666a1 to 8d86dbb Compare July 10, 2023 01:36
@Zalathar
Don't clone symbol names for coverage hashing
29c53d8 
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.
@Zalathar
Pass a byte slice to coverageinfo::hash_bytes instead of an owned v…
7a5ad35 
…ector

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.
@Zalathar
Fix the length parameter type of LLVMRustCoverageHashByteArray
7292608 
The Rust-side declaration uses `libc::size_t` for the number of bytes, but the
C++ declaration was using `unsigned` instead of `size_t`.
@Zalathar
Remove LLVMRustCoverageHashCString
352d031 
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.
@b-naber
Copy link
Contributor

b-naber commented Jul 14, 2023

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jul 14, 2023

📌 Commit 352d031 has been approved by b-naber

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 14, 2023
@bors
Copy link
Contributor

bors commented Jul 15, 2023

⌛ Testing commit 352d031 with merge acdec7cc8e385d6fbaeb432fac1206829b883586...

@bors
Copy link
Contributor

bors commented Jul 15, 2023

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 15, 2023
@rust-log-analyzer
Copy link
Collaborator

The job armhf-gnu failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot) 
curl: (22) The requested URL returned error: 404 

error: failed to download llvm from ci

    help: old builds get deleted after a certain time
    help: if trying to compile an old commit of rustc, disable `download-ci-llvm` in config.toml:
    [llvm]
    download-ci-llvm = false
    
Build completed unsuccessfully in 0:00:00

@b-naber
Copy link
Contributor

b-naber commented Jul 15, 2023

Looks spurious.

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jul 15, 2023

💡 This pull request was already approved, no need to approve it again.

@bors
Copy link
Contributor

bors commented Jul 15, 2023

📌 Commit 352d031 has been approved by b-naber

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 15, 2023
@b-naber
Copy link
Contributor

b-naber commented Jul 15, 2023

@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 15, 2023
@b-naber
Copy link
Contributor

b-naber commented Jul 15, 2023

@bors retry

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 15, 2023
@ehuss
Copy link
Contributor

ehuss commented Jul 16, 2023

@bors r=b-naber

@bors
Copy link
Contributor

bors commented Jul 16, 2023

📌 Commit 352d031 has been approved by b-naber

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Jul 16, 2023

⌛ Testing commit 352d031 with merge ffb9b61...

@bors
Copy link
Contributor

bors commented Jul 16, 2023

☀️ Test successful - checks-actions
Approved by: b-naber
Pushing ffb9b61 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 16, 2023
@bors bors merged commit ffb9b61 into rust-lang:master Jul 16, 2023
11 checks passed
@rustbot rustbot added this to the 1.73.0 milestone Jul 16, 2023
@Zalathar Zalathar deleted the hash branch July 16, 2023 03:44
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (ffb9b61): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 -0.7% [-0.7%, -0.7%] 2
Improvements ✅
(secondary)		 -2.8% [-3.2%, -2.4%] 6
All ❌✅ (primary) -0.7% [-0.7%, -0.7%] 2

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 2.2% [2.2%, 2.2%] 2
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -1.6% [-2.1%, -1.2%] 6
All ❌✅ (primary) 2.2% [2.2%, 2.2%] 2

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 657.636s -> 659.067s (0.22%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@b-naber b-naber b-naber approved these changes

Assignees

@b-naber b-naber

Labels
A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.73.0
Development

Successfully merging this pull request may close these issues.
7 participants
@Zalathar @rustbot @b-naber @bors @rust-log-analyzer @ehuss @rust-timer