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

coverage: Dismantle map_data.rs by moving its responsibilities elsewhere #134323

Merged
merged 6 commits into from
Dec 18, 2024

Conversation

Zalathar
Copy link
Contributor

This is a series of incremental changes that combine to let us get rid of coverageinfo/map_data.rs, by moving all of its responsibilities into more appropriate places.

Some of the notable consequences are:

  • We once again build the per-CGU file table on the fly while preparing individual covfun records, instead of building the whole table up-front. The up-front approach was introduced by coverage: Emit the filenames section before encoding per-function mappings #117042 to work around various other problems in generating the covmap/covfun records, but subsequent cleanups have made that approach no longer necessary.
  • Expression conversion and mapping-region conversion are now performed directly in mapgen::covfun, which should make future changes easier.
  • We no longer insert unused function instances into the same map that is also used to track used function instances. This helps to decouple the handling of used vs unused functions.

There should be no meaningful change to compiler output. The file table is no longer sorted, because reordering it would invalidate the file indices stored in individual covfun records, but the table order should still be deterministic (albeit arbitrary).

There are some subsequent cleanups that I intend to investigate, but this is enough change for one PR.

@Zalathar Zalathar added the A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) label Dec 15, 2024
@rustbot
Copy link
Collaborator

rustbot commented Dec 15, 2024

r? @petrochenkov

rustbot has assigned @petrochenkov.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@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 Dec 15, 2024
@Zalathar
Copy link
Contributor Author

One of the big enablers for this was #134029, which made map_data no longer responsible for identifying unused counter/expression IDs.

@jieyouxu
Copy link
Member

r? jieyouxu

@rustbot rustbot assigned jieyouxu and unassigned petrochenkov Dec 15, 2024
@Zalathar
Copy link
Contributor Author

I don't remember exactly why I reversed the order in the first place, but I think it had something to do with wanting to change the relative order of some other bits of code that happened to be intertwined with the construction of the file table.

Those intertwinings have been dissolved by other changes, so now there's no particular reason to avoid the more “natural” ordering of steps.

@Zalathar
Copy link
Contributor Author

I noticed that IndexSet::insert_full exists, which does precisely what I want when interacting with the global file table (diff).

@jieyouxu
Copy link
Member

@Zalathar could you elaborate on

The file table is no longer sorted, because reordering it would invalidate the file indices stored in individual covfun records, but the table order should still be deterministic (albeit arbitrary).

So what is the deterministic order?

Copy link
Member

@jieyouxu jieyouxu left a comment

Choose a reason for hiding this comment

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

Thanks, this LGTM overall, I just have some nits.

And also would be very cool if you could include a brief comment on the table order.

@rustbot rustbot 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-review Status: Awaiting review from the assignee but also interested parties. labels Dec 16, 2024
@Zalathar
Copy link
Contributor Author

Addressed review feedback (diff).

@Zalathar
Copy link
Contributor Author

My argument for the global file table being deterministic relies on MIR traversal during codegen (within a single CGU) also being deterministic.

I believe that's the case, but I've created a Zulip thread to see if anyone disagrees.

Copy link
Member

@jieyouxu jieyouxu left a comment

Choose a reason for hiding this comment

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

Thanks. All other changes LGTM, just waiting on confirmation if we can rely on the traversal order (or whatever order) to be deterministic.

r=me if that is indeed the case -- that we can rely on the traversal order being guaranteed deterministic, unless it only happens to currently be that way, but is subject to changes.

The checks in `is_eligible_for_coverage` include `is_fn_like`, but will also
exclude various function-like things that cannot possibly have coverage
instrumentation.
@Zalathar
Copy link
Contributor Author

OK, even though I'm confident that the traversal order is deterministic, I ended up adding a layer of sorting (by symbol name) so that changes to stable-hashing won't influence the order of the global file table, which can be observed by coverage test snapshots (diff).

