Uh oh!
There was an error while loading. Please reload this page.
coverage: Consolidate creation of covmap/covfun records - #132124
Merged
Conversation
There is no need for this code to be split across multiple functions in multiple files.
In all the situations where this context is needed, it should always be available.
Adding an extra `OnceCell` to `CrateCoverageContext` is much nicer than trying to thread this string through multiple layers of function calls that already have access to the context.
Zalathar
commented
Oct 25, 2024
MemberAuthor
I was originally planning to do #131962 (comment), but doing this first will make that a little cleaner. |
Zalathar
commented
Oct 25, 2024
MemberAuthor
Rustbot is sleepy. r? compiler |
jieyouxu
approved these changes
Oct 26, 2024
jieyouxu
left a comment
Member
There was a problem hiding this comment.
Thanks, this LGTM, minor docs nit. Feel free to r=me with or without addressing those.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
jieyouxu
commented
Oct 26, 2024
Member
Thanks, you can r=me after PR CI is green. |
Zalathar
commented
Oct 26, 2024
MemberAuthor
🟩 |
bors
commented
Oct 26, 2024
Collaborator
Zalathar added a commit
to Zalathar/rust
that referenced
this pull request
Oct 26, 2024
…eyouxu coverage: Consolidate creation of covmap/covfun records This code for creating covmap/covfun records during codegen was split across multiple functions and files for dubious historical reasons. Having it all in one place makes it easier to follow. This PR also includes two semi-related cleanups: - Getting the codegen context's `coverage_cx` state is made infallible, since it should always exist when running the code paths that need it. - The value of `covfun_section_name` is saved in the codegen context, since it never changes at runtime, and the code that needs it has access to the context anyway. --- Background: Coverage instrumentation generates two kinds of metadata that are embedded in the final binary. There is per-CGU information that goes in the `__llvm_covmap` linker section, and per-function information that goes in the `__llvm_covfun` section (except on Windows, where slightly different section names are used).
jieyouxu added a commit
to jieyouxu/rust
that referenced
this pull request
Oct 26, 2024
…eyouxu coverage: Consolidate creation of covmap/covfun records This code for creating covmap/covfun records during codegen was split across multiple functions and files for dubious historical reasons. Having it all in one place makes it easier to follow. This PR also includes two semi-related cleanups: - Getting the codegen context's `coverage_cx` state is made infallible, since it should always exist when running the code paths that need it. - The value of `covfun_section_name` is saved in the codegen context, since it never changes at runtime, and the code that needs it has access to the context anyway. --- Background: Coverage instrumentation generates two kinds of metadata that are embedded in the final binary. There is per-CGU information that goes in the `__llvm_covmap` linker section, and per-function information that goes in the `__llvm_covfun` section (except on Windows, where slightly different section names are used).
bors added a commit
to rust-lang-ci/rust
that referenced
this pull request
Oct 26, 2024
Rollup of 4 pull requests Successful merges: - rust-lang#132124 (coverage: Consolidate creation of covmap/covfun records) - rust-lang#132167 (Replace some LLVMRust wrappers with calls to the LLVM C API) - rust-lang#132169 (Deny calls to non-`#[const_trait]` methods in MIR constck) - rust-lang#132174 (x86 target features: make pclmulqdq imply sse2) r? `@ghost` `@rustbot` modify labels: rollup
bors added a commit
to rust-lang-ci/rust
that referenced
this pull request
Oct 26, 2024
Rollup of 5 pull requests Successful merges: - rust-lang#132124 (coverage: Consolidate creation of covmap/covfun records) - rust-lang#132140 (Enable LSX feature for LoongArch Linux targets) - rust-lang#132169 (Deny calls to non-`#[const_trait]` methods in MIR constck) - rust-lang#132174 (x86 target features: make pclmulqdq imply sse2) - rust-lang#132180 (Print unsafety of attribute in AST pretty print) r? `@ghost` `@rustbot` modify labels: rollup
rust-timer added a commit
to rust-lang-ci/rust
that referenced
this pull request
Oct 26, 2024
Rollup merge of rust-lang#132124 - Zalathar:consolidate-covstar, r=jieyouxu coverage: Consolidate creation of covmap/covfun records This code for creating covmap/covfun records during codegen was split across multiple functions and files for dubious historical reasons. Having it all in one place makes it easier to follow. This PR also includes two semi-related cleanups: - Getting the codegen context's `coverage_cx` state is made infallible, since it should always exist when running the code paths that need it. - The value of `covfun_section_name` is saved in the codegen context, since it never changes at runtime, and the code that needs it has access to the context anyway. --- Background: Coverage instrumentation generates two kinds of metadata that are embedded in the final binary. There is per-CGU information that goes in the `__llvm_covmap` linker section, and per-function information that goes in the `__llvm_covfun` section (except on Windows, where slightly different section names are used).
Zalathar
commented
Oct 31, 2024
MemberAuthor
I've seen at least one report of |
matthiaskrgr added a commit
to matthiaskrgr/rust
that referenced
this pull request
Oct 31, 2024
coverage: Avoid ICE when `coverage_cx` is unexpectedly unavailable In rust-lang#132124, `coverage_cx()` was changed to panic if the context was unavailable, under the assumption that it would always be available whenever coverage instrumentation is enabled. However, there have been reports of this change causing ICEs in `polars` CI. I don't yet understand why this is happening, but for now it seems wisest to revert that part of the change, restoring the two early returns that had been replaced with panics.
matthiaskrgr added a commit
to matthiaskrgr/rust
that referenced
this pull request
Oct 31, 2024
coverage: Avoid ICE when `coverage_cx` is unexpectedly unavailable In rust-lang#132124, `coverage_cx()` was changed to panic if the context was unavailable, under the assumption that it would always be available whenever coverage instrumentation is enabled. However, there have been reports of this change causing ICEs in `polars` CI. I don't yet understand why this is happening, but for now it seems wisest to revert that part of the change, restoring the two early returns that had been replaced with panics.
rust-timer added a commit
to rust-lang-ci/rust
that referenced
this pull request
Oct 31, 2024
Rollup merge of rust-lang#132395 - Zalathar:coverage-cx-ice, r=jieyouxu coverage: Avoid ICE when `coverage_cx` is unexpectedly unavailable In rust-lang#132124, `coverage_cx()` was changed to panic if the context was unavailable, under the assumption that it would always be available whenever coverage instrumentation is enabled. However, there have been reports of this change causing ICEs in `polars` CI. I don't yet understand why this is happening, but for now it seems wisest to revert that part of the change, restoring the two early returns that had been replaced with panics.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This code for creating covmap/covfun records during codegen was split across multiple functions and files for dubious historical reasons. Having it all in one place makes it easier to follow.
This PR also includes two semi-related cleanups:
coverage_cxstate is made infallible, since it should always exist when running the code paths that need it.covfun_section_nameis saved in the codegen context, since it never changes at runtime, and the code that needs it has access to the context anyway.Background: Coverage instrumentation generates two kinds of metadata that are embedded in the final binary. There is per-CGU information that goes in the
__llvm_covmaplinker section, and per-function information that goes in the__llvm_covfunsection (except on Windows, where slightly different section names are used).