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

[Zequan Wu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG9caa3fbe03f4: [Coverage] Add empty line regions to 
SkippedRegions (authored by zequanwu).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84988

Files:
  clang/include/clang/Lex/Lexer.h
  clang/include/clang/Lex/Preprocessor.h
  clang/lib/CodeGen/CoverageMappingGen.cpp
  clang/lib/CodeGen/CoverageMappingGen.h
  clang/lib/Lex/Lexer.cpp
  clang/lib/Lex/Preprocessor.cpp
  clang/test/CoverageMapping/abspath.cpp
  clang/test/CoverageMapping/block-storage-starts-region.m
  clang/test/CoverageMapping/break.c
  clang/test/CoverageMapping/builtinmacro.c
  clang/test/CoverageMapping/casts.c
  clang/test/CoverageMapping/classtemplate.cpp
  clang/test/CoverageMapping/comment-in-macro.c
  clang/test/CoverageMapping/continue.c
  clang/test/CoverageMapping/control-flow-macro.c
  clang/test/CoverageMapping/coroutine.cpp
  clang/test/CoverageMapping/decl.c
  clang/test/CoverageMapping/default-method.cpp
  clang/test/CoverageMapping/deferred-region.cpp
  clang/test/CoverageMapping/empty-destructor.cpp
  clang/test/CoverageMapping/header.cpp
  clang/test/CoverageMapping/if.cpp
  clang/test/CoverageMapping/implicit-def-in-macro.m
  clang/test/CoverageMapping/include-macros.c
  clang/test/CoverageMapping/includehell.cpp
  clang/test/CoverageMapping/ir.c
  clang/test/CoverageMapping/label.cpp
  clang/test/CoverageMapping/lambda.cpp
  clang/test/CoverageMapping/logical.cpp
  clang/test/CoverageMapping/loopmacro.c
  clang/test/CoverageMapping/loops.cpp
  clang/test/CoverageMapping/macro-expansion.c
  clang/test/CoverageMapping/macro-expressions.cpp
  clang/test/CoverageMapping/macro-stringize-twice.cpp
  clang/test/CoverageMapping/macroception.c
  clang/test/CoverageMapping/macroparams.c
  clang/test/CoverageMapping/macroparams2.c
  clang/test/CoverageMapping/macros.c
  clang/test/CoverageMapping/macroscopes.cpp
  clang/test/CoverageMapping/md.cpp
  clang/test/CoverageMapping/moremacros.c
  clang/test/CoverageMapping/nestedclass.cpp
  clang/test/CoverageMapping/objc.m
  clang/test/CoverageMapping/openmp.c
  clang/test/CoverageMapping/pr32679.cpp
  clang/test/CoverageMapping/preprocessor.c
  clang/test/CoverageMapping/return.c
  clang/test/CoverageMapping/switch.cpp
  clang/test/CoverageMapping/switchmacro.c
  clang/test/CoverageMapping/system_macro.cpp
  clang/test/CoverageMapping/templates.cpp
  clang/test/CoverageMapping/test.c
  clang/test/CoverageMapping/trycatch.cpp
  clang/test/CoverageMapping/trymacro.cpp
  clang/test/CoverageMapping/unreachable-macro.c
  clang/test/CoverageMapping/unused_function.cpp
  clang/test/CoverageMapping/unused_names.c
  clang/test/CoverageMapping/while.c
  clang/test/lit.cfg.py
  compiler-rt/test/profile/coverage_emptylines.cpp
  compiler-rt/test/profile/instrprof-set-file-object-merging.c
  compiler-rt/test/profile/instrprof-set-file-object.c
  llvm/lib/ProfileData/Coverage/CoverageMapping.cpp

Index: llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
===
--- llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
+++ llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
@@ -485,9 +485,15 @@
   if (CurStartLoc == CR.value().endLoc()) {
 // Avoid making zero-length regions active. If it's the last region,
 // emit a skipped segment. Otherwise use its predecessor's count.
-const bool Skipped = (CR.index() + 1) == Regions.size();
+const bool Skipped =
+(CR.index() + 1) == Regions.size() ||
+CR.value().Kind == CounterMappingRegion::SkippedRegion;
 startSegment(ActiveRegions.empty() ? CR.value() : *ActiveRegions.back(),
  CurStartLoc, !GapRegion, Skipped);
+// If it is skipped segment, create a segment with last pushed
+// regions's count at CurStartLoc.
+if (Skipped && !ActiveRegions.empty())
+  startSegment(*ActiveRegions.back(), CurStartLoc, false);
 continue;
   }
   if (CR.index() + 1 == Regions.size() ||
@@ -587,6 +593,8 @@
   const auto  = Segments[I - 1];
   const auto  = Segments[I];
   if (!(L.Line < R.Line) && !(L.Line == R.Line && L.Col < R.Col)) {
+if (L.Line == R.Line && L.Col == R.Col && !L.HasCount)
+  continue;
 LLVM_DEBUG(dbgs() << " ! Segment " << L.Line << ":" << L.Col
   << " followed by " << R.Line << ":" << R.Col << "\n");
 assert(false && "Coverage segments not unique or sorted");
Index: compiler-rt/test/profile/instrprof-set-file-object.c
===
--- compiler-rt/test/profile/instrprof-set-file-object.c
+++ compiler-rt/test/profile/instrprof-set-file-object.c
@@ -24,7 +24,7 @@
 // CHECK:   12|  1|int main(int argc, const char *argv[]) {
 // CHECK:   13|  1|  if (argc < 2)
 // CHECK: 

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

[Zequan Wu via Phabricator via cfe-commits]

zequanwu added a comment.

In D84988#2282849 , @vsk wrote:

> @zequanwu FTR, I don't have any outstanding concerns (I understand you might 
> be asking for a second reviewer to chime in though).

Thanks for reviewing. Then, I think it might be ready to land.


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

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

[Vedant Kumar via Phabricator via cfe-commits]

vsk added a comment.

@zequanwu FTR, I don't have any outstanding concerns (I understand you might be 
asking for a second reviewer to chime in though).


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

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

[Zequan Wu via Phabricator via cfe-commits]

zequanwu added a comment.

Friendly ping.


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

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

[Zequan Wu via Phabricator via cfe-commits]

zequanwu updated this revision to Diff 291630.
zequanwu added a comment.

Rename option EmptyLineCoverage to EmptyLineCommentCoverage and mark it 
cl::Hidden.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84988

Files:
  clang/include/clang/Lex/Lexer.h
  clang/include/clang/Lex/Preprocessor.h
  clang/lib/CodeGen/CoverageMappingGen.cpp
  clang/lib/CodeGen/CoverageMappingGen.h
  clang/lib/Lex/Lexer.cpp
  clang/lib/Lex/Preprocessor.cpp
  clang/test/CoverageMapping/abspath.cpp
  clang/test/CoverageMapping/block-storage-starts-region.m
  clang/test/CoverageMapping/break.c
  clang/test/CoverageMapping/builtinmacro.c
  clang/test/CoverageMapping/casts.c
  clang/test/CoverageMapping/classtemplate.cpp
  clang/test/CoverageMapping/comment-in-macro.c
  clang/test/CoverageMapping/continue.c
  clang/test/CoverageMapping/control-flow-macro.c
  clang/test/CoverageMapping/coroutine.cpp
  clang/test/CoverageMapping/decl.c
  clang/test/CoverageMapping/default-method.cpp
  clang/test/CoverageMapping/deferred-region.cpp
  clang/test/CoverageMapping/empty-destructor.cpp
  clang/test/CoverageMapping/header.cpp
  clang/test/CoverageMapping/if.cpp
  clang/test/CoverageMapping/implicit-def-in-macro.m
  clang/test/CoverageMapping/include-macros.c
  clang/test/CoverageMapping/includehell.cpp
  clang/test/CoverageMapping/ir.c
  clang/test/CoverageMapping/label.cpp
  clang/test/CoverageMapping/lambda.cpp
  clang/test/CoverageMapping/logical.cpp
  clang/test/CoverageMapping/loopmacro.c
  clang/test/CoverageMapping/loops.cpp
  clang/test/CoverageMapping/macro-expansion.c
  clang/test/CoverageMapping/macro-expressions.cpp
  clang/test/CoverageMapping/macro-stringize-twice.cpp
  clang/test/CoverageMapping/macroception.c
  clang/test/CoverageMapping/macroparams.c
  clang/test/CoverageMapping/macroparams2.c
  clang/test/CoverageMapping/macros.c
  clang/test/CoverageMapping/macroscopes.cpp
  clang/test/CoverageMapping/md.cpp
  clang/test/CoverageMapping/moremacros.c
  clang/test/CoverageMapping/nestedclass.cpp
  clang/test/CoverageMapping/objc.m
  clang/test/CoverageMapping/openmp.c
  clang/test/CoverageMapping/pr32679.cpp
  clang/test/CoverageMapping/preprocessor.c
  clang/test/CoverageMapping/return.c
  clang/test/CoverageMapping/switch.cpp
  clang/test/CoverageMapping/switchmacro.c
  clang/test/CoverageMapping/system_macro.cpp
  clang/test/CoverageMapping/templates.cpp
  clang/test/CoverageMapping/test.c
  clang/test/CoverageMapping/trycatch.cpp
  clang/test/CoverageMapping/trymacro.cpp
  clang/test/CoverageMapping/unreachable-macro.c
  clang/test/CoverageMapping/unused_function.cpp
  clang/test/CoverageMapping/unused_names.c
  clang/test/CoverageMapping/while.c
  clang/test/lit.cfg.py
  compiler-rt/test/profile/coverage_emptylines.cpp
  compiler-rt/test/profile/instrprof-set-file-object-merging.c
  compiler-rt/test/profile/instrprof-set-file-object.c
  llvm/lib/ProfileData/Coverage/CoverageMapping.cpp

Index: llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
===
--- llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
+++ llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
@@ -485,9 +485,15 @@
   if (CurStartLoc == CR.value().endLoc()) {
 // Avoid making zero-length regions active. If it's the last region,
 // emit a skipped segment. Otherwise use its predecessor's count.
-const bool Skipped = (CR.index() + 1) == Regions.size();
+const bool Skipped =
+(CR.index() + 1) == Regions.size() ||
+CR.value().Kind == CounterMappingRegion::SkippedRegion;
 startSegment(ActiveRegions.empty() ? CR.value() : *ActiveRegions.back(),
  CurStartLoc, !GapRegion, Skipped);
+// If it is skipped segment, create a segment with last pushed
+// regions's count at CurStartLoc.
+if (Skipped && !ActiveRegions.empty())
+  startSegment(*ActiveRegions.back(), CurStartLoc, false);
 continue;
   }
   if (CR.index() + 1 == Regions.size() ||
@@ -587,6 +593,8 @@
   const auto  = Segments[I - 1];
   const auto  = Segments[I];
   if (!(L.Line < R.Line) && !(L.Line == R.Line && L.Col < R.Col)) {
+if (L.Line == R.Line && L.Col == R.Col && !L.HasCount)
+  continue;
 LLVM_DEBUG(dbgs() << " ! Segment " << L.Line << ":" << L.Col
   << " followed by " << R.Line << ":" << R.Col << "\n");
 assert(false && "Coverage segments not unique or sorted");
Index: compiler-rt/test/profile/instrprof-set-file-object.c
===
--- compiler-rt/test/profile/instrprof-set-file-object.c
+++ compiler-rt/test/profile/instrprof-set-file-object.c
@@ -24,7 +24,7 @@
 // CHECK:   12|  1|int main(int argc, const char *argv[]) {
 // CHECK:   13|  1|  if (argc < 2)
 // CHECK:   14|  0|return 

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

[Vedant Kumar via Phabricator via cfe-commits]

vsk added inline comments.



Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:38
+   "disable it on test)"),
+llvm::cl::init(true));
+

We might consider marking this as cl::Hidden.


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

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

[Zequan Wu via Phabricator via cfe-commits]

zequanwu updated this revision to Diff 291118.
zequanwu added a comment.
Herald added a reviewer: jdoerfert.
Herald added a subscriber: sstefan1.

- Address comment.
- Add cl::opt to disable emitting skipped regions for empty lines and comments 
(used on test case only), and update test cases under CoverageMapping with 
`-mllvm -emptyline-comment-coverage=false`.
- Remove the logic which only emits skipped regions for comments and empty 
lines parsing function body, because can't tell if it's inside class member 
function.
- Always set `SR.ColumnStart` to 1 in `adjustSkippedRange`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84988

Files:
  clang/include/clang/Lex/Lexer.h
  clang/include/clang/Lex/Preprocessor.h
  clang/lib/CodeGen/CoverageMappingGen.cpp
  clang/lib/CodeGen/CoverageMappingGen.h
  clang/lib/Lex/Lexer.cpp
  clang/lib/Lex/Preprocessor.cpp
  clang/test/CoverageMapping/abspath.cpp
  clang/test/CoverageMapping/block-storage-starts-region.m
  clang/test/CoverageMapping/break.c
  clang/test/CoverageMapping/builtinmacro.c
  clang/test/CoverageMapping/casts.c
  clang/test/CoverageMapping/classtemplate.cpp
  clang/test/CoverageMapping/comment-in-macro.c
  clang/test/CoverageMapping/continue.c
  clang/test/CoverageMapping/control-flow-macro.c
  clang/test/CoverageMapping/coroutine.cpp
  clang/test/CoverageMapping/decl.c
  clang/test/CoverageMapping/default-method.cpp
  clang/test/CoverageMapping/deferred-region.cpp
  clang/test/CoverageMapping/empty-destructor.cpp
  clang/test/CoverageMapping/header.cpp
  clang/test/CoverageMapping/if.cpp
  clang/test/CoverageMapping/implicit-def-in-macro.m
  clang/test/CoverageMapping/include-macros.c
  clang/test/CoverageMapping/includehell.cpp
  clang/test/CoverageMapping/ir.c
  clang/test/CoverageMapping/label.cpp
  clang/test/CoverageMapping/lambda.cpp
  clang/test/CoverageMapping/logical.cpp
  clang/test/CoverageMapping/loopmacro.c
  clang/test/CoverageMapping/loops.cpp
  clang/test/CoverageMapping/macro-expansion.c
  clang/test/CoverageMapping/macro-expressions.cpp
  clang/test/CoverageMapping/macro-stringize-twice.cpp
  clang/test/CoverageMapping/macroception.c
  clang/test/CoverageMapping/macroparams.c
  clang/test/CoverageMapping/macroparams2.c
  clang/test/CoverageMapping/macros.c
  clang/test/CoverageMapping/macroscopes.cpp
  clang/test/CoverageMapping/md.cpp
  clang/test/CoverageMapping/moremacros.c
  clang/test/CoverageMapping/nestedclass.cpp
  clang/test/CoverageMapping/objc.m
  clang/test/CoverageMapping/openmp.c
  clang/test/CoverageMapping/pr32679.cpp
  clang/test/CoverageMapping/preprocessor.c
  clang/test/CoverageMapping/return.c
  clang/test/CoverageMapping/switch.cpp
  clang/test/CoverageMapping/switchmacro.c
  clang/test/CoverageMapping/system_macro.cpp
  clang/test/CoverageMapping/templates.cpp
  clang/test/CoverageMapping/test.c
  clang/test/CoverageMapping/trycatch.cpp
  clang/test/CoverageMapping/trymacro.cpp
  clang/test/CoverageMapping/unreachable-macro.c
  clang/test/CoverageMapping/unused_function.cpp
  clang/test/CoverageMapping/unused_names.c
  clang/test/CoverageMapping/while.c
  clang/test/lit.cfg.py
  compiler-rt/test/profile/coverage_emptylines.cpp
  compiler-rt/test/profile/instrprof-set-file-object-merging.c
  compiler-rt/test/profile/instrprof-set-file-object.c
  llvm/lib/ProfileData/Coverage/CoverageMapping.cpp

Index: llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
===
--- llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
+++ llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
@@ -485,9 +485,15 @@
   if (CurStartLoc == CR.value().endLoc()) {
 // Avoid making zero-length regions active. If it's the last region,
 // emit a skipped segment. Otherwise use its predecessor's count.
-const bool Skipped = (CR.index() + 1) == Regions.size();
+const bool Skipped =
+(CR.index() + 1) == Regions.size() ||
+CR.value().Kind == CounterMappingRegion::SkippedRegion;
 startSegment(ActiveRegions.empty() ? CR.value() : *ActiveRegions.back(),
  CurStartLoc, !GapRegion, Skipped);
+// If it is skipped segment, create a segment with last pushed
+// regions's count at CurStartLoc.
+if (Skipped && !ActiveRegions.empty())
+  startSegment(*ActiveRegions.back(), CurStartLoc, false);
 continue;
   }
   if (CR.index() + 1 == Regions.size() ||
@@ -587,6 +593,8 @@
   const auto  = Segments[I - 1];
   const auto  = Segments[I];
   if (!(L.Line < R.Line) && !(L.Line == R.Line && L.Col < R.Col)) {
+if (L.Line == R.Line && L.Col == R.Col && !L.HasCount)
+  continue;
 LLVM_DEBUG(dbgs() << " ! Segment " << L.Line << ":" << L.Col
   << " followed by " << R.Line << ":" << R.Col << "\n");
 assert(false && "Coverage 

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

[Vedant Kumar via Phabricator via cfe-commits]

vsk added a comment.

In D84988#2264199 , @zequanwu wrote:

> Thanks for reviewing. One last thing I need to resolve is to handle the 
> coverage test case as skipped regions are emitted for empty lines, and 
> haven't thought a good way other than adding checks for those new skipped 
> regions.

How does it sound to add a testing-only cl::opt that disables whitespace 
skipped regions? Tests that aren't focused on validating skipped regions can 
set this cl::opt.


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

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

[Zequan Wu via Phabricator via cfe-commits]

zequanwu added a comment.

Thanks for reviewing. One last thing I need to resolve is to handle the 
coverage test case as skipped regions are emitted for empty lines, and haven't 
thought a good way other than adding checks for those new skipped regions.


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

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

[Vedant Kumar via Phabricator via cfe-commits]

vsk accepted this revision.
vsk added a comment.
This revision is now accepted and ready to land.

I took a closer look at the lexer changes, and think that these look fine. 
Thanks again for working on this. LGTM with a minor change, described inline.




Comment at: clang/lib/Lex/Lexer.cpp:2228
+if (!NewLinePtr && *CurPtr == '\n')
+  NewLinePtr = CurPtr;
 SawNewline = true;

It'd be more consistent to write this the way `lastNewLine` is initialized 
earlier in `SkipWhitespace`, e.g. as:

```
if (*CurPtr == '\n') {
  lastNewLine = CurPtr;
  if (!NewLinePtr)
NewLinePtr = CurPtr;
}
```

Maybe this could even be factored into a helper lambda, like 
`setLastNewLine(char *Ptr)` -- I'm not sure whether that'd be cleaner.


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

[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  = 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 

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

[Zequan Wu via Phabricator via cfe-commits]

zequanwu updated this revision to Diff 289617.
zequanwu added a comment.

address comment.
emit second segment when first segment is skipped and activeregions is not 
empty.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84988

Files:
  clang/include/clang/Lex/Lexer.h
  clang/include/clang/Lex/Preprocessor.h
  clang/lib/CodeGen/CoverageMappingGen.cpp
  clang/lib/CodeGen/CoverageMappingGen.h
  clang/lib/Lex/Lexer.cpp
  clang/lib/Lex/Preprocessor.cpp
  clang/lib/Parse/ParseStmt.cpp
  compiler-rt/test/profile/coverage_emptylines.cpp
  compiler-rt/test/profile/instrprof-set-file-object-merging.c
  compiler-rt/test/profile/instrprof-set-file-object.c
  llvm/lib/ProfileData/Coverage/CoverageMapping.cpp

Index: llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
===
--- llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
+++ llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
@@ -485,9 +485,15 @@
   if (CurStartLoc == CR.value().endLoc()) {
 // Avoid making zero-length regions active. If it's the last region,
 // emit a skipped segment. Otherwise use its predecessor's count.
-const bool Skipped = (CR.index() + 1) == Regions.size();
+const bool Skipped =
+(CR.index() + 1) == Regions.size() ||
+CR.value().Kind == CounterMappingRegion::SkippedRegion;
 startSegment(ActiveRegions.empty() ? CR.value() : *ActiveRegions.back(),
  CurStartLoc, !GapRegion, Skipped);
+// If it is skipped segment, create a segment with last pushed
+// regions's count at CurStartLoc.
+if (Skipped && !ActiveRegions.empty())
+  startSegment(*ActiveRegions.back(), CurStartLoc, false);
 continue;
   }
   if (CR.index() + 1 == Regions.size() ||
@@ -587,6 +593,8 @@
   const auto  = Segments[I - 1];
   const auto  = Segments[I];
   if (!(L.Line < R.Line) && !(L.Line == R.Line && L.Col < R.Col)) {
+if (L.Line == R.Line && L.Col == R.Col && !L.HasCount)
+  continue;
 LLVM_DEBUG(dbgs() << " ! Segment " << L.Line << ":" << L.Col
   << " followed by " << R.Line << ":" << R.Col << "\n");
 assert(false && "Coverage segments not unique or sorted");
Index: compiler-rt/test/profile/instrprof-set-file-object.c
===
--- compiler-rt/test/profile/instrprof-set-file-object.c
+++ compiler-rt/test/profile/instrprof-set-file-object.c
@@ -24,7 +24,7 @@
 // CHECK:   12|  1|int main(int argc, const char *argv[]) {
 // CHECK:   13|  1|  if (argc < 2)
 // CHECK:   14|  0|return 1;
-// CHECK:   15|  1|
+// CHECK:   15|   |
 // CHECK:   16|  1|  FILE *F = fopen(argv[1], "w+b");
 // CHECK:   17|  1|  __llvm_profile_set_file_object(F, 0);
 // CHECK:   18|  1|  return 0;
Index: compiler-rt/test/profile/instrprof-set-file-object-merging.c
===
--- compiler-rt/test/profile/instrprof-set-file-object-merging.c
+++ compiler-rt/test/profile/instrprof-set-file-object-merging.c
@@ -31,13 +31,13 @@
 // CHECK:   14|  2|int main(int argc, const char *argv[]) {
 // CHECK:   15|  2|  if (argc < 2)
 // CHECK:   16|  0|return 1;
-// CHECK:   17|  2|
+// CHECK:   17|   |
 // CHECK:   18|  2|  FILE *F = fopen(argv[1], "r+b");
 // CHECK:   19|  2|  if (!F) {
 // CHECK:   20|   |// File might not exist, try opening with truncation
 // CHECK:   21|  1|F = fopen(argv[1], "w+b");
 // CHECK:   22|  1|  }
 // CHECK:   23|  2|  __llvm_profile_set_file_object(F, 1);
-// CHECK:   24|  2|
+// CHECK:   24|   |
 // CHECK:   25|  2|  return 0;
 // CHECK:   26|  2|}
Index: compiler-rt/test/profile/coverage_emptylines.cpp
===
--- /dev/null
+++ compiler-rt/test/profile/coverage_emptylines.cpp
@@ -0,0 +1,61 @@
+// Remove comments first.
+// RUN: sed 's/[ \t]*\/\/.*//' %s > %t.stripped.cpp
+// RUN: %clangxx_profgen -fcoverage-mapping -o %t %t.stripped.cpp
+// RUN: env LLVM_PROFILE_FILE=%t.profraw %run %t
+// RUN: llvm-profdata merge -o %t.profdata %t.profraw
+// RUN: llvm-cov show %t -instr-profile %t.profdata -path-equivalence=/tmp,%S 2>&1 | FileCheck %s
+
+
+int main() {// CHECK:   [[# @LINE]]| 1|int main() {
+int x = 0;  // CHECK-NEXT:  [[# @LINE]]| 1|
+// CHECK-NEXT:  [[# @LINE]]|  |
+x = 1;  // CHECK-NEXT:  [[# @LINE]]| 1|
+if (x)  // CHECK-NEXT:  [[# @LINE]]| 1|
+// CHECK-NEXT:  [[# @LINE]]|  |
+x   // CHECK-NEXT:  [[# @LINE]]| 1|
+ 

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

[Vedant Kumar via Phabricator via cfe-commits]

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  = 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 

[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:483
   bool GapRegion = CR.value().Kind == CounterMappingRegion::GapRegion;
 
   if (CR.index() + 1 == Regions.size() ||

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.


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

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

[Zequan Wu via Phabricator via cfe-commits]

zequanwu updated this revision to Diff 289582.
zequanwu added a comment.

Emit second segment as wrapped segment only when first segment is RegionEntry


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84988

Files:
  clang/include/clang/Lex/Lexer.h
  clang/include/clang/Lex/Preprocessor.h
  clang/lib/CodeGen/CoverageMappingGen.cpp
  clang/lib/CodeGen/CoverageMappingGen.h
  clang/lib/Lex/Lexer.cpp
  clang/lib/Lex/Preprocessor.cpp
  clang/lib/Parse/ParseStmt.cpp
  compiler-rt/test/profile/coverage_emptylines.cpp
  compiler-rt/test/profile/instrprof-set-file-object-merging.c
  compiler-rt/test/profile/instrprof-set-file-object.c
  llvm/lib/ProfileData/Coverage/CoverageMapping.cpp

Index: llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
===
--- llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
+++ llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
@@ -485,9 +485,17 @@
   if (CurStartLoc == CR.value().endLoc()) {
 // Avoid making zero-length regions active. If it's the last region,
 // emit a skipped segment. Otherwise use its predecessor's count.
-const bool Skipped = (CR.index() + 1) == Regions.size();
+const bool Skipped =
+(CR.index() + 1) == Regions.size() ||
+CR.value().Kind == CounterMappingRegion::SkippedRegion;
 startSegment(ActiveRegions.empty() ? CR.value() : *ActiveRegions.back(),
  CurStartLoc, !GapRegion, Skipped);
+// If it is skipped segment, create a segment with last pushed
+// regions's count at CurStartLoc.
+if (Skipped)
+  startSegment(ActiveRegions.empty() ? CR.value()
+ : *ActiveRegions.back(),
+   CurStartLoc, false);
 continue;
   }
   if (CR.index() + 1 == Regions.size() ||
@@ -586,7 +594,7 @@
 for (unsigned I = 1, E = Segments.size(); I < E; ++I) {
   const auto  = Segments[I - 1];
   const auto  = 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
   << " followed by " << R.Line << ":" << R.Col << "\n");
 assert(false && "Coverage segments not unique or sorted");
Index: compiler-rt/test/profile/instrprof-set-file-object.c
===
--- compiler-rt/test/profile/instrprof-set-file-object.c
+++ compiler-rt/test/profile/instrprof-set-file-object.c
@@ -24,7 +24,7 @@
 // CHECK:   12|  1|int main(int argc, const char *argv[]) {
 // CHECK:   13|  1|  if (argc < 2)
 // CHECK:   14|  0|return 1;
-// CHECK:   15|  1|
+// CHECK:   15|   |
 // CHECK:   16|  1|  FILE *F = fopen(argv[1], "w+b");
 // CHECK:   17|  1|  __llvm_profile_set_file_object(F, 0);
 // CHECK:   18|  1|  return 0;
Index: compiler-rt/test/profile/instrprof-set-file-object-merging.c
===
--- compiler-rt/test/profile/instrprof-set-file-object-merging.c
+++ compiler-rt/test/profile/instrprof-set-file-object-merging.c
@@ -31,13 +31,13 @@
 // CHECK:   14|  2|int main(int argc, const char *argv[]) {
 // CHECK:   15|  2|  if (argc < 2)
 // CHECK:   16|  0|return 1;
-// CHECK:   17|  2|
+// CHECK:   17|   |
 // CHECK:   18|  2|  FILE *F = fopen(argv[1], "r+b");
 // CHECK:   19|  2|  if (!F) {
 // CHECK:   20|   |// File might not exist, try opening with truncation
 // CHECK:   21|  1|F = fopen(argv[1], "w+b");
 // CHECK:   22|  1|  }
 // CHECK:   23|  2|  __llvm_profile_set_file_object(F, 1);
-// CHECK:   24|  2|
+// CHECK:   24|   |
 // CHECK:   25|  2|  return 0;
 // CHECK:   26|  2|}
Index: compiler-rt/test/profile/coverage_emptylines.cpp
===
--- /dev/null
+++ compiler-rt/test/profile/coverage_emptylines.cpp
@@ -0,0 +1,61 @@
+// Remove comments first.
+// RUN: sed 's/[ \t]*\/\/.*//' %s > %t.stripped.cpp
+// RUN: %clangxx_profgen -fcoverage-mapping -o %t %t.stripped.cpp
+// RUN: env LLVM_PROFILE_FILE=%t.profraw %run %t
+// RUN: llvm-profdata merge -o %t.profdata %t.profraw
+// RUN: llvm-cov show %t -instr-profile %t.profdata -path-equivalence=/tmp,%S 2>&1 | FileCheck %s
+
+
+int main() {// CHECK:   [[# @LINE]]| 1|int main() {
+int x = 0;  // CHECK-NEXT:  [[# @LINE]]| 1|
+// CHECK-NEXT:  [[# @LINE]]|  |
+x = 1;  // CHECK-NEXT:  [[# @LINE]]| 1|
+if (x)  // CHECK-NEXT:  [[# @LINE]]| 1|
+// CHECK-NEXT:  [[# 

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

[Vedant Kumar via Phabricator via cfe-commits]

vsk added inline comments.



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:
> > > > 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?


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

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

[Zequan Wu via Phabricator via cfe-commits]

zequanwu updated this revision to Diff 289047.
zequanwu added a comment.

update.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84988

Files:
  clang/include/clang/Lex/Lexer.h
  clang/include/clang/Lex/Preprocessor.h
  clang/lib/CodeGen/CoverageMappingGen.cpp
  clang/lib/CodeGen/CoverageMappingGen.h
  clang/lib/Lex/Lexer.cpp
  clang/lib/Lex/Preprocessor.cpp
  clang/lib/Parse/ParseStmt.cpp
  compiler-rt/test/profile/coverage_emptylines.cpp
  compiler-rt/test/profile/instrprof-set-file-object-merging.c
  compiler-rt/test/profile/instrprof-set-file-object.c
  llvm/lib/ProfileData/Coverage/CoverageMapping.cpp

Index: llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
===
--- llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
+++ llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
@@ -480,16 +480,23 @@
   }
 
   bool GapRegion = CR.value().Kind == CounterMappingRegion::GapRegion;
+  bool SkippedRegion =
+  CR.value().Kind == CounterMappingRegion::SkippedRegion;
 
   // Try to emit a segment for the current region.
   if (CurStartLoc == CR.value().endLoc()) {
 // Avoid making zero-length regions active. If it's the last region,
 // emit a skipped segment. Otherwise use its predecessor's count.
-const bool Skipped = (CR.index() + 1) == Regions.size();
+const bool Skipped =
+(CR.index() + 1) == Regions.size() || SkippedRegion;
 startSegment(ActiveRegions.empty() ? CR.value() : *ActiveRegions.back(),
  CurStartLoc, !GapRegion, Skipped);
+// Create a segment with last pushed regions's count at CurStartLoc.
+startSegment(ActiveRegions.empty() ? CR.value() : *ActiveRegions.back(),
+ CurStartLoc, false);
 continue;
   }
+
   if (CR.index() + 1 == Regions.size() ||
   CurStartLoc != Regions[CR.index() + 1].startLoc()) {
 // Emit a segment if the next region doesn't start at the same location
@@ -586,7 +593,7 @@
 for (unsigned I = 1, E = Segments.size(); I < E; ++I) {
   const auto  = Segments[I - 1];
   const auto  = 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
   << " followed by " << R.Line << ":" << R.Col << "\n");
 assert(false && "Coverage segments not unique or sorted");
Index: compiler-rt/test/profile/instrprof-set-file-object.c
===
--- compiler-rt/test/profile/instrprof-set-file-object.c
+++ compiler-rt/test/profile/instrprof-set-file-object.c
@@ -24,7 +24,7 @@
 // CHECK:   12|  1|int main(int argc, const char *argv[]) {
 // CHECK:   13|  1|  if (argc < 2)
 // CHECK:   14|  0|return 1;
-// CHECK:   15|  1|
+// CHECK:   15|   |
 // CHECK:   16|  1|  FILE *F = fopen(argv[1], "w+b");
 // CHECK:   17|  1|  __llvm_profile_set_file_object(F, 0);
 // CHECK:   18|  1|  return 0;
Index: compiler-rt/test/profile/instrprof-set-file-object-merging.c
===
--- compiler-rt/test/profile/instrprof-set-file-object-merging.c
+++ compiler-rt/test/profile/instrprof-set-file-object-merging.c
@@ -31,13 +31,13 @@
 // CHECK:   14|  2|int main(int argc, const char *argv[]) {
 // CHECK:   15|  2|  if (argc < 2)
 // CHECK:   16|  0|return 1;
-// CHECK:   17|  2|
+// CHECK:   17|   |
 // CHECK:   18|  2|  FILE *F = fopen(argv[1], "r+b");
 // CHECK:   19|  2|  if (!F) {
 // CHECK:   20|   |// File might not exist, try opening with truncation
 // CHECK:   21|  1|F = fopen(argv[1], "w+b");
 // CHECK:   22|  1|  }
 // CHECK:   23|  2|  __llvm_profile_set_file_object(F, 1);
-// CHECK:   24|  2|
+// CHECK:   24|   |
 // CHECK:   25|  2|  return 0;
 // CHECK:   26|  2|}
Index: compiler-rt/test/profile/coverage_emptylines.cpp
===
--- /dev/null
+++ compiler-rt/test/profile/coverage_emptylines.cpp
@@ -0,0 +1,61 @@
+// Remove comments first.
+// RUN: sed 's/[ \t]*\/\/.*//' %s > %t.stripped.cpp
+// RUN: %clangxx_profgen -fcoverage-mapping -o %t %t.stripped.cpp
+// RUN: env LLVM_PROFILE_FILE=%t.profraw %run %t
+// RUN: llvm-profdata merge -o %t.profdata %t.profraw
+// RUN: llvm-cov show %t -instr-profile %t.profdata -path-equivalence=/tmp,%S 2>&1 | FileCheck %s
+
+
+int main() {// CHECK:   [[# @LINE]]| 1|int main() {
+int x = 0;  // CHECK-NEXT:  [[# @LINE]]| 1|
+// CHECK-NEXT:  [[# @LINE]]|  |
+x = 1;  // 

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

[Zequan Wu via Phabricator via cfe-commits]

zequanwu added a comment.

Friendly ping.


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

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

[Zequan Wu via Phabricator via cfe-commits]

zequanwu updated this revision to Diff 286688.
zequanwu added a comment.

minor fix.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84988

Files:
  clang/include/clang/Lex/Lexer.h
  clang/include/clang/Lex/Preprocessor.h
  clang/lib/CodeGen/CoverageMappingGen.cpp
  clang/lib/CodeGen/CoverageMappingGen.h
  clang/lib/Lex/Lexer.cpp
  clang/lib/Lex/Preprocessor.cpp
  clang/lib/Parse/ParseStmt.cpp
  compiler-rt/test/profile/coverage_emptylines.cpp
  compiler-rt/test/profile/instrprof-set-file-object-merging.c
  compiler-rt/test/profile/instrprof-set-file-object.c
  llvm/lib/ProfileData/Coverage/CoverageMapping.cpp

Index: llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
===
--- llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
+++ llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
@@ -488,8 +488,12 @@
 const bool Skipped = (CR.index() + 1) == Regions.size();
 startSegment(ActiveRegions.empty() ? CR.value() : *ActiveRegions.back(),
  CurStartLoc, !GapRegion, Skipped);
+// Create a segment with last pushed regions's count at CurStartLoc.
+startSegment(ActiveRegions.empty() ? CR.value() : *ActiveRegions.back(),
+ CurStartLoc, false);
 continue;
   }
+
   if (CR.index() + 1 == Regions.size() ||
   CurStartLoc != Regions[CR.index() + 1].startLoc()) {
 // Emit a segment if the next region doesn't start at the same location
@@ -586,7 +590,7 @@
 for (unsigned I = 1, E = Segments.size(); I < E; ++I) {
   const auto  = Segments[I - 1];
   const auto  = 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
   << " followed by " << R.Line << ":" << R.Col << "\n");
 assert(false && "Coverage segments not unique or sorted");
Index: compiler-rt/test/profile/instrprof-set-file-object.c
===
--- compiler-rt/test/profile/instrprof-set-file-object.c
+++ compiler-rt/test/profile/instrprof-set-file-object.c
@@ -24,7 +24,7 @@
 // CHECK:   12|  1|int main(int argc, const char *argv[]) {
 // CHECK:   13|  1|  if (argc < 2)
 // CHECK:   14|  0|return 1;
-// CHECK:   15|  1|
+// CHECK:   15|   |
 // CHECK:   16|  1|  FILE *F = fopen(argv[1], "w+b");
 // CHECK:   17|  1|  __llvm_profile_set_file_object(F, 0);
 // CHECK:   18|  1|  return 0;
Index: compiler-rt/test/profile/instrprof-set-file-object-merging.c
===
--- compiler-rt/test/profile/instrprof-set-file-object-merging.c
+++ compiler-rt/test/profile/instrprof-set-file-object-merging.c
@@ -31,13 +31,13 @@
 // CHECK:   14|  2|int main(int argc, const char *argv[]) {
 // CHECK:   15|  2|  if (argc < 2)
 // CHECK:   16|  0|return 1;
-// CHECK:   17|  2|
+// CHECK:   17|   |
 // CHECK:   18|  2|  FILE *F = fopen(argv[1], "r+b");
 // CHECK:   19|  2|  if (!F) {
 // CHECK:   20|   |// File might not exist, try opening with truncation
 // CHECK:   21|  1|F = fopen(argv[1], "w+b");
 // CHECK:   22|  1|  }
 // CHECK:   23|  2|  __llvm_profile_set_file_object(F, 1);
-// CHECK:   24|  2|
+// CHECK:   24|   |
 // CHECK:   25|  2|  return 0;
 // CHECK:   26|  2|}
Index: compiler-rt/test/profile/coverage_emptylines.cpp
===
--- /dev/null
+++ compiler-rt/test/profile/coverage_emptylines.cpp
@@ -0,0 +1,61 @@
+// Remove comments first.
+// RUN: sed 's/[ \t]*\/\/.*//' %s > %t.stripped.cpp
+// RUN: %clangxx_profgen -fcoverage-mapping -o %t %t.stripped.cpp
+// RUN: env LLVM_PROFILE_FILE=%t.profraw %run %t
+// RUN: llvm-profdata merge -o %t.profdata %t.profraw
+// RUN: llvm-cov show %t -instr-profile %t.profdata -path-equivalence=/tmp,%S 2>&1 | FileCheck %s
+
+
+int main() {// CHECK:   [[# @LINE]]| 1|int main() {
+int x = 0;  // CHECK-NEXT:  [[# @LINE]]| 1|
+// CHECK-NEXT:  [[# @LINE]]|  |
+x = 1;  // CHECK-NEXT:  [[# @LINE]]| 1|
+if (x)  // CHECK-NEXT:  [[# @LINE]]| 1|
+// CHECK-NEXT:  [[# @LINE]]|  |
+x   // CHECK-NEXT:  [[# @LINE]]| 1|
+// CHECK-NEXT:  [[# @LINE]]|  |
+=   // CHECK-NEXT:  [[# @LINE]]| 1|
+// CHECK-NEXT:  [[# @LINE]]|  |
+// CHECK-NEXT:  [[# @LINE]]|  |
+0;

[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:483
   bool GapRegion = CR.value().Kind == CounterMappingRegion::GapRegion;
 
   if (CR.index() + 1 == Regions.size() ||

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
```


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

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

[Zequan Wu via Phabricator via cfe-commits]

zequanwu updated this revision to Diff 286622.
zequanwu added a comment.

Add a wrapped segment at location of zero-length segment with last pushed 
region count.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84988

Files:
  clang/include/clang/Lex/Lexer.h
  clang/include/clang/Lex/Preprocessor.h
  clang/lib/CodeGen/CoverageMappingGen.cpp
  clang/lib/CodeGen/CoverageMappingGen.h
  clang/lib/Lex/Lexer.cpp
  clang/lib/Lex/Preprocessor.cpp
  clang/lib/Parse/ParseStmt.cpp
  compiler-rt/test/profile/coverage_emptylines.cpp
  compiler-rt/test/profile/instrprof-set-file-object-merging.c
  compiler-rt/test/profile/instrprof-set-file-object.c
  llvm/lib/ProfileData/Coverage/CoverageMapping.cpp

Index: llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
===
--- llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
+++ llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
@@ -488,8 +488,11 @@
 const bool Skipped = (CR.index() + 1) == Regions.size();
 startSegment(ActiveRegions.empty() ? CR.value() : *ActiveRegions.back(),
  CurStartLoc, !GapRegion, Skipped);
+// Create a segment with last pushed regions's count after CurStartLoc.
+startSegment(*ActiveRegions.back(), CurStartLoc, false);
 continue;
   }
+
   if (CR.index() + 1 == Regions.size() ||
   CurStartLoc != Regions[CR.index() + 1].startLoc()) {
 // Emit a segment if the next region doesn't start at the same location
@@ -586,7 +589,7 @@
 for (unsigned I = 1, E = Segments.size(); I < E; ++I) {
   const auto  = Segments[I - 1];
   const auto  = 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
   << " followed by " << R.Line << ":" << R.Col << "\n");
 assert(false && "Coverage segments not unique or sorted");
Index: compiler-rt/test/profile/instrprof-set-file-object.c
===
--- compiler-rt/test/profile/instrprof-set-file-object.c
+++ compiler-rt/test/profile/instrprof-set-file-object.c
@@ -24,7 +24,7 @@
 // CHECK:   12|  1|int main(int argc, const char *argv[]) {
 // CHECK:   13|  1|  if (argc < 2)
 // CHECK:   14|  0|return 1;
-// CHECK:   15|  1|
+// CHECK:   15|   |
 // CHECK:   16|  1|  FILE *F = fopen(argv[1], "w+b");
 // CHECK:   17|  1|  __llvm_profile_set_file_object(F, 0);
 // CHECK:   18|  1|  return 0;
Index: compiler-rt/test/profile/instrprof-set-file-object-merging.c
===
--- compiler-rt/test/profile/instrprof-set-file-object-merging.c
+++ compiler-rt/test/profile/instrprof-set-file-object-merging.c
@@ -31,13 +31,13 @@
 // CHECK:   14|  2|int main(int argc, const char *argv[]) {
 // CHECK:   15|  2|  if (argc < 2)
 // CHECK:   16|  0|return 1;
-// CHECK:   17|  2|
+// CHECK:   17|   |
 // CHECK:   18|  2|  FILE *F = fopen(argv[1], "r+b");
 // CHECK:   19|  2|  if (!F) {
 // CHECK:   20|   |// File might not exist, try opening with truncation
 // CHECK:   21|  1|F = fopen(argv[1], "w+b");
 // CHECK:   22|  1|  }
 // CHECK:   23|  2|  __llvm_profile_set_file_object(F, 1);
-// CHECK:   24|  2|
+// CHECK:   24|   |
 // CHECK:   25|  2|  return 0;
 // CHECK:   26|  2|}
Index: compiler-rt/test/profile/coverage_emptylines.cpp
===
--- /dev/null
+++ compiler-rt/test/profile/coverage_emptylines.cpp
@@ -0,0 +1,61 @@
+// Remove comments first.
+// RUN: sed 's/[ \t]*\/\/.*//' %s > %t.stripped.cpp
+// RUN: %clangxx_profgen -fcoverage-mapping -o %t %t.stripped.cpp
+// RUN: env LLVM_PROFILE_FILE=%t.profraw %run %t
+// RUN: llvm-profdata merge -o %t.profdata %t.profraw
+// RUN: llvm-cov show %t -instr-profile %t.profdata -path-equivalence=/tmp,%S 2>&1 | FileCheck %s
+
+
+int main() {// CHECK:   [[# @LINE]]| 1|int main() {
+int x = 0;  // CHECK-NEXT:  [[# @LINE]]| 1|
+// CHECK-NEXT:  [[# @LINE]]|  |
+x = 1;  // CHECK-NEXT:  [[# @LINE]]| 1|
+if (x)  // CHECK-NEXT:  [[# @LINE]]| 1|
+// CHECK-NEXT:  [[# @LINE]]|  |
+x   // CHECK-NEXT:  [[# @LINE]]| 1|
+// CHECK-NEXT:  [[# @LINE]]|  |
+=   // CHECK-NEXT:  [[# @LINE]]| 1|
+// CHECK-NEXT:  [[# @LINE]]|  |
+// CHECK-NEXT:  [[# @LINE]]|  |
+

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

[Vedant Kumar via Phabricator via cfe-commits]

vsk added inline comments.



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:
> > 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


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

[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:580
   const auto  = 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

zequanwu wrote:
> vsk wrote:
> > I don't think this relaxation is correct, since it permits duplicate 
> > segments. This is confusing for reporting tools because it's not possible 
> > to work out which segment applies at a given source location.
> I don't remember why I made this change. Reverting it seems nothing changed.
Oh, since single empty line will be a 0 length regions. `L.Col`  could equal to 
`R.Col`. 


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

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

[Zequan Wu via Phabricator via cfe-commits]

zequanwu updated this revision to Diff 286123.
zequanwu marked 3 inline comments as done.
zequanwu edited the summary of this revision.
zequanwu added a comment.

Address comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84988

Files:
  clang/include/clang/Lex/Lexer.h
  clang/include/clang/Lex/Preprocessor.h
  clang/lib/CodeGen/CoverageMappingGen.cpp
  clang/lib/CodeGen/CoverageMappingGen.h
  clang/lib/Lex/Lexer.cpp
  clang/lib/Lex/Preprocessor.cpp
  clang/lib/Parse/ParseStmt.cpp
  compiler-rt/test/profile/coverage_emptylines.cpp
  compiler-rt/test/profile/instrprof-set-file-object-merging.c
  compiler-rt/test/profile/instrprof-set-file-object.c
  llvm/lib/ProfileData/Coverage/CoverageMapping.cpp

Index: llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
===
--- llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
+++ llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
@@ -481,15 +481,6 @@
 
   bool GapRegion = CR.value().Kind == CounterMappingRegion::GapRegion;
 
-  // Try to emit a segment for the current region.
-  if (CurStartLoc == CR.value().endLoc()) {
-// Avoid making zero-length regions active. If it's the last region,
-// emit a skipped segment. Otherwise use its predecessor's count.
-const bool Skipped = (CR.index() + 1) == Regions.size();
-startSegment(ActiveRegions.empty() ? CR.value() : *ActiveRegions.back(),
- CurStartLoc, !GapRegion, Skipped);
-continue;
-  }
   if (CR.index() + 1 == Regions.size() ||
   CurStartLoc != Regions[CR.index() + 1].startLoc()) {
 // Emit a segment if the next region doesn't start at the same location
@@ -586,7 +577,7 @@
 for (unsigned I = 1, E = Segments.size(); I < E; ++I) {
   const auto  = Segments[I - 1];
   const auto  = 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
   << " followed by " << R.Line << ":" << R.Col << "\n");
 assert(false && "Coverage segments not unique or sorted");
Index: compiler-rt/test/profile/instrprof-set-file-object.c
===
--- compiler-rt/test/profile/instrprof-set-file-object.c
+++ compiler-rt/test/profile/instrprof-set-file-object.c
@@ -24,7 +24,7 @@
 // CHECK:   12|  1|int main(int argc, const char *argv[]) {
 // CHECK:   13|  1|  if (argc < 2)
 // CHECK:   14|  0|return 1;
-// CHECK:   15|  1|
+// CHECK:   15|   |
 // CHECK:   16|  1|  FILE *F = fopen(argv[1], "w+b");
 // CHECK:   17|  1|  __llvm_profile_set_file_object(F, 0);
 // CHECK:   18|  1|  return 0;
Index: compiler-rt/test/profile/instrprof-set-file-object-merging.c
===
--- compiler-rt/test/profile/instrprof-set-file-object-merging.c
+++ compiler-rt/test/profile/instrprof-set-file-object-merging.c
@@ -31,13 +31,13 @@
 // CHECK:   14|  2|int main(int argc, const char *argv[]) {
 // CHECK:   15|  2|  if (argc < 2)
 // CHECK:   16|  0|return 1;
-// CHECK:   17|  2|
+// CHECK:   17|   |
 // CHECK:   18|  2|  FILE *F = fopen(argv[1], "r+b");
 // CHECK:   19|  2|  if (!F) {
 // CHECK:   20|   |// File might not exist, try opening with truncation
 // CHECK:   21|  1|F = fopen(argv[1], "w+b");
 // CHECK:   22|  1|  }
 // CHECK:   23|  2|  __llvm_profile_set_file_object(F, 1);
-// CHECK:   24|  2|
+// CHECK:   24|   |
 // CHECK:   25|  2|  return 0;
 // CHECK:   26|  2|}
Index: compiler-rt/test/profile/coverage_emptylines.cpp
===
--- /dev/null
+++ compiler-rt/test/profile/coverage_emptylines.cpp
@@ -0,0 +1,61 @@
+// Remove comments first.
+// RUN: sed 's/[ \t]*\/\/.*//' %s > %t.stripped.cpp
+// RUN: %clangxx_profgen -fcoverage-mapping -o %t %t.stripped.cpp
+// RUN: env LLVM_PROFILE_FILE=%t.profraw %run %t
+// RUN: llvm-profdata merge -o %t.profdata %t.profraw
+// RUN: llvm-cov show %t -instr-profile %t.profdata -path-equivalence=/tmp,%S 2>&1 | FileCheck %s
+
+
+int main() {// CHECK:   [[# @LINE]]| 1|int main() {
+int x = 0;  // CHECK-NEXT:  [[# @LINE]]| 1|
+// CHECK-NEXT:  [[# @LINE]]|  |
+x = 1;  // CHECK-NEXT:  [[# @LINE]]| 1|
+if (x)  // CHECK-NEXT:  [[# @LINE]]| 1|
+// CHECK-NEXT:  [[# @LINE]]|  |
+x   // CHECK-NEXT:  [[# @LINE]]| 1|
+// CHECK-NEXT:  [[# @LINE]]|  |
+

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

[Zequan Wu via Phabricator via cfe-commits]

zequanwu added inline comments.



Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:326
+if (PrevTokLoc.isValid()) {
+  unsigned PrevTokLine = SM.getSpellingLineNumber(PrevTokLoc);
+  if (SR.LineStart == PrevTokLine) {

vsk wrote:
> It looks like this assumes there is some guarantee that the skipped range (as 
> given by SR) is in the same file as {Prev,Next}TokLoc. If that's correct, can 
> we go ahead and `assert` that?
Oh, it's a bug in https://reviews.llvm.org/D83592. There is no guarantee that 
they are in the same file. 



Comment at: clang/lib/Lex/Lexer.cpp:3290
   IsAtPhysicalStartOfLine = true;
+  NewLinePtr = CurPtr - 1;
 

vsk wrote:
> Is NewLinePtr supposed to point to the start of the most recent newline 
> sequence? For "\r\n", is it supposed to be "\r\n" or 
> "\r\n"?
I didn't consider this. Updated.
NewLinePtr is supposed to point to the '\n' character. For "\r\n", it will 
point to '\n'.



Comment at: llvm/lib/ProfileData/Coverage/CoverageMapping.cpp:483
   bool GapRegion = CR.value().Kind == CounterMappingRegion::GapRegion;
 
   if (CR.index() + 1 == Regions.size() ||

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|}
```



Comment at: llvm/lib/ProfileData/Coverage/CoverageMapping.cpp:580
   const auto  = 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:
> I don't think this relaxation is correct, since it permits duplicate 
> segments. This is confusing for reporting tools because it's not possible to 
> work out which segment applies at a given source location.
I don't remember why I made this change. Reverting it seems nothing changed.


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

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

[Vedant Kumar via Phabricator via cfe-commits]

vsk added inline comments.



Comment at: clang/include/clang/Lex/Lexer.h:131
 
+  const char *NewLinePtr;
+

Could you leave a comment describing what this is?



Comment at: clang/include/clang/Lex/Preprocessor.h:960
+  void setParsingFunctionBody(bool parsing) { ParsingFunctionBody = parsing; }
+  bool isParsingFunctionBody() { return ParsingFunctionBody; }
+

const



Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:326
+if (PrevTokLoc.isValid()) {
+  unsigned PrevTokLine = SM.getSpellingLineNumber(PrevTokLoc);
+  if (SR.LineStart == PrevTokLine) {

It looks like this assumes there is some guarantee that the skipped range (as 
given by SR) is in the same file as {Prev,Next}TokLoc. If that's correct, can 
we go ahead and `assert` that?



Comment at: clang/lib/Lex/Lexer.cpp:3290
   IsAtPhysicalStartOfLine = true;
+  NewLinePtr = CurPtr - 1;
 

Is NewLinePtr supposed to point to the start of the most recent newline 
sequence? For "\r\n", is it supposed to be "\r\n" or 
"\r\n"?



Comment at: llvm/lib/ProfileData/Coverage/CoverageMapping.cpp:483
   bool GapRegion = CR.value().Kind == CounterMappingRegion::GapRegion;
 
   if (CR.index() + 1 == Regions.size() ||

Why is this deletion necessary? Do we need to introduce 0-length regions which 
alter the count? An example would help.



Comment at: llvm/lib/ProfileData/Coverage/CoverageMapping.cpp:580
   const auto  = 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

I don't think this relaxation is correct, since it permits duplicate segments. 
This is confusing for reporting tools because it's not possible to work out 
which segment applies at a given source location.


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

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

[Zequan Wu via Phabricator via cfe-commits]

zequanwu added a comment.

In D84988#2221805 , @vsk wrote:

> Hi @zequanwu, are you looking for review for this patch? I wasn't sure 
> because of the WIP label.

Yes, I removed the label.


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