(Redacting this detail in the tests themselves would be annoying, because it's embedded in the bytes of each individual covfun record.)

@Zalathar
Copy link
Contributor Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 17, 2024
Copy link
Member

@jieyouxu jieyouxu left a comment

Choose a reason for hiding this comment

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

Thanks, the changes LGTM.

@jieyouxu
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Dec 17, 2024

📌 Commit 541d4e8 has been approved by jieyouxu

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 Dec 17, 2024
jieyouxu added a commit to jieyouxu/rust that referenced this pull request Dec 17, 2024
…youxu

coverage: Dismantle `map_data.rs` by moving its responsibilities elsewhere

This is a series of incremental changes that combine to let us get rid of `coverageinfo/map_data.rs`, by moving all of its responsibilities into more appropriate places.

Some of the notable consequences are:

- We once again build the per-CGU file table on the fly while preparing individual covfun records, instead of building the whole table up-front. The up-front approach was introduced by rust-lang#117042 to work around various other problems in generating the covmap/covfun records, but subsequent cleanups have made that approach no longer necessary.
- Expression conversion and mapping-region conversion are now performed directly in `mapgen::covfun`, which should make future changes easier.
- We no longer insert unused function instances into the same map that is also used to track used function instances. This helps to decouple the handling of used vs unused functions.

---

There should be no meaningful change to compiler output. The file table is no longer sorted, because reordering it would invalidate the file indices stored in individual covfun records, but the table order should still be deterministic (albeit arbitrary).

There are some subsequent cleanups that I intend to investigate, but this is enough change for one PR.
jieyouxu added a commit to jieyouxu/rust that referenced this pull request Dec 17, 2024
…youxu

coverage: Dismantle `map_data.rs` by moving its responsibilities elsewhere

This is a series of incremental changes that combine to let us get rid of `coverageinfo/map_data.rs`, by moving all of its responsibilities into more appropriate places.

Some of the notable consequences are:

- We once again build the per-CGU file table on the fly while preparing individual covfun records, instead of building the whole table up-front. The up-front approach was introduced by rust-lang#117042 to work around various other problems in generating the covmap/covfun records, but subsequent cleanups have made that approach no longer necessary.
- Expression conversion and mapping-region conversion are now performed directly in `mapgen::covfun`, which should make future changes easier.
- We no longer insert unused function instances into the same map that is also used to track used function instances. This helps to decouple the handling of used vs unused functions.

---

There should be no meaningful change to compiler output. The file table is no longer sorted, because reordering it would invalidate the file indices stored in individual covfun records, but the table order should still be deterministic (albeit arbitrary).

There are some subsequent cleanups that I intend to investigate, but this is enough change for one PR.
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 18, 2024
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#133265 (Add a range argument to vec.extract_if)
 - rust-lang#133801 (Promote powerpc64le-unknown-linux-musl to tier 2 with host tools)
 - rust-lang#134323 (coverage: Dismantle `map_data.rs` by moving its responsibilities elsewhere)
 - rust-lang#134378 (An octuple of polonius fact generation cleanups)
 - rust-lang#134408 (Regression test for RPIT inheriting lifetime from projection)
 - rust-lang#134423 (bootstrap: use specific-purpose ui test path for `test_valid` self-test)
 - rust-lang#134426 (Fix typo in uint_macros.rs)

Failed merges:

 - rust-lang#133103 (Pass FnAbi to find_mir_or_eval_fn)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit e696f5c into rust-lang:master Dec 18, 2024
6 checks passed
@rustbot rustbot added this to the 1.85.0 milestone Dec 18, 2024
@Zalathar Zalathar deleted the dismantle-map-data branch December 18, 2024 03:00
@marxin
Copy link
Contributor

marxin commented Dec 29, 2024

@Zalathar
Can you please address the following rustc-dev-guide orphan links:

error: Server returned 404 Not Found for https://doc.rust-lang.org/nightly/nightly-rustc/rustc_codegen_llvm/coverageinfo/map_data/struct.FunctionCoverage.html
    ┌─ llvm-coverage-instrumentation.md:199:56
    │
199 │   expression ID is passed through to the corresponding [`FunctionCoverage`]
    │                                                        ^^^^^^^^^^^^^^^^^^^^ Server returned 404 Not Found for https://doc.rust-lang.org/nightly/nightly-rustc/rustc_codegen_llvm/coverageinfo/map_data/struct.FunctionCoverage.html

error: Server returned 404 Not Found for https://doc.rust-lang.org/nightly/nightly-rustc/rustc_codegen_llvm/coverageinfo/mapgen/fn.add_unused_functions.html
    ┌─ llvm-coverage-instrumentation.md:287:1
    │
287 │ [`add_unused_functions()`][add-unused-functions]:
    │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Server returned 404 Not Found for https://doc.rust-lang.org/nightly/nightly-rustc/rustc_codegen_llvm/coverageinfo/mapgen/fn.add_unused_functions.html

error: Server returned 404 Not Found for https://doc.rust-lang.org/nightly/nightly-rustc/rustc_mir_transform/coverage/graph/struct.TraverseCoverageGraphWithLoops.html
    ┌─ llvm-coverage-instrumentation.md:447:1
    │
447 │ [`TraverseCoverageGraphWithLoops`][traverse-coverage-graph-with-loops]
    │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Server returned 404 Not Found for https://doc.rust-lang.org/nightly/nightly-rustc/rustc_mir_transform/coverage/graph/struct.TraverseCoverageGraphWithLoops.html

error: Server returned 404 Not Found for https://doc.rust-lang.org/nightly/nightly-rustc/rustc_interface/queries/struct.Query.html

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.

6 participants