[PATCH] D16934: [Coverage] Fix crash in VisitIfStmt

[Justin Bogner via cfe-commits]

Vedant Kumar <vsk at apple.com> writes:
> vsk created this revision.
> vsk added a reviewer: bogner.
> vsk added a subscriber: cfe-commits.
>
> When handling 'if' statements, we crash if the condition and the consequent
> branch are spanned by a single macro expansion.
>
> The crash occurs because of a sanity 'reset' in `popRegions()`: if an expansion
> exactly spans an entire region, we set `MostRecentLocation` to the start of the
> expansion (its 'include location').  This ensures we don't `handleFileExit()`
> ourselves out of the expansion before we're done processing all of the regions
> within it. This is tested in test/CoverageMapping/macro-expressions.c.
>
> This causes a problem when an expansion spans both the condition and the
> consequent branch of an 'if' statement. `MostRecentLocation` is updated to the
> start of the 'if' statement in `popRegions()`, so the file for the expansion
> isn't exited by the time we're done handling the statement. We then crash with
> 'fatal: File exit not handled before popRegions'.
>
> The fix for this is to detect these kinds of expansions, and conservatively
> update `MostRecentLocation` to the end of expansion region containing the
> conditional. I've added tests to make sure we don't have the same problem with
> other kinds of statements.

LGTM, with a question about generalising a bit below.

> rdar://problem/23630316
>
> http://reviews.llvm.org/D16934
>
> Files:
>   lib/CodeGen/CoverageMappingGen.cpp
>   test/CoverageMapping/macro-expressions.cpp
>
> Index: test/CoverageMapping/macro-expressions.cpp
> ===================================================================
> --- test/CoverageMapping/macro-expressions.cpp
> +++ test/CoverageMapping/macro-expressions.cpp
> @@ -12,6 +12,38 @@
>  #define PRIo64 PRI_64_LENGTH_MODIFIER "o"
>  #define PRIu64 PRI_64_LENGTH_MODIFIER "u"
>  
> +#define STMT(s) s
> +
> +void fn1() {
> +  STMT(if (1));
> +  STMT(while (1));
> +  STMT(for (;;));
> +  if (1)
> +    STMT(if (1)) 0;
> +  if (1)
> +    STMT(while (1)) 0;
> +  if (1)
> +    STMT(for (;;)) 0;
> +  while (1)
> +    STMT(if (1)) 0;
> +  while (1)
> +    STMT(while (1)) 0;
> +  while (1)
> +    STMT(for (;;)) 0;
> +  for (;;)
> +    STMT(if (1)) 0;
> +  for (;;)
> +    STMT(while (1)) 0;
> +  for (;;)
> +    STMT(for (;;)) 0;
> +}
> +
> +void STMT(fn2()) {
> +}
> +
> +void STMT(fn3)() {
> +}
> +
>  // CHECK: foo
>  // CHECK-NEXT: File 0, [[@LINE+1]]:17 -> {{[0-9]+}}:2 = #0
>  void foo(int i) {
> Index: lib/CodeGen/CoverageMappingGen.cpp
> ===================================================================
> --- lib/CodeGen/CoverageMappingGen.cpp
> +++ lib/CodeGen/CoverageMappingGen.cpp
> @@ -807,6 +807,14 @@
>      // counter for the body when looking at the coverage.
>      propagateCounts(ParentCount, S->getCond());
>  
> +    // The region containing the condition may be spanned by an expansion.
> +    // Make sure we handle a file exit out of this expansion before handling
> +    // the consequent branch.
> +    if (SM.isBeforeInTranslationUnit(getStart(S->getCond()),
> +                                     S->getCond()->getLocStart())) {
> +      MostRecentLocation = getEnd(S->getCond());
> +    }

Can/should this be folded into propagateCounts? I can't think of a place
where that would be harmful, but I guess if it can't come up in other
situations this is fine here.

> +
>      extendRegion(S->getThen());
>      Counter OutCount = propagateCounts(ThenCount, S->getThen());
>  
>