[PATCH] D14454: [WinEH] Fix mutli-parent cloning

[Yaron Keren via llvm-commits]

Hi Andy,

The change from std::set to SetVector gains stable iteration order but
loses stable iterators...
Specifically, it invalidates BlockColors[BB].end() after every remove(), so
it should not be cached in the End variable. Even if we solve this by
testing It != BlockColors[BB].end(), after removing the last item 'It' will
be at the (new after removal) end()+1 and then compared to the (new after
removal) end() which is probably undefined behaviour for an iterator.
Finally, erasing items in a loop in a SetVector is quadratic complexity.

To solve all at once, we could split the loop into two steps:

        for (BasicBlock *ContainingFunclet : BlockColors[BB])
          if (ContainingFunclet != CorrectColor)
            FuncletBlocks[ContainingFunclet].erase(BB);
        BlockColors[BB].remove_if([&](BasicBlock *ContainingFunclet) {
          return ContainingFunclet != CorrectColor;
        });

Please feel free to modify the attached patch and commit,

Yaron
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20151111/25db1c0c/attachment.html>
-------------- next part --------------
Index: lib/CodeGen/WinEHPrepare.cpp
===================================================================
--- lib/CodeGen/WinEHPrepare.cpp	(revision 252742)
+++ lib/CodeGen/WinEHPrepare.cpp	(working copy)
@@ -1665,18 +1665,12 @@
 
         // Remove this block from the FuncletBlocks set of any funclet that
         // isn't the funclet whose color we just selected.
-        for (auto It = BlockColors[BB].begin(), End = BlockColors[BB].end();
-             It != End; ) {
-          // The iterator must be incremented here because we are removing
-          // elements from the set we're walking.
-          auto Temp = It++;
-          BasicBlock *ContainingFunclet = *Temp;
-          if (ContainingFunclet != CorrectColor) {
+        for (BasicBlock *ContainingFunclet : BlockColors[BB])
+          if (ContainingFunclet != CorrectColor)
             FuncletBlocks[ContainingFunclet].erase(BB);
-            BlockColors[BB].remove(ContainingFunclet);
-          }
-        }
-
+        BlockColors[BB].remove_if([&](BasicBlock *ContainingFunclet) {
+          return ContainingFunclet != CorrectColor;
+        });
         // This should leave just one color for BB.
         assert(BlockColors[BB].size() == 1);
         continue;