[SimplifyCFG] Remove pointer from SmallPtrSet before deletion

[NutshellySima]

Summary

Previously, eraseFromParent() calls delete which invalidates the value of the pointer. Copying the value of the pointer later is undefined behavior in C++11 and implementation-defined (which may cause a segfault on implementations having strict pointer safety) in C++14.

This patch removes the BasicBlock pointer from related SmallPtrSet before delete invalidates it in the SimplifyCFG pass.

[dmgreen]

LGTM

I guess because it's only erasing it from a SmallPtrSet, it's not actually _using_ it, but this looks like a very sensible change.

[NutshellySima]

In D50717#1199198, @dmgreen wrote:

LGTM

I guess because it's only erasing it from a SmallPtrSet, it's not actually _using_ it, but this looks like a very sensible change.

Thanks for pointing it out. I didn't notice that. :)

[kuhar]

In D50717#1199198, @dmgreen wrote:

I guess because it's only erasing it from a SmallPtrSet, it's not actually _using_ it, but this looks like a very sensible change.

I thought that erase calls delete on the link list node which invalidates not only the node itself but also the pointer, doesn't it?

[dmgreen]

I'm no expert, but I thought that SmallPtrSet->erase(BB) would treat BB as an opaque value that it removes from it's set, not accessing it's content in any way. You may be right about the eraseFromParent invalidating things though.

It's certainly much better this new way :)

[kuhar]

In D50717#1199288, @dmgreen wrote:

I'm no expert, but I thought that SmallPtrSet->erase(BB) would treat BB as an opaque value that it removes from it's set, not accessing it's content in any way. You may be right about the eraseFromParent invalidating things though.

It's certainly much better this new way :)

I found this (C++14 [basic.stc.dynamic.deallocation]/4):

Indirection through an invalid pointer value and passing an invalid pointer value to a deallocation function have undefined behavior. Any other use of an invalid pointer value has implementation-defined behavior.

[ Footnote: Some implementations might define that copying an invalid pointer value causes a system-generated runtime fault. — end footnote ]

Seems like erasing from a set can cause both a copy (possibly a segfault) and produce unexpected values when used in comparison with other pointers.

llvm/trunk/lib/Transforms/Utils/SimplifyCFG.cpp

bool SimplifyCFGOpt::SimplifySingleResume(ResumeInst *RI) {
 
   // Turn all invokes that unwind here into calls and delete the basic block.
   for (pred_iterator PI = pred_begin(BB), PE = pred_end(BB); PI != PE;) {
     BasicBlock *Pred = *PI++;
     removeUnwindEdge(Pred);
   }
 
   // The landingpad is now unreachable.  Zap it.
-  BB->eraseFromParent();
   if (LoopHeaders)
     LoopHeaders->erase(BB);
+  BB->eraseFromParent();
   return true;
 }
 
 static bool removeEmptyCleanup(CleanupReturnInst *RI) {
   // If this is a trivial cleanup pad that executes no instructions, it can be
   // eliminated.  If the cleanup pad continues to the caller, any predecessor
   // that is an EH pad will be updated to continue to the caller and any
   // predecessor that terminates with an invoke instruction will have its invoke
while (!UncondBranchPreds.empty()) {
       LLVM_DEBUG(dbgs() << "FOLDING: " << *BB
                         << "INTO UNCOND BRANCH PRED: " << *Pred);
       (void)FoldReturnIntoUncondBranch(RI, BB, Pred);
     }
 
     // If we eliminated all predecessors of the block, delete the block now.
     if (pred_empty(BB)) {
       // We know there are no successors, so just nuke the block.
-      BB->eraseFromParent();
       if (LoopHeaders)
         LoopHeaders->erase(BB);
+      BB->eraseFromParent();
     }
 
     return true;
   }
 
   // Check out all of the conditional branches going to this return
   // instruction.  If any of them just select between returns, change the
   // branch itself into a select/return pair.
if (auto *BI = dyn_cast<BranchInst>(TI)) {
       TI->eraseFromParent();
       Changed = true;
     }
   }
 
   // If this block is now dead, remove it.
   if (pred_empty(BB) && BB != &BB->getParent()->getEntryBlock()) {
     // We know there are no successors, so just nuke the block.
-    BB->eraseFromParent();
     if (LoopHeaders)
       LoopHeaders->erase(BB);
+    BB->eraseFromParent();
     return true;
   }
 
   return Changed;
 }
 
 static bool CasesAreContiguous(SmallVectorImpl<ConstantInt *> &Cases) {
   assert(Cases.size() >= 1);