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

[Alan Phipps via Phabricator via cfe-commits]

alanphipps marked an inline comment as done.
alanphipps added inline comments.


================
Comment at: clang/lib/CodeGen/CodeGenFunction.h:4359
+  /// condition (i.e. no "&&" or "||").
+  static bool isLeafCondition(const Expr *C);
+
----------------
vsk wrote:
> 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.
I just realized I didn't make this name change to isInstrumentedCondition(). I will do that.


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

https://reviews.llvm.org/D84467