Fix stack overflow when finding blanket impls #56722
Merged
+117
−38
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
@nikomatsakis: Are there any changes that you'd like me to make? |
|
@Aaron1011 sorry, I haven't really reviewed this yet. I actually have it on my calendar to do later today (around 4pm if I stick to my schedule =) so I'll let you know! I'm trying hard to catch up, but conferences and other things have left me with quite a backlog. :( |
There was a problem hiding this comment.
The code makes sense, but I can't escape the feeling that the bug is a failure to incremental the recursion depth in the recursive obligations, and not that we need a distinct recursion tracker for evaluation. Do you have ready access to the stack backtrace that causes the overflow in the example? (I can do a local build, too)
src/librustc/traits/select.rs
Outdated
| @@ -661,14 +665,15 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> { | |||
| &mut self, | |||
| stack: TraitObligationStackList<'o, 'tcx>, | |||
| predicates: I, | |||
| recursion_depth: usize | |||
There was a problem hiding this comment.
can we call this the evaluation_depth or something like that?
It also feels like there should be a comment that links to the explanation below of why we have a distinct recursion depth tracking in use here.
src/librustc/traits/select.rs
Outdated
| @@ -719,7 +742,8 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> { | |||
| obligation.cause.span, | |||
| ) { | |||
| Some(obligations) => { | |||
| self.evaluate_predicates_recursively(previous_stack, obligations.iter()) | |||
| self.evaluate_predicates_recursively(previous_stack, obligations.iter(), | |||
There was a problem hiding this comment.
Still, I feel like the intention here was that the predicates we evaluate recursively would have a recursion depth that was incremented relative to obligation.recursion_depth -- but there are clearly some cases where that's not true. Perhaps that is really the bug?
There was a problem hiding this comment.
I think you're right - it should be possible to increment the recursion depth of the obligations before passing them to evaluate_predicates_recursively, and check that recursion depth at the start of evaluate_predicates_recursively.
|
@nikomatsakis: Here's the bottom part of the stack trace: https://gist.github.com/Aaron1011/6ee3bab4a34d6ef895cfb19d25bcd36c |
Aaron1011
force-pushed
the
fix/blanket-eval-overflow
branch
from
c0f4080
to
9af9d8b
Compare
|
@nikomatsakis: I've modified the PR to update the depth on nested obligations, instead of passing a separate |
|
The job Click to expand the log.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 |
|
It looks as though the new overflow check is getting hit before the old one, resulting in slightly worse error messages on overflow. I'll investigate more later today. |
nikomatsakis
added
S-waiting-on-author
|
@Aaron1011 did you ever get a chance to investigate? |
Aaron1011
force-pushed
the
fix/blanket-eval-overflow
branch
from
75b9e60
to
6c8398b
Compare
|
@nikomatsakis: Sorry for the delay. I ended up chaging the
I don't think that's it's possible to avoid these UI changes without significantly complicating the implementation. Since they only represent slight modifications to existing error messages, I think it should be fine. |
|
The job Click to expand the log.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 |
nikomatsakis
requested changes
Jan 3, 2019
src/librustc/traits/select.rs
Outdated
|
|
||
| match obligation.predicate { | ||
| ty::Predicate::Trait(ref t) => { | ||
| debug_assert!(!t.has_escaping_bound_vars()); | ||
| let obligation = obligation.with(t.clone()); | ||
| let mut obligation = obligation.with(t.clone()); | ||
| obligation.recursion_depth += 1; |
There was a problem hiding this comment.
I don't think we need to increase the recursion depth here -- this is just translating the predicate from one form to another.
There was a problem hiding this comment.
I believe it is necessary here. Consider the following call stack:
evaluate_predicate_recursivelyevaluate_trait_predicate_recursivelyevaluate_stackevaluate_candidateevaluate_predicates_recursivelyevaluate_predicate_recursively
Here, we've recursed back to evaluate_predicate_recursively without ever incrementing the recursion depth. To ensure that repeatedly recursing through the ty::Predicate::Trait match arm doesn't lead to a stack overflow, we need to increment the depth here.
There was a problem hiding this comment.
I think that there should be an increase in between steps 4 and 5. In particular, the "nested obligations" returned by evaluate_candidate ought to be at a recursion level one higher than the candidate that spawned them.
| @@ -813,10 +827,11 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> { | |||
| ty::Predicate::Projection(ref data) => { | |||
| let project_obligation = obligation.with(data.clone()); | |||
| match project::poly_project_and_unify_type(self, &project_obligation) { | |||
| Ok(Some(subobligations)) => { | |||
| Ok(Some(mut subobligations)) => { | |||
| self.add_depth(subobligations.iter_mut(), obligation.recursion_depth); | |||
src/librustc/traits/select.rs
Outdated
| @@ -1155,6 +1170,38 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> { | |||
| .insert(trait_ref, WithDepNode::new(dep_node, result)); | |||
| } | |||
|
|
|||
| // Due to caching of projection results, it's possible for a subobligation | |||
There was a problem hiding this comment.
I don't really think this is the reason why. It's more that things like subtype and wf don't take the "current recursion depth" as input and seem to just produce obligations at recursion depth 0, right? I would describe this function as a "workaround" for that scenario (which strikes me as odd but prob not worth changing in this PR)
There was a problem hiding this comment.
I was referring to the ty::Predicate::Projection match arm, where we call poly_project_and_unify_type. If the projection result is cached, the subobligations will have an 'old' recursion_depth.
I'll update the comment to mention that several different factors create the need to ensure that our recursion_depth doesn't decrease.
|
☔ The latest upstream changes (presumably #55517) made this pull request unmergeable. Please resolve the merge conflicts. |
Currently, SelectionContext tries to prevent stack overflow by keeping track of the current recursion depth. However, this depth tracking is only used when performing normal section (which includes confirmation). No such tracking is performed for evaluate_obligation_recursively, which can allow a stack overflow to occur. To fix this, this commit tracks the current predicate evaluation depth. This is done separately from the existing obligation depth tracking: an obligation overflow can occur across multiple calls to 'select' (e.g. when fulfilling a trait), while a predicate evaluation overflow can only happen as a result of a deep recursive call stack. Fixes rust-lang#56701
Aaron1011
force-pushed
the
fix/blanket-eval-overflow
branch
from
c2747f2
to
726bdec
Compare
|
@nikomatsakis: Are there any other changes you'd like me to make? |
|
@Aaron1011 will look shortly, trying to keep up with reviews, sorry =) |
|
Er, huh, I can't figure out the GH display. |
|
I am referring specifically to this comment, but let's take the replies out of that comment thread, since that is always kind of confusing. An example of where rust/src/librustc/traits/select.rs Lines 2760 to 2767 in 14ea6e5 |
|
@nikomatsakis: You were right! I've removed the uncessary increment (which also made some of the UI tests more closely resemble the current output). |
nikomatsakis
approved these changes
Jan 18, 2019
|
@bors r+ |
|
📌 Commit 9b68dcd has been approved by |
bors
added
S-waiting-on-bors
bors
added a commit
that referenced
this pull request
Jan 19, 2019
…sakis Fix stack overflow when finding blanket impls Currently, SelectionContext tries to prevent stack overflow by keeping track of the current recursion depth. However, this depth tracking is only used when performing normal section (which includes confirmation). No such tracking is performed for evaluate_obligation_recursively, which can allow a stack overflow to occur. To fix this, this commit tracks the current predicate evaluation depth. This is done separately from the existing obligation depth tracking: an obligation overflow can occur across multiple calls to 'select' (e.g. when fulfilling a trait), while a predicate evaluation overflow can only happen as a result of a deep recursive call stack. Fixes #56701 I've re-used `tcx.sess.recursion_limit` when checking for predication evaluation overflows. This is such a weird corner case that I don't believe it's necessary to have a separate setting controlling the maximum depth.
|
☀️ Test successful - checks-travis, status-appveyor |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, SelectionContext tries to prevent stack overflow by keeping
track of the current recursion depth. However, this depth tracking is
only used when performing normal section (which includes confirmation).
No such tracking is performed for evaluate_obligation_recursively, which
can allow a stack overflow to occur.
To fix this, this commit tracks the current predicate evaluation depth.
This is done separately from the existing obligation depth tracking:
an obligation overflow can occur across multiple calls to 'select' (e.g.
when fulfilling a trait), while a predicate evaluation overflow can only
happen as a result of a deep recursive call stack.
Fixes #56701
I've re-used
tcx.sess.recursion_limitwhen checking for predication evaluation overflows. This is such a weird corner case that I don't believe it's necessary to have a separate setting controlling the maximum depth.