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

richkadel · 2020-08-23T06:51:19Z

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

r? @tmandry
FYI: @wesleywiser

src/test/run-make-fulldeps/instrument-coverage/Makefile

bors · 2020-08-25T21:17:45Z

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

src/test/mir-opt/instrument_coverage.main.InstrumentCoverage.diff

wesleywiser · 2020-08-26T14:34:47Z

src/librustc_session/options.rs

@@ -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],


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?

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.

wesleywiser · 2020-08-26T14:36:47Z

library/profiler_builtins/build.rs

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


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?

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.

This is now in its own commit, #75998

wesleywiser · 2020-08-26T14:38:51Z

src/librustc_span/source_map.rs

+    /// 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 {


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

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 · 2020-08-26T16:55:51Z

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 · 2020-08-26T16:59:24Z

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 · 2020-08-26T21:06:35Z

...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 · 2020-08-27T01:44:02Z

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

richkadel · 2020-08-27T02:05:18Z

Can we split this PR up?

Will do.

richkadel · 2020-08-27T19:01:26Z

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

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 · 2020-08-27T21:40:14Z

Can we split this PR up?

Done

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

…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

…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

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)

@tmandry

… 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

rust-highfive assigned tmandry Aug 23, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 23, 2020

richkadel force-pushed the llvm-coverage-map-gen-6b branch 5 times, most recently from 9ef2c30 to 2da7371 Compare August 24, 2020 08:42

richkadel mentioned this pull request Aug 24, 2020

Enable some of profiler tests on Windows-gnu #75872

Merged

richkadel commented Aug 24, 2020

View reviewed changes

src/test/run-make-fulldeps/instrument-coverage/Makefile Outdated Show resolved Hide resolved

This was referenced Aug 24, 2020

WIP: Injects counters before every block (experimental only) #74856

Closed

Linker error because -wL as-needed conflicts with -C link-dead-code #64685

Open

richkadel force-pushed the llvm-coverage-map-gen-6b branch from 2da7371 to 1600b1e Compare August 26, 2020 01:13

wesleywiser reviewed Aug 26, 2020

View reviewed changes

richkadel force-pushed the llvm-coverage-map-gen-6b branch from 1600b1e to 5d13cbc Compare August 26, 2020 21:06

richkadel marked this pull request as draft August 27, 2020 19:00

richkadel mentioned this pull request Aug 27, 2020

Add InstrProfilingPlatformFuchsia.c to profiler_builtins #75998

Merged

richkadel mentioned this pull request Aug 27, 2020

Adds --bless support to test/run-make-fulldeps #76000

Merged

This was referenced Aug 27, 2020

Fix -Z instrument-coverage on MSVC #76002

Merged

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

Merged

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 mentioned this pull request Aug 27, 2020

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

Merged

richkadel closed this Aug 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

richkadel commented Aug 23, 2020 •

edited

Loading

bors commented Aug 25, 2020

wesleywiser Aug 26, 2020

richkadel Aug 26, 2020

wesleywiser Aug 26, 2020

richkadel Aug 26, 2020

richkadel Aug 27, 2020

wesleywiser Aug 26, 2020

richkadel Aug 26, 2020

richkadel commented Aug 26, 2020

richkadel commented Aug 26, 2020

richkadel commented Aug 26, 2020

tmandry commented Aug 27, 2020

richkadel commented Aug 27, 2020

richkadel commented Aug 27, 2020

richkadel commented Aug 27, 2020

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

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

Conversation

richkadel commented Aug 23, 2020 • edited Loading

bors commented Aug 25, 2020

wesleywiser Aug 26, 2020

Choose a reason for hiding this comment

richkadel Aug 26, 2020

Choose a reason for hiding this comment

wesleywiser Aug 26, 2020

Choose a reason for hiding this comment

richkadel Aug 26, 2020

Choose a reason for hiding this comment

richkadel Aug 27, 2020

Choose a reason for hiding this comment

wesleywiser Aug 26, 2020

Choose a reason for hiding this comment

richkadel Aug 26, 2020

Choose a reason for hiding this comment

richkadel commented Aug 26, 2020

richkadel commented Aug 26, 2020

richkadel commented Aug 26, 2020

tmandry commented Aug 27, 2020

richkadel commented Aug 27, 2020

richkadel commented Aug 27, 2020

richkadel commented Aug 27, 2020

richkadel commented Aug 23, 2020 •

edited

Loading