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

Make `into` schedule drop for the destination #61430

Open
wants to merge 3 commits into
base: master
from

Conversation

@matthewjasper
Copy link
Contributor

commented Jun 1, 2019

closes #47949

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Jun 1, 2019

r? @petrochenkov

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

@rust-highfive

This comment was marked as outdated.

Copy link
Collaborator

commented Jun 1, 2019

The job x86_64-gnu-llvm-6.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
travis_time:end:2d691168:start=1559404621976588641,finish=1559404623755202286,duration=1778613645
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
$ export GCP_CACHE_BUCKET=rust-lang-ci-cache
$ export AWS_ACCESS_KEY_ID=AKIA46X5W6CZEJZ6XT55
---
[00:47:04]    Compiling rustc_passes v0.0.0 (/checkout/src/librustc_passes)
[00:48:08]    Compiling rustc_codegen_ssa v0.0.0 (/checkout/src/librustc_codegen_ssa)
[00:48:08]    Compiling rustc_save_analysis v0.0.0 (/checkout/src/librustc_save_analysis)
[00:49:10]    Compiling rustc_interface v0.0.0 (/checkout/src/librustc_interface)
[00:49:12] error: internal compiler error: src/librustc_mir/transform/generator.rs:532: Broken MIR: generator contains type passes::ExpansionResult in MIR, but typeck only knows about for<'r, 's, 't0, 't1, 't2, 't3, 't4, 't5, 't6> {std::rc::Rc<rustc::session::Session>, &'r std::rc::Rc<rustc::session::Session>, rustc::session::Session, &'s rustc::session::Session, rustc_metadata::creader::CrateLoader<'t0>, rustc_resolve::ResolverArenas<'t1>, std::result::Result<(syntax::ast::Crate, rustc_resolve::Resolver<'t2>), rustc::util::common::ErrorReported>, rustc::util::common::ErrorReported, fn(std::result::Result<syntax::ast::Crate, rustc::util::common::ErrorReported>) -> rustc_data_structures::box_region::YieldType<std::result::Result<syntax::ast::Crate, rustc::util::common::ErrorReported>, for<'t7, 't8> fn((&'t7 mut rustc_resolve::Resolver<'t8>,))> {passes::BoxedResolver::initial_yield}, fn(rustc::util::common::ErrorReported) -> std::result::Result<syntax::ast::Crate, rustc::util::common::ErrorReported> {std::result::Result::<syntax::ast::Crate, rustc::util::common::ErrorReported>::Err}, std::result::Result<syntax::ast::Crate, rustc::util::common::ErrorReported>, rustc_data_structures::box_region::YieldType<std::result::Result<syntax::ast::Crate, rustc::util::common::ErrorReported>, for<'t7, 't8> fn((&'t7 mut rustc_resolve::Resolver<'t8>,))>, (), syntax::ast::Crate, rustc_resolve::Resolver<'t3>, fn(syntax::ast::Crate) -> std::result::Result<syntax::ast::Crate, rustc::util::common::ErrorReported> {std::result::Result::<syntax::ast::Crate, rustc::util::common::ErrorReported>::Ok}, std::thread::LocalKey<std::cell::Cell<rustc_data_structures::box_region::Action>>, &'t4 std::thread::LocalKey<std::cell::Cell<rustc_data_structures::box_region::Action>>, [closure@<::rustc_data_structures::box_region::box_region_allow_access macros>:5:56: 5:74], rustc_data_structures::box_region::Action, rustc_data_structures::box_region::AccessAction, &'t5 mut (dyn for<'t7, 't8> std::ops::FnMut(&'t7 mut rustc_resolve::Resolver<'t8>) + 't6), rustc_data_structures::box_region::Marker<for<'t7, 't8> fn((&'t7 mut rustc_resolve::Resolver<'t8>,))>, fn(rustc_data_structures::box_region::Marker<for<'t7, 't8> fn((&'t7 mut rustc_resolve::Resolver<'t8>,))>) -> rustc_data_structures::box_region::YieldType<std::result::Result<syntax::ast::Crate, rustc::util::common::ErrorReported>, for<'t7, 't8> fn((&'t7 mut rustc_resolve::Resolver<'t8>,))> {rustc_data_structures::box_region::YieldType::<std::result::Result<syntax::ast::Crate, rustc::util::common::ErrorReported>, for<'t7, 't8> fn((&'t7 mut rustc_resolve::Resolver<'t8>,))>::Accessor}}
[00:49:12]    --> src/librustc_interface/passes.rs:140:49
[00:49:12]     |
[00:49:12] 140 |       let (result, resolver) = BoxedResolver::new(static move || {
[00:49:12]     |  _________________________________________________^
[00:49:12] 141 | |         let sess = &*sess;
[00:49:12] 142 | |         let mut crate_loader = CrateLoader::new(sess, &*cstore, &crate_name);
[00:49:12] 143 | |         let resolver_arenas = Resolver::arenas();
[00:49:12] 164 | |         ExpansionResult::from_owned_resolver(resolver)
[00:49:12] 165 | |     });
[00:49:12]     | |_____^
[00:49:12] 
---
[00:49:12] note: we would appreciate a bug report: https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#bug-reports
[00:49:12] 
[00:49:12] note: rustc 1.37.0-dev running on x86_64-unknown-linux-gnu
[00:49:12] 
[00:49:12] note: compiler flags: -Z external-macro-backtrace -Z unstable-options -Z force-unstable-if-unmarked -C prefer-dynamic -C opt-level=2 -C debuginfo=0 -C prefer-dynamic -C debug-assertions=y -C link-args=-Wl,-rpath,$ORIGIN/../lib --crate-type dylib
[00:49:12] note: some of the compiler flags provided by cargo are hidden
[00:49:12] 
[00:49:12] error: Could not compile `rustc_interface`.
[00:49:12] 
---
travis_time:end:03fda2e0:start=1559407588230531836,finish=1559407588235205295,duration=4673459
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:0be1d98e
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:0785f9bd
travis_time:start:0785f9bd
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:022a6a78
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@Centril

This comment has been minimized.

Copy link
Member

commented Jun 1, 2019

unpack!(block = this.into(&temp, block, expr));

// Attribute drops of the statement's temps to the
// semicolon at the statement's end.

This comment has been minimized.

Copy link
@pnkfelix

pnkfelix Jun 4, 2019

Member

wow, if I'm reading al this right, even though you got rid of all that StatementSpan stuff, you didn't have to change any of the tests that are testing the attribution of drops of a statement's temps to the semicolon at the statements end, right?

That's great.

@pnkfelix

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

@matthewjasper the code changes here look fine to me; do you know what is up with the Travis CI failure, though?

@pnkfelix

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

r=me once travis is happy.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Jun 4, 2019

The job x86_64-gnu-llvm-6.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
travis_time:end:1c0b6090:start=1559669689852908094,finish=1559669785790925895,duration=95938017801
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
$ export GCP_CACHE_BUCKET=rust-lang-ci-cache
$ export AWS_ACCESS_KEY_ID=AKIA46X5W6CZEJZ6XT55
---
travis_time:start:test_codegen
Check compiletest suite=codegen mode=codegen (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
[01:04:55] 
[01:04:55] running 144 tests
[01:04:58] i..iii.....iii..iiii.....i......................i..i.................i.....i..........ii.i..i..i.ii. 100/144
[01:05:00] test result: ok. 114 passed; 0 failed; 30 ignored; 0 measured; 0 filtered out
[01:05:00] 
[01:05:00]  finished in 4.628
[01:05:00] travis_fold:end:test_codegen
---
travis_time:start:test_assembly
Check compiletest suite=assembly mode=assembly (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
[01:05:02] 
[01:05:02] running 9 tests
[01:05:02] iiiiiiiii
[01:05:02] 
[01:05:02]  finished in 0.147
[01:05:02] travis_fold:end:test_assembly

---
travis_time:start:test_debuginfo
Check compiletest suite=debuginfo mode=debuginfo-gdb+lldb (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
[01:05:18] 
[01:05:18] running 122 tests
[01:05:44] .iiiii...i.....i..i...i..i.i.i..i.ii.Fi.i.....i..i....i..........iiii..........i...ii...i.......ii.i 100/122
[01:05:49] note: Run with `RUST_BACKTRACE=1` environment variable to display a backtrace.
[01:05:49] .i.i......iii.i.....ii
[01:05:49] failures:
[01:05:49] 
[01:05:49] 
[01:05:49] ---- [debuginfo-gdb+lldb] debuginfo/generator-locals.rs stdout ----
[01:05:49] NOTE: compiletest thinks it is using GDB without native rust support
[01:05:49] error: compilation failed!
[01:05:49] status: exit code: 101
[01:05:49] status: exit code: 101
[01:05:49] command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/debuginfo/generator-locals.rs" "-Zthreads=1" "--target=x86_64-unknown-linux-gnu" "-C" "prefer-dynamic" "-o" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/debuginfo/generator-locals/a" "-Crpath" "-Zunstable-options" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-g" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/debuginfo/generator-locals/auxiliary.gdb"
[01:05:49] ------------------------------------------
[01:05:49] 
[01:05:49] ------------------------------------------
[01:05:49] stderr:
[01:05:49] stderr:
[01:05:49] ------------------------------------------
[01:05:49] thread 'rustc' panicked at 'assertion failed: `(left != right)`
[01:05:49]   left: `_1`,
[01:05:49]  right: `_1`', src/librustc_mir/transform/generator.rs:99:9
[01:05:49] 
[01:05:49] error: internal compiler error: unexpected panic
[01:05:49] 
[01:05:49] note: the compiler unexpectedly panicked. this is a bug.
[01:05:49] note: the compiler unexpectedly panicked. this is a bug.
[01:05:49] 
[01:05:49] note: we would appreciate a bug report: https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#bug-reports
[01:05:49] 
[01:05:49] note: rustc 1.37.0-dev running on x86_64-unknown-linux-gnu
[01:05:49] 
[01:05:49] note: compiler flags: -Z threads=1 -Z unstable-options -C prefer-dynamic -C rpath
[01:05:49] 
[01:05:49] ------------------------------------------
[01:05:49] 
[01:05:49] 
---
[01:05:49] test result: FAILED. 82 passed; 1 failed; 39 ignored; 0 measured; 0 filtered out
[01:05:49] 
[01:05:49] 
[01:05:49] 
[01:05:49] 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/debuginfo" "--build-base" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/debuginfo" "--stage-id" "stage2-x86_64-unknown-linux-gnu" "--mode" "debuginfo-gdb+lldb" "--target" "x86_64-unknown-linux-gnu" "--host" "x86_64-unknown-linux-gnu" "--llvm-filecheck" "/usr/lib/llvm-6.0/bin/FilTue, 04 Jun 2019 18:42:25 GMT
The command "date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
" exited with 0.
travis_fold:start:after_failure.1
travis_time:start:2264ba10
---
travis_time:end:0af43a91:start=1559673745995971198,finish=1559673746002010947,duration=6039749
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:34b6a4c3
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@matthewjasper matthewjasper force-pushed the matthewjasper:drop-on-into-panic branch from 93422c5 to 4c38f26 Jun 4, 2019

@matthewjasper

This comment has been minimized.

Copy link
Contributor Author

commented Jun 4, 2019

I think that the ICE is fixed now. I've had to change how we are checking generators for the transform. This should not affect any generator layout. cc @rust-lang/compiler

Since this now seems quite a risky PR:
@bors rollup=never

@Centril

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

@bors p=10

loc: Location) {
match &self.mir[loc.block].terminator().kind {
TerminatorKind::DropAndReplace { location: _, .. } => {
debug_assert!(false, "Generator transform should be run after drop elaboration");

This comment has been minimized.

Copy link
@eddyb

eddyb Jun 4, 2019

Member

Let's not ever do this in the compiler, if the condition is simple enough we should use span_bug!.

}
TerminatorKind::Drop { location, .. } => {
if let Place::Base(PlaceBase::Local(l)) = *location {
sets.kill(l);

This comment has been minimized.

Copy link
@eddyb

eddyb Jun 4, 2019

Member

cc @RalfJung I thought you said we shouldn't disallow capture during, and reinitialization after, Drop?

This comment has been minimized.

Copy link
@Zoxc

Zoxc Jun 5, 2019

Contributor

Yes. Only StatementKind::StorageDead should kill storage.

This comment has been minimized.

Copy link
@RalfJung

RalfJung Jun 5, 2019

Member

Agreed with @Zoxc.

/// It's possible that something like the following could happen:
///
/// let mut x = String::new();
/// let p = &mut x as *mut String;

This comment has been minimized.

Copy link
@eddyb

eddyb Jun 4, 2019

Member

I prefer "maybe borrowed up to here" or even "ever borrowed" be part of the analysis, as a lot of locals never are.

// We can have multiple assignments to the
// destination, so we cannot schedule drop after
// each assignment. We have to rely on drop
// elaboration for this.

This comment has been minimized.

Copy link
@eddyb

eddyb Jun 4, 2019

Member

I see this comment in a few places but I am confused by it, hopefully answering these can help:

  • "we can have multiple assignments" where? maybe talk about what can contain them?
  • "so we cannot" doesn't explain why we're scheduling a drop here. maybe saying "so we're doing X instead" would be clearer
  • and what's the "this" we rely on drop elaboration for? removing noop drops? making drops conditional? how does it relate to everything above?
/// this ends up the same as it's ever been used. If we add a way to
/// reference uninitialized variables then that would also `gen` this.
/// This would also be a problem if we ran the generator transform after
/// deaggregation.

This comment has been minimized.

Copy link
@Zoxc

Zoxc Jun 5, 2019

Contributor

MIR certainly has a way to reference uninitialized variables and we do want to run the generator transform after deaggregation.

@Zoxc

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2019

@bors r-

@bors

This comment has been minimized.

Copy link
Contributor

commented Jun 6, 2019

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

/// is assumed to be uninitialized.
/// Compile `expr`, storing the result into `destination`, which is assumed
/// to be uninitialized.
/// If a `drop_scop` is provided, `destination` is scheduled to be dropped

This comment has been minimized.

Copy link
@tmandry

tmandry Jun 6, 2019

Contributor

nit sp: drop_scope

@jonas-schievink

This comment has been minimized.

Copy link
Member

commented Jun 22, 2019

Triage ping @matthewjasper, a few review comments need to be addressed and merge conflicts need to be fixed

@matthewjasper

This comment has been minimized.

Copy link
Contributor Author

commented Jun 22, 2019

This is blocked on #61872 for now

@pnkfelix

This comment has been minimized.

Copy link
Member

commented Jul 10, 2019

I'll be going on leave for two months, so I'm going to reassign this to @matthewjasper so that they know that they need to find a new official reviewer to take point on this.

@pnkfelix pnkfelix assigned matthewjasper and unassigned pnkfelix Jul 10, 2019

};
}

fn uninitilized_local_is_not_live_across_yield(c: bool) {

This comment has been minimized.

Copy link
@lqd

lqd Jul 10, 2019

Contributor

tiny typo

Suggested change
fn uninitilized_local_is_not_live_across_yield(c: bool) {
fn uninitialized_local_is_not_live_across_yield(c: bool) {
@joelpalmer

This comment has been minimized.

Copy link

commented Jul 22, 2019

Ping from Triage, hi @matthewjasper - any updates?

@Dylan-DPC

This comment has been minimized.

Copy link
Member

commented Jul 22, 2019

can anyone from @rust-lang/compiler take over this PR? (need a reviewer but not blocked on a review)

@tmandry

This comment has been minimized.

Copy link
Contributor

commented Jul 22, 2019

It looks like there were still some significant changes planned for this PR; see #59123 (comment)

@Dylan-DPC

This comment has been minimized.

Copy link
Member

commented Jul 22, 2019

Yes but we still need a reviewer

@oli-obk

This comment has been minimized.

Copy link
Contributor

commented Jul 23, 2019

@hdhoang

This comment has been minimized.

Copy link
Contributor

commented Aug 2, 2019

Ping from Triage @matthewjasper, any update on this PR?

@JohnCSimon JohnCSimon added S-blocked and removed S-blocked labels Aug 10, 2019

@JohnCSimon

This comment has been minimized.

Copy link

commented Aug 10, 2019

Ping from triage - @oli-obk - this hasn't moved since you'd assigned yourself 18 days ago.
Thanks.

@matthewjasper

This comment has been minimized.

Copy link
Contributor Author

commented Aug 15, 2019

Just to provide a quick update. I'm still working on this, and I think I have a way to generate drops on precisely the paths where the value has been initialized. that I'm happy with. It will require some more refactoring of the MIR drop generation code that I'll probably pull out into a separate PR. I'll try to get that PR ready this weekend.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.