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

MIR Inline is incompatible with coverage #80521

Merged
merged 1 commit into from
Jan 7, 2021

Conversation

richkadel
Copy link
Contributor

@richkadel richkadel commented Dec 30, 2020

Fixes: #80060

Fixed by disabling inlining if -Zinstrument-coverage is set.

The PR also adds additional use cases to the coverage test for doctests.

r? @wesleywiser
cc: @tmandry

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 30, 2020
Copy link
Member

@wesleywiser wesleywiser left a comment

Choose a reason for hiding this comment

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

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+.

@nagisa
Copy link
Member

nagisa commented Dec 31, 2020

If that's pretty easy to do

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 -Zinstrument-coverage are stabilized this will need to be resolved more properly by renumbering or something along those lines.

@wesleywiser
Copy link
Member

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 mir-opt test that verifies we don't inline anything at the MIR level. This is a bit fragile as, for example, in the current MIR inliner, the ordering of baz and bar here is important because of how we avoid query cycles (#68828 is working to improve that).

Although, having another compile-fail test that verifies -Zmir-opt-level=2 (or above) combined with -Zcode-coverage generates this error is also a good idea!

@richkadel
Copy link
Contributor Author

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 mir_opt_level values, defined in session's config.rs, via:

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.

@richkadel
Copy link
Contributor Author

richkadel commented Dec 31, 2020

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;
        }

64| | assert_eq!(1, 2);
65| | }
66| 1|}
23| 2|//! #[derive(Debug, PartialEq)]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

@@ -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>>:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

@rust-log-analyzer
Copy link
Collaborator

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
configure: rust.channel         := nightly
configure: rust.debug-assertions := True
configure: llvm.assertions      := True
configure: dist.missing-tools   := True
configure: build.configure-args := ['--enable-sccache', '--disable-manage-submodu ...
configure: writing `config.toml` in current directory
configure: 
configure: run `python /checkout/x.py --help`
configure: 
---
    Checking rustc_traits v0.0.0 (/checkout/compiler/rustc_traits)
    Checking rustc_plugin_impl v0.0.0 (/checkout/compiler/rustc_plugin_impl)
    Checking rustc_privacy v0.0.0 (/checkout/compiler/rustc_privacy)
    Checking rustc_interface v0.0.0 (/checkout/compiler/rustc_interface)
error[E0599]: no associated item named `CONST_IDENTITY_ELIM` found for struct `MinMirOptLevel` in the current scope
    |
    |
564 |     tracked!(mir_opt_level, MinMirOptLevel::CONST_IDENTITY_ELIM);
    |                                             |
    |                                             |
    |                                             associated item not found in `MinMirOptLevel`
    |                                             help: there is an associated constant with a similar name: `CONST_IDENTITY`
    Checking rustc_driver v0.0.0 (/checkout/compiler/rustc_driver)
error: aborting due to previous error

For more information about this error, try `rustc --explain E0599`.
For more information about this error, try `rustc --explain E0599`.
error: could not compile `rustc_interface`

To learn more, run the command again with --verbose.
warning: build failed, waiting for other jobs to finish...
error: build failed
command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "check" "--target" "i686-pc-windows-gnu" "-Zbinary-dep-depinfo" "-j" "16" "--release" "--locked" "--color" "always" "--features" " llvm" "--manifest-path" "/checkout/compiler/rustc/Cargo.toml" "--all-targets" "-p" "rustc-main" "-p" "rustc_driver" "-p" "rustc_errors" "-p" "rustc_macros" "-p" "rustc_lint_defs" "-p" "rustc_mir" "-p" "rustc_trait_selection" "-p" "rustc_parse_format" "-p" "rustc_index" "-p" "coverage_test_macros" "-p" "rustc_lexer" "-p" "rustc_infer" "-p" "rustc_apfloat" "-p" "rustc_graphviz" "-p" "rustc_attr" "-p" "rustc_plugin_impl" "-p" "rustc_save_analysis" "-p" "rustc_middle" "-p" "rustc_type_ir" "-p" "rustc_query_system" "-p" "rustc_arena" "-p" "rustc_interface" "-p" "rustc_typeck" "-p" "rustc_incremental" "-p" "rustc_fs_util" "-p" "rustc_ty_utils" "-p" "rustc_codegen_llvm" "-p" "rustc_llvm" "-p" "rustc_builtin_macros" "-p" "rustc_privacy" "-p" "rustc_ast_lowering" "-p" "rustc_mir_build" "-p" "rustc_expand" "-p" "rustc_traits" "-p" "rustc_resolve" "-p" "rustc_ast_passes" "-p" "rustc_symbol_mangling" "-p" "rustc_passes" "-p" "rustc_error_codes" "-p" "rustc_ast_pretty" "-p" "rustc_parse" "-p" "rustc_feature" "-p" "rustc_session" "-p" "rustc_span" "-p" "rustc_lint" "-p" "rustc_hir_pretty" "-p" "rustc_data_structures" "-p" "rustc_target" "-p" "rustc_hir" "-p" "rustc_ast" "-p" "rustc_serialize" "-p" "rustc_metadata" "-p" "rustc_codegen_ssa" "--message-format" "json-render-diagnostics"
failed to run: /checkout/obj/build/bootstrap/debug/bootstrap check --target=i686-pc-windows-gnu --host=i686-pc-windows-gnu --all-targets
Build completed unsuccessfully in 0:02:09

@rust-log-analyzer
Copy link
Collaborator

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
configure: rust.channel         := nightly
configure: rust.debug-assertions := True
configure: llvm.assertions      := True
configure: dist.missing-tools   := True
configure: build.configure-args := ['--enable-sccache', '--disable-manage-submodu ...
configure: writing `config.toml` in current directory
configure: 
configure: run `python /checkout/x.py --help`
configure: 
---
Checking which error codes lack tests...
Found 434 error codes
Found 0 error codes with no tests
Done!
tidy error: /checkout/src/test/mir-opt/instrument_coverage.rs:6: line longer than 100 chars
some tidy checks failed

command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "/checkout/obj/build"
expected success, got: exit code: 1

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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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 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.

@richkadel
Copy link
Contributor Author

@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!)

@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-llvm-9 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
.................................................................................................... 9000/11240
.................................................................................................... 9100/11240
.................................................................................................... 9200/11240
....................................i......i........................................................ 9300/11240
...........................................................................iiiiii..iiiiii.i......... 9400/11240
.................................................................................................... 9600/11240
.................................................................................................... 9700/11240
.................................................................................................... 9800/11240
.................................................................................................... 9900/11240
---

---- [ui] ui/mir/mir-inlining/inline-instrument-coverage-fail.rs stdout ----
diff of stderr:

1 warning: `-Z mir-opt-level=2` (or any level > 1) enables function inlining, which is incompatible with `-Z instrument-coverage`. Inlining will be disabled.
+ error[E0463]: can't find crate for `profiler_builtins`
+    |
+    |
+    = note: the compiler may have been built without the profiler runtime
+ error: aborting due to previous error
+ 
+ For more information about this error, try `rustc --explain E0463`.
3 
3 


The actual stderr differed from the expected stderr.
Actual stderr saved to /checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/mir/mir-inlining/inline-instrument-coverage-fail/inline-instrument-coverage-fail.stderr
To update references, rerun the tests and pass the `--bless` flag
To only update this specific test, also pass `--test-args mir/mir-inlining/inline-instrument-coverage-fail.rs`
error: 1 errors occurred comparing output.
status: exit code: 1
status: exit code: 1
command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/ui/mir/mir-inlining/inline-instrument-coverage-fail.rs" "-Zthreads=1" "--target=x86_64-unknown-linux-gnu" "--error-format" "json" "-Zui-testing" "-Zdeduplicate-diagnostics=no" "-Zemit-future-incompat-report" "--emit" "metadata" "-C" "prefer-dynamic" "--out-dir" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/mir/mir-inlining/inline-instrument-coverage-fail" "-A" "unused" "-Crpath" "-O" "-Cdebuginfo=0" "-Zunstable-options" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-Zmir-opt-level=2" "-Zinstrument-coverage" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/mir/mir-inlining/inline-instrument-coverage-fail/auxiliary"
------------------------------------------

------------------------------------------
stderr:
stderr:
------------------------------------------
warning: `-Z mir-opt-level=2` (or any level > 1) enables function inlining, which is incompatible with `-Z instrument-coverage`. Inlining will be disabled.
error[E0463]: can't find crate for `profiler_builtins`
   |
   |
   = note: the compiler may have been built without the profiler runtime
error: aborting due to previous error

For more information about this error, try `rustc --explain E0463`.

---

Some tests failed in compiletest suite=ui mode=ui host=x86_64-unknown-linux-gnu target=x86_64-unknown-linux-gnu


command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/compiletest" "--compile-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib" "--run-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib" "--rustc-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "--src-base" "/checkout/src/test/ui" "--build-base" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui" "--stage-id" "stage2-x86_64-unknown-linux-gnu" "--suite" "ui" "--mode" "ui" "--target" "x86_64-unknown-linux-gnu" "--host" "x86_64-unknown-linux-gnu" "--llvm-filecheck" "/usr/lib/llvm-9/bin/FileCheck" "--nodejs" "/usr/bin/node" "--host-rustcflags" "-Crpath -O -Cdebuginfo=0 -Zunstable-options  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--target-rustcflags" "-Crpath -O -Cdebuginfo=0 -Zunstable-options  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--docck-python" "/usr/bin/python2.7" "--lldb-python" "/usr/bin/python2.7" "--gdb" "/usr/bin/gdb" "--quiet" "--llvm-version" "9.0.0" "--llvm-components" "aarch64 aarch64asmparser aarch64codegen aarch64desc aarch64disassembler aarch64info aarch64utils aggressiveinstcombine all all-targets amdgpu amdgpuasmparser amdgpucodegen amdgpudesc amdgpudisassembler amdgpuinfo amdgpuutils analysis arm armasmparser armcodegen armdesc armdisassembler arminfo armutils asmparser asmprinter avr avrasmparser avrcodegen avrdesc avrdisassembler avrinfo binaryformat bitreader bitstreamreader bitwriter bpf bpfasmparser bpfcodegen bpfdesc bpfdisassembler bpfinfo codegen core coroutines coverage debuginfocodeview debuginfodwarf debuginfogsym debuginfomsf debuginfopdb demangle dlltooldriver engine executionengine fuzzmutate globalisel hexagon hexagonasmparser hexagoncodegen hexagondesc hexagondisassembler hexagoninfo instcombine instrumentation interpreter ipo irreader jitlink lanai lanaiasmparser lanaicodegen lanaidesc lanaidisassembler lanaiinfo libdriver lineeditor linker lto mc mca mcdisassembler mcjit mcparser mips mipsasmparser mipscodegen mipsdesc mipsdisassembler mipsinfo mirparser msp430 msp430asmparser msp430codegen msp430desc msp430disassembler msp430info native nativecodegen nvptx nvptxcodegen nvptxdesc nvptxinfo objcarcopts object objectyaml option orcjit passes perfjitevents powerpc powerpcasmparser powerpccodegen powerpcdesc powerpcdisassembler powerpcinfo profiledata remarks riscv riscvasmparser riscvcodegen riscvdesc riscvdisassembler riscvinfo riscvutils runtimedyld scalaropts selectiondag sparc sparcasmparser sparccodegen sparcdesc sparcdisassembler sparcinfo support symbolize systemz systemzasmparser systemzcodegen systemzdesc systemzdisassembler systemzinfo tablegen target textapi transformutils vectorize webassembly webassemblyasmparser webassemblycodegen webassemblydesc webassemblydisassembler webassemblyinfo windowsmanifest x86 x86asmparser x86codegen x86desc x86disassembler x86info x86utils xcore xcorecodegen xcoredesc xcoredisassembler xcoreinfo xray" "--system-llvm" "--cc" "" "--cxx" "" "--cflags" "" "--adb-path" "adb" "--adb-test-dir" "/data/tmp/work" "--android-cross-path" "" "--color" "always"


failed to run: /checkout/obj/build/bootstrap/debug/bootstrap --stage 2 test --exclude src/tools/tidy
Build completed unsuccessfully in 0:16:11

@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-llvm-9 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
.................................................................................................... 9000/11240
.................................................................................................... 9100/11240
.................................................................................................... 9200/11240
....................................i......i........................................................ 9300/11240
...........................................................................iiiiii..iiiiii.i......... 9400/11240
.................................................................................................... 9600/11240
.................................................................................................... 9700/11240
.................................................................................................... 9800/11240
.................................................................................................... 9900/11240
---
failures:

---- [ui] ui/mir/mir-inlining/inline-instrument-coverage-fail.rs stdout ----

error: test compilation failed although it shouldn't!
status: exit code: 1
command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/ui/mir/mir-inlining/inline-instrument-coverage-fail.rs" "-Zthreads=1" "--target=x86_64-unknown-linux-gnu" "--error-format" "json" "-Zui-testing" "-Zdeduplicate-diagnostics=no" "-Zemit-future-incompat-report" "-C" "prefer-dynamic" "--out-dir" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/mir/mir-inlining/inline-instrument-coverage-fail" "-A" "unused" "-Crpath" "-O" "-Cdebuginfo=0" "-Zunstable-options" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-Zmir-opt-level=2" "-Zinstrument-coverage" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/mir/mir-inlining/inline-instrument-coverage-fail/auxiliary"
------------------------------------------

------------------------------------------
stderr:
stderr:
------------------------------------------
warning: `-Z mir-opt-level=2` (or any level > 1) enables function inlining, which is incompatible with `-Z instrument-coverage`. Inlining will be disabled.
error[E0463]: can't find crate for `profiler_builtins`
   |
   |
   = note: the compiler may have been built without the profiler runtime
error: aborting due to previous error

For more information about this error, try `rustc --explain E0463`.

---

Some tests failed in compiletest suite=ui mode=ui host=x86_64-unknown-linux-gnu target=x86_64-unknown-linux-gnu


command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/compiletest" "--compile-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib" "--run-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib" "--rustc-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "--src-base" "/checkout/src/test/ui" "--build-base" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui" "--stage-id" "stage2-x86_64-unknown-linux-gnu" "--suite" "ui" "--mode" "ui" "--target" "x86_64-unknown-linux-gnu" "--host" "x86_64-unknown-linux-gnu" "--llvm-filecheck" "/usr/lib/llvm-9/bin/FileCheck" "--nodejs" "/usr/bin/node" "--host-rustcflags" "-Crpath -O -Cdebuginfo=0 -Zunstable-options  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--target-rustcflags" "-Crpath -O -Cdebuginfo=0 -Zunstable-options  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--docck-python" "/usr/bin/python2.7" "--lldb-python" "/usr/bin/python2.7" "--gdb" "/usr/bin/gdb" "--quiet" "--llvm-version" "9.0.0" "--llvm-components" "aarch64 aarch64asmparser aarch64codegen aarch64desc aarch64disassembler aarch64info aarch64utils aggressiveinstcombine all all-targets amdgpu amdgpuasmparser amdgpucodegen amdgpudesc amdgpudisassembler amdgpuinfo amdgpuutils analysis arm armasmparser armcodegen armdesc armdisassembler arminfo armutils asmparser asmprinter avr avrasmparser avrcodegen avrdesc avrdisassembler avrinfo binaryformat bitreader bitstreamreader bitwriter bpf bpfasmparser bpfcodegen bpfdesc bpfdisassembler bpfinfo codegen core coroutines coverage debuginfocodeview debuginfodwarf debuginfogsym debuginfomsf debuginfopdb demangle dlltooldriver engine executionengine fuzzmutate globalisel hexagon hexagonasmparser hexagoncodegen hexagondesc hexagondisassembler hexagoninfo instcombine instrumentation interpreter ipo irreader jitlink lanai lanaiasmparser lanaicodegen lanaidesc lanaidisassembler lanaiinfo libdriver lineeditor linker lto mc mca mcdisassembler mcjit mcparser mips mipsasmparser mipscodegen mipsdesc mipsdisassembler mipsinfo mirparser msp430 msp430asmparser msp430codegen msp430desc msp430disassembler msp430info native nativecodegen nvptx nvptxcodegen nvptxdesc nvptxinfo objcarcopts object objectyaml option orcjit passes perfjitevents powerpc powerpcasmparser powerpccodegen powerpcdesc powerpcdisassembler powerpcinfo profiledata remarks riscv riscvasmparser riscvcodegen riscvdesc riscvdisassembler riscvinfo riscvutils runtimedyld scalaropts selectiondag sparc sparcasmparser sparccodegen sparcdesc sparcdisassembler sparcinfo support symbolize systemz systemzasmparser systemzcodegen systemzdesc systemzdisassembler systemzinfo tablegen target textapi transformutils vectorize webassembly webassemblyasmparser webassemblycodegen webassemblydesc webassemblydisassembler webassemblyinfo windowsmanifest x86 x86asmparser x86codegen x86desc x86disassembler x86info x86utils xcore xcorecodegen xcoredesc xcoredisassembler xcoreinfo xray" "--system-llvm" "--cc" "" "--cxx" "" "--cflags" "" "--adb-path" "adb" "--adb-test-dir" "/data/tmp/work" "--android-cross-path" "" "--color" "always"


failed to run: /checkout/obj/build/bootstrap/debug/bootstrap --stage 2 test --exclude src/tools/tidy
Build completed unsuccessfully in 0:16:04

@richkadel
Copy link
Contributor Author

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?

@richkadel
Copy link
Contributor Author

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?

@richkadel
Copy link
Contributor Author

The UI test won't pass with just a warning now, because it does not require profiler_builtins.

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 -Zinstrument-coverage is set.

@richkadel richkadel changed the title -mir-opt-level > 1 is incompatible with coverage MIR Inline is incompatible with coverage Jan 2, 2021
Fixes: rust-lang#80060

Also adds additional test cases for coverage of doctests.
@richkadel
Copy link
Contributor Author

@wesleywiser - I remembered there is a test directive:

// needs-profiler-support

So I'm trying that and if it works I can keep the test.

@richkadel
Copy link
Contributor Author

OK, that worked.

Copy link
Member

@wesleywiser wesleywiser left a comment

Choose a reason for hiding this comment

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

This looks great to me. Sorry for the review delay!

@wesleywiser
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Jan 7, 2021

📌 Commit e4aa99f has been approved by wesleywiser

@bors 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 Jan 7, 2021
@bors
Copy link
Contributor

bors commented Jan 7, 2021

⌛ Testing commit e4aa99f with merge d581f4e4a1d33683923ea05c23e01fee8a497214...

@bors
Copy link
Contributor

bors commented Jan 7, 2021

💔 Test failed - checks-actions

@bors 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 Jan 7, 2021
@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@wesleywiser
Copy link
Member

Looks like the aarch64-apple builder timed out perhaps?

@bors retry

@bors 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 Jan 7, 2021
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
@bors bors merged commit 3acd75d into rust-lang:master Jan 7, 2021
@rustbot rustbot added this to the 1.51.0 milestone Jan 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ICE; glacier fixed/27925.rs -Zmir-opt-level=2 -Zinstrument-coverage: add_counter called with duplicate id
9 participants