LLVM IR coverage encoding aligns closer to Clang's #75416

richkadel · 2020-08-11T18:30:04Z

I found some areas for improvement while attempting to debug the
SegFault issue when running rust programs compiled using MSVC, with
coverage instrumentation.

I discovered that LLVM's coverage writer was generating incomplete
function name variable names (that's not a typo: the name of the
variable that holds a function name).

The existing implementation used one-up numbers to distinguish
variables, and correcting the names did not fix the MSVC coverage bug,
but the fix in this PR makes the names and resulting LLVM IR easier to
follow and more consistent with Clang's implementation.

I also changed the way the -Zinstrument-coverage option is supported in
symbol_export.rs. The original implementation was incorrect, and the
corrected version matches the handling for -Zprofile-generate, as it
turns out.

(An argument could be made that maybe -Zinstrument-coverage should
automatically enable -Cprofile-generate. In fact, if
-Cprofile-generate is analagous to Clang's -fprofile-generate, as
some documentation implies, Clang always requires this flag for its
implementation of source-based code coverage. This would require a
little more validation, and if implemented, would probably require
updating some of the user-facing messages related to
-Cprofile-generate to not be so specific to the PGO use case.)

None of these changes fixed the MSVC coverage problems, but they should
still be welcome improvements.

Lastly, I added some additional FIXME comments in instrument_coverage.rs
describing issues I found with the generated LLVM IR that would be
resolved if the coverage instrumentation is injected with a Statement
instead of as a new BasicBlock. I describe seven advantages of this
change, but it requires some discussion before making a change like
this.

r? @tmandry

src/librustc_mir/transform/instrument_coverage.rs

richkadel · 2020-08-12T07:18:46Z

@tmandry @wesleywiser - Since this PR modifies LLVM IR without changing the final result (coverage counts), I just added a new test for LLVM IR expected patterns, using LLVM's "FileCheck" tool (and following a similar approach used to test Rust PGO).

See: src/test/run-make-fulldeps/instrument-coverage/filecheck-patterns.txt

richkadel · 2020-08-12T18:33:37Z

I'm pasting the FIXME comment mentioned above into a PR comment here.

Actually, after discussing with Tyler offline, I'm moving this discussion about using a Statement instead of BasicBlock to Zulip.

richkadel · 2020-08-12T18:43:37Z

I removed the FIXME comment, now replaced by the live Zulip thread discussing imlementing a new StatementKind.

src/librustc_codegen_llvm/coverageinfo/mod.rs

src/librustc_codegen_llvm/llvm_util.rs

src/test/run-make-fulldeps/instrument-coverage/filecheck-patterns.txt

wesleywiser · 2020-08-13T11:38:47Z

src/librustc_mir/transform/instrument_coverage.rs

+        // @alloc4 = private unnamed_addr constant <{ [43 x i8] }> \
+        //   <{ [43 x i8] c"C:\\msys64\\home\\richkadel\\rust\\rust_basic.rs" }>, align 1
+        //
+        // Can I flag the alloc as something not to be added to codegen? Or somehow remove it before


Hmm... I'm not aware of any facility currently to do this but this might be something we should look into since MIR optimizations could also cause some allocations to no longer be needed.

tmandry · 2020-08-14T00:20:10Z

@bors r+ rollup=never (due to the LLVM IR tests)

bors · 2020-08-14T00:20:11Z

📌 Commit a670540017a7e61532684ef333a60c0722229190 has been approved by tmandry

bors · 2020-08-14T00:41:41Z

⌛ Testing commit a670540017a7e61532684ef333a60c0722229190 with merge b3afc68b9ff9e258b98c98a3915160f280f0e0eb...

tmandry · 2020-08-14T01:01:10Z

@bors retry yielding to rollup

tmandry · 2020-08-14T01:03:05Z

I actually think we should do a try run before running a CI cycle on this. Not sure if all these commands will work in one go, but let's see:

