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

Added some unit tests as requested #78963

Merged
merged 4 commits into from
Nov 15, 2020

Conversation

richkadel
Copy link
Contributor

As discussed in PR #78267, for example:

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.

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

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

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
Copy link
Member

tmandry commented Nov 12, 2020

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

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

Also succeeds for me with --all-targets

@richkadel
Copy link
Contributor Author

I'm trying the command shown in CI:

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

@richkadel
Copy link
Contributor Author

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
Copy link
Member

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

I already have this in my config.toml:

parallel-compiler = true

@Mark-Simulacrum
Copy link
Member

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

@richkadel
Copy link
Contributor Author

Yes, it fails without it.

Should it?

@richkadel
Copy link
Contributor Author

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

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

@Mark-Simulacrum
Copy link
Member

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

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

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

@richkadel
Copy link
Contributor Author

@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.
Copy link
Member

@tmandry tmandry left a comment

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
}

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

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@tmandry
Copy link
Member

tmandry commented Nov 13, 2020

@bors delegate+

@bors
Copy link
Contributor

bors commented Nov 13, 2020

✌️ @richkadel can now approve this pull request

@richkadel
Copy link
Contributor Author

@bors r=tmandry rollup=always

@bors
Copy link
Contributor

bors commented Nov 13, 2020

📌 Commit bb7fe84f1e761c2ba9be74993b319144a41cf1cb has been approved by tmandry

@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 Nov 13, 2020
@richkadel
Copy link
Contributor Author

@bors r=tmandry rollup=always

@bors
Copy link
Contributor

bors commented Nov 13, 2020

📌 Commit b4b0ef3 has been approved by tmandry

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Nov 13, 2020
…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.
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Nov 13, 2020
…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.
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Nov 14, 2020
…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.
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 15, 2020
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
@bors bors merged commit 335a255 into rust-lang:master Nov 15, 2020
@rustbot rustbot added this to the 1.50.0 milestone Nov 15, 2020
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.

7 participants