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

[Alan Phipps via Phabricator via llvm-commits]

alanphipps marked 2 inline comments as done.
alanphipps added inline comments.


================
Comment at: clang/lib/CodeGen/CGStmt.cpp:1368
       CaseDest = createBasicBlock("sw.bb");
-      EmitBlockWithFallThrough(CaseDest, &S);
+      EmitBlockWithFallThrough(CaseDest, CurCase);
     }
----------------
This fixes a defect that resulted in the wrong counter being instrumented for each Case.


================
Comment at: clang/lib/CodeGen/CodeGenFunction.h:4359
+  /// condition (i.e. no "&&" or "||").
+  static bool isLeafCondition(const Expr *C);
+
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84467