[Coverage] Add empty line regions to SkippedRegions

[zequanwu]

Summary

Add a hook to track empty line regions when lexing.

[zequanwu]

• Rebase.
• Merging adjcent skipped regions, if the regions are in the same file, could reduce binary size.
• The new empty lines regions break existing coverage tests, and don't know a good way to fix those. (probably add a null statement ; at each empty line?)
• For stage2 build of clang, I didn't notice a performance slowdown this time.

[vsk]

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

[zequanwu]

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.

[vsk]

clang/include/clang/Lex/Lexer.h

131

Could you leave a comment describing what this is?

clang/include/clang/Lex/Preprocessor.h

960

const

clang/lib/CodeGen/CoverageMappingGen.cpp

326

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?

clang/lib/Lex/Lexer.cpp

3290

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

llvm/lib/ProfileData/Coverage/CoverageMapping.cpp

483–484

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

580

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.

[zequanwu]

Address comments.

clang/lib/CodeGen/CoverageMappingGen.cpp

326

Oh, it's a bug in https://reviews.llvm.org/D83592. There is no guarantee that they are in the same file.

clang/lib/Lex/Lexer.cpp

3290

I didn't consider this. Updated.
NewLinePtr is supposed to point to the '\n' character. For "\r\n", it will point to '\n'.

llvm/lib/ProfileData/Coverage/CoverageMapping.cpp

483–484

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

580

I don't remember why I made this change. Reverting it seems nothing changed.

[zequanwu]

llvm/lib/ProfileData/Coverage/CoverageMapping.cpp

580

Oh, since single empty line will be a 0 length regions. L.Col could equal to R.Col.

[vsk]

llvm/lib/ProfileData/Coverage/CoverageMapping.cpp

483–484

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

[zequanwu]

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

[zequanwu]

llvm/lib/ProfileData/Coverage/CoverageMapping.cpp

483–484

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

[zequanwu]

minor fix.

[zequanwu]

Friendly ping.

[zequanwu]

update.

[vsk]

llvm/lib/ProfileData/Coverage/CoverageMapping.cpp

483–484

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?

[zequanwu]

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

[zequanwu]

llvm/lib/ProfileData/Coverage/CoverageMapping.cpp

483–484

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.

[vsk]

I'm not as familiar with the preprocessor bits, but this looks like it's in good shape.

llvm/lib/ProfileData/Coverage/CoverageMapping.cpp

483–484

That carveout sounds reasonable to me.

580

Could you keep the original check and simply 'continue' if L.Col == R.Col && L.IsSkipped?

[zequanwu]

address comment.
emit second segment when first segment is skipped and activeregions is not empty.
I don't know who can be added as reviewers for the preprocessor part

[zequanwu]

llvm/lib/ProfileData/Coverage/CoverageMapping.cpp

483–484

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.

580

if (L.Line == R.Line && L.Col == R.Col && !L.HasCount) is more specific.

[vsk]

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.

clang/lib/Lex/Lexer.cpp

2228

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.

[zequanwu]

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.

[vsk]

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.

[zequanwu]

• 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.

[vsk]

clang/lib/CodeGen/CoverageMappingGen.cpp

38

We might consider marking this as cl::Hidden.

[zequanwu]

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

[zequanwu]

Friendly ping.

[vsk]

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

[zequanwu]

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.

clang/include/clang/Lex/Lexer.h

class Lexer : public PreprocessorLexer {
   bool IsAtStartOfLine;
 
   bool IsAtPhysicalStartOfLine;
 
   bool HasLeadingSpace;
 
   bool HasLeadingEmptyMacro;
 
+  // NewLinePtr - A pointer to new line character '\n' being
+  // lexed. For '\r\n', it also points to '\n.'
+  const char *NewLinePtr;
+
   // CurrentConflictMarkerState - The kind of conflict marker we are handling.
   ConflictMarkerKind CurrentConflictMarkerState;
 
   void InitLexer(const char *BufStart, const char *BufPtr, const char *BufEnd);
 
 public:
   /// Lexer constructor - Create a new lexer object for the specified buffer
   /// with the specified preprocessor managing the lexing process.  This lexer

clang/include/clang/Lex/Preprocessor.h

 } // namespace llvm
 
 namespace clang {
 
 class CodeCompletionHandler;
 class CommentHandler;
 class DirectoryEntry;
 class DirectoryLookup;
+class EmptylineHandler;
 class ExternalPreprocessorSource;
 class FileEntry;
 class FileManager;
 class HeaderSearch;
 class MacroArgs;
 class PragmaHandler;
 class PragmaNamespace;
 class PreprocessingRecord;
class Preprocessor {
   bool PreprocessedOutput : 1;
 
   /// True if we are currently preprocessing a #if or #elif directive
   bool ParsingIfOrElifDirective;
 
   /// True if we are pre-expanding macro arguments.
   bool InMacroArgPreExpansion;
 
+  // True if we are currently parsing function body.
+  bool ParsingFunctionBody;
+
   /// Mapping/lookup information for all identifiers in
   /// the program, including program keywords.
   mutable IdentifierTable Identifiers;
 
   /// This table contains all the selectors in the program.
   ///
   /// Unlike IdentifierTable above, this table *isn't* populated by the
   /// preprocessor. It is declared/expanded here because its role/lifetime is
class Preprocessor {
   /// Pragma handlers of the original source is stored here during the
   /// parsing of a model file.
   std::unique_ptr<PragmaNamespace> PragmaHandlersBackup;
 
   /// Tracks all of the comment handlers that the client registered
   /// with this preprocessor.
   std::vector<CommentHandler *> CommentHandlers;
 
+  /// Empty line handler.
+  EmptylineHandler *Emptyline = nullptr;
+
   /// True if we want to ignore EOF token and continue later on (thus
   /// avoid tearing the Lexer and etc. down).
   bool IncrementalProcessing = false;
 
   /// The kind of translation unit we are processing.
   TranslationUnitKind TUKind;
 
   /// The code-completion handler.
public:
   }
 
   /// Retrieve the number of Directives that have been processed by the
   /// Preprocessor.
   unsigned getNumDirectives() const {
     return NumDirectives;
   }
 
+  void setParsingFunctionBody(bool parsing) { ParsingFunctionBody = parsing; }
+  bool isParsingFunctionBody() const { return ParsingFunctionBody; }
+
   /// True if we are currently preprocessing a #if or #elif directive
   bool isParsingIfOrElifDirective() const {
     return ParsingIfOrElifDirective;
   }
 
   /// Control whether the preprocessor retains comments in output.
   void SetCommentRetentionState(bool KeepComments, bool KeepMacroComments) {
     this->KeepComments = KeepComments | KeepMacroComments;
public:
   void RemovePragmaHandler(StringRef Namespace, PragmaHandler *Handler);
   void RemovePragmaHandler(PragmaHandler *Handler) {
     RemovePragmaHandler(StringRef(), Handler);
   }
 
   /// Install empty handlers for all pragmas (making them ignored).
   void IgnorePragmas();
 
+  /// Set empty line handler.
+  void setEmptylineHandler(EmptylineHandler *Handler) { Emptyline = Handler; }
+
+  EmptylineHandler *getEmptylineHandler() const { return Emptyline; }
+
   /// Add the specified comment handler to the preprocessor.
   void addCommentHandler(CommentHandler *Handler);
 
   /// Remove the specified comment handler.
   ///
   /// It is an error to remove a handler that has not been registered.
   void removeCommentHandler(CommentHandler *Handler);
 

 public:
   virtual ~CommentHandler();
 
   // The handler shall return true if it has pushed any tokens
   // to be read using e.g. EnterToken or EnterTokenStream.
   virtual bool HandleComment(Preprocessor &PP, SourceRange Comment) = 0;
 };
 
+/// Abstract base class that describes a handler that will receive
+/// source ranges for empty lines encountered in the source file.
+class EmptylineHandler {
+public:
+  virtual ~EmptylineHandler();
+
+  // The handler handles empty lines.
+  virtual void HandleEmptyline(SourceRange Range) = 0;
+};
+
 /// Registry of pragma handlers added by plugins
 using PragmaHandlerRegistry = llvm::Registry<PragmaHandler>;
 
 } // namespace clang
 
 #endif // LLVM_CLANG_LEX_PREPROCESSOR_H

clang/lib/CodeGen/CoverageMappingGen.h

struct SkippedRange {
   SkippedRange(SourceRange Range, SourceLocation PrevTokLoc = SourceLocation(),
                SourceLocation NextTokLoc = SourceLocation())
       : Range(Range), PrevTokLoc(PrevTokLoc), NextTokLoc(NextTokLoc) {}
 };
 
 /// Stores additional source code information like skipped ranges which
 /// is required by the coverage mapping generator and is obtained from
 /// the preprocessor.
-class CoverageSourceInfo : public PPCallbacks, public CommentHandler {
+class CoverageSourceInfo : public PPCallbacks,
+                           public CommentHandler,
+                           public EmptylineHandler {
   // A vector of skipped source ranges and PrevTokLoc with NextTokLoc.
   std::vector<SkippedRange> SkippedRanges;
-  bool AfterComment = false;
+
+  SourceManager &SourceMgr;
 
 public:
   // Location of the token parsed before HandleComment is called. This is
   // updated every time Preprocessor::Lex lexes a new token.
   SourceLocation PrevTokLoc;
-  // The location of token before comment.
-  SourceLocation BeforeCommentLoc;
+
+  CoverageSourceInfo(SourceManager &SourceMgr) : SourceMgr(SourceMgr) {}
 
   std::vector<SkippedRange> &getSkippedRanges() { return SkippedRanges; }
 
+  void AddSkippedRange(SourceRange Range);
+
   void SourceRangeSkipped(SourceRange Range, SourceLocation EndifLoc) override;
 
+  void HandleEmptyline(SourceRange Range) override;
+
   bool HandleComment(Preprocessor &PP, SourceRange Range) override;
 
   void updateNextTokLoc(SourceLocation Loc);
 };
 
 namespace CodeGen {
 
 class CodeGenModule;

clang/lib/CodeGen/CoverageMappingGen.cpp

 // This selects the coverage mapping format defined when `InstrProfData.inc`
 // is textually included.
 #define COVMAP_V3
 
 using namespace clang;
 using namespace CodeGen;
 using namespace llvm::coverage;
 
 CoverageSourceInfo *
 CoverageMappingModuleGen::setUpCoverageCallbacks(Preprocessor &PP) {
-  CoverageSourceInfo *CoverageInfo = new CoverageSourceInfo();
+  CoverageSourceInfo *CoverageInfo =
+      new CoverageSourceInfo(PP.getSourceManager());
   PP.addPPCallbacks(std::unique_ptr<PPCallbacks>(CoverageInfo));
   PP.addCommentHandler(CoverageInfo);
+  PP.setEmptylineHandler(CoverageInfo);
   PP.setPreprocessToken(true);
   PP.setTokenWatcher([CoverageInfo](clang::Token Tok) {
     // Update previous token location.
     CoverageInfo->PrevTokLoc = Tok.getLocation();
     CoverageInfo->updateNextTokLoc(Tok.getLocation());
   });
   return CoverageInfo;
 }
 
+void CoverageSourceInfo::AddSkippedRange(SourceRange Range) {
+  if (!SkippedRanges.empty() && PrevTokLoc == SkippedRanges.back().PrevTokLoc &&
+      SourceMgr.isWrittenInSameFile(SkippedRanges.back().Range.getEnd(),
+                                    Range.getBegin()))
+    SkippedRanges.back().Range.setEnd(Range.getEnd());
+  else
+    SkippedRanges.push_back({Range, PrevTokLoc});
+}
+
 void CoverageSourceInfo::SourceRangeSkipped(SourceRange Range, SourceLocation) {
-  SkippedRanges.push_back({Range});
+  AddSkippedRange(Range);
+}
+
+void CoverageSourceInfo::HandleEmptyline(SourceRange Range) {
+  AddSkippedRange(Range);
 }
 
 bool CoverageSourceInfo::HandleComment(Preprocessor &PP, SourceRange Range) {
-  SkippedRanges.push_back({Range, PrevTokLoc});
-  AfterComment = true;
+  if (PP.isParsingFunctionBody())
+    AddSkippedRange(Range);
   return false;
 }
 
 void CoverageSourceInfo::updateNextTokLoc(SourceLocation Loc) {
-  if (AfterComment) {
+  if (!SkippedRanges.empty() && SkippedRanges.back().NextTokLoc.isInvalid())
     SkippedRanges.back().NextTokLoc = Loc;
-    AfterComment = false;
-  }
 }
 
 namespace {
 
 /// A region of source code that can be mapped to a counter.
 class SourceMappingRegion {
   Counter Count;
 
public:
 
   /// This shrinks the skipped range if it spans a line that contains a
   /// non-comment token. If shrinking the skipped range would make it empty,
   /// this returns None.
   Optional<SpellingRegion> adjustSkippedRange(SourceManager &SM,
                                               SpellingRegion SR,
                                               SourceLocation PrevTokLoc,
                                               SourceLocation NextTokLoc) {
-    // If Range begin location is invalid, it's not a comment region.
-    if (PrevTokLoc.isInvalid())
-      return SR;
-    unsigned PrevTokLine = SM.getSpellingLineNumber(PrevTokLoc);
-    unsigned NextTokLine = SM.getSpellingLineNumber(NextTokLoc);
     SpellingRegion newSR(SR);
+    if (PrevTokLoc.isValid()) {
+      unsigned PrevTokLine = SM.getSpellingLineNumber(PrevTokLoc);
       if (SR.LineStart == PrevTokLine) {
         newSR.LineStart = SR.LineStart + 1;
         newSR.ColumnStart = 1;
       }
+    }
+    if (NextTokLoc.isValid()) {
+      unsigned NextTokLine = SM.getSpellingLineNumber(NextTokLoc);
       if (SR.LineEnd == NextTokLine) {
         newSR.LineEnd = SR.LineEnd - 1;
-      newSR.ColumnEnd = SR.ColumnStart + 1;
+        newSR.ColumnEnd = newSR.ColumnStart + 1;
+      }
     }
     if (newSR.isInSourceOrder())
       return newSR;
     return None;
   }
 
   /// Gather all the regions that were skipped by the preprocessor
   /// using the constructs like #if or comments.

clang/lib/Lex/Lexer.cpp

void Lexer::InitLexer(const char *BufStart, const char *BufPtr,
   // We are not in raw mode.  Raw mode disables diagnostics and interpretation
   // of tokens (e.g. identifiers, thus disabling macro expansion).  It is used
   // to quickly lex the tokens of the buffer, e.g. when handling a "#if 0" block
   // or otherwise skipping over tokens.
   LexingRawMode = false;
 
   // Default to not keeping comments.
   ExtendedTokenMode = 0;
+
+  NewLinePtr = nullptr;
 }
 
 /// Lexer constructor - Create a new lexer object for the specified buffer
 /// with the specified preprocessor managing the lexing process.  This lexer
 /// assumes that the associated file buffer and Preprocessor objects will
 /// outlive it, so it doesn't take ownership of either of them.
 Lexer::Lexer(FileID FID, const llvm::MemoryBuffer *InputFile, Preprocessor &PP)
     : PreprocessorLexer(&PP, FID),

 /// This method forms a token and returns true if KeepWhitespaceMode is enabled.
 bool Lexer::SkipWhitespace(Token &Result, const char *CurPtr,
                            bool &TokAtPhysicalStartOfLine) {
   // Whitespace - Skip it, then return the token after the whitespace.
   bool SawNewline = isVerticalWhitespace(CurPtr[-1]);
 
   unsigned char Char = *CurPtr;
 
+  const char *lastNewLine = nullptr;
+  if (SawNewline) {
+    lastNewLine = CurPtr - 1;
+    if (!NewLinePtr)
+      NewLinePtr = CurPtr - 1;
+  }
+
   // Skip consecutive spaces efficiently.
   while (true) {
     // Skip horizontal whitespace very aggressively.
     while (isHorizontalWhitespace(Char))
       Char = *++CurPtr;
 
     // Otherwise if we have something other than whitespace, we're done.
     if (!isVerticalWhitespace(Char))
       break;
 
     if (ParsingPreprocessorDirective) {
       // End of preprocessor directive line, let LexTokenInternal handle this.
       BufferPtr = CurPtr;
       return false;
     }
 
     // OK, but handle newline.
+    lastNewLine = *CurPtr == '\n' ? CurPtr : lastNewLine;
+    if (!NewLinePtr && *CurPtr == '\n')
+      NewLinePtr = CurPtr;
     SawNewline = true;
     Char = *++CurPtr;
   }
 
   // If the client wants us to return whitespace, return it now.
   if (isKeepWhitespaceMode()) {
     FormTokenWithChars(Result, CurPtr, tok::unknown);
     if (SawNewline) {
       IsAtStartOfLine = true;
       IsAtPhysicalStartOfLine = true;
     }
     // FIXME: The next token will not have LeadingSpace set.
     return true;
   }
 
   // If this isn't immediately after a newline, there is leading space.
   char PrevChar = CurPtr[-1];
   bool HasLeadingSpace = !isVerticalWhitespace(PrevChar);
 
   Result.setFlagValue(Token::LeadingSpace, HasLeadingSpace);
   if (SawNewline) {
     Result.setFlag(Token::StartOfLine);
     TokAtPhysicalStartOfLine = true;
+
+    if (NewLinePtr && lastNewLine && NewLinePtr != lastNewLine && PP &&
+        PP->isParsingFunctionBody()) {
+      if (auto *Handler = PP->getEmptylineHandler())
+        Handler->HandleEmptyline(SourceRange(getSourceLocation(NewLinePtr + 1),
+                                             getSourceLocation(lastNewLine)));
+    }
   }
 
   BufferPtr = CurPtr;
   return false;
 }
 
 /// We have just read the // characters from input.  Skip until we find the
 /// newline character that terminates the comment.  Then update BufferPtr and
if (ParsingPreprocessorDirective || CurPtr == BufferEnd) {
     return false;
   }
 
   // Otherwise, eat the \n character.  We don't care if this is a \n\r or
   // \r\n sequence.  This is an efficiency hack (because we know the \n can't
   // contribute to another token), it isn't needed for correctness.  Note that
   // this is ok even in KeepWhitespaceMode, because we would have returned the
   /// comment above in that mode.
-  ++CurPtr;
+  NewLinePtr = CurPtr++;
 
   // The next returned token is at the start of the line.
   Result.setFlag(Token::StartOfLine);
   TokAtPhysicalStartOfLine = true;
   // No leading whitespace seen so far.
   Result.clearFlag(Token::LeadingSpace);
   BufferPtr = CurPtr;
   return false;
LexNextToken:
   }
 
   unsigned SizeTmp, SizeTmp2;   // Temporaries for use in cases below.
 
   // Read a character, advancing over it.
   char Char = getAndAdvanceChar(CurPtr, Result);
   tok::TokenKind Kind;
 
+  if (!isVerticalWhitespace(Char))
+    NewLinePtr = nullptr;
+
   switch (Char) {
   case 0:  // Null.
     // Found end of file?
     if (CurPtr-1 == BufferEnd)
       return LexEndOfFile(Result, CurPtr-1);
 
     // Check if we are performing code completion.
     if (isCodeCompletionPoint(CurPtr-1)) {
if (ParsingPreprocessorDirective) {
 
       // Restore comment saving mode, in case it was disabled for directive.
       if (PP)
         resetExtendedTokenMode();
 
       // Since we consumed a newline, we are back at the start of a line.
       IsAtStartOfLine = true;
       IsAtPhysicalStartOfLine = true;
+      NewLinePtr = CurPtr - 1;
 
       Kind = tok::eod;
       break;
     }
 
     // No leading whitespace seen so far.
     Result.clearFlag(Token::LeadingSpace);
 

clang/lib/Lex/Preprocessor.cpp

Preprocessor::Preprocessor(std::shared_ptr<PreprocessorOptions> PPOpts,
   MacroExpansionInDirectivesOverride = false;
   InMacroArgs = false;
   ArgMacro = nullptr;
   InMacroArgPreExpansion = false;
   NumCachedTokenLexers = 0;
   PragmasEnabled = true;
   ParsingIfOrElifDirective = false;
   PreprocessedOutput = false;
+  ParsingFunctionBody = false;
 
   // We haven't read anything from the external source.
   ReadMacrosFromExternalSource = false;
 
   BuiltinInfo = std::make_unique<Builtin::Context>();
 
   // "Poison" __VA_ARGS__, __VA_OPT__ which can only appear in the expansion of
   // a macro. They get unpoisoned where it is allowed.
bool Preprocessor::HandleComment(Token &result, SourceRange Comment) {
   Lex(result);
   return true;
 }
 
 ModuleLoader::~ModuleLoader() = default;
 
 CommentHandler::~CommentHandler() = default;
 
+EmptylineHandler::~EmptylineHandler() = default;
+
 CodeCompletionHandler::~CodeCompletionHandler() = default;
 
 void Preprocessor::createPreprocessingRecord() {
   if (Record)
     return;
 
   Record = new PreprocessingRecord(getSourceManager());
   addPPCallbacks(std::unique_ptr<PPCallbacks>(Record));
 }

clang/lib/Parse/ParseStmt.cpp

PrettyDeclStackTraceEntry CrashInfo(Actions.Context, Decl, LBraceLoc,
                                       "parsing function body");
 
   // Save and reset current vtordisp stack if we have entered a C++ method body.
   bool IsCXXMethod =
       getLangOpts().CPlusPlus && Decl && isa<CXXMethodDecl>(Decl);
   Sema::PragmaStackSentinelRAII
     PragmaStackSentinel(Actions, "InternalPragmaState", IsCXXMethod);
 
+  PP.setParsingFunctionBody(true);
+
   // Do not enter a scope for the brace, as the arguments are in the same scope
   // (the function body) as the body itself.  Instead, just read the statement
   // list and put it into a CompoundStmt for safe keeping.
   StmtResult FnBody(ParseCompoundStatementBody());
 
+  PP.setParsingFunctionBody(false);
+
   // If the function body could not be parsed, make a bogus compoundstmt.
   if (FnBody.isInvalid()) {
     Sema::CompoundScopeRAII CompoundScope(Actions);
     FnBody = Actions.ActOnCompoundStmt(LBraceLoc, LBraceLoc, None, false);
   }
 
   BodyScope.Exit();
   return Actions.ActOnFinishFunctionBody(Decl, FnBody.get());

compiler-rt/test/profile/coverage_emptylines.cpp

+// 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;                          // CHECK-NEXT:  [[# @LINE]]| 1|
+                                    // CHECK-NEXT:  [[# @LINE]]|  |
+    if (x)                          // CHECK-NEXT:  [[# @LINE]]| 1|
+                                    // CHECK-NEXT:  [[# @LINE]]|  |
+                                    // CHECK-NEXT:  [[# @LINE]]|  |
+        x = 1;                      // CHECK-NEXT:  [[# @LINE]]| 0|
+                                    // CHECK-NEXT:  [[# @LINE]]|  |
+    #ifdef UNDEFINED                // CHECK-NEXT:  [[# @LINE]]|  |
+                                    // CHECK-NEXT:  [[# @LINE]]|  |
+    int y = 0;                      // CHECK-NEXT:  [[# @LINE]]|  |
+                                    // CHECK-NEXT:  [[# @LINE]]|  |
+    y = 1;                          // CHECK-NEXT:  [[# @LINE]]|  |
+    if (y)                          // CHECK-NEXT:  [[# @LINE]]|  |
+                                    // CHECK-NEXT:  [[# @LINE]]|  |
+        y                           // CHECK-NEXT:  [[# @LINE]]|  |
+                                    // CHECK-NEXT:  [[# @LINE]]|  |
+        =                           // CHECK-NEXT:  [[# @LINE]]|  |
+                                    // CHECK-NEXT:  [[# @LINE]]|  |
+                                    // CHECK-NEXT:  [[# @LINE]]|  |
+        0;                          // CHECK-NEXT:  [[# @LINE]]|  |
+                                    // CHECK-NEXT:  [[# @LINE]]|  |
+    #endif                          // CHECK-NEXT:  [[# @LINE]]|  |
+                                    // CHECK-NEXT:  [[# @LINE]]|  |
+    #define DEFINED 1               // CHECK-NEXT:  [[# @LINE]]| 1|
+                                    // CHECK-NEXT:  [[# @LINE]]|  |
+    #ifdef DEFINED                  // CHECK-NEXT:  [[# @LINE]]| 1|
+                                    // CHECK-NEXT:  [[# @LINE]]|  |
+    int y = 0;                      // CHECK-NEXT:  [[# @LINE]]| 1|
+                                    // CHECK-NEXT:  [[# @LINE]]|  |
+    y = 1;                          // CHECK-NEXT:  [[# @LINE]]| 1|
+    if (y)                          // CHECK-NEXT:  [[# @LINE]]| 1|
+                                    // CHECK-NEXT:  [[# @LINE]]|  |
+        y                           // CHECK-NEXT:  [[# @LINE]]| 1|
+                                    // CHECK-NEXT:  [[# @LINE]]|  |
+        =                           // CHECK-NEXT:  [[# @LINE]]| 1|
+                                    // CHECK-NEXT:  [[# @LINE]]|  |
+                                    // CHECK-NEXT:  [[# @LINE]]|  |
+        0;                          // CHECK-NEXT:  [[# @LINE]]| 1|
+    #endif                          // CHECK-NEXT:  [[# @LINE]]| 1|
+                                    // CHECK-NEXT:  [[# @LINE]]|  |
+    return 0;                       // CHECK-NEXT:  [[# @LINE]]| 1|
+}                                   // CHECK-NEXT:  [[# @LINE]]| 1|
 No newline at end of file

compiler-rt/test/profile/instrprof-set-file-object-merging.c

 }
 // CHECK:   10|       |#include <stdio.h>
 // CHECK:   11|       |
 // CHECK:   12|       |extern void __llvm_profile_set_file_object(FILE *, int);
 // CHECK:   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|}

compiler-rt/test/profile/instrprof-set-file-object.c

 }
 // CHECK:    8|       |#include <stdio.h>
 // CHECK:    9|       |
 // CHECK:   10|       |extern void __llvm_profile_set_file_object(FILE *, int);
 // CHECK:   11|       |
 // 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;
 // CHECK:   19|      1|}

llvm/lib/ProfileData/Coverage/CoverageMapping.cpp

for (const auto &CR : enumerate(Regions)) {
                                 });
       if (CompletedRegions != ActiveRegions.end()) {
         unsigned FirstCompletedRegion =
             std::distance(ActiveRegions.begin(), CompletedRegions);
         completeRegionsUntil(CurStartLoc, FirstCompletedRegion);
       }
 
       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
         // as this one.
         startSegment(CR.value(), CurStartLoc, !GapRegion);
       }
 
       // This region is active (i.e not completed).
       ActiveRegions.push_back(&CR.value());
buildSegments(MutableArrayRef<CountedRegion> Regions) {
     });
 
     Builder.buildSegmentsImpl(CombinedRegions);
 
 #ifndef NDEBUG
     for (unsigned I = 1, E = Segments.size(); I < E; ++I) {
       const auto &L = Segments[I - 1];
       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
                           << " followed by " << R.Line << ":" << R.Col << "\n");
         assert(false && "Coverage segments not unique or sorted");
       }
     }
 #endif
 
     return Segments;