Fix a block color copying problem in LICM

[andrew.w.kaylor]

Summary

This fixes a problem in LICM where copying the ColorVector in SafetyInfo::BlockColors from one block to another can cause a crash if creating the new entry forces the map to be re-allocated. I have seen this problem occur in a real-world situation, but I don't have a small reproducer.

[• dberlin]

Sigh.
(a quick scan of things named Map in LLVM doesn't find any other obvious cases of this).

I wonder if we shouldn't have a debug/expensive checks mode where it moves all the memory on find and construct to make all these situations fail obviously and instantly so it could be found by bots.

[andrew.w.kaylor]

In D44817#1046470, @dberlin wrote:

Sigh.
(a quick scan of things named Map in LLVM doesn't find any other obvious cases of this).

I wonder if we shouldn't have a debug/expensive checks mode where it moves all the memory on find and construct to make all these situations fail obviously and instantly so it could be found by bots.

There used to be a case exactly like this in WinEHPrepare::replaceUseWithLoad(), which is how I knew how to fix this when I saw it crash. The debug mode is a good idea. Could that trigger off of one of the sanitizers somehow?

[junbuml]

Wow, thank you so much for fixing this.

llvm/trunk/lib/Transforms/Scalar/LICM.cpp

while (!PredBBs.empty()) {
     assert(CurLoop->contains(PredBB) &&
            "Expect all predecessors are in the loop");
     if (PN->getBasicBlockIndex(PredBB) >= 0) {
       BasicBlock *NewPred = SplitBlockPredecessors(
           ExitBB, PredBB, ".split.loop.exit", DT, LI, true);
       // Since we do not allow splitting EH-block with BlockColors in
       // canSplitPredecessors(), we can simply assign predecessor's color to
       // the new block.
-      if (!BlockColors.empty())
-        BlockColors[NewPred] = BlockColors[PredBB];
+      if (!BlockColors.empty()) {
+        // Grab a reference to the ColorVector to be inserted before getting the
+        // reference to the vector we are copying because inserting the new
+        // element in BlockColors might cause the map to be reallocated.
+        ColorVector &ColorsForNewBlock = BlockColors[NewPred];
+        ColorVector &ColorsForOldBlock = BlockColors[PredBB];
+        ColorsForNewBlock = ColorsForOldBlock;
+      }
     }
     PredBBs.remove(PredBB);
   }
 }
 
 /// When an instruction is found to only be used outside of the loop, this
 /// function moves it to the exit blocks and patches up SSA form as needed.
 /// This method is guaranteed to remove the original instruction from its