Disable gcov instrumentation of functions using funclet-based exception handling

[marco-c]

Summary

This patch fixes the crash from https://bugs.llvm.org/show_bug.cgi?id=34659 and https://bugs.llvm.org/show_bug.cgi?id=34833.

[majnemer]

Please upload a diff with full context.

[majnemer]

lib/Transforms/Instrumentation/GCOVProfiling.cpp

637

I think I'd skip BB if TI->IsEHPad(), that would save a bunch of the checks you are doing on BB.

[rnk]

I think any invoke or cleanupret that unwinds to a catchswitch needs to look through the successors of the catchswitch. The unwind edge may be another catchswitch, which we must also look through. We should add all the non-catchswitch successors to ComplexEdgeSuccs.

lib/Transforms/Instrumentation/GCOVProfiling.cpp

642

I think continue is problematic because it fails to update Edge.

[marco-c]

I think any invoke or cleanupret that unwinds to a catchswitch needs to look through the successors of the catchswitch. The unwind edge may be another catchswitch, which we must also look through. We should add all the non-catchswitch successors to ComplexEdgeSuccs.

The current patch is achieving this, no? All successors from all basic blocks are stored in ComplexEdgeSuccs, but then catchswitchs are skipped.

BTW, the following test case crashes at runtime (after applying this patch, otherwise it doesn't compile at all):

#include <windows.h>
#include <stdio.h>

DWORD FilterFunction() {
  printf("1 ");
  return EXCEPTION_EXECUTE_HANDLER;
}

int main(void) {
  __try {
    __try {
      RaiseException(1, 0, 0, NULL);
    }
    __finally {
      printf("2 ");
    }
  }
  __except(FilterFunction()) {
    printf("3\n");
  }

  return 0;
}

[rnk]

In D38223#888141, @marco-c wrote:

I think any invoke or cleanupret that unwinds to a catchswitch needs to look through the successors of the catchswitch. The unwind edge may be another catchswitch, which we must also look through. We should add all the non-catchswitch successors to ComplexEdgeSuccs.

The current patch is achieving this, no? All successors from all basic blocks are stored in ComplexEdgeSuccs, but then catchswitchs are skipped.

Almost. I think what's missing is that single successor catchswitches should hit the else case instead of continuing the first loop. Also, notice how Edge indexes into a global array of length Edges. When you continue the loop, some of the slots in that array go unused. Maybe that's OK, but it seems like a bug. Sending catchswitch through the else case instead of continuing should fix that.

[marco-c]

Updated patch to add catchswitch successors to ComplexEdgeSuccs

[rnk]

I'm positive something is off here, but let's ship it and worry about that later.

[rnk]

On second thought, since we know this doesn't actually work (https://bugs.llvm.org/show_bug.cgi?id=34833), we should just disable gcov instrumentation of functions with funclets.

[rnk]

Actually marking it for changes...

[marco-c]

Disabled instrumentation for functions using funclet-based EH.

[rnk]

Oh, one consequence of this is that, when normal C++ exceptions are enabled (/EHsc), C++ functions with destructors will not be instrumented for coverage. Basically, this would make gcov incompatible with codebases that use exceptions on Windows. If you disable exceptions and occaisionally use SEH, they'll work fine, which I'm guessing is the case, since you only hit this on SEH code, not normal C++ try/catch.

I'm happy to defer fixing this until a user comes by who cares about enabling C++ EH and gcov. looks good

[marco-c]

Simply moved the isUsingFuncletBasedEH check before setting Result, otherwise it could be set to true even when it wasn't supposed to.

[rnk]

Thanks!

lib/Transforms/Instrumentation/GCOVProfiling.cpp

for (auto &F : M->functions()) {
           Edges += TI->getNumSuccessors();
       }
 
       ArrayType *CounterTy =
         ArrayType::get(Type::getInt64Ty(*Ctx), Edges);
       GlobalVariable *Counters =
         new GlobalVariable(*M, CounterTy, false,
                            GlobalValue::InternalLinkage,
                            Constant::getNullValue(CounterTy),
                            "__llvm_gcov_ctr");
       CountersBySP.push_back(std::make_pair(Counters, SP));
 
       UniqueVector<BasicBlock *> ComplexEdgePreds;
       UniqueVector<BasicBlock *> ComplexEdgeSuccs;
 
       unsigned Edge = 0;
       for (auto &BB : F) {
         TerminatorInst *TI = BB.getTerminator();
         int Successors = isa<ReturnInst>(TI) ? 1 : TI->getNumSuccessors();
         if (Successors) {
-          if (Successors == 1) {
+          if (Successors == 1 && !isa<CatchSwitchInst>(TI)) {
             IRBuilder<> Builder(&*BB.getFirstInsertionPt());
             Value *Counter = Builder.CreateConstInBoundsGEP2_64(Counters, 0,
                                                                 Edge);
             Value *Count = Builder.CreateLoad(Counter);
             Count = Builder.CreateAdd(Count, Builder.getInt64(1));
             Builder.CreateStore(Count, Counter);
           } else if (BranchInst *BI = dyn_cast<BranchInst>(TI)) {
             IRBuilder<> Builder(BI);
for (auto &F : M->functions()) {
 
       if (!ComplexEdgePreds.empty()) {
         GlobalVariable *EdgeTable =
           buildEdgeLookupTable(&F, Counters,
                                ComplexEdgePreds, ComplexEdgeSuccs);
         GlobalVariable *EdgeState = getEdgeStateValue();
 
         for (int i = 0, e = ComplexEdgePreds.size(); i != e; ++i) {
-          IRBuilder<> Builder(&*ComplexEdgePreds[i + 1]->getFirstInsertionPt());
+          auto *BB = ComplexEdgePreds[i + 1];
+          BasicBlock::iterator insertPtr = BB->getFirstInsertionPt();
+          if (insertPtr == BB->end()) continue;
+
+          IRBuilder<> Builder(&*insertPtr);
           Builder.CreateStore(Builder.getInt32(i), EdgeState);
         }
 
         for (int i = 0, e = ComplexEdgeSuccs.size(); i != e; ++i) {
+          auto *BB = ComplexEdgeSuccs[i + 1];
+          BasicBlock::iterator insertPtr = BB->getFirstInsertionPt();
+          if (insertPtr == BB->end()) continue;
+
           // Call runtime to perform increment.
-          IRBuilder<> Builder(&*ComplexEdgeSuccs[i + 1]->getFirstInsertionPt());
+          IRBuilder<> Builder(&*insertPtr);
           Value *CounterPtrArray =
             Builder.CreateConstInBoundsGEP2_64(EdgeTable, 0,
                                                i * ComplexEdgePreds.size());
 
           // Build code to increment the counter.
           InsertIndCounterIncrCode = true;
           Builder.CreateCall(getIncrementIndirectCounterFunc(),
                              {EdgeState, CounterPtrArray});
GlobalVariable *GCOVProfiler::buildEdgeLookupTable(
   Constant *NullValue = Constant::getNullValue(Int64PtrTy);
   for (size_t i = 0; i != TableSize; ++i)
     EdgeTable[i] = NullValue;
 
   unsigned Edge = 0;
   for (BasicBlock &BB : *F) {
     TerminatorInst *TI = BB.getTerminator();
     int Successors = isa<ReturnInst>(TI) ? 1 : TI->getNumSuccessors();
-    if (Successors > 1 && !isa<BranchInst>(TI) && !isa<ReturnInst>(TI)) {
+    if ((Successors > 1 && !isa<BranchInst>(TI)) || isa<CatchSwitchInst>(TI)) {
       for (int i = 0; i != Successors; ++i) {
         BasicBlock *Succ = TI->getSuccessor(i);
         IRBuilder<> Builder(Succ);
         Value *Counter = Builder.CreateConstInBoundsGEP2_64(Counters, 0,
                                                             Edge + i);
         EdgeTable[((Succs.idFor(Succ) - 1) * Preds.size()) +
                   (Preds.idFor(&BB) - 1)] = cast<Constant>(Counter);
       }