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

[Draft] Tools and tests to experimentally add more counters per function #75828

Closed

Conversation

richkadel
Copy link
Contributor

@richkadel richkadel commented Aug 23, 2020

Note: This PR has been replaced by multiple PRs, as noted in the references on Aug 27, 2020.

Adds a new mir_dump output file in HTML/CSS to visualize code regions
and the MIR features that they came from (including overlapping spans).
See example below:

Adds --bless support to test/run-make-fulldeps so expected coverage
output can be quickly regenerated.

Adds a new -Zexperimental-coverage option that implies
-Zinstrument-coverage, but can be used to enable experimental coverage
counter injection.

Includes a basic, MIR-block-based implementation of coverage injection,
available via -Zexperimental-coverage. This implementation has known
flaws and omissions, but is simple enough to validate the new tools and
tests.

The existing -Zinstrument-coverage option currently enables
function-level coverage only, which at least appears to generate
accurate coverage reports at that level.

Experimental coverage is not accurate at this time. When branch coverage works as
intended, the -Zexperimental-coverage option should be removed.

Also, learned that -C link-dead-code (which was enabled automatically
under -Z instrument-coverage) was causing the linking error that
resulted in segmentation faults in coverage instrumented binaries. Link
dead code is now disabled under MSVC, allowing -Zinstrument-coverage
to be enabled under MSVC for the first time.

Rust compiler MCP rust-lang/compiler-team#278

Relevant issue: #34701 - Implement support for LLVMs code coverage
instrumentation

Screen-Recording-2020-08-21-at-2

r? @tmandry
FYI: @wesleywiser

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 23, 2020
@richkadel richkadel force-pushed the llvm-coverage-map-gen-6b branch 5 times, most recently from 9ef2c30 to 2da7371 Compare August 24, 2020 08:42
@bors
Copy link
Contributor

bors commented Aug 25, 2020

