Skip to content

Conversation

@xedin
Copy link
Contributor

@xedin xedin commented Dec 9, 2016

Since retired constraints are re-added back to the circulation in LIFO
order, make sure that all of the constraints are added to the front of
SolverScope::retiredConstraints list.

@xedin
Copy link
Contributor Author

xedin commented Dec 9, 2016

/cc @rudkx @DougGregor

@xedin xedin changed the title [TypeChecker] Add retired constraints to the front of the list in SolverState [TypeChecker] Add retired constraints to the front of the list in SolverScope Dec 9, 2016
@DougGregor
Copy link
Member

I don't understand how it is that this resolves a crash?

@xedin
Copy link
Contributor Author

xedin commented Dec 10, 2016

@DougGregor The reason why it helps is because crash happens in salvage where the last SolverScope (created by solveRec) have been deallocated and all retired constraints should be returned but since in removeInactiveConstraint I'm calling push_back instead of push_front disjunction generated by optimizeConstraints has not returned to the InactiveConstraints - since constraints are re-inserted in LIFO order - and, at the same time, was returned to the graph... I've also went through all of the usages of retiredConstraints and made sure that everything is using push_front.

@rudkx
Copy link
Contributor

rudkx commented Dec 11, 2016

Yeah the basic problem is that in ~SolverScope we restore the retiredConstraints to where they were prior to this scope, but that code assumes new retired constraints are pushed on the front of the list.

It would be good to clean this up to refactor into clearly documented functions that capture the current state, restore the previous current state, and add newly retired constraints so that all the logic is kept together in one place. Otherwise this is just too easy to get wrong.

@xedin Would you mind taking a crack at that? It doesn't have to be anything fancy - just something that makes it less likely to put constraints at the wrong end of the list in the future.

@xedin
Copy link
Contributor Author

xedin commented Dec 11, 2016

@rudkx Absolutely!

…verScope

Since retired constraints are re-added back to the circulation in LIFO
order, make sure that all of the constraints are added to the front of
SolverScope::retiredConstraints list.
…generated constraints

Instead of relying on the SolverScope to rollback all of the changes
done to constraints in the system at the end of its life time, move
all of the logic handling that into SolverState where retired/generated
constraints live.
@xedin
Copy link
Contributor Author

xedin commented Dec 12, 2016

@rudkx Done! I've abstracted coupling between state and scope into register/rollback sequence, so state is now responsible for actual rollback logic, and added getter/setter methods to retired/generated constraints.

@rudkx
Copy link
Contributor

rudkx commented Dec 12, 2016

@swift-ci Please smoke test

Copy link
Contributor

@rudkx rudkx left a comment

Choose a reason for hiding this comment

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

Awesome, LGTM!

@rudkx rudkx merged commit ef94156 into swiftlang:master Dec 12, 2016
@DougGregor
Copy link
Member

I understand now, thank you for the fix (and the education)!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants