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

[Alan Phipps via Phabricator via cfe-commits]

alanphipps added a comment.

In D84467#2180421 <https://reviews.llvm.org/D84467#2180421>, @vsk wrote:

> I haven't taken a close look at the tests yet, but plan on getting to it soon.
>
> I'm not sure whether this is something you've already tried, but for this kind of change, it can be helpful to verify that a stage2 clang self-host with assertions enabled completes successfully. For coverage specifically, it's possible to do that by setting '-DLLVM_BUILD_INSTRUMENTED_COVERAGE=On' during the stage2 cmake step.

I actually haven't tried that -- I've attempted to, but I've run into some cmake issues trying to generate the bootstrap build configuration, but I'll look into it further.  But I have successfully been able to use a host clang compiler to build my target compiler with assertions enabled and coverage instrumentation enabled, and I was able to use that to confirm that the coverage tests (with my additional tests) sufficiently cover the code I added, and no assertions fired.



================
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:
> > > 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.


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

Will do; I'll upload a diff soon with formatting cleanup.


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




================
Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:623
             // separate region for each expansion.
+            // Don't do this for branch regions
             SourceLocation NestedLoc = getStartOfFileOrMacro(EndLoc);
----------------
vsk wrote:
> Could you explain why there's an exception for branch regions here?
This was a design decision -- if a branch region is split across expansions, I still want to keep a single branch region, and I want the startloc and endloc to be consistently in the same file/expansion.  

So don't create a separate region for branch regions, but do reset the StartLoc/EndLoc accordingly.  I can add this as a comment, or perhaps there is a better way to do this. 

Admittedly the cases that bring this about are strange, e.g.:


```
extern void func2();
void func1(bool a, bool b) {
  #define CONDEND b)
  if (a == CONDEND {
      func2();
  }
}
```

but they can happen.


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


================
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)) {
----------------
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().


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84467