[Coverage] Do not push a new region after a CXXTryStmt

[vsk]

Summary

There are three possible scenarios when entering a CXXTryStmt:

1. No exceptions thrown.
2. An exception is thrown but caught by one of the handlers.
3. An exception is thrown but not caught by one of the handlers.

The region we push after a CXXTryStmt isn't useful in scenarios (1) or (2). In
these cases, if control flows to the try block it must flow to the region
following the CXXTryStmt.

This may not be the case in situation (3). However, we don't handle this
scenario anywhere else (e.g after function calls). Having a dedicated region
here could only be helpful if its counter expression differed from the try
block, and even then it's questionable.

[ikudrin]

So, our tool isn't accurate in the case of throwing an exception. Is there a case where this patch makes things better than they were before? Is it possible to improve handling of exceptions instead?

[vsk]

In D20878#446690, @ikudrin wrote:

Is there a case where this patch makes things better than they were before? Is it possible to improve handling of exceptions instead?

This patch removes something which doesn't appear to serve a useful purpose. In general, I don't see a way to handle stack unwinding well. Consider:

1| void f() {
2|  may_throw();
3|  may_throw();
4|  return;
5|}

It would be prohibitively expensive to create separate regions and counters for lines 3 and 4, and after every callsite of f, and all its callsites, etc. We could improve the situation slightly by (1) documenting that stack unwinding is not handled precisely, and/or (2) emitting a diagnostic if the TU contains a throw statement. This would make the resulting coverage reports less surprising to users.

[ikudrin]

I agree that it'd be expensive to put a new counter after each function call. Even if there is no throw statement within a TU, called functions still may raise exceptions.
Anyway, if we come across a try statement, we can guess that some exceptions are expected and do our best to support constructions like this:

try {
  ...
}
catch (...) {
  ...
  throw;
}

[vsk]

I see your point that it isn't expensive to do a best-effort job here. I updated docs/SourceBasedCodeCoverage.rst with a limitations section as per my earlier comment.

lib/CodeGen/CoverageMappingGen.cpp

       pushRegion(OutCount);
   }
 
   void VisitCXXTryStmt(const CXXTryStmt *S) {
     extendRegion(S);
     Visit(S->getTryBlock());
     for (unsigned I = 0, E = S->getNumHandlers(); I < E; ++I)
       Visit(S->getHandler(I));
-
-    Counter ExitCount = getRegionCounter(S);
-    pushRegion(ExitCount);
   }
 
   void VisitCXXCatchStmt(const CXXCatchStmt *S) {
     propagateCounts(getRegionCounter(S), S->getHandlerBlock());
   }
 
   void VisitAbstractConditionalOperator(const AbstractConditionalOperator *E) {
     extendRegion(E);

test/CoverageMapping/trycatch.cpp

int main() {                          // CHECK-NEXT: File 0, [[@LINE]]:12 -> [[@LINE+13]]:2 = #0
   } catch(const Error &e) {           // CHECK-NEXT: File 0, [[@LINE]]:27 -> [[@LINE+2]]:4 = #2
     j = 1;
   } catch(const ImportantError &e) {  // CHECK-NEXT: File 0, [[@LINE]]:36 -> [[@LINE+2]]:4 = #3
     j = 11;
   }
   catch(const Warning &w) {           // CHECK-NEXT: File 0, [[@LINE]]:27 -> [[@LINE+2]]:4 = #4
     j = 0;
   }
-  return 0;                           // CHECK-NEXT: File 0, [[@LINE]]:3 -> [[@LINE]]:11 = #1
+  return 0;
 }