[PATCH] D84467: Add support for Branch Coverage in LLVM Source-Based Code Coverage

[Vedant Kumar via Phabricator via cfe-commits]

vsk removed a subscriber: loic-joly-sonarsource.
vsk added inline comments.


================
Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:1169
+    createBranchRegion(S->getCond(), BodyCount,
+                       subtractCounters(CondCount, BodyCount));
   }
----------------
alanphipps wrote:
> vsk wrote:
> > If `theWhileStmt->getCond()` is-a BinaryOperator, it would not be considered an instrumented condition and no branch region would be created here. OTOH, if the condition is instrumented (e.g. as it would be in `while (x);`), a branch region would be created.
> > 
> > Is this the expected outcome? It seems a little bit inconsistent compared to either a) allowing the RecursiveASTVisitor to identify expressions that require branch regions, or b) guaranteeing that each while statement comes with a branch region for its condition.
> > 
> > I have the same question about the `createBranchRegion` calls in VisitDoStmt, VisitForStmt, etc. (But I'm not concerned about the calls to `createBranchRegion` in VisitBinL{And,Or} and VisitSwitch*.)
> Right, that's the expected outcome, but I think I see what you're saying in that it could be confusing to call "createBranchRegion()" that may not actually create a branch region in some cases.  
> 
> I could move the check for isInstrumentedCondition() out of createBranchRegion() to make it clear that a branch region is only created for when that is TRUE, but I wanted to encapsulate as much as I could in the function to avoid duplication.  Perhaps it would be better to rename "createBranchRegion()" to something more like "createBranchRegionForInstrumentedCond()"?
> 
> The same behavior exists for VisitBinL{And,Or} since those conditions could also be nested BinaryOperators.
I think it's fine for createBranchRegion to not guarantee that a region is created, since the logic is better-encapsulated this way.

I misunderstood how the feature is supposed to work. I assumed that all interesting branch regions would "automatically" be created during the recursive AST visit, by virtue of the logic you've added to VisitBinL{And,Or}. But it looks like the goal is to also show branch regions around loop constructs, regardless of whether or not the loop condition contains a binop.


================
Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:1277
+    // Create Branch Region around condition.
+    createBranchRegion(S->getCond(), BodyCount,
+                       subtractCounters(LoopCount, BodyCount));
----------------
alanphipps wrote:
> vsk wrote:
> > Is the condition of a CXXForRangeStmt something that's visible to the user? It looks more like a clang implementation detail (but I could be mistaken).
> Technically the condition isn't visible, per se, but the range-based construct implies a branchable condition that other vendors do track, so I still think it's useful to show it.
Seems reasonable -- I was just wondering whether a CXXForRangeStmt ever has a non-null condition attached. (I didn't see an example of this in loops.cpp, but perhaps I just missed the correct test case.)


================
Comment at: llvm/include/llvm/ProfileData/InstrProfData.inc:662
 /* Indexed profile format version (start from 1). */
-#define INSTR_PROF_INDEX_VERSION 6
+#define INSTR_PROF_INDEX_VERSION 7
 /* Coverage mapping format version (start from 0). */
----------------
Note that there's a copy of this file in llvm-project/compiler-rt (this allows the compiler-rt subtree to be checked out and built without any llvm dependency). Please copy over this change to compiler-rt/include/profile/InstrProfData.inc.


================
Comment at: llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp:258
           break;
+        case CounterMappingRegion::BranchRegion:
+          // For a Branch Region, read two successive counters.
----------------
Could you bump the coverage mapping format version, and document the extension to the format in docs/CoverageMappingFormat.rst?

This should prevent an older llvm-cov from reporting "malformed object" when encountering a branch region.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84467/new/

https://reviews.llvm.org/D84467