MIR Inline is incompatible with coverage #80521
Merged
Conversation
This file contains 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
rust-highfive
added
the
S-waiting-on-review
wesleywiser
approved these changes
Dec 30, 2020
There was a problem hiding this comment.
LGTM! My only feedback would be that it would be nice to have a test with mir-opt-level=1 -Zcode-coverage that would fail (somehow) if we were to promote MIR inlining to a mir-opt-level=1 pass. That would ensure we don't forget to to disable the inliner in that situation.
If that's pretty easy to do, I think it would be best to include that, otherwise let me know and I'll r+.
It is – compile_fail ui test should be able to handle this case gracefully. This seems like a reasonable stop-gap solution. Longer term when both inlining and |
|
I was thinking of something along the lines of // compile-flags: -Zmir-opt-level=1 -Zcode-coverage
// EMIT_MIR test_name.bar.Inline.diff
#[inline(never)]
fn foo() {}
pub fn baz() {
bar();
}
#[inline(always)]
fn bar() {
foo();
}as a Although, having another |
|
I was thinking about making a different change (but I guess I could also add a test): if debugging_opts.mir_opt_level >= MinMirOptLevel::INLINE {With the different features enabled by various pub struct MinMirOptLevel;
impl MinMirOptLevel {
pub const CONST_PROPAGATION: usize = 1;
pub const INLINE: usize = 2;
...
}That way, if someone wants to change inlining to level 1, they just update the value in one place. |
|
FYI, I don't think there are that many places to add this kind of check for mir opt level, and it is certainly much less cryptic, for example: /// Returns `true` if and only if this `op` should be const-propagated into.
fn should_const_prop(&mut self, op: OpTy<'tcx>) -> bool {
let mir_opt_level = self.tcx.sess.opts.debugging_opts.mir_opt_level;
if mir_opt_level == 0 {
return false;
}becomes: ...
if mir_opt_level < MinMirOptLevel::CONST_PROPAGATION {
return false;
} |
richkadel
force-pushed
the
llvm-coverage-counters-2.4.0
branch
from
a22efb3 to
ba339be
Compare
richkadel
commented
Dec 31, 2020
| 64| | assert_eq!(1, 2); | ||
| 65| | } | ||
| 66| 1|} | ||
| 23| 2|//! #[derive(Debug, PartialEq)] |
There was a problem hiding this comment.
@Swatinem - just a heads up, I'm planning to submit a minor change for coverage here, but I'm also adding some changes and a FIXME comment at the bottom of doctest.rs describing and showing the column offset issue. I suggested one possible solution in the comment.
richkadel
commented
Dec 31, 2020
| @@ -19,12 +19,12 @@ | |||
| 18| 2| println!("used_only_from_bin_crate_generic_function with {:?}", arg); | |||
| 19| 2|} | |||
| ------------------ | |||
| | used_crate::used_only_from_bin_crate_generic_function::<&alloc::vec::Vec<i32>>: | |||
There was a problem hiding this comment.
@wesleywiser - Ignore the change here. When @Swatinem pushed his recent changes, built from his Windows dev environment, the order of these variants got swapped in the output. (Windows known issue, but tests pass either way.)
When I re-built the blessed results, it swapped them back again.
|
The job Click to see the possible cause of the failure (guessed by this bot) |
richkadel
force-pushed
the
llvm-coverage-counters-2.4.0
branch
from
ba339be to
a051348
Compare
|
The job Click to see the possible cause of the failure (guessed by this bot) |
richkadel
force-pushed
the
llvm-coverage-counters-2.4.0
branch
from
a051348 to
3bf5235
Compare
bjorn3
reviewed
Dec 31, 2020
richkadel
force-pushed
the
llvm-coverage-counters-2.4.0
branch
from
3bf5235 to
3083a65
Compare
Swatinem
reviewed
Dec 31, 2020
Comment on lines
+95
to
+99
| // NOTE: Since the doc comment line prefix may vary, one possible solution is to replace each | ||
| // character stripped from the beginning of doc comment lines with a space. This will give coverage | ||
| // results the correct column offsets, and I think it should compile correctly, but I don't know | ||
| // what affect it might have on diagnostic messages from the compiler, and whether anyone would care | ||
| // if the indentation changed. I don't know if there is a more viable solution. |
There was a problem hiding this comment.
Thanks for the heads up!
And I thought about doing exactly this, maybe also for the line numbers (just insert blank lines), instead of the offset from the start.
I think it would be a viable hack, but it would mean splitting up the code that generates the "run in the playground" links from the code that actually generates the testcase.
A more general solution would probably be something similar to JS sourcemaps, although they themselves are horrible, lol.
There was a problem hiding this comment.
I don't know anything about how playground works, but I can't see any reason to run coverage-enabled binaries from playground, so applying this conversion only conditionally (when not executing in playground)--as I think you suggested elsewhere--seems reasonable to me.
|
@wesleywiser - I believe this change is ready for final review and r+ if you're happy with the changes. The review comments are technically resolved, but left open because I think they are informative. Just FYI. Thanks! (Happy New Year!) |
wesleywiser
requested changes
Dec 31, 2020
richkadel
force-pushed
the
llvm-coverage-counters-2.4.0
branch
from
3083a65 to
7fbc154
Compare
|
The job Click to see the possible cause of the failure (guessed by this bot) |
richkadel
force-pushed
the
llvm-coverage-counters-2.4.0
branch
from
7fbc154 to
668b76a
Compare
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
The UI test won't pass with just a warning now, because it does not require profiler_builtins. What if I just remove the test? |
|
On a related note, I personally think it might be a good idea to enable profiler=true for rustc by default, everywhere, assuming there aren't reasons why we can't. It will resolve some of the other bugs that have been filed related to not finding profiler_builtins. What do you think? |
I removed the test. This PR no longer changes the command line warning to an error. Instead, it still warns, but the message notifies the user that inlining will be disabled. The PR disables inlining if |
richkadel
changed the title
Fixes: rust-lang#80060 Also adds additional test cases for coverage of doctests.
richkadel
force-pushed
the
llvm-coverage-counters-2.4.0
branch
from
f75b794 to
e4aa99f
Compare
|
@wesleywiser - I remembered there is a test directive: // needs-profiler-supportSo I'm trying that and if it works I can keep the test. |
|
OK, that worked. |
wesleywiser
approved these changes
Jan 7, 2021
|
@bors r+ |
|
📌 Commit e4aa99f has been approved by |
bors
added
S-waiting-on-bors
|
⌛ Testing commit e4aa99f with merge d581f4e4a1d33683923ea05c23e01fee8a497214... |
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
|
Looks like the @bors retry |
bors
added
S-waiting-on-bors
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jan 7, 2021
Rollup of 10 pull requests Successful merges: - rust-lang#80012 (Add pointing const identifier when emitting E0435) - rust-lang#80521 (MIR Inline is incompatible with coverage) - rust-lang#80659 (Edit rustc_ast::tokenstream docs) - rust-lang#80660 (Properly handle primitive disambiguators in rustdoc) - rust-lang#80738 (Remove bottom margin from crate version when the docs sidebar is collapsed) - rust-lang#80744 (rustdoc: Turn `next_def_id` comments into docs) - rust-lang#80750 (Don't use to_string on Symbol in rustc_passes/check_attr.rs) - rust-lang#80769 (Improve wording of parse doc) - rust-lang#80780 (Return EOF_CHAR constant instead of magic char.) - rust-lang#80784 (rustc_parse: Better spans for synthesized token streams) Failed merges: - rust-lang#80785 (rustc_ast_pretty: Remove `PrintState::insert_extra_parens`) r? `@ghost` `@rustbot` modify labels: rollup
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixes: #80060
Fixed by disabling inlining if
-Zinstrument-coverageis set.The PR also adds additional use cases to the coverage test for doctests.
r? @wesleywiser
cc: @tmandry