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.

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

Isolate coverage FFI type layouts from their underlying LLVM C++ types #111382

Merged
merged 2 commits into from
May 11, 2023

Conversation

Zalathar
Copy link
Contributor

@Zalathar Zalathar commented May 9, 2023

I noticed that several of the types used to send coverage information through FFI are not properly isolated from the layout of their corresponding C++ types in the LLVM API.

This PR adds more explicitly-defined FFI struct/enum types in CoverageMappingWrapper.cpp, so that Rust source files in rustc_codegen_ssa and rustc_codegen_llvm aren't directly exposed to LLVM C++ types.

@rustbot
Copy link
Collaborator

rustbot commented May 9, 2023

r? @cuviper

(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 May 9, 2023
@Zalathar
Copy link
Contributor Author

Zalathar commented May 9, 2023

@rustbot label +A-code-coverage +A-LLVM

@rustbot rustbot added A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. labels May 9, 2023
@Zalathar
Copy link
Contributor Author

Zalathar commented May 9, 2023

(Note that most or all coverage tests don't run in PR CI, and many of them are silently broken in CI until #111179 lands.)

The regions don't need to be mutable because we pass a copy of them to LLVM
instead, and this matches the `*const` in the Rust-side signature.
@cuviper
Copy link
Member

cuviper commented May 10, 2023

Yeesh, that's tedious, but AFAICT it looks good...

@bors r+

@bors
Copy link
Contributor

bors commented May 10, 2023

📌 Commit 9addf06 has been approved by cuviper

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented May 10, 2023

🌲 The tree is currently closed for pull requests below priority 50. This pull request will be tested once the tree is reopened.

@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 May 10, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request May 11, 2023
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#108705 (Prevent ICE with broken borrow in closure)
 - rust-lang#111292 (Fix mishandled `--check-cfg` arguments order)
 - rust-lang#111382 (Isolate coverage FFI type layouts from their underlying LLVM C++ types)
 - rust-lang#111385 (vec-shrink-panik: update expectations to work on LLVM 17)
 - rust-lang#111389 (Add esp-idf platform-support page)
 - rust-lang#111432 (Use visit_assign to detect SSA locals.)
 - rust-lang#111448 (Use proper impl self type for alias impl in rustdoc)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 39761b0 into rust-lang:master May 11, 2023
@rustbot rustbot added this to the 1.71.0 milestone May 11, 2023
@Zalathar Zalathar deleted the ffi branch May 31, 2023 01:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. 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
Development

Successfully merging this pull request may close these issues.

4 participants