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

[Vedant Kumar via Phabricator via cfe-commits]

vsk added inline comments.


================
Comment at: clang/lib/CodeGen/CodeGenFunction.h:4359
+  /// condition (i.e. no "&&" or "||").
+  static bool isLeafCondition(const Expr *C);
+
----------------
alanphipps wrote:
> vsk wrote:
> > alanphipps wrote:
> > > vsk wrote:
> > > > vsk wrote:
> > > > > It might be helpful to either require that `C` be the RHS of a logical binop (e.g. by passing the binop expr in), or to document that requirement.
> > > > Given a logical binop like `E = a && !(b || c)`, `isLeafCondition(E->getRHS())` is true. That seems a bit counter-intuitive, because `E->getRHS()` contains another leaf condition. Would it make sense to rename the condition (perhaps to something like 'InstrumentedCondition')? Have I misunderstood what a leaf condition is?
> > > Background: isLeafCondition() is an auxiliary routine that is used during 1.) counter allocation on binop RHS (CodeGenPGO.cpp), 2.) counter instrumentation (CodeGenFunction.cpp), and 3.) branch region generation (CoverageMappingGen.cpp).  In the #3 case, it isn't always looking at the RHS of a logical binop but can be used to assess whether any condition is instrumented/evaluated.
> > > 
> > > Given your example condition:
> > > 
> > > E = a && !(b || c)
> > > 
> > > You are correct that isLeafCondition(E->getRHS()) will return true, but I think it should actually return false here (and bypass logical-NOT), so I will adapt this. 
> > > 
> > > However, given a condition that includes an binary operator (like assign):
> > > 
> > > E = a && (x = !(b || c))
> > > 
> > > isLeafCondition(E->getRHS()) will also return true, and I think that is the correct behavior.  Given that, then I agree that maybe isLeafCondition() should be renamed to isInstrumentedCondition() since it's not technically just leaves that are tracked in the presence of operators.
> > What is special about the assignment expression nested within "a && (x = !(b || c))" that necessitates an extra counter compared to "a && !(b || c)"?
> I'm exempting the logical NOT operator basically to match the functionality of other coverage vendors (Bullseye, GCOV).  It's a simplistic operator in the sense that (as far as I can tell) it only affects the sense of the generated branch on a condition.
> 
> As for the assignment, we're effectively creating a new condition that is evaluatable (even if a branch may technically not be generated), and so creating a counter for it is more interesting (and matches other vendor behavior).
> 
> But I'm open to persuasion.  We could just be more conservative and create counters for logical NOT, but I think there's value in matching some of the expected behavior of other vendors.  This is also something we could continue to fine-tune in future patches.
I agree that there isn't a need to insert a fresh counter to accommodate a logical NOT operator in a condition. But I don't see how an assignment expression effectively creates a new, evaluatable condition. It seems like the count for the assignment expr can be derived by looking up the count for its parent region.

At this stage I'm more interested in understanding the high-level design. If the question of whether/not to add fresh counters for assignment exprs in a condition is effectively about optimization, then I'm fine with tabling it.


================
Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:523
+
+    RegionStack.emplace_back(Count, FalseCount, StartLoc, EndLoc,
+                             FalseCount.hasValue());
----------------
alanphipps wrote:
> vsk wrote:
> > It looks like this sets up a deferred region when a branch count (FalseCount) is available. Could you explain why, and when the deferred region is expected to be complete?
> Does adding a region to the RegionStack always imply setting up a deferred region that needs to be completed?  My intention was to add the branch region to the RegionStack but not to setup or complete a deferred region for branch regions.  
> 
> Branch regions are straightforward since we already know start location and end location when they are created, but I have probably misunderstood what a deferred region implies.
> 
> 
Oops, I see now that this selects a different SourceMappingRegion constructor than the one I'd thought of. In general, no, adding a region to RegionStack doesn't always entail setting up a deferred region. It looks like you're not doing that here. (An aside: deferred regions were introduced to fix line execution counts for lines following terminator statements, like a 'return'.)


================
Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:742
+    // function's SourceRegions) because it doesn't apply to any other source
+    // code other than the Condition.
+    if (CodeGenFunction::isLeafCondition(C)) {
----------------
alanphipps wrote:
> vsk wrote:
> > Is there some alternative to pushing and then immediately popping branch regions? Did you try adding the region directly to the function's SourceRegions, and find that there was some issue with the more direct approach?
> The push/pop combo does look strange, and you're right -- I can just add it directly to SourceRegions, but the algorithm in popRegions() will do additional things like adjust the start/end location if the region spans a nested file depth. I think that's still useful to do.  Another option is I could factor that algorithm out of popRegions() into a new function and also call that from createBranchRegions().
If it's possible/straightforward to factor the region start/end adjustment code out of popRegions(), that would be really nice. If not, the current approach looks reasonable.


================
Comment at: llvm/tools/llvm-cov/SourceCoverageViewHTML.cpp:659
 
+void SourceCoverageViewHTML::renderBranchView(raw_ostream &OS,
+                                              BranchView &BRV,
----------------
alanphipps wrote:
> vsk wrote:
> > Wdyt of displaying branch-taken counts using tooltips? This would match the way region execution counts are shown, which could be nice, because the information wouldn't interrupt the flow of source code.
> I think using tooltips is a great idea, and I did look into it.  Ultimately, I decided to defer that work to a future patch to allow for some time to research a good way to properly distinguish the branch-taken counts from the region counts.
Sounds reasonable. Can the branch coverage text/html output should be opt-in via a cl::opt until it's finalized? That should make it possible to iterate on the format without changing what users see.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84467