[BOLT] Make BinaryFunction::eraseInvalidBBs() thread-safe

[maksfb]

Summary

Calculation of the size of a basic block is not thread-safe, hence add a
lock.

Additionally, free CFG-related members after the deletion.

[yota9]

Is there a reason now to keep the DeletedBasicBlocks list at all?

[maksfb]

In D119674#3317894, @yota9 wrote:

Is there a reason now to keep the DeletedBasicBlocks list at all?

These blocks are still referenced from BasicBlockOffsets that is used for debug info updates, so we cannot delete them until the function is destroyed.

[yota9]

@maksfb I see, thanks for explanation!

[yota9]

bolt/lib/Core/BinaryFunction.cpp

352

Maybe move lock under estimateSize()?

[Amir]

bolt/lib/Core/BinaryFunction.cpp

352

BB->estimateSize is called from two more places:

• ExtTSPReorderAlgorithm
• ReorderAlgorithm

which operate with an IndependentCodeEmitter and don't need locks.

[yota9]

bolt/lib/Core/BinaryFunction.cpp

352

Than it might be lock under condition. I just feel that it is not the best place. The estimateSize might be used in other places, e.g. the passes like golang support and longjmp. And we will need to think about locking each time in each place. Anyway I don't insist on this :)

bolt/include/bolt/Core/BinaryFunction.h

private:
   /// disassembled state was later invalidated.
   void clearDisasmState();
 
   /// Release memory allocated for CFG and instructions.
   /// We still keep basic blocks for address translation/mapping purposes.
   void releaseCFG() {
     for (BinaryBasicBlock *BB : BasicBlocks)
       BB->releaseCFG();
-    for (BinaryBasicBlock *BB : DeletedBasicBlocks)
-      BB->releaseCFG();
 
     clearList(CallSites);
     clearList(ColdCallSites);
     clearList(LSDATypeTable);
     clearList(LSDATypeAddressTable);
 
     clearList(LabelToBB);
 

bolt/lib/Core/BinaryFunction.cpp

std::pair<unsigned, uint64_t> BinaryFunction::eraseInvalidBBs() {
   unsigned Count = 0;
   uint64_t Bytes = 0;
   for (BinaryBasicBlock *BB : layout()) {
     if (BB->isValid()) {
       NewLayout.push_back(BB);
     } else {
       assert(!isEntryPoint(*BB) && "all entry blocks must be valid");
       ++Count;
-      Bytes += BC.computeCodeSize(BB->begin(), BB->end());
+      if (BB->size()) {
+        std::unique_lock<std::shared_timed_mutex> Lock(BC.CtxMutex);
+        Bytes += BB->estimateSize();
+      }
     }
   }
   BasicBlocksLayout = std::move(NewLayout);
 
   BasicBlockListType NewBasicBlocks;
   for (auto I = BasicBlocks.begin(), E = BasicBlocks.end(); I != E; ++I) {
     BinaryBasicBlock *BB = *I;
     if (BB->isValid()) {
       NewBasicBlocks.push_back(BB);
     } else {
       // Make sure the block is removed from the list of predecessors.
       BB->removeAllSuccessors();
       DeletedBasicBlocks.push_back(BB);
     }
   }
   BasicBlocks = std::move(NewBasicBlocks);
 
   assert(BasicBlocks.size() == BasicBlocksLayout.size());
 
   // Update CFG state if needed
   if (Count > 0)
     recomputeLandingPads();
 
+  for (BinaryBasicBlock *BB : DeletedBasicBlocks)
+    BB->releaseCFG();
+
   return std::make_pair(Count, Bytes);
 }
 
 bool BinaryFunction::isForwardCall(const MCSymbol *CalleeSymbol) const {
   // This function should work properly before and after function reordering.
   // In order to accomplish this, we use the function index (if it is valid).
   // If the function indices are not valid, we fall back to the original
   // addresses.  This should be ok because the functions without valid indices