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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