@bors r-
@bors try
@bors delegate=richkadel

bors · 2020-08-14T01:03:07Z

✌️ @richkadel can now approve this pull request

bors · 2020-08-14T01:03:17Z

⌛ Trying commit a670540017a7e61532684ef333a60c0722229190 with merge 0c5491bd41285af2f061dc1f496b4920fdd119c2...

bors · 2020-08-14T01:48:14Z

☀️ Try build successful - checks-actions, checks-azure
Build commit: 0c5491bd41285af2f061dc1f496b4920fdd119c2 (0c5491bd41285af2f061dc1f496b4920fdd119c2)

tmandry · 2020-08-14T02:08:08Z

@bors r+

bors · 2020-08-14T02:08:09Z

📌 Commit a670540017a7e61532684ef333a60c0722229190 has been approved by tmandry

bors · 2020-08-14T02:52:34Z

⌛ Testing commit a670540017a7e61532684ef333a60c0722229190 with merge 9da5d2aa6077b7b41f068a537e1968c1c3f9e946...

tmandry · 2020-08-14T02:59:40Z

@bors retry yielding

rust-log-analyzer · 2020-08-14T03:02:56Z

Your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

##[group]Run exit 1
exit 1
shell: /bin/bash --noprofile --norc -e -o pipefail {0}
##[endgroup]
##[error]Process completed with exit code 1.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @rust-lang/infra. (Feature Requests)

richkadel · 2020-08-14T03:55:42Z

@bors retry yielding

bors · 2020-08-14T05:18:28Z

⌛ Testing commit a670540017a7e61532684ef333a60c0722229190 with merge 8a6c2ef7f1239c35370200c23e2d435a5bf2b54d...

bors · 2020-08-14T07:10:31Z

💔 Test failed - checks-azure

richkadel · 2020-08-14T07:40:10Z

I forgot MacOS section names are a little different. I'll compile on my Mac and fix this.

I found some areas for improvement while attempting to debug the SegFault issue when running rust programs compiled using MSVC, with coverage instrumentation. I discovered that LLVM's coverage writer was generating incomplete function name variable names (that's not a typo: the name of the variable that holds a function name). The existing implementation used one-up numbers to distinguish variables, and correcting the names did not fix the MSVC coverage bug, but the fix in this PR makes the names and resulting LLVM IR easier to follow and more consistent with Clang's implementation. I also changed the way the `-Zinstrument-coverage` option is supported in symbol_export.rs. The original implementation was incorrect, and the corrected version matches the handling for `-Zprofile-generate`, as it turns out. (An argument could be made that maybe `-Zinstrument-coverage` should automatically enable `-Cprofile-generate`. In fact, if `-Cprofile-generate` is analagous to Clang's `-fprofile-generate`, as some documentation implies, Clang always requires this flag for its implementation of source-based code coverage. This would require a little more validation, and if implemented, would probably require updating some of the user-facing messages related to `-Cprofile-generate` to not be so specific to the PGO use case.) None of these changes fixed the MSVC coverage problems, but they should still be welcome improvements. Lastly, I added some additional FIXME comments in instrument_coverage.rs describing issues I found with the generated LLVM IR that would be resolved if the coverage instrumentation is injected with a `Statement` instead of as a new `BasicBlock`. I describe seven advantages of this change, but it requires some discussion before making a change like this.

richkadel · 2020-08-14T09:26:14Z

@bors retry yielding

richkadel · 2020-08-14T14:20:26Z

@bors r+ rollup=never (due to the LLVM IR tests)

bors · 2020-08-14T14:20:27Z

📌 Commit ba18978 has been approved by richkadel

bors · 2020-08-14T16:27:05Z

⌛ Testing commit ba18978 with merge 8e21bd0...

bors · 2020-08-14T18:30:59Z

☀️ Test successful - checks-actions, checks-azure
Approved by: richkadel
Pushing 8e21bd0 to master...

