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