[coverage] Make smaller regions for the first case of a switch.

[efriedma]

Summary

We never overwrite the end location of region, so we would end up with
an overly large region when we reused the switch's region.

[vsk]

Thanks for the patch! LGTM.

lib/CodeGen/CoverageMappingGen.cpp

686 ↗

(On Diff #104581)

As should return/continue.

[vsk]

Hi Eli, are you waiting on something before landing this?

[efriedma]

I'm going to look over the overall algorithm one more time to make sure this direction makes sense. The current code for ending regions on break/continue/return/noreturn doesn't really work well, and the way we handle multiple consecutive case statments isn't really right, and I want to make sure I'm not going to end up reverting this. And I need to run some tests to make sure I'm not introducing any crashes. If nothing is wrong, I'll commit tomorrow.

[vsk]

In D34801#828382, @efriedma wrote:

I'm going to look over the overall algorithm one more time to make sure this direction makes sense. The current code for ending regions on break/continue/return/noreturn doesn't really work well,

Maybe it's worth taking a step back and finding a way to fix that. As you alluded to with the FIXME, there are still some basic cases the current patch won't address:

switch (x) {
  case 1: // Region set to end at the end of the second break.
  case 2:
    break;
  case 3:
  case 4:
    break;
}

and the way we handle multiple consecutive case statments isn't really right,

I agree. It'd be worth seeing whether we can avoid pushing a new region for a case if we know fallthrough occurs.

and I want to make sure I'm not going to end up reverting this. And I need to run some tests to make sure I'm not introducing any crashes. If nothing is wrong, I'll commit tomorrow.

I suspect that fixing switch/cases in a more general way would require reverting this and I know I'll revisit this in the coming weeks. Still, this is an improvement and if nothing goes wrong during testing your plan sgtm.

cfe/trunk/lib/CodeGen/CoverageMappingGen.cpp

void VisitLabelStmt(const LabelStmt *S) {
     pushRegion(getRegionCounter(S), Start);
     Visit(S->getSubStmt());
   }
 
   void VisitBreakStmt(const BreakStmt *S) {
     assert(!BreakContinueStack.empty() && "break not in a loop or switch!");
     BreakContinueStack.back().BreakCount = addCounters(
         BreakContinueStack.back().BreakCount, getRegion().getCounter());
+    // FIXME: a break in a switch should terminate regions for all preceding
+    // case statements, not just the most recent one.
     terminateRegion(S);
   }
 
   void VisitContinueStmt(const ContinueStmt *S) {
     assert(!BreakContinueStack.empty() && "continue stmt not in a loop!");
     BreakContinueStack.back().ContinueCount = addCounters(
         BreakContinueStack.back().ContinueCount, getRegion().getCounter());
     terminateRegion(S);
void VisitSwitchStmt(const SwitchStmt *S) {
     Visit(S->getCond());
 
     BreakContinueStack.push_back(BreakContinue());
 
     const Stmt *Body = S->getBody();
     extendRegion(Body);
     if (const auto *CS = dyn_cast<CompoundStmt>(Body)) {
       if (!CS->body_empty()) {
-        // The body of the switch needs a zero region so that fallthrough counts
-        // behave correctly, but it would be misleading to include the braces of
-        // the compound statement in the zeroed area, so we need to handle this
-        // specially.
+        // Make a region for the body of the switch.  If the body starts with
+        // a case, that case will reuse this region; otherwise, this covers
+        // the unreachable code at the beginning of the switch body.
         size_t Index =
-            pushRegion(Counter::getZero(), getStart(CS->body_front()),
-                       getEnd(CS->body_back()));
+            pushRegion(Counter::getZero(), getStart(CS->body_front()));
         for (const auto *Child : CS->children())
           Visit(Child);
+
+        // Set the end for the body of the switch, if it isn't already set.
+        for (size_t i = RegionStack.size(); i != Index; --i) {
+          if (!RegionStack[i - 1].hasEndLoc())
+            RegionStack[i - 1].setEndLoc(getEnd(CS->body_back()));
+        }
+
         popRegions(Index);
       }
     } else
       propagateCounts(Counter::getZero(), Body);
     BreakContinue BC = BreakContinueStack.pop_back_val();
 
     if (!BreakContinueStack.empty())
       BreakContinueStack.back().ContinueCount = addCounters(

cfe/trunk/test/CoverageMapping/switch.cpp

 // RUN: %clang_cc1 -fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping -emit-llvm-only -std=c++1z -triple %itanium_abi_triple -main-file-name switch.cpp %s | FileCheck %s
 
                     // CHECK: foo
 void foo(int i) {   // CHECK-NEXT: File 0, [[@LINE]]:17 -> [[@LINE+8]]:2 = #0
   switch(i) {
-  case 1:           // CHECK-NEXT: File 0, [[@LINE]]:3 -> [[@LINE+3]]:10 = #2
+  case 1:           // CHECK-NEXT: File 0, [[@LINE]]:3 -> [[@LINE+1]]:11 = #2
     return;
   case 2:           // CHECK-NEXT: File 0, [[@LINE]]:3 -> [[@LINE+1]]:10 = #3
     break;
   }
   int x = 0;        // CHECK-NEXT: File 0, [[@LINE]]:3 -> [[@LINE+1]]:2 = #1
 }
 
 int nop() { return 0; }
switch (int i = true ? nop()  // CHECK-NEXT: [[@LINE]]:26 -> [[@LINE]]:31 = #2
           i) {}
   nop();            // CHECK-NEXT: [[@LINE]]:3 -> [[@LINE+1]]:2 = #1
 }
 
                     // CHECK-NEXT: main
 int main() {        // CHECK-NEXT: File 0, [[@LINE]]:12 -> [[@LINE+35]]:2 = #0
   int i = 0;
   switch(i) {
-  case 0:           // CHECK-NEXT: File 0, [[@LINE]]:3 -> [[@LINE+7]]:10 = #2
+  case 0:           // CHECK-NEXT: File 0, [[@LINE]]:3 -> [[@LINE+2]]:10 = #2
     i = 1;
     break;
   case 1:           // CHECK-NEXT: File 0, [[@LINE]]:3 -> [[@LINE+2]]:10 = #3
     i = 2;
     break;
   default:          // CHECK-NEXT: File 0, [[@LINE]]:3 -> [[@LINE+1]]:10 = #4
     break;
   }
   switch(i) {       // CHECK-NEXT: File 0, [[@LINE]]:3 -> [[@LINE+23]]:2 = #1
-  case 0:           // CHECK-NEXT: File 0, [[@LINE]]:3 -> [[@LINE+6]]:10 = #6
+  case 0:           // CHECK-NEXT: File 0, [[@LINE]]:3 -> [[@LINE+2]]:10 = #6
     i = 1;
     break;
   case 1:           // CHECK-NEXT: File 0, [[@LINE]]:3 -> [[@LINE+3]]:10 = #7
     i = 2;
   default:          // CHECK-NEXT: File 0, [[@LINE]]:3 -> [[@LINE+1]]:10 = (#7 + #8)
     break;
   }
 
   switch(i) {       // CHECK-NEXT: File 0, [[@LINE]]:3 -> [[@LINE+13]]:2 = #5
   case 1:           // CHECK-NEXT: File 0, [[@LINE]]:3 -> [[@LINE+5]]:11 = #10
   case 2:           // CHECK-NEXT: File 0, [[@LINE]]:3 -> [[@LINE+4]]:11 = (#10 + #11)
     i = 11;
   case 3:           // CHECK-NEXT: File 0, [[@LINE]]:3 -> [[@LINE+2]]:11 = ((#10 + #11) + #12)
   case 4:           // CHECK-NEXT: File 0, [[@LINE]]:3 -> [[@LINE+1]]:11 = (((#10 + #11) + #12) + #13)
     i = 99;
   }
 
   foo(1);           // CHECK-NEXT: File 0, [[@LINE]]:3 -> [[@LINE+3]]:11 = #9
   bar(1);
   baz();
   return 0;
 }
+
+// FIXME: End location for "case 1" shouldn't point at the end of the switch.
+                         // CHECK: fallthrough
+int fallthrough(int i) { // CHECK-NEXT: File 0, [[@LINE]]:24 -> [[@LINE+12]]:2 = #0
+  switch(i) {
+  case 1:           // CHECK-NEXT: File 0, [[@LINE]]:3 -> [[@LINE+8]]:10 = #2
+    i = 23;
+  case 2:           // CHECK-NEXT: File 0, [[@LINE]]:3 -> [[@LINE+2]]:10 = (#2 + #3)
+    i = 11;
+    break;
+  case 3:           // CHECK-NEXT: File 0, [[@LINE]]:3 -> [[@LINE+3]]:10 = #4
+  case 4:           // CHECK-NEXT: File 0, [[@LINE]]:3 -> [[@LINE+2]]:10 = (#4 + #5)
+    i = 99;
+    break;
+  }
+}