[PATCH] D84988: [Coverage] Add empty line regions to SkippedRegions

[Zequan Wu via Phabricator via cfe-commits]

zequanwu added inline comments.


================
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
----------------
vsk wrote:
> Could you keep the original check and simply 'continue' if `L.Col == R.Col && L.IsSkipped`?
`if (L.Line == R.Line && L.Col == R.Col && !L.HasCount)` is more specific. 


================
Comment at: llvm/lib/ProfileData/Coverage/CoverageMapping.cpp:483
       bool GapRegion = CR.value().Kind == CounterMappingRegion::GapRegion;
 
       if (CR.index() + 1 == Regions.size() ||
----------------
vsk wrote:
> 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.
Since this broke a unit test, which has 5 zero-length regions points to same location, I think it's reasonable only emitting the second segment when active regions is not empty.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84988