Page MenuHomePhabricator

Disable gcov instrumentation of functions using funclet-based exception handling
ClosedPublic

Authored by marco-c on Sep 24 2017, 9:49 AM.

Diff Detail

Event Timeline

marco-c created this revision.Sep 24 2017, 9:49 AM

Please upload a diff with full context.

majnemer set the repository for this revision to rL LLVM.Sep 24 2017, 10:07 PM
majnemer accepted this revision.Sep 25 2017, 9:21 AM
majnemer added inline comments.
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.

This revision is now accepted and ready to land.Sep 25 2017, 9:21 AM
rnk edited edge metadata.Sep 25 2017, 9:55 AM

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.

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 added a comment.Oct 4 2017, 10:10 AM

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 this revision to Diff 117716.Oct 4 2017, 12:57 PM

Updated patch to add catchswitch successors to ComplexEdgeSuccs

rnk accepted this revision.Oct 4 2017, 2:45 PM

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

rnk added a comment.Oct 4 2017, 2:51 PM

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 requested changes to this revision.Oct 4 2017, 2:51 PM

Actually marking it for changes...

This revision now requires changes to proceed.Oct 4 2017, 2:51 PM
marco-c updated this revision to Diff 117865.Oct 5 2017, 12:22 PM
marco-c edited edge metadata.

Disabled instrumentation for functions using funclet-based EH.

rnk accepted this revision.Oct 5 2017, 12:45 PM

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

This revision is now accepted and ready to land.Oct 5 2017, 12:45 PM
marco-c retitled this revision from Make sure the basic block has an insertion point before dereferencing it to Disable gcov instrumentation of functions using funclet-based exception handling.Oct 12 2017, 3:32 AM
marco-c edited the summary of this revision. (Show Details)
marco-c updated this revision to Diff 118768.Oct 12 2017, 3:41 AM

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

rnk accepted this revision.Oct 12 2017, 3:06 PM

Thanks!

marco-c closed this revision.Oct 13 2017, 6:49 AM