vsk added a comment.
I'm not as familiar with the preprocessor bits, but this looks like it's in
good shape.
================
Comment at: llvm/lib/ProfileData/Coverage/CoverageMapping.cpp:597
const auto &R = Segments[I];
- if (!(L.Line < R.Line) && !(L.Line == R.Line && L.Col < R.Col)) {
+ if (!(L.Line < R.Line) && !(L.Line == R.Line && L.Col <= R.Col)) {
LLVM_DEBUG(dbgs() << " ! Segment " << L.Line << ":" << L.Col
----------------
Could you keep the original check and simply 'continue' if `L.Col == R.Col &&
L.IsSkipped`?
================
Comment at: llvm/lib/ProfileData/Coverage/CoverageMapping.cpp:483
bool GapRegion = CR.value().Kind == CounterMappingRegion::GapRegion;
if (CR.index() + 1 == Regions.size() ||
----------------
zequanwu wrote:
> vsk wrote:
> > zequanwu wrote:
> > > vsk wrote:
> > > > zequanwu wrote:
> > > > > vsk wrote:
> > > > > > Why is this deletion necessary? Do we need to introduce 0-length
> > > > > > regions which alter the count? An example would help.
> > > > > Because a single empty line will be a 0 length region. I don't know
> > > > > why is this condition necessary before. Does zero-length region
> > > > > exists before this change?
> > > > >
> > > > > example:
> > > > > ```
> > > > > int main() {
> > > > >
> > > > > return 0;
> > > > > }
> > > > > ```
> > > > > Before, llvm-cov gives the following.
> > > > > ```
> > > > > Counter in file 0 1:12 -> 4:2, #0
> > > > > Counter in file 0 2:1 -> 2:1, 0
> > > > > Emitting segments for file: /tmp/a.c
> > > > > Combined regions:
> > > > > 1:12 -> 4:2 (count=1)
> > > > > 2:1 -> 2:1 (count=0)
> > > > > Segment at 1:12 (count = 1), RegionEntry
> > > > > Segment at 2:1 (count = 0), RegionEntry, Skipped
> > > > > Segment at 4:2 (count = 0), Skipped
> > > > > 1| 1|int main() {
> > > > > 2| |
> > > > > 3| | return 0;
> > > > > 4| |}
> > > > > ```
> > > > > After:
> > > > > ```
> > > > > Counter in file 0 1:12 -> 4:2, #0
> > > > > Counter in file 0 2:1 -> 2:1, 0
> > > > > Emitting segments for file: /tmp/a.c
> > > > > Combined regions:
> > > > > 1:12 -> 4:2 (count=1)
> > > > > 2:1 -> 2:1 (count=0)
> > > > > Segment at 1:12 (count = 1), RegionEntry
> > > > > Segment at 2:1 (count = 0), RegionEntry, Skipped
> > > > > Segment at 2:1 (count = 1)
> > > > > Segment at 4:2 (count = 0), Skipped
> > > > > 1| 1|int main() {
> > > > > 2| |
> > > > > 3| 1| return 0;
> > > > > 4| 1|}
> > > > > ```
> > > > It looks like we do occasionally see 0-length regions, possibly due to
> > > > bugs in the frontend
> > > > (http://lab.llvm.org:8080/coverage/coverage-reports/coverage/Users/buildslave/jenkins/workspace/coverage/llvm-project/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp.html#L485).
> > > >
> > > > I don't expect the reporting tools to be able to handle duplicate
> > > > segments in a robust way. Having duplicate segments was the source of
> > > > "messed up" coverage bugs in the past, due to the contradiction
> > > > inherent in having two different segments begin at the same source
> > > > location.
> > > >
> > > > Do you see some other way to represent empty lines? Perhaps, if we emit
> > > > a skipped region for an empty line, we can emit a follow-up segment
> > > > that restores the previously-active region starting on the next line?
> > > > So in this case:
> > > >
> > > > Segment at 1:12 (count = 1), RegionEntry
> > > > Segment at 2:1 (count = 0), RegionEntry, Skipped
> > > > Segment at 3:1 (count = 1)
> > > > Segment at 4:2 (count = 0), Skipped
> > > I think we should have the following, because the wrapped segment's count
> > > will be used in next line (e.g. line 3).
> > > ```
> > > Segment at 1:12 (count = 1), RegionEntry
> > > Segment at 2:1 (count = 0), RegionEntry, Skipped
> > > Segment at 2:1 (count = 1)
> > > Segment at 4:2 (count = 0), Skipped
> > > ```
> > I think one issue with that output is that it contains two segments that
> > start at the same location (2:1). Historically, this sort of duplication
> > was a source of bugs/inconsistencies (e.g two entry regions beginning at
> > the same location with different counts), and I’m concerned that
> > re-allowing multiple segments with the same start location could lead to
> > regressions down the road.
> >
> > OTOH, your change is consistent with how non-zero length segments are
> > handled, and it could be fragile to look for an alternative start location
> > (like 3:1) that restores the count from before the skipped region.
> >
> > I’d be curious to know your thoughts on how to prevent regressions related
> > to segments which share the same start location. Maybe they could only be
> > allowed in this limited case?
> Yes, they are two segments with the same location, but only the first is a
> RegionEntry (might not cause the same bug you saw before). So the second one
> is only used as wrapped segment, and will not conflict with first segment
> RegionEntry.
> We could only emit the second segment when the first segment is skipped
> segment.
That carveout sounds reasonable to me.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D84988/new/
https://reviews.llvm.org/D84988
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