rust-highfive assigned tmandry Aug 11, 2020

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

richkadel commented Aug 11, 2020

View reviewed changes

src/librustc_mir/transform/instrument_coverage.rs Outdated Show resolved Hide resolved

richkadel force-pushed the llvm-coverage-map-gen-5.3 branch from 8d80dd3 to cbb073c Compare August 12, 2020 07:15

This comment has been minimized.

Sign in to view

richkadel force-pushed the llvm-coverage-map-gen-5.3 branch from cbb073c to 799ef9c Compare August 12, 2020 18:42

tmandry reviewed Aug 12, 2020

View reviewed changes

richkadel force-pushed the llvm-coverage-map-gen-5.3 branch from 799ef9c to a670540 Compare August 13, 2020 01:28

wesleywiser reviewed Aug 13, 2020

View reviewed changes

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 Aug 14, 2020

bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 14, 2020

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 14, 2020

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 14, 2020

richkadel force-pushed the llvm-coverage-map-gen-5.3 branch from a670540 to 49e5445 Compare August 14, 2020 09:13

richkadel force-pushed the llvm-coverage-map-gen-5.3 branch from 49e5445 to ba18978 Compare August 14, 2020 09:24

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 Aug 14, 2020

bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 14, 2020

bors merged commit 8e21bd0 into rust-lang:master Aug 14, 2020

cuviper added this to the 1.47.0 milestone May 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LLVM IR coverage encoding aligns closer to Clang's #75416

LLVM IR coverage encoding aligns closer to Clang's #75416

richkadel commented Aug 11, 2020 •

edited

Loading

richkadel commented Aug 12, 2020

This comment has been minimized.

richkadel commented Aug 12, 2020

richkadel commented Aug 12, 2020

wesleywiser Aug 13, 2020

tmandry commented Aug 14, 2020

bors commented Aug 14, 2020

bors commented Aug 14, 2020

tmandry commented Aug 14, 2020

tmandry commented Aug 14, 2020

bors commented Aug 14, 2020

bors commented Aug 14, 2020

bors commented Aug 14, 2020

tmandry commented Aug 14, 2020

bors commented Aug 14, 2020

bors commented Aug 14, 2020

tmandry commented Aug 14, 2020

rust-log-analyzer commented Aug 14, 2020

richkadel commented Aug 14, 2020

bors commented Aug 14, 2020

bors commented Aug 14, 2020

richkadel commented Aug 14, 2020

richkadel commented Aug 14, 2020

richkadel commented Aug 14, 2020

bors commented Aug 14, 2020

bors commented Aug 14, 2020

bors commented Aug 14, 2020

LLVM IR coverage encoding aligns closer to Clang's #75416

LLVM IR coverage encoding aligns closer to Clang's #75416

Conversation

richkadel commented Aug 11, 2020 • edited Loading

richkadel commented Aug 12, 2020

This comment has been minimized.

richkadel commented Aug 12, 2020

richkadel commented Aug 12, 2020

wesleywiser Aug 13, 2020

Choose a reason for hiding this comment

tmandry commented Aug 14, 2020

bors commented Aug 14, 2020

bors commented Aug 14, 2020

tmandry commented Aug 14, 2020

tmandry commented Aug 14, 2020

bors commented Aug 14, 2020

bors commented Aug 14, 2020

bors commented Aug 14, 2020

tmandry commented Aug 14, 2020

bors commented Aug 14, 2020

bors commented Aug 14, 2020

tmandry commented Aug 14, 2020

rust-log-analyzer commented Aug 14, 2020

richkadel commented Aug 14, 2020

bors commented Aug 14, 2020

bors commented Aug 14, 2020

richkadel commented Aug 14, 2020

richkadel commented Aug 14, 2020

richkadel commented Aug 14, 2020

bors commented Aug 14, 2020

bors commented Aug 14, 2020

bors commented Aug 14, 2020

richkadel commented Aug 11, 2020 •

edited

Loading