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.
Details
Diff Detail
Event Timeline
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. |
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; }
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.
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.
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
Simply moved the isUsingFuncletBasedEH check before setting Result, otherwise it could be set to true even when it wasn't supposed to.
I think I'd skip BB if TI->IsEHPad(), that would save a bunch of the checks you are doing on BB.