Skip to content

Conversation

shajrawi
Copy link

@shajrawi shajrawi commented Oct 13, 2017

radar rdar://problem/34966696

• Explanation: Fixes an infinite loop. The infinite loop was there for over a year but it got exposed by the following bug fix: rdar://problem/34753633

• Scope of Issue: Fixes an infinite loop in SILCombiner: we should not create a new witness method instruction if it exactly the same as the old one: doing so might cause the transformation to go into an infinite loop: The new method instruction will be added to the Builder’s tracker list. SILCombine, in turn, uses the tracker list to populate the worklist. As such, if we don’t remove the witness method later on in the pass, we are stuck: We will re-create the same instruction and re-populate the worklist with it. This SILCombiner bug was always there, we just never hit it before because we always removed the witness method instruction later on in the transformation (creating the new apply / making the witness instruction dead). Since we now bail on creating new apply instructions, which is the right thing to do as seen in rdar://problem/34753633, it got exposed.

• Origination: This was discovered while debugging rdar://problem/34966696 Swift CI: 2. Swift Source Compatibility Suite (master)

• Risk: Low - The bug fix is a very simple if statement: if we are trying to replace a witness method instruction with itself (exact same conformance and lookup type) bail. Don’t do so. There’s no point in creating the exact same instruction, replacing all uses with the new one, and deleting the old one. The old one should do the job. That way we are not adding a new instruction to the builder’s tracker list.

• Reviewed By: @eeckstein

SILCombiner: fix an infinite loop corner-case
@shajrawi
Copy link
Author

@swift-ci Please test

@tkremenek
Copy link
Member

@swift-ci test source compatibility

@shajrawi shajrawi merged commit e37175d into swift-4.0-branch Oct 13, 2017
@shajrawi shajrawi deleted the cherrypick_silcombine_infinite_loop_fix branch October 13, 2017 03:10
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.

2 participants