This is an archive of the discontinued LLVM Phabricator instance.

[LowerSwitch] Fix a bug when LowerSwitch deletes the default block
ClosedPublic

Authored by chenli on Aug 7 2015, 4:06 PM.

Details

Summary

LowerSwitch crashed with the attached test case after deleting the default block. This happened because the current implementation of deleting dead blocks is wrong. After the default block being deleted, it contains no instruction or terminator, and it should no be traversed anymore. However, since the iterator is advanced before processSwitchInst() function is executed, the block advanced to could be deleted inside processSwitchInst(). The deleted block would then be visited next and crash dyn_cast<SwitchInst>(Cur->getTerminator()) because Cur->getTerminator() returns a nullptr. This patch fixes this problem by recording dead default blocks into a list, and delete them after all processSwitchInst() has been done. It still possible to visit dead default blocks and waste time process them. But it is a compile time issue, and I plan to have another patch to add support to skip dead blocks.

Diff Detail

Event Timeline

chenli updated this revision to Diff 31553.Aug 7 2015, 4:06 PM
chenli retitled this revision from to [LowerSwitch] Fix a bug when LowerSwitch deletes the default block.
chenli updated this object.
chenli added reviewers: reames, hans, kariddi, resistor.
chenli added a subscriber: llvm-commits.
hans added inline comments.Aug 10 2015, 11:11 AM
lib/Transforms/Utils/LowerSwitch.cpp
526–527

I don't think maintaining a DeleteList is very nice, and as you say it has the problem that we might call processSwitchInst on a block even if it's dead.

How about we remove the DeleteDeadBlock call here, and in the loop in runOnFunction(), we add something like:

if (block has no predecessors) {
  delete it
  continue
}
chenli added inline comments.Aug 10 2015, 11:13 AM
lib/Transforms/Utils/LowerSwitch.cpp
526–527

Yes, I think that should work too. Though we need to handle carefully with entry block, which also has no predecessors.

chenli updated this revision to Diff 31735.Aug 10 2015, 3:24 PM

Update patch w.r.t Hans' comments.

hans accepted this revision.Aug 10 2015, 3:54 PM
hans edited edge metadata.

Looks good to me.

This revision is now accepted and ready to land.Aug 10 2015, 3:54 PM
reames edited edge metadata.Aug 10 2015, 7:43 PM

The revised patch is strictly less powerful than the current code and the previous patch. If a block occurs earlier in the iteration order than the only block which reaches it and we then delete the reaching edge, we will now fail to remove the unreachable block. I would prefer the first patch posted over the revised one.

The revised patch is strictly less powerful than the current code and the previous patch. If a block occurs earlier in the iteration order than the only block which reaches it and we then delete the reaching edge, we will now fail to remove the unreachable block. I would prefer the first patch posted over the revised one.

Yes, you are correct. I didn't think about this case. I will submit the initial patch if Hans is ok with that.

hans added a comment.Aug 11 2015, 8:40 AM

The revised patch is strictly less powerful than the current code and the previous patch. If a block occurs earlier in the iteration order than the only block which reaches it and we then delete the reaching edge, we will now fail to remove the unreachable block. I would prefer the first patch posted over the revised one.

Yes, you are correct. I didn't think about this case. I will submit the initial patch if Hans is ok with that.

Sure.

chenli closed this revision.Aug 11 2015, 11:13 AM
This revision was automatically updated to reflect the committed changes.