[llvm] [clang] `llvm-cov` assertion failure when handling MC/DC that involves macros (PR #80098)
@@ -1973,6 +1981,8 @@ struct CounterCoverageMappingBuilder
void VisitBinLAnd(const BinaryOperator *E) {
bool IsRootNode = MCDCBuilder.isIdle();
+MCDCDebugCounter++;
whentojump wrote:
Hi thanks again for taking the look!
My intuition was to make the ID different whenever a new decision region is
going to be added by calling `createDecisionRegion()`. There might be something
obvious that I ignored.
And after reading some of the other posts (I'm still learning most of them,
please bear with that!), I generally agree with the approaches whose
modifications are minimized within `llvm-cov`, instead of touching the front
end.
Regards
https://github.com/llvm/llvm-project/pull/80098
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[llvm] [clang] `llvm-cov` assertion failure when handling MC/DC that involves macros (PR #80098)
https://github.com/whentojump closed https://github.com/llvm/llvm-project/pull/80098 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[llvm] [clang] `llvm-cov` assertion failure when handling MC/DC that involves macros (PR #80098)
chapuni wrote: See also, #78920, I guess it is also relevant to you. https://github.com/llvm/llvm-project/pull/80098 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[llvm] [clang] `llvm-cov` assertion failure when handling MC/DC that involves macros (PR #80098)
@@ -857,6 +857,8 @@ struct CounterCoverageMappingBuilder
unsigned getRegionBitmap(const Stmt *S) { return MCDCBitmapMap[S]; }
+ unsigned long MCDCDebugCounter;
chapuni wrote:
What is its preferable name? I guess you expects unique ID for debug for now.
https://github.com/llvm/llvm-project/pull/80098
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[llvm] [clang] `llvm-cov` assertion failure when handling MC/DC that involves macros (PR #80098)
https://github.com/chapuni requested changes to this pull request. I suppose this is still a draft. https://github.com/llvm/llvm-project/pull/80098 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[llvm] [clang] `llvm-cov` assertion failure when handling MC/DC that involves macros (PR #80098)
@@ -246,16 +248,18 @@ void CoverageMappingWriter::write(raw_ostream ) {
encodeULEB128(unsigned(I->MCDCParams.ID), OS);
encodeULEB128(unsigned(I->MCDCParams.TrueID), OS);
encodeULEB128(unsigned(I->MCDCParams.FalseID), OS);
+ encodeULEB128(unsigned(I->MCDCParams.GroupID), OS);
break;
case CounterMappingRegion::MCDCDecisionRegion:
encodeULEB128(unsigned(I->Kind)
<<
Counter::EncodingCounterTagAndExpansionRegionTagBits,
OS);
encodeULEB128(unsigned(I->MCDCParams.BitmapIdx), OS);
encodeULEB128(unsigned(I->MCDCParams.NumConditions), OS);
+ encodeULEB128(unsigned(I->MCDCParams.GroupID), OS);
chapuni wrote:
Is it the enhancement?
https://github.com/llvm/llvm-project/pull/80098
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[llvm] [clang] `llvm-cov` assertion failure when handling MC/DC that involves macros (PR #80098)
https://github.com/chapuni edited https://github.com/llvm/llvm-project/pull/80098 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[llvm] [clang] `llvm-cov` assertion failure when handling MC/DC that involves macros (PR #80098)
whentojump wrote: Hi thanks for the prompt reply and the pointer! Will definitely check. https://github.com/llvm/llvm-project/pull/80098 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[llvm] [clang] `llvm-cov` assertion failure when handling MC/DC that involves macros (PR #80098)
evodius96 wrote: Hi! I think this may be the same issue as https://github.com/llvm/llvm-project/issues/77871 and a fix is under review by @chapuni Can you verify? https://github.com/llvm/llvm-project/pull/80098 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[llvm] [clang] `llvm-cov` assertion failure when handling MC/DC that involves macros (PR #80098)
github-actions[bot] wrote:
:warning: C/C++ code formatter, clang-format found issues in your code.
:warning:
You can test this locally with the following command:
``bash
git-clang-format --diff c19436eec1c236cbe622c04e33f35f1f9478fa15
cd5a33851187c872c0de7a766528be097bcc68ad --
clang/lib/CodeGen/CoverageMappingGen.cpp
llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h
llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp
llvm/lib/ProfileData/Coverage/CoverageMappingWriter.cpp
``
View the diff from clang-format here.
``diff
diff --git a/clang/lib/CodeGen/CoverageMappingGen.cpp
b/clang/lib/CodeGen/CoverageMappingGen.cpp
index ecd90fe058..565979c5c3 100644
--- a/clang/lib/CodeGen/CoverageMappingGen.cpp
+++ b/clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -888,20 +888,20 @@ struct CounterCoverageMappingBuilder
if (EndLoc && EndLoc->isInvalid())
EndLoc = std::nullopt;
RegionStack.emplace_back(Count, FalseCount,
- MCDCParameters{
- 0, 0,
- ID, TrueID, FalseID, GroupID},
+ MCDCParameters{0, 0, ID, TrueID, FalseID,
GroupID},
StartLoc, EndLoc);
return RegionStack.size() - 1;
}
- size_t pushRegion(unsigned BitmapIdx, unsigned Conditions, MCDCConditionID
GroupID,
+ size_t pushRegion(unsigned BitmapIdx, unsigned Conditions,
+MCDCConditionID GroupID,
std::optional StartLoc = std::nullopt,
std::optional EndLoc = std::nullopt) {
-RegionStack.emplace_back(MCDCParameters{BitmapIdx, Conditions, 0, 0, 0,
GroupID}, StartLoc,
- EndLoc);
+RegionStack.emplace_back(
+MCDCParameters{BitmapIdx, Conditions, 0, 0, 0, GroupID}, StartLoc,
+EndLoc);
return RegionStack.size() - 1;
}
@@ -1059,7 +1059,8 @@ struct CounterCoverageMappingBuilder
// CodeGenFunction.c always returns false, but that is very heavy-handed.
if (ConditionFoldsToBool(C))
popRegions(pushRegion(Counter::getZero(), getStart(C), getEnd(C),
- Counter::getZero(), ID, TrueID, FalseID,
GroupID));
+ Counter::getZero(), ID, TrueID, FalseID,
+ GroupID));
else
// Otherwise, create a region with the True counter and False counter.
popRegions(pushRegion(TrueCnt, getStart(C), getEnd(C), FalseCnt, ID,
@@ -1070,7 +1071,8 @@ struct CounterCoverageMappingBuilder
/// Create a Decision Region with a BitmapIdx and number of Conditions. This
/// type of region "contains" branch regions, one for each of the conditions.
/// The visualization tool will group everything together.
- void createDecisionRegion(const Expr *C, unsigned BitmapIdx, unsigned Conds,
MCDCConditionID GroupID) {
+ void createDecisionRegion(const Expr *C, unsigned BitmapIdx, unsigned Conds,
+MCDCConditionID GroupID) {
popRegions(pushRegion(BitmapIdx, Conds, GroupID, getStart(C), getEnd(C)));
}
@@ -1156,10 +1158,9 @@ struct CounterCoverageMappingBuilder
if (I.isBranch())
SourceRegions.emplace_back(
I.getCounter(), I.getFalseCounter(),
-MCDCParameters{0, 0, I.getMCDCParams().ID,
- I.getMCDCParams().TrueID,
- I.getMCDCParams().FalseID,
- I.getMCDCParams().GroupID},
+MCDCParameters{
+0, 0, I.getMCDCParams().ID, I.getMCDCParams().TrueID,
+I.getMCDCParams().FalseID, I.getMCDCParams().GroupID},
Loc, getEndOfFileOrMacro(Loc), I.isBranch());
else
SourceRegions.emplace_back(I.getCounter(), Loc,
@@ -2016,11 +2017,13 @@ struct CounterCoverageMappingBuilder
// Create Branch Region around LHS condition.
createBranchRegion(E->getLHS(), RHSExecCnt,
- subtractCounters(ParentCnt, RHSExecCnt), DecisionLHS,
MCDCDebugCounter);
+ subtractCounters(ParentCnt, RHSExecCnt), DecisionLHS,
+ MCDCDebugCounter);
// Create Branch Region around RHS condition.
createBranchRegion(E->getRHS(), RHSTrueCnt,
- subtractCounters(RHSExecCnt, RHSTrueCnt), DecisionRHS,
MCDCDebugCounter);
+ subtractCounters(RHSExecCnt, RHSTrueCnt), DecisionRHS,
+ MCDCDebugCounter);
}
// Determine whether the right side of OR operation need to be visited.
``
https://github.com/llvm/llvm-project/pull/80098
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[llvm] [clang] `llvm-cov` assertion failure when handling MC/DC that involves macros (PR #80098)
llvmbot wrote:
@llvm/pr-subscribers-clang-codegen
@llvm/pr-subscribers-clang
Author: Wentao Zhang (whentojump)
Changes
## Problem
The behavior is described with this tiny example:
https://github.com/whentojump/llvm-mcdc-assertion-failure
Essentially, current MC/DC implementation cannot properly handle decisions that
involve macros, like this example:
## Root cause
Some terms I'll use:
1. Decision region: the composite logical expression
2. Branch region: smaller conditions or branches within a larger expression
In the full lifecycle of these regions, here are several important stages:
1. Compilation
1. Generated at front end: [source
code](https://github.com/llvm/llvm-project/blob/a0b6747804e46665ecfd00295b60432bfe1775b6/clang/lib/CodeGen/CoverageMappingGen.cpp#L860-L903)
2. Written to the binary as coverage mapping sections: [source
code](https://github.com/llvm/llvm-project/blob/a0b6747804e46665ecfd00295b60432bfe1775b6/llvm/lib/ProfileData/Coverage/CoverageMappingWriter.cpp#L240-L256)
2. Generate report
1. Read from the binary: [source
code](https://github.com/llvm/llvm-project/blob/a0b6747804e46665ecfd00295b60432bfe1775b6/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp#L650-L703)
The assertion in question is in step 2.i and goes like this:
1. `llvm-cov` walks all regions of the target program
2. It encounters a decision region, say **R3**. By reading **R3**'s metadata,
it knows it has two branches
3. `llvm-cov` then asserts: the next two regions it's gonna see must be two
branch regions, not otherwise
This assertion assumes the order of MCDC regions. However, this order doesn't
always hold
In step 1.ii all regions are
[sorted](https://github.com/llvm/llvm-project/blob/a0b6747804e46665ecfd00295b60432bfe1775b6/llvm/lib/ProfileData/Coverage/CoverageMappingWriter.cpp#L162-L171)
first by their file IDs, then by locations within the file and finally by
region types. Macros, however, are traced back to definitions, which can be far
away from their invocations or even separated in different files. As a result,
the MC/DC regions could be sorted in a way where they are separated from each
other. In the shown example, the order could be: `R3 (many irrelevant regions)
R1 R2` which apparently breaks the assertion at step 2.i.
## Solution
This can be solved in two ways
1. (This PR) Sort with MC/DC in consideration and group relevant decisions and
branches together.
I honestly don't know if this's gonna break other things. But at least I
can confirm it solves the problem mentioned. Again please see an example in
this repo https://github.com/whentojump/llvm-mcdc-assertion-failure
2. In `CoverageMapping::loadFunctionRecord()`, use a cleverer way to correlate
decisions and branches that are sorted far away from each other.
## Other to-do's
- Tests are not yet taken care of
- A bit comments/docs
Last but not least, a huge thanks to @evodius96 et al for the great
work regarding MC/DC :))
---
Full diff: https://github.com/llvm/llvm-project/pull/80098.diff
4 Files Affected:
- (modified) clang/lib/CodeGen/CoverageMappingGen.cpp (+29-17)
- (modified) llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h (+1)
- (modified) llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp (+6-2)
- (modified) llvm/lib/ProfileData/Coverage/CoverageMappingWriter.cpp (+7-3)
``diff
diff --git a/clang/lib/CodeGen/CoverageMappingGen.cpp
b/clang/lib/CodeGen/CoverageMappingGen.cpp
index 8b5e6c4ad8272..ecd90fe0585fc 100644
--- a/clang/lib/CodeGen/CoverageMappingGen.cpp
+++ b/clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -857,6 +857,8 @@ struct CounterCoverageMappingBuilder
unsigned getRegionBitmap(const Stmt *S) { return MCDCBitmapMap[S]; }
+ unsigned long MCDCDebugCounter;
+
/// Push a region onto the stack.
///
/// Returns the index on the stack where the region was pushed. This can be
@@ -866,7 +868,7 @@ struct CounterCoverageMappingBuilder
std::optional EndLoc = std::nullopt,
std::optional FalseCount = std::nullopt,
MCDCConditionID ID = 0, MCDCConditionID TrueID = 0,
-MCDCConditionID FalseID = 0) {
+MCDCConditionID FalseID = 0, MCDCConditionID GroupID = 0) {
if (StartLoc && !FalseCount) {
MostRecentLocation = *StartLoc;
@@ -886,17 +888,19 @@ struct CounterCoverageMappingBuilder
if (EndLoc && EndLoc->isInvalid())
EndLoc = std::nullopt;
RegionStack.emplace_back(Count, FalseCount,
- MCDCParameters{0, 0, ID, TrueID, FalseID},
+ MCDCParameters{
+ 0, 0,
+ ID, TrueID, FalseID, GroupID},
StartLoc, EndLoc);
return RegionStack.size() - 1;
}
- size_t
[llvm] [clang] `llvm-cov` assertion failure when handling MC/DC that involves macros (PR #80098)
github-actions[bot] wrote: Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using `@` followed by their GitHub username. If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the [LLVM GitHub User Guide](https://llvm.org/docs/GitHub.html). You can also ask questions in a comment on this PR, on the [LLVM Discord](https://discord.com/invite/xS7Z362) or on the [forums](https://discourse.llvm.org/). https://github.com/llvm/llvm-project/pull/80098 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[llvm] [clang] `llvm-cov` assertion failure when handling MC/DC that involves macros (PR #80098)
https://github.com/whentojump created
https://github.com/llvm/llvm-project/pull/80098
## Problem
The behavior is described with this tiny example:
https://github.com/whentojump/llvm-mcdc-assertion-failure
Essentially, current MC/DC implementation cannot properly handle decisions that
involve macros, like this example:
## Root cause
Some terms I'll use:
1. Decision region: the composite logical expression
2. Branch region: smaller conditions or branches within a larger expression
In the full lifecycle of these regions, here are several important stages:
1. Compilation
1. Generated at front end: [source
code](https://github.com/llvm/llvm-project/blob/a0b6747804e46665ecfd00295b60432bfe1775b6/clang/lib/CodeGen/CoverageMappingGen.cpp#L860-L903)
2. Written to the binary as coverage mapping sections: [source
code](https://github.com/llvm/llvm-project/blob/a0b6747804e46665ecfd00295b60432bfe1775b6/llvm/lib/ProfileData/Coverage/CoverageMappingWriter.cpp#L240-L256)
2. Generate report
1. Read from the binary: [source
code](https://github.com/llvm/llvm-project/blob/a0b6747804e46665ecfd00295b60432bfe1775b6/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp#L650-L703)
The assertion in question is in step 2.i and goes like this:
1. `llvm-cov` walks all regions of the target program
2. It encounters a decision region, say **R3**. By reading **R3**'s metadata,
it knows it has two branches
3. `llvm-cov` then asserts: the next two regions it's gonna see must be two
branch regions, not otherwise
This assertion assumes the order of MCDC regions. However, this order doesn't
always hold
In step 1.ii all regions are
[sorted](https://github.com/llvm/llvm-project/blob/a0b6747804e46665ecfd00295b60432bfe1775b6/llvm/lib/ProfileData/Coverage/CoverageMappingWriter.cpp#L162-L171)
first by their file IDs, then by locations within the file and finally by
region types. Macros, however, are traced back to definitions, which can be far
away from their invocations or even separated in different files. As a result,
the MC/DC regions could be sorted in a way where they are separated from each
other. In the shown example, the order could be: `R3 (many irrelevant regions)
R1 R2` which apparently breaks the assertion at step 2.i.
## Solution
This can be solved in two ways
1. (This PR) Sort with MC/DC in consideration and group relevant decisions and
branches together.
I honestly don't know if this's gonna break other things. But at least I
can confirm it solves the problem mentioned. Again please see an example in
this repo https://github.com/whentojump/llvm-mcdc-assertion-failure
2. In `CoverageMapping::loadFunctionRecord()`, use a cleverer way to correlate
decisions and branches that are sorted far away from each other.
## Other to-do's
- Tests are not yet taken care of
- A bit comments/docs
Last but not least, a huge thanks to @evodius96 et al for the great work
regarding MC/DC :))
>From daf74be7f20d20bcc54a541d50fc891bff8ae388 Mon Sep 17 00:00:00 2001
From: Wentao Zhang
Date: Tue, 30 Jan 2024 21:32:26 -0500
Subject: [PATCH 1/2] [Coverage][llvm-cov] Fix failures to handle MC/DC
decisions involving macros (1/2)
This patch lets MC/DC code regions for the same composite logical
expression get sorted next to each other, so that `llvm-cov show` won't
hit assertion failures.
The problematic cases are mostly when macros are involved:
(FOO(x) && BAR(y))
<-R2-><-R3->
<---R1--->
Let's call the full expression Region 1, `FOO(x)` Region 2 and `BAR(y)`
Region 3. If these macros are defined far away from their invocations,
the eventual order of all regions would look like: Region 1, (many other
irrelevant regions), Region 2, Region 3. This will break an assertion in
CoverageMapping::loadFunctionRecord() which assumes branch regions of an
MC/DC decision would immediately follow the decision region itself.
This patch adds a new field `GroupID` to `MCDCParameters` struct, sort
all regions based on this information, and place related MC/DC regions
together. The `GroupID` assignment is included in the other patch to
Clang CodeGen. This patch also disables 3 assertions that's based on the
old sorting criteria.
---
.../llvm/ProfileData/Coverage/CoverageMapping.h| 1 +
.../lib/ProfileData/Coverage/CoverageMappingReader.cpp | 8 ++--
.../lib/ProfileData/Coverage/CoverageMappingWriter.cpp | 10 +++---
3 files changed, 14 insertions(+), 5 deletions(-)
diff --git a/llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h
b/llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h
index 33fa17ba811fa..5ca5923cbd82e 100644
--- a/llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h
+++ b/llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h
@@ -260,6 +260,7 @@ struct CounterMappingRegion {
/// IDs used to represent a branch region and other branch
