vsk removed a subscriber: loic-joly-sonarsource.
vsk added inline comments.
================
Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:1169
+ createBranchRegion(S->getCond(), BodyCount,
+ subtractCounters(CondCount, BodyCount));
}
----------------
alanphipps wrote:
> vsk wrote:
> > If `theWhileStmt->getCond()` is-a BinaryOperator, it would not be
> > considered an instrumented condition and no branch region would be created
> > here. OTOH, if the condition is instrumented (e.g. as it would be in `while
> > (x);`), a branch region would be created.
> >
> > Is this the expected outcome? It seems a little bit inconsistent compared
> > to either a) allowing the RecursiveASTVisitor to identify expressions that
> > require branch regions, or b) guaranteeing that each while statement comes
> > with a branch region for its condition.
> >
> > I have the same question about the `createBranchRegion` calls in
> > VisitDoStmt, VisitForStmt, etc. (But I'm not concerned about the calls to
> > `createBranchRegion` in VisitBinL{And,Or} and VisitSwitch*.)
> Right, that's the expected outcome, but I think I see what you're saying in
> that it could be confusing to call "createBranchRegion()" that may not
> actually create a branch region in some cases.
>
> I could move the check for isInstrumentedCondition() out of
> createBranchRegion() to make it clear that a branch region is only created
> for when that is TRUE, but I wanted to encapsulate as much as I could in the
> function to avoid duplication. Perhaps it would be better to rename
> "createBranchRegion()" to something more like
> "createBranchRegionForInstrumentedCond()"?
>
> The same behavior exists for VisitBinL{And,Or} since those conditions could
> also be nested BinaryOperators.
I think it's fine for createBranchRegion to not guarantee that a region is
created, since the logic is better-encapsulated this way.
I misunderstood how the feature is supposed to work. I assumed that all
interesting branch regions would "automatically" be created during the
recursive AST visit, by virtue of the logic you've added to VisitBinL{And,Or}.
But it looks like the goal is to also show branch regions around loop
constructs, regardless of whether or not the loop condition contains a binop.
================
Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:1277
+ // Create Branch Region around condition.
+ createBranchRegion(S->getCond(), BodyCount,
+ subtractCounters(LoopCount, BodyCount));
----------------
alanphipps wrote:
> vsk wrote:
> > Is the condition of a CXXForRangeStmt something that's visible to the user?
> > It looks more like a clang implementation detail (but I could be mistaken).
> Technically the condition isn't visible, per se, but the range-based
> construct implies a branchable condition that other vendors do track, so I
> still think it's useful to show it.
Seems reasonable -- I was just wondering whether a CXXForRangeStmt ever has a
non-null condition attached. (I didn't see an example of this in loops.cpp, but
perhaps I just missed the correct test case.)
================
Comment at: llvm/include/llvm/ProfileData/InstrProfData.inc:662
/* Indexed profile format version (start from 1). */
-#define INSTR_PROF_INDEX_VERSION 6
+#define INSTR_PROF_INDEX_VERSION 7
/* Coverage mapping format version (start from 0). */
----------------
Note that there's a copy of this file in llvm-project/compiler-rt (this allows
the compiler-rt subtree to be checked out and built without any llvm
dependency). Please copy over this change to
compiler-rt/include/profile/InstrProfData.inc.
================
Comment at: llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp:258
break;
+ case CounterMappingRegion::BranchRegion:
+ // For a Branch Region, read two successive counters.
----------------
Could you bump the coverage mapping format version, and document the extension
to the format in docs/CoverageMappingFormat.rst?
This should prevent an older llvm-cov from reporting "malformed object" when
encountering a branch region.
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