☔ The latest upstream changes (presumably #74275) made this pull request unmergeable. Please resolve the merge conflicts.

@@ -851,6 +851,11 @@ options! {DebuggingOptions, DebuggingSetter, basic_debugging_options,
"in addition to `.mir` files, create graphviz `.dot` files (default: no)"),
emit_stack_sizes: bool = (false, parse_bool, [UNTRACKED],
"emit a section containing stack size metadata (default: no)"),
experimental_coverage: bool = (false, parse_bool, [TRACKED],
Copy link
Member

Choose a reason for hiding this comment

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

I don't see any particular need to have another flag for this since the profiling instrumentation is already guarded by an unstable flag. Is having this second flag helpful for you?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I feel like it's helpful to me, but it's a gut feeling to be sure. I know the function-level coverage is valid and results are clean, and though experimental, I didn't want the -Zinstrument-coverage option to regress from a "working" state to a completely useless state (which is what -Zexperimental-coverage is right now).

But other than regression testing, I will be working with -Zexperimental-coverage primarily, so if you feel I should remove the extra flag, I will.

@@ -20,6 +20,7 @@ fn main() {
"InstrProfilingMergeFile.c",
"InstrProfilingNameVar.c",
"InstrProfilingPlatformDarwin.c",
"InstrProfilingPlatformFuchsia.c",
Copy link
Member

Choose a reason for hiding this comment

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

This feels kind of out of place with the other changes. Is there anything else needed to bring up profiling support on Fuchsia or is this it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I only recently noticed it was missing, and since it was an obvious omission (all of the other supported platforms were there), and the fact that I'm about to start testing on Fuchsia with the nightly build, I wanted to close that gap. As far as I know, there's nothing else missing, but, no I haven't tested this yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is now in its own commit, #75998

/// Returns a new `Span` covering the start and end `BytePos`s of the file containing the given
/// `pos`. This can be used to quickly determine if another `BytePos` or `Span` is from the same
/// file.
pub fn lookup_file_span(&self, pos: BytePos) -> Span {
Copy link
Member

Choose a reason for hiding this comment

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

Is this still needed? I couldn't find any uses at a glance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a good question. I know the function is not being called by the code in what I ended up committing in this PR; but I did use it, while experimenting prior to staging my commit. Before I checked in this version of the code, I stripped out some of my experimental stuff, to keep the committed code a little cleaner.

I suspect I will need this and I didn't want to lose it. I think it's helpful, and (to me) not entirely obvious, otherwise, that someone could confirm if two Spans come from the same file without having to lookup the actual file name of both.

If you want it removed, I'll remove it. Just let me know.

@richkadel
Copy link
Contributor Author

Thanks for the review @wesleywiser ! I think all of your comments were questions, asking my reasoning for including certain changes, so I replied to those.

Let me know if, after reading my response, you want me to change any or all of these.

Thanks!

@richkadel
Copy link
Contributor Author

Also, FYI, I think @tmandry also plans to review in the next day or two, so we should not submit to bors just yet.

And I'm testing my run-make-fulldeps refactoring changes (uploaded yesterday) on Windows. I see some unexpected behavior regarding the directory paths for test Makefiles that -include other Makefiles. (They seem to be using the same paths, rather than their own original paths, so half of the tests are ignored.) I'm going to have to work around that.

Adds a new mir_dump output file in HTML/CSS to visualize code regions
and the MIR features that they came from (including overlapping spans).

Adds `--bless` support to `test/run-make-fulldeps` so expected coverage
output can be quickly regenerated.

Adds a new `-Zexperimental-coverage` option that implies
`-Zinstrument-coverage`, but can be used to enable experimental coverage
counter injection.

Includes a basic, MIR-block-based implementation of coverage injection,
available via `-Zexperimental-coverage`. This implementation has known
flaws and omissions, but is simple enough to validate the new tools and
tests.

The existing `-Zinstrument-coverage` option currently enables
function-level coverage only, which at least appears to generate
accurate coverage reports at that level.

Experimental coverage is not accurate at this time. When branch coverage works as
intended, the `-Zexperimental-coverage` option should be removed.

Also, learned that `-C link-dead-code` (which was enabled automatically
under `-Z instrument-coverage`) was causing the linking error that
resulted in segmentation faults in coverage instrumented binaries. Link
dead code is now disabled under MSVC, allowing `-Zinstrument-coverage`
to be enabled under MSVC for the first time.

Rust compiler MCP rust-lang/compiler-team#278

Relevant issue: rust-lang#34701 - Implement support for LLVMs code coverage
instrumentation
@richkadel
Copy link
Contributor Author

...on Windows. I see some unexpected behavior ... half of the tests are ignored.

LOL, I'm an idiot. Half the tests are ignored on Windows (under MSVC) because they are set to # ignore-msvc!

This is WAI. The tests with link-dead-code would fail, as we know, on MSVC, which is why tests with that flag enabled exist in the first place.

So no change required for that; but while investigating, I did notice there's a way to ignore test directories, by including a file named compiletest-ignore-dir. Since the instrument-coverage directory only exists to host the common make settings in coverage_tools.mk, and the common test sources (starting with coverage_of_if_else.rs), I can replace the faux Makefile that does nothing with this compiletest-ignore-dir file instead.

I verified this works on both Linux and Windows, so I'll upload that change now.

FYI, all of the instrument_coverage tests can still be tested with one test target, by:

./x.py [options] test src/test/run-make-fulldeps/instrument-coverage

This runs 6 tests on Linux and Mac, but on Windows it runs 3 tests and ignores 3 tests (as intended).

@tmandry
Copy link
Member

tmandry commented Aug 27, 2020

Can we split this PR up? It would be much easier to review one logical change at a time.

@richkadel
Copy link
Contributor Author

Can we split this PR up?

Will do.

@richkadel richkadel marked this pull request as draft August 27, 2020 19:00
@richkadel
Copy link
Contributor Author

I'm converting this PR to "Draft" while I submit separate PRs broken out from this one.

richkadel added a commit to richkadel/rust that referenced this pull request Aug 27, 2020
The ability to "bless" output for some of these tests is critical to
making it practical to adapt tests to unrelated changes.

This is needed for new coverage tests, as shown in PR rust-lang#75828 (or its
derivative).
@richkadel richkadel changed the title Tools and tests to experimentally add more counters per function [Draft] Tools and tests to experimentally add more counters per function Aug 27, 2020
@richkadel
Copy link
Contributor Author

Can we split this PR up?

Done

@richkadel richkadel closed this Aug 27, 2020
richkadel added a commit to richkadel/rust that referenced this pull request Sep 1, 2020
Found that -C link-dead-code (which was enabled automatically
under -Z instrument-coverage) was causing the linking error that
resulted in segmentation faults in coverage instrumented binaries. Link
dead code is now disabled under MSVC, allowing `-Z instrument-coverage`
to be enabled under MSVC for the first time.

More details are included in Issue rust-lang#76038.

(This PR was broken out from PR rust-lang#75828)
tmandry added a commit to tmandry/rust that referenced this pull request Sep 1, 2020
…3, r=tmandry

Fix `-Z instrument-coverage` on MSVC

Found that `-C link-dead-code` (which was enabled automatically
under `-Z instrument-coverage`) was causing the linking error that
resulted in segmentation faults in coverage instrumented binaries. Link
dead code is now disabled under MSVC, allowing `-Z instrument-coverage`
to be enabled under MSVC for the first time.

More details are included in Issue rust-lang#76038 .

Note this PR makes it possible to support `Z instrument-coverage` but
does not enable instrument coverage for MSVC in existing tests. It will be
enabled in another PR to follow this one (both PRs coming from original
PR rust-lang#75828).

r? @tmandry
FYI: @wesleywiser
tmandry added a commit to tmandry/rust that referenced this pull request Sep 1, 2020
…4, r=wesleywiser

Adds two source span utility functions used in source-based coverage

`span.is_empty()` - returns true if `lo()` and `hi()` are equal. This is
not only a convenience, but makes it clear that a `Span` can be empty
(that is, retrieving the source for an empty `Span` will return an empty
string), and codifies the (otherwise undocumented--in the rustc_span
package, at least) fact that `Span` is a half-open interval (where
`hi()` is the open end).

`source_map.lookup_file_span()` - returns an enclosing `Span`
representing the start and end positions of the file enclosing the given
`BytePos`. This gives developers a clear way to quickly determine if any
any other `BytePos` or `Span` is also from the same file (for example,
by simply calling `file_span.contains(span)`).

This results in much simpler code and is much more runtime efficient
compared with the obvious alternative: calling `source_map.lookup_line()`
for any two `Span`'s byte positions, handle both arms of the `Result`
(both contain the file), and then compare files. It is also more
efficient than the non-public method `lookup_source_file_idx()` for each
`BytePos`, because, while comparing the internal source file indexes
would be efficient, looking up the source file index for every `BytePos`
or `Span` to be compared requires a binary search (worst case
performance being O(log n) for every lookup).

`source_map.lookup_file_span()` performs the binary search only once, to
get the `file_span` result that can be used to compare to any number of
other `BytePos` or `Span` values and those comparisons are always O(1).

This PR was split out from PR rust-lang#75828 .

r? @tmandry
FYI: @wesleywiser
richkadel added a commit to richkadel/rust that referenced this pull request Sep 3, 2020
Adds a new mir_dump output file in HTML/CSS to visualize code regions
and the MIR features that they came from (including overlapping spans).
See example below:

Includes a basic, MIR-block-based implementation of coverage injection,
available via `-Zexperimental-coverage`. This implementation has known
flaws and omissions, but is simple enough to validate the new tools and
tests.

The existing `-Zinstrument-coverage` option currently enables
function-level coverage only, which at least appears to generate
accurate coverage reports at that level.

Experimental coverage is not accurate at this time. When branch coverage
works as intended, the `-Zexperimental-coverage` option should be
removed.

This PR replaces the bulk of PR rust-lang#75828, with the remaining parts of
that PR distributed among other separate and indentpent PRs.

This PR depends on three of those other PRs: rust-lang#76000, rust-lang#76002, and

Rust compiler MCP rust-lang/compiler-team#278

Relevant issue: rust-lang#34701 - Implement support for LLVMs code coverage
instrumentation

![Screen-Recording-2020-08-21-at-2](https://user-images.githubusercontent.com/3827298/90972923-ff417880-e4d1-11ea-92bb-8713c6198f6d.gif)
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 4, 2020
… r=tmandry

Tools, tests, and experimenting with MIR-derived coverage counters

Leverages the new mir_dump output file in HTML+CSS (from rust-lang#76074) to visualize coverage code regions
and the MIR features that they came from (including overlapping spans).

See example below.

The `run-make-fulldeps/instrument-coverage` test has been refactored to maximize test coverage and reduce code duplication. The new tests support testing with and without `-Clink-dead-code`, so Rust coverage can be tested on MSVC (which, currently, only works with `link-dead-code` _disabled_).

New tests validate coverage region generation and coverage reports with multiple counters per function. Starting with a simple `if-else` branch tests, coverage tests for each additional syntax type can be added by simply dropping in a new Rust sample program.

Includes a basic, MIR-block-based implementation of coverage injection,
available via `-Zexperimental-coverage`. This implementation has known
flaws and omissions, but is simple enough to validate the new tools and
tests.

The existing `-Zinstrument-coverage` option currently enables
function-level coverage only, which at least appears to generate
accurate coverage reports at that level.

Experimental coverage is not accurate at this time. When branch coverage
works as intended, the `-Zexperimental-coverage` option should be
removed.

This PR replaces the bulk of PR rust-lang#75828, with the remaining parts of
that PR distributed among other separate and indentpent PRs.

This PR depends on two of those other PRs: rust-lang#76002, rust-lang#76003 and rust-lang#76074

Rust compiler MCP rust-lang/compiler-team#278

Relevant issue: rust-lang#34701 - Implement support for LLVMs code coverage
instrumentation

![Screen-Recording-2020-08-21-at-2](https://user-images.githubusercontent.com/3827298/90972923-ff417880-e4d1-11ea-92bb-8713c6198f6d.gif)

r? @tmandry
FYI: @wesleywiser
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants