Skip to content

Fix issue #34101 #34109

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 1 commit into from
Jun 9, 2016
Merged

Fix issue #34101 #34109

merged 1 commit into from
Jun 9, 2016

Conversation

pnkfelix
Copy link
Member

@pnkfelix pnkfelix commented Jun 6, 2016

Fix issue #34101: do not track subcontent of type with dtor nor gather flags for untracked content.

(Includes a regression test, which needed to go into compile-fail/
due to weaknesses when combining #[deny(warnings)] with
tcx.sess.span_warn(..))

@rust-highfive
Copy link
Contributor

r? @jroesch

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

@pnkfelix
Copy link
Member Author

pnkfelix commented Jun 6, 2016

r? @arielb1

@rust-highfive rust-highfive assigned arielb1 and unassigned jroesch Jun 6, 2016
@pnkfelix
Copy link
Member Author

pnkfelix commented Jun 6, 2016

This patch was meant to be a somewhat minimal change to the code in question, at the potential cost of wasting compilation time evaluating lvalue_is_tracked on each call to path_needs_drop.

I am assuming that the recursive chain there almost never gets terribly long in most (human-generated) code. But its worth keeping an eye on, and I'm open to alternative suggestions.

@arielb1
Copy link
Contributor

arielb1 commented Jun 6, 2016

@pnkfelix

I would rather have that logic inside is_terminal rather than path_needs_drop. That's what I added it for.

Maybe also add a soft assertion that all non-DropAndReplace drops are of tracked data?

r+ modulo that

///
/// Lvalues behind ADT's with a Drop impl are not tracked by
/// elaboration since they can never have a drop-flag state that
/// differs fom that of the parent with the Drop impl.
Copy link
Contributor

Choose a reason for hiding this comment

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

*differs from

@pnkfelix
Copy link
Member Author

pnkfelix commented Jun 6, 2016

@arielb1 oh good: I think moving the logic into fn is_terminal_path allows me to avoid adding the recursive walk up the parent chain that I was concerned about.

…nor gather flags for untracked content.

(Includes a regression test, which needed to go into `compile-fail/`
due to weaknesses when combining `#[deny(warnings)]` with
`tcx.sess.span_warn(..)`)

(updated with review feedback from arielb1.)
@pnkfelix
Copy link
Member Author

pnkfelix commented Jun 7, 2016

@bors r=arielb1

@bors
Copy link
Collaborator

bors commented Jun 7, 2016

📌 Commit 4b6a68e has been approved by arielb1

@pnkfelix
Copy link
Member Author

pnkfelix commented Jun 7, 2016

hmm actually I think this patch might break -Z orbit

@pnkfelix
Copy link
Member Author

pnkfelix commented Jun 7, 2016

In particular:

run-pass/issue-4401.rs regresses (when compiled without -O):

% rustc -Z orbit ../src/test/run-pass/issue-4401.rs
% ./issue-4401 
999999
% ./x86_64-apple-darwin/stage1/bin/rustc -Z orbit ../src/test/run-pass/issue-4401.rs
% ./issue-4401 

thread 'main' has overflowed its stack
fatal runtime error: stack overflow
Abort trap: 6
% 

(I also saw a failure associated with run-pass/issue-28950.rs, but that seems to also segfault without this PR under -Z orbit, so I'm ignoring it for now...)

@pnkfelix
Copy link
Member Author

pnkfelix commented Jun 7, 2016

ah, problem appears fixed when I rebase atop PR #34128 ; crisis averted, thanks @eddyb !

Manishearth added a commit to Manishearth/rust that referenced this pull request Jun 8, 2016
Fix issue rust-lang#34101

Fix issue rust-lang#34101: do not track subcontent of type with dtor nor gather flags for untracked content.

(Includes a regression test, which needed to go into `compile-fail/`
due to weaknesses when combining `#[deny(warnings)]` with
`tcx.sess.span_warn(..)`)
@bors
Copy link
Collaborator

bors commented Jun 9, 2016

⌛ Testing commit 4b6a68e with merge 33c8992...

bors added a commit that referenced this pull request Jun 9, 2016
Fix issue #34101

Fix issue #34101: do not track subcontent of type with dtor nor gather flags for untracked content.

(Includes a regression test, which needed to go into `compile-fail/`
due to weaknesses when combining `#[deny(warnings)]` with
`tcx.sess.span_warn(..)`)
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants