[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:
> > 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)"?


================
Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:46
 class SourceMappingRegion {
+  /// Primary Counter that is also used for Branch Regions for "True" branches
   Counter Count;
----------------
nit: Please end comments with a '.'


================
Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:523
+
+    RegionStack.emplace_back(Count, FalseCount, StartLoc, EndLoc,
+                             FalseCount.hasValue());
----------------
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?


================
Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:623
             // separate region for each expansion.
+            // Don't do this for branch regions
             SourceLocation NestedLoc = getStartOfFileOrMacro(EndLoc);
----------------
Could you explain why there's an exception for branch regions here?


================
Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:724
 
+  /// Determine whether the given condition folds to true or false
+  bool ConditionFoldsToBool(const Expr *Cond) {
----------------
nit: 'can be constant folded' may be slightly more idiomatic.


================
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)) {
----------------
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?


================
Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:749
+      // constant-folding by ensuring that ConstantFoldsToSimpleInteger() in
+      // CodeGenFunction.c always returns false, but that is very heavy-handed.
+      if (ConditionFoldsToBool(C))
----------------
Right, I don't think that alternative is practicable.


================
Comment at: llvm/tools/llvm-cov/SourceCoverageViewHTML.cpp:659
 
+void SourceCoverageViewHTML::renderBranchView(raw_ostream &OS,
+                                              BranchView &BRV,
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84467