Add MIR Validate statement

[RalfJung]

This adds statements to MIR that express when types are to be validated (following Types as Contracts). Obviously nothing is stabilized, and in fact a -Z flag has to be passed for behavior to even change at all.

This is meant to make experimentation with Types as Contracts in miri possible. The design is definitely not final.

Cc @nikomatsakis@aturon

[rust-highfive]

r? @pnkfelix

(rust_highfive has picked a reviewer for you, use r? to override)

[oli-obk]

Tidy fails due to lines longer than 100 chars

[RalfJung]

Yeah I find it very hard to follow that requirement. Always feel like I have to make my code ugly to comply. Seems like I missed some places. :/

[nikomatsakis]

r? @nikomatsakis

[RalfJung]

Ideally I'd like to run AddValidation before ElaborateDrops and after EraseRegions. For this to work, I need to move ElaborateDrops up, but RalfJung@3eb4141 fails badly -- many tests now report "Broken MIR: moving out of lvalue".

@arielb1 I suppose just naively moving the pass around doesn't work.^^ Anything else I should try?

EDIT: Also I wasn't sure about the comments saying what cannot be done starting where. Those are probably wrong in my patch.

[RalfJung]

With some more help from @arielb1 I managed to reorder the passes without breaking everything (well, the test suite still passes). They also advised me to change move_path::abs_domain to do erasure, which was not actually necessary. Unfortunately, doing region erasure requires access to the TyCtxt, so this commit touches lots of other code as well. For MovePathLookup::find where no TyCtxt was available, I was not sure whether it should be added to the struct (so all methods just have it) or as a new parameter to the functions that need it -- I now went with a new parameter.

[RalfJung]

This relies on #43512, which however is already r+'ed so it should land soon-ish.

[bors]

☔ The latest upstream changes (presumably #43324) made this pull request unmergeable. Please resolve the merge conflicts.

[RalfJung]

I rebased to resolve the merge conflict.

[RalfJung]

Still failing to build libstd with the flag turned on:

error: internal compiler error: src/librustc_mir/transform/add_validation.rs:142: Expected function, method or closure, found struct_ctor mem::Discriminant (id=12499)

I am at loss here. Struct ctors are functions that have their own MIR...?

[arielb1]

I am at loss here. Struct ctors are functions that have their own MIR...?

Yes they are. The MIR is created by https://github.com/rust-lang/rust/blob/master/src/librustc_mir/shim.rs

[RalfJung]

I see. I fixed the code to handle that, but now I get

error: internal compiler error: src/librustc_mir/transform/add_validation.rs:146: Expected function, method or closure, found trait method ne in cmp::PartialEq::ne (id=15796)

I suppose defaulted trait methods are yet another category that needs separate handling.

[RalfJung]

Okay, now I am seeing really strange behavior... I thought by returning OnlyBodies to the intravisitor, I would only visit one function at a time, but that does not seem to be the case. In libcore/slice/sort.rs, around line 567, I see a backtrace like so:

 8: <rustc_mir::transform::add_validation::fn_contains_unsafe::FindUnsafe<'b, 'tcx> as rustc::hir::intravisit::Visitor<'tcx>>::visit_fn
9: rustc::hir::intravisit::walk_expr
10: rustc::hir::intravisit::walk_block
11: rustc::hir::intravisit::walk_expr
12: rustc::hir::intravisit::walk_expr
13: rustc::hir::intravisit::walk_block
14: rustc::hir::intravisit::walk_expr
15: <rustc_mir::transform::add_validation::fn_contains_unsafe::FindUnsafe<'b, 'tcx> as rustc::hir::intravisit::Visitor<'tcx>>::visit_fn
16: rustc::hir::intravisit::walk_item
17: rustc_mir::transform::add_validation::fn_contains_unsafe
18: <rustc_mir::transform::add_validation::AddValidation as rustc::mir::transform::MirPass>::run_pass

I also just realized that closures in an unsafe fn probably can use unsafe features as well, so I will have to extend my scan into the closure environment accordingly.

[RalfJung]

Okay, it seems to all work now. This is ready to be merged from my side. I hope I got all this HIR code right.

I would really like to test whether closures are correctly detected to be unsafe, but unfortunately, mir-opt tests are not suited for this: Closures contain the filename in their type and name, which shows up in the lines we would like to test for.

[RalfJung]

Ah dang it, I got another ICE in miri's test suite...

error: internal compiler error: src/librustc/hir/map/mod.rs:537: couldn't find node id 0 in the AST map

[RalfJung]

Fixed that as well (thanks @eddyb)

[RalfJung]

Thanks :)

So, what has to happen now to get r+?

[nikomatsakis]

@bors r+

[bors]

📌 Commit 7d8dc7a has been approved by nikomatsakis

[bors]

⌛ Testing commit 7d8dc7a with merge c523b3f...

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing c523b3f to master...

[alexcrichton]

It looks like this PR regressed a number of benchmarks 5-7% on perf.rust-lang.org, @RalfJung do you know if this could perhaps cause a performance impact when not enabled?

[alexcrichton]

A more clear way to see this regression is perhaps here

[RalfJung]

Is this compile tune or run time? I guess compile time, but it's not entirely clear to me.

That is strange, the new pass does nothing when not enabled. There is a slight chance that more EndRegion are left around briefly because CleanEndRegions is a little more cautious, but they will all be gone by the time EraseRegions has happened.

@arielb1 could reordering the passes have a bad effect on performance?

[kennytm]

@RalfJung Compile time. In particular it seems to be the LLVM pass that takes longer time. Trans time is also increasing but the contribution to increased total time is small.

[alexcrichton]

Ok I'll open a dedicated issue for this in that case!

#43827