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

Move most coverage code out of rustc_codegen_ssa #113355

Merged
merged 3 commits into from
Jul 5, 2023
Merged

Conversation

Zalathar
Copy link
Contributor

@Zalathar Zalathar commented Jul 5, 2023

This is one step in my larger coverage refactoring ambitions described at rust-lang/compiler-team#645.

The backend implementation of coverage instrumentation was originally split between SSA and LLVM, perhaps in the hopes that it could be used by other backends.

In practice, this split mostly just makes the coverage implementation harder to navigate and harder to modify. It seems unlikely that any backend will actually implement coverage instrumentation in the foreseeable future, especially since many parts of the existing implementation (outside the LLVM backend) are heavily tied to the specific details of LLVM's coverage instrumentation features.

The current shared implementation of codegen_coverage is heavily tied to the details of StatementKind::Coverage, which makes those details difficult to change. I have reason to want to change those details as part of future fixes/improvements, so this will reduce the amount of interface churn caused by those later changes.


This is intended to be a pure refactoring change, with no changes to actual behaviour. All of the “added” code has really just been moved from other files.

@rustbot
Copy link
Collaborator

rustbot commented Jul 5, 2023

r? @oli-obk

(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 5, 2023
@rustbot
Copy link
Collaborator

rustbot commented Jul 5, 2023

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo

@Zalathar
Copy link
Contributor Author

Zalathar commented Jul 5, 2023

@rustbot label +A-code-coverage

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

Zalathar commented Jul 5, 2023

The GCC backend changes just replace one set of stubbed-out methods with a newer, smaller set of stubbed-out methods, so that should be uncontroversial.

(Part of the motivation behind doing this before my other planned changes is to avoid the need for further churn in other backends.)

Zalathar added 3 commits July 5, 2023 20:40
This effectively inlines most of `FunctionCx::codegen_coverage` into the LLVM
implementation of `CoverageInfoBuilderMethods`.
…d it

These methods are only ever called from within `rustc_codegen_llvm`, so they
can just be declared there as well.
@oli-obk
Copy link
Contributor

oli-obk commented Jul 5, 2023

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jul 5, 2023

📌 Commit cb570d6 has been approved by oli-obk

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 5, 2023
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Jul 5, 2023
Move most coverage code out of `rustc_codegen_ssa`

*This is one step in my larger coverage refactoring ambitions described at <https://github.com/rust-lang/compiler-team/issues/645>.*

The backend implementation of coverage instrumentation was originally split between SSA and LLVM, perhaps in the hopes that it could be used by other backends.

In practice, this split mostly just makes the coverage implementation harder to navigate and harder to modify. It seems unlikely that any backend will actually implement coverage instrumentation in the foreseeable future, especially since many parts of the existing implementation (outside the LLVM backend) are heavily tied to the specific details of LLVM's coverage instrumentation features.

The current shared implementation of `codegen_coverage` is heavily tied to the details of `StatementKind::Coverage`, which makes those details difficult to change. I have reason to want to change those details as part of future fixes/improvements, so this will reduce the amount of interface churn caused by those later changes.

---

This is intended to be a pure refactoring change, with no changes to actual behaviour. All of the “added” code has really just been moved from other files.
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 5, 2023
…mpiler-errors

Rollup of 8 pull requests

Successful merges:

 - rust-lang#113010 (rust-installer & rls: remove exclusion from rustfmt & tidy )
 - rust-lang#113317 ( -Ztrait-solver=next: stop depending on old solver)
 - rust-lang#113319 (`TypeParameterDefinition` always require a `DefId`)
 - rust-lang#113320 (Add some extra information to opaque type cycle errors)
 - rust-lang#113321 (Move `ty::ConstKind` to `rustc_type_ir`)
 - rust-lang#113337 (Winnow specialized impls during selection in new solver)
 - rust-lang#113355 (Move most coverage code out of `rustc_codegen_ssa`)
 - rust-lang#113356 (Add support for NetBSD/riscv64 aka. riscv64gc-unknown-netbsd.)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 6f9addf into rust-lang:master Jul 5, 2023
@rustbot rustbot added this to the 1.72.0 milestone Jul 5, 2023
@Zalathar Zalathar deleted the ssa branch July 6, 2023 00:05
Zalathar added a commit to Zalathar/rust that referenced this pull request Aug 2, 2023
Coverage FFI types were historically split across two modules, because some of
them were needed by code in `rustc_codegen_ssa`.

Now that all of the coverage codegen code has been moved into
`rustc_codegen_llvm` (rust-lang#113355), it's possible to move all of the FFI types into
a single module, making it easier to see all of them at once.
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 2, 2023
coverage: Consolidate FFI types into one module

Coverage FFI types were historically split across two modules, because some of them were needed by code in `rustc_codegen_ssa`.

Now that all of the coverage codegen code has been moved into `rustc_codegen_llvm` (rust-lang#113355), it's possible to move all of the FFI types into a single module, making it easier to see all of them at once.

---

This PR only moves code and adjusts imports; there should be no functional changes.
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) 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