Added some unit tests as requested by richkadel · Pull Request #78963 · rust-lang/rust

richkadel · 2020-11-12T00:35:54Z

As discussed in PR #78267, for example:

Working expression optimization, and some improvements to branch-level source coverage #78267 (comment)
Working expression optimization, and some improvements to branch-level source coverage #78267 (comment)

This is pretty much self contained, but depending on feedback and timing, I may have a chance to add a few more unit tests requested against counters.rs. I'm looking at those now.

As discussed in PR rust-lang#78267, for example: * rust-lang#78267 (comment) * rust-lang#78267 (comment)

richkadel · 2020-11-12T01:12:52Z

error[E0277]: `rustc_span::Span` cannot be shared between threads safely
  --> compiler/rustc_mir/src/transform/coverage/tests.rs:16:5
   |
16 |     static DUMMY_TYS: SyncOnceCell<TyS<'_>> = SyncOnceCell::new();
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `rustc_span::Span` cannot be shared between threads safely
   |
   = help: within `VariantDef`, the trait `std::marker::Sync` is not implemented for `rustc_span::Span`
   = note: required because it appears within the type `rustc_span::symbol::Ident`
   = note: required because it appears within the type `VariantDef`
   = note: required because of the requirements on the impl of `std::marker::Sync` for `std::ptr::Unique<VariantDef>`
   = note: required because it appears within the type `RawVec<VariantDef>`
   = note: required because it appears within the type `Vec<VariantDef>`
   = note: required because it appears within the type `rustc_index::vec::IndexVec<VariantIdx, VariantDef>`
   = note: required because it appears within the type `AdtDef`
   = note: required because of the requirements on the impl of `std::marker::Send` for `&AdtDef`
   = note: required because it appears within the type `rustc_middle::ty::TyKind`
   = note: required because it appears within the type `TyS`
   = note: required because of the requirements on the impl of `std::marker::Sync` for `SyncOnceCell<TyS>`
   = note: shared static variables must have a type that implements `Sync`

This was working before I rebased. I'm recompiling locally now and will resolve it shortly.

richkadel · 2020-11-12T01:57:30Z

Huh...

This does not fail when I run:

$ ./x.py test compiler/rustc_mir --test-args '--show-output coverage'

I have to figure out what build command IS failing and why.

tmandry · 2020-11-12T02:03:13Z

Does ./x.py check work?

The step above the failure I see is

Checking compiler artifacts (x86_64-unknown-linux-gnu -> i686-pc-windows-gnu)

Also, I doubt it's relevant that it's cross compiling, but it could be.

richkadel · 2020-11-12T02:08:04Z

I've never used ./x.py check before (not sure what it does), but I saw that was where it failed here in CI, so I just ran it (without options):

$ ./x.py check

and got Build completed successfully.

I'll try with the --all-targets flag.

richkadel · 2020-11-12T02:09:22Z

Also succeeds for me with --all-targets

richkadel · 2020-11-12T02:10:26Z

I'm trying the command shown in CI:

$ ./x.py check --target=i686-pc-windows-gnu --host=i686-pc-windows-gnu --all-targets

richkadel · 2020-11-12T02:13:56Z

That also succeeds (compiling locally on linux).

I see the CI failure was from PR (mingw-check, ubuntu-latest-xl)

But this is a Rust error, AFAICT, not something platform-dependent, right?

Mark-Simulacrum · 2020-11-12T02:14:47Z

You likely need to turn parallel compiler on in config.toml, it should reproduce on x.py check --all-targets after that I suspect.

x.py check is the equivalent of cargo check.

richkadel · 2020-11-12T02:16:26Z

I already have this in my config.toml:

parallel-compiler = true

Mark-Simulacrum · 2020-11-12T02:24:53Z

Hm maybe try without then? Let me also check locally.

richkadel · 2020-11-12T02:27:46Z

Yes, it fails without it.

Should it?

richkadel · 2020-11-12T02:32:27Z

Hmm... I see this in compiler/rustc_span/src/lib.rs:

// The interner is pointed to by a thread local value which is only set on the main thread
// with parallelization is disabled. So we don't allow `Span` to transfer between threads
// to avoid panics and other errors, even though it would be memory safe to do so.
#[cfg(not(parallel_compiler))]
impl !Send for Span {}
#[cfg(not(parallel_compiler))]
impl !Sync for Span {}

richkadel · 2020-11-12T02:34:03Z

It seems odd to allow Sync and Send when parallel_compiler is on. Makes locally testing kind of hard.

Mark-Simulacrum · 2020-11-12T02:35:34Z

Hm, I think yes, and really I think the dummy_tys function is likely UB or at least quite worrisome, we really should not be synthesizing references to mem::zeroed (or equivalent) like that. I guess we don't readily have a tcx or anything available since this is a unit test? Easiest thing might be to add a constructor in ty/mod.rs that creates a dummy value called for_unit_testing or something like that. I suspect to get the reference we can just &*Box::leak(constructor()), we don't really care about leaking memory in unit tests I imagine and that'll avoid the SyncOnceCell problems.

richkadel · 2020-11-12T02:38:08Z

Ok, I'll try that. Thanks for the suggestion!

(Hopefully I understand this well enough to implement it... 😕 .)

Per Mark's recommendation at: rust-lang#78963 (comment)

compiler/rustc_mir/src/transform/coverage/tests.rs

richkadel · 2020-11-13T00:18:18Z

@tmandry - other than addressing code review feedback, let's submit this PR with the tests included at this stage. I think it gives a reasonable starting point to help understand the code, and to build additional tests later. WDYT?

Restructured the code a little, to allow getting both the mir::Body and coverage graph.

tmandry

Looks good! Left a few comments but feel free to respond however you like and r=me.

compiler/rustc_mir/src/transform/coverage/mod.rs

tmandry · 2020-11-13T01:20:07Z

compiler/rustc_mir/src/transform/coverage/tests.rs

+    }
+
+    fn switchint(&mut self, some_from_block: Option<BasicBlock>) -> BasicBlock {
+        let move_ = |place: Place<'tcx>| Operand::Move(place);


nit: seems a bit unnecessary if you're only using it in one place

tmandry · 2020-11-13T01:23:58Z

compiler/rustc_mir/src/transform/coverage/tests.rs

+        let switchint_kind = TerminatorKind::SwitchInt {
+            discr: move_(discriminant),
+            switch_ty: dummy_ty(),
+            targets: SwitchTargets::static_if(0, START_BLOCK, START_BLOCK),


One side effect of defaulting everything to point to START_BLOCK is that by default, you're wrapping everything in your function up to the current point in a loop.

One strategy to avoid this would be to make block 1 (the second block) return, and have all branches go there by default.

I think that would add some significant complexity during init though, not sure if it's worth it.

I'm not sure what you're concern is.

These defaults are meant to be replaced (none of them should remain as START_BLOCK) before constructing the mir::Body.

I can change this to a new constant (maybe set to the max BasicBlock index value) to make it more clear that we're setting a temporary value.

Let me know if I'm missing your point.

Replaced with TEMP_BLOCK and a comment at its definition. Let me know if this doesn't resolve your concern. Thanks!

compiler/rustc_mir/src/transform/coverage/test_macros/src/lib.rs

tmandry · 2020-11-13T02:02:35Z

@bors delegate+

bors · 2020-11-13T02:02:38Z

✌️ @richkadel can now approve this pull request

richkadel · 2020-11-13T17:05:47Z

@bors r=tmandry rollup=always

bors · 2020-11-13T17:05:48Z

📌 Commit bb7fe84f1e761c2ba9be74993b319144a41cf1cb has been approved by tmandry

richkadel · 2020-11-13T17:09:41Z

@bors r=tmandry rollup=always

bors · 2020-11-13T17:09:42Z

📌 Commit b4b0ef3 has been approved by tmandry

…0.4, r=tmandry Added some unit tests as requested As discussed in PR rust-lang#78267, for example: * rust-lang#78267 (comment) * rust-lang#78267 (comment) r? `@tmandry` FYI: `@wesleywiser` This is pretty much self contained, but depending on feedback and timing, I may have a chance to add a few more unit tests requested against `counters.rs`. I'm looking at those now.

…0.4, r=tmandry Added some unit tests as requested As discussed in PR rust-lang#78267, for example: * rust-lang#78267 (comment) * rust-lang#78267 (comment) r? ``@tmandry`` FYI: ``@wesleywiser`` This is pretty much self contained, but depending on feedback and timing, I may have a chance to add a few more unit tests requested against `counters.rs`. I'm looking at those now.

…0.4, r=tmandry Added some unit tests as requested As discussed in PR rust-lang#78267, for example: * rust-lang#78267 (comment) * rust-lang#78267 (comment) r? ```@tmandry``` FYI: ```@wesleywiser``` This is pretty much self contained, but depending on feedback and timing, I may have a chance to add a few more unit tests requested against `counters.rs`. I'm looking at those now.

Rollup of 15 pull requests Successful merges: - rust-lang#78352 (Do not call `unwrap` with `signatures` option enabled) - rust-lang#78590 (refactor: removing alloc::collections::vec_deque ignore-tidy-filelength) - rust-lang#78848 (Bump minimal supported LLVM version to 9) - rust-lang#78856 (Explicitly checking for or-pattern before test) - rust-lang#78948 (test: add `()=()=()=...` to weird-exprs.rs) - rust-lang#78962 (Add a test for r# identifiers) - rust-lang#78963 (Added some unit tests as requested) - rust-lang#78966 (Never inline C variadics, cold functions, functions with incompatible attributes ...) - rust-lang#78968 (Include llvm-as in llvm-tools-preview component) - rust-lang#78969 (Normalize function type during validation) - rust-lang#78980 (Fix rustc_ast_pretty print_qpath resulting in invalid macro input) - rust-lang#78986 (Avoid installing external LLVM dylibs) - rust-lang#78988 (Fix an intrinsic invocation on threaded wasm) - rust-lang#78993 (rustc_target: Fix dash vs underscore mismatches in option names) - rust-lang#79013 (Clean up outdated `use_once_payload` pretty printer comment) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

rust-highfive assigned tmandry Nov 12, 2020

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

Added some unit tests as requested

bd0eb07

As discussed in PR rust-lang#78267, for example: * rust-lang#78267 (comment) * rust-lang#78267 (comment)

richkadel force-pushed the llvm-coverage-counters-2.0.4 branch from 8b23016 to bd0eb07 Compare November 12, 2020 00:40

Overcome Sync issues with non-parallel compiler

eb9f2bb

Per Mark's recommendation at: rust-lang#78963 (comment)

richkadel force-pushed the llvm-coverage-counters-2.0.4 branch from e396af9 to eb9f2bb Compare November 12, 2020 04:36

richkadel commented Nov 12, 2020

View reviewed changes

compiler/rustc_mir/src/transform/coverage/tests.rs Show resolved Hide resolved

richkadel force-pushed the llvm-coverage-counters-2.0.4 branch from c1cbbf6 to 29a4621 Compare November 13, 2020 00:21

Added a unit test for BcbCounters

c131063

Restructured the code a little, to allow getting both the mir::Body and coverage graph.

richkadel force-pushed the llvm-coverage-counters-2.0.4 branch from 29a4621 to c131063 Compare November 13, 2020 00:22

tmandry approved these changes Nov 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 Nov 13, 2020

Addressed feedback

b4b0ef3

richkadel force-pushed the llvm-coverage-counters-2.0.4 branch from bb7fe84 to b4b0ef3 Compare November 13, 2020 17:07

Dylan-DPC-zz mentioned this pull request Nov 13, 2020

Rollup of 12 pull requests #79029

Closed

Dylan-DPC-zz mentioned this pull request Nov 13, 2020

Rollup of 12 pull requests #79030

Closed

Dylan-DPC-zz mentioned this pull request Nov 14, 2020

Rollup of 12 pull requests #79037

Closed

jonas-schievink mentioned this pull request Nov 14, 2020

Rollup of 26 pull requests #79053

Closed

jonas-schievink mentioned this pull request Nov 14, 2020

Rollup of 25 pull requests #79056

Closed

jonas-schievink mentioned this pull request Nov 14, 2020

Rollup of 24 pull requests #79057

Closed

Dylan-DPC-zz mentioned this pull request Nov 15, 2020

Rollup of 15 pull requests #79065

Merged

bors merged commit 335a255 into rust-lang:master Nov 15, 2020

rustbot added this to the 1.50.0 milestone Nov 15, 2020

Uh oh!

Conversation

richkadel commented Nov 12, 2020

Uh oh!

richkadel commented Nov 12, 2020

Uh oh!

richkadel commented Nov 12, 2020

Uh oh!

tmandry commented Nov 12, 2020

Uh oh!

richkadel commented Nov 12, 2020

Uh oh!

richkadel commented Nov 12, 2020

Uh oh!

richkadel commented Nov 12, 2020

Uh oh!

richkadel commented Nov 12, 2020

Uh oh!

Mark-Simulacrum commented Nov 12, 2020

Uh oh!

richkadel commented Nov 12, 2020

Uh oh!

Mark-Simulacrum commented Nov 12, 2020

Uh oh!

richkadel commented Nov 12, 2020

Uh oh!

richkadel commented Nov 12, 2020

Uh oh!

richkadel commented Nov 12, 2020

Uh oh!

Mark-Simulacrum commented Nov 12, 2020

Uh oh!

richkadel commented Nov 12, 2020

Uh oh!

Uh oh!

richkadel commented Nov 13, 2020

Uh oh!

tmandry left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tmandry Nov 13, 2020

Choose a reason for hiding this comment

Uh oh!

tmandry Nov 13, 2020

Choose a reason for hiding this comment

Uh oh!

richkadel Nov 13, 2020

Choose a reason for hiding this comment

Uh oh!

richkadel Nov 13, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tmandry commented Nov 13, 2020

Uh oh!

bors commented Nov 13, 2020

Uh oh!

richkadel commented Nov 13, 2020

Uh oh!

bors commented Nov 13, 2020

Uh oh!

richkadel commented Nov 13, 2020

Uh oh!

bors commented Nov 13, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants