[Coverage] Adjust skipped regions only if {Prev,Next}TokLoc is in the same file as regions' {start, end}Loc

[zequanwu]

Summary

Fix a bug if {Prev, Next}TokLoc is in different file from skipped regions' {start, end}Loc

[zequanwu]

Update test.
The bug is when start or end location skipped regions has the same spelling line number as PrevTokLoc or NextTokLoc but in different files.
In the test case, end location of skipped regions is in line 6 and PreTokLoc is also in line 6, but since they are in different files, it shouldn't be shrinked.

[vsk]

Lgtm, thank you!

[zequanwu]

Minor fix on test case.

clang/lib/CodeGen/CoverageMappingGen.cpp

 CoverageMappingModuleGen::setUpCoverageCallbacks(Preprocessor &PP) {
   CoverageSourceInfo *CoverageInfo = new CoverageSourceInfo();
   PP.addPPCallbacks(std::unique_ptr<PPCallbacks>(CoverageInfo));
   PP.addCommentHandler(CoverageInfo);
   PP.setPreprocessToken(true);
   PP.setTokenWatcher([CoverageInfo](clang::Token Tok) {
     // Update previous token location.
     CoverageInfo->PrevTokLoc = Tok.getLocation();
+    if (Tok.getKind() != clang::tok::eod)
       CoverageInfo->updateNextTokLoc(Tok.getLocation());
   });
   return CoverageInfo;
 }
 
 void CoverageSourceInfo::SourceRangeSkipped(SourceRange Range, SourceLocation) {
   SkippedRanges.push_back({Range});
 }
 
if (Mapping != FileIDMapping.end())
       return Mapping->second.first;
     return None;
   }
 
   /// 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 LocStart,
+                                              SourceLocation LocEnd,
                                               SourceLocation PrevTokLoc,
                                               SourceLocation NextTokLoc) {
+    SpellingRegion SR{SM, LocStart, LocEnd};
     // 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 (SR.LineStart == PrevTokLine) {
+    if (SM.isWrittenInSameFile(LocStart, PrevTokLoc) &&
+        SR.LineStart == PrevTokLine) {
       newSR.LineStart = SR.LineStart + 1;
       newSR.ColumnStart = 1;
     }
-    if (SR.LineEnd == NextTokLine) {
+    if (SM.isWrittenInSameFile(LocEnd, NextTokLoc) &&
+        SR.LineEnd == NextTokLine) {
       newSR.LineEnd = SR.LineEnd - 1;
       newSR.ColumnEnd = SR.ColumnStart + 1;
     }
     if (newSR.isInSourceOrder())
       return newSR;
     return None;
   }
 
for (auto &I : SkippedRanges) {
       auto LocStart = Range.getBegin();
       auto LocEnd = Range.getEnd();
       assert(SM.isWrittenInSameFile(LocStart, LocEnd) &&
              "region spans multiple files");
 
       auto CovFileID = getCoverageFileID(LocStart);
       if (!CovFileID)
         continue;
-      SpellingRegion SR{SM, LocStart, LocEnd};
-      if (Optional<SpellingRegion> res =
-              adjustSkippedRange(SM, SR, I.PrevTokLoc, I.NextTokLoc))
-        SR = res.getValue();
-      else
+      Optional<SpellingRegion> SR =
+          adjustSkippedRange(SM, LocStart, LocEnd, I.PrevTokLoc, I.NextTokLoc);
+      if (!SR.hasValue())
         continue;
       auto Region = CounterMappingRegion::makeSkipped(
-          *CovFileID, SR.LineStart, SR.ColumnStart, SR.LineEnd, SR.ColumnEnd);
+          *CovFileID, SR->LineStart, SR->ColumnStart, SR->LineEnd,
+          SR->ColumnEnd);
       // Make sure that we only collect the regions that are inside
       // the source code of this function.
       if (Region.LineStart >= FileLineRanges[*CovFileID].first &&
           Region.LineEnd <= FileLineRanges[*CovFileID].second)
         MappingRegions.push_back(Region);
     }
   }
 

clang/test/CoverageMapping/Inputs/comment.h

+
+
+
+
+
+x = 0;

clang/test/CoverageMapping/comment.cpp

+// RUN: %clang_cc1 -fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping -emit-llvm-only %s | FileCheck %s
+
+int f() {
+  int x = 0;
+#include "Inputs/comment.h" /*
+    */
+  return x;
+}
+
+// CHECK: File 0, 3:9 -> 8:2 = #0
+// CHECK-NEXT: Expansion,File 0, 5:10 -> 5:28 = #0
+// CHECK-NEXT: Skipped,File 0, 6:1 -> 6:7 = 0
+// CHECK-NEXT: File 1, 1:1 -> 7:1 = #0