[Lexer] Report more precise skipped regions (PR34166)

This patch teaches the preprocessor to report more precise source ranges for
code that is skipped due to conditional directives.

The new behavior includes the '#' from the opening directive and the full text
of the line containing the closing directive in the skipped area. This matches
up clang's behavior (we don't IRGen the code between the closing "endif" and
the end of a line).

This also affects the code coverage implementation. See llvm.org/PR34166 (this
also happens to be rdar://problem/23224058).

The old behavior (report the end of the skipped range as the end
location of the 'endif' token) is preserved for indexing clients.

Differential Revision: https://reviews.llvm.org/D36642

llvm-svn: 312947

10 files changed, 104 insertions, 54 deletions

diff --git a/clang/include/clang/Lex/PPCallbacks.h b/clang/include/clang/Lex/PPCallbacks.h
index c1e1a549608..0e0a6c14067 100644
--- a/clang/include/clang/Lex/PPCallbacks.h
+++ b/clang/include/clang/Lex/PPCallbacks.h
@@ -266,7 +266,10 @@ public:
   /// \brief Hook called when a source range is skipped.
   /// \param Range The SourceRange that was skipped. The range begins at the
   /// \#if/\#else directive and ends after the \#endif/\#else directive.
-  virtual void SourceRangeSkipped(SourceRange Range) {
+  /// \param EndifLoc The end location of the 'endif' token, which may precede
+  /// the range skipped by the directive (e.g excluding comments after an
+  /// 'endif').
+  virtual void SourceRangeSkipped(SourceRange Range, SourceLocation EndifLoc) {
   }
 
   enum ConditionValueKind {
@@ -468,9 +471,9 @@ public:
     Second->Defined(MacroNameTok, MD, Range);
   }
 
-  void SourceRangeSkipped(SourceRange Range) override {
-    First->SourceRangeSkipped(Range);
-    Second->SourceRangeSkipped(Range);
+  void SourceRangeSkipped(SourceRange Range, SourceLocation EndifLoc) override {
+    First->SourceRangeSkipped(Range, EndifLoc);
+    Second->SourceRangeSkipped(Range, EndifLoc);
   }
 
   /// \brief Hook called whenever an \#if is seen.
diff --git a/clang/include/clang/Lex/PreprocessingRecord.h b/clang/include/clang/Lex/PreprocessingRecord.h
index fc2c5070040..882b8aedf7c 100644
--- a/clang/include/clang/Lex/PreprocessingRecord.h
+++ b/clang/include/clang/Lex/PreprocessingRecord.h
@@ -504,7 +504,8 @@ namespace clang {
     void Defined(const Token &MacroNameTok, const MacroDefinition &MD,
                  SourceRange Range) override;
 
-    void SourceRangeSkipped(SourceRange Range) override;
+    void SourceRangeSkipped(SourceRange Range,
+                            SourceLocation EndifLoc) override;
 
     void addMacroExpansion(const Token &Id, const MacroInfo *MI,
                            SourceRange Range);
diff --git a/clang/include/clang/Lex/Preprocessor.h b/clang/include/clang/Lex/Preprocessor.h
index 3dd84d8cfb2..ae6f2018304 100644
--- a/clang/include/clang/Lex/Preprocessor.h
+++ b/clang/include/clang/Lex/Preprocessor.h
@@ -1837,7 +1837,8 @@ private:
   /// \p FoundElse is false, then \#else directives are ok, if not, then we have
   /// already seen one so a \#else directive is a duplicate.  When this returns,
   /// the caller can lex the first valid token.
-  void SkipExcludedConditionalBlock(SourceLocation IfTokenLoc,
+  void SkipExcludedConditionalBlock(const Token &HashToken,
+                                    SourceLocation IfTokenLoc,
                                     bool FoundNonSkipPortion, bool FoundElse,
                                     SourceLocation ElseLoc = SourceLocation());
 
@@ -2031,12 +2032,13 @@ private:
   void HandleUndefDirective();
 
   // Conditional Inclusion.
-  void HandleIfdefDirective(Token &Tok, bool isIfndef,
-                            bool ReadAnyTokensBeforeDirective);
-  void HandleIfDirective(Token &Tok, bool ReadAnyTokensBeforeDirective);
+  void HandleIfdefDirective(Token &Tok, const Token &HashToken,
+                            bool isIfndef, bool ReadAnyTokensBeforeDirective);
+  void HandleIfDirective(Token &Tok, const Token &HashToken,
+                         bool ReadAnyTokensBeforeDirective);
   void HandleEndifDirective(Token &Tok);
-  void HandleElseDirective(Token &Tok);
-  void HandleElifDirective(Token &Tok);
+  void HandleElseDirective(Token &Tok, const Token &HashToken);
+  void HandleElifDirective(Token &Tok, const Token &HashToken);
 
   // Pragmas.
   void HandlePragmaDirective(SourceLocation IntroducerLoc,
diff --git a/clang/lib/CodeGen/CoverageMappingGen.cpp b/clang/lib/CodeGen/CoverageMappingGen.cpp
index f47dd07f0cc..cf736a484e5 100644
--- a/clang/lib/CodeGen/CoverageMappingGen.cpp
+++ b/clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -29,7 +29,7 @@ using namespace clang;
 using namespace CodeGen;
 using namespace llvm::coverage;
 
-void CoverageSourceInfo::SourceRangeSkipped(SourceRange Range) {
+void CoverageSourceInfo::SourceRangeSkipped(SourceRange Range, SourceLocation) {
   SkippedRanges.push_back(Range);
 }
 
diff --git a/clang/lib/CodeGen/CoverageMappingGen.h b/clang/lib/CodeGen/CoverageMappingGen.h
index b6789c2a79f..d07ed5ebcf2 100644
--- a/clang/lib/CodeGen/CoverageMappingGen.h
+++ b/clang/lib/CodeGen/CoverageMappingGen.h
@@ -39,7 +39,7 @@ class CoverageSourceInfo : public PPCallbacks {
 public:
   ArrayRef<SourceRange> getSkippedRanges() const { return SkippedRanges; }
 
-  void SourceRangeSkipped(SourceRange Range) override;
+  void SourceRangeSkipped(SourceRange Range, SourceLocation EndifLoc) override;
 };
 
 namespace CodeGen {
diff --git a/clang/lib/Lex/PPDirectives.cpp b/clang/lib/Lex/PPDirectives.cpp
index 6b09398e468..556e76356da 100644
--- a/clang/lib/Lex/PPDirectives.cpp
+++ b/clang/lib/Lex/PPDirectives.cpp
@@ -79,7 +79,8 @@ Preprocessor::AllocateVisibilityMacroDirective(SourceLocation Loc,
 }
 
 /// \brief Read and discard all tokens remaining on the current line until
-/// the tok::eod token is found.
+/// the tok::eod token is found. If the discarded tokens are in a skipped range,
+/// complete the range and pass it to the \c SourceRangeSkipped callback.
 void Preprocessor::DiscardUntilEndOfDirective() {
   Token Tmp;
   do {
@@ -350,7 +351,8 @@ void Preprocessor::CheckEndOfDirective(const char *DirType, bool EnableMacros) {
 /// If ElseOk is true, then \#else directives are ok, if not, then we have
 /// already seen one so a \#else directive is a duplicate.  When this returns,
 /// the caller can lex the first valid token.
-void Preprocessor::SkipExcludedConditionalBlock(SourceLocation IfTokenLoc,
+void Preprocessor::SkipExcludedConditionalBlock(const Token &HashToken,
+                                                SourceLocation IfTokenLoc,
                                                 bool FoundNonSkipPortion,
                                                 bool FoundElse,
                                                 SourceLocation ElseLoc) {
@@ -558,10 +560,10 @@ void Preprocessor::SkipExcludedConditionalBlock(SourceLocation IfTokenLoc,
   // the #if block.
   CurPPLexer->LexingRawMode = false;
 
-  if (Callbacks) {
-    SourceLocation BeginLoc = ElseLoc.isValid() ? ElseLoc : IfTokenLoc;
-    Callbacks->SourceRangeSkipped(SourceRange(BeginLoc, Tok.getLocation()));
-  }
+  if (Callbacks)
+    Callbacks->SourceRangeSkipped(
+        SourceRange(HashToken.getLocation(), CurPPLexer->getSourceLocation()),
+        Tok.getLocation());
 }
 
 void Preprocessor::PTHSkipExcludedConditionalBlock() {
@@ -949,15 +951,17 @@ void Preprocessor::HandleDirective(Token &Result) {
     default: break;
     // C99 6.10.1 - Conditional Inclusion.
     case tok::pp_if:
-      return HandleIfDirective(Result, ReadAnyTokensBeforeDirective);
+      return HandleIfDirective(Result, SavedHash, ReadAnyTokensBeforeDirective);
     case tok::pp_ifdef:
-      return HandleIfdefDirective(Result, false, true/*not valid for miopt*/);
+      return HandleIfdefDirective(Result, SavedHash, false,
+                                  true /*not valid for miopt*/);
     case tok::pp_ifndef:
-      return HandleIfdefDirective(Result, true, ReadAnyTokensBeforeDirective);
+      return HandleIfdefDirective(Result, SavedHash, true,
+                                  ReadAnyTokensBeforeDirective);
     case tok::pp_elif:
-      return HandleElifDirective(Result);
+      return HandleElifDirective(Result, SavedHash);
     case tok::pp_else:
-      return HandleElseDirective(Result);
+      return HandleElseDirective(Result, SavedHash);
     case tok::pp_endif:
       return HandleEndifDirective(Result);
 
@@ -2613,7 +2617,9 @@ void Preprocessor::HandleUndefDirective() {
 /// true if any tokens have been returned or pp-directives activated before this
 /// \#ifndef has been lexed.
 ///
-void Preprocessor::HandleIfdefDirective(Token &Result, bool isIfndef,
+void Preprocessor::HandleIfdefDirective(Token &Result,
+                                        const Token &HashToken,
+                                        bool isIfndef,
                                         bool ReadAnyTokensBeforeDirective) {
   ++NumIf;
   Token DirectiveTok = Result;
@@ -2625,8 +2631,8 @@ void Preprocessor::HandleIfdefDirective(Token &Result, bool isIfndef,
   if (MacroNameTok.is(tok::eod)) {
     // Skip code until we get to #endif.  This helps with recovery by not
     // emitting an error when the #endif is reached.
-    SkipExcludedConditionalBlock(DirectiveTok.getLocation(),
-                                 /*Foundnonskip*/false, /*FoundElse*/false);
+    SkipExcludedConditionalBlock(HashToken, DirectiveTok.getLocation(),
+                                 /*Foundnonskip*/ false, /*FoundElse*/ false);
     return;
   }
 
@@ -2674,15 +2680,16 @@ void Preprocessor::HandleIfdefDirective(Token &Result, bool isIfndef,
                                      /*foundelse*/false);
   } else {
     // No, skip the contents of this block.
-    SkipExcludedConditionalBlock(DirectiveTok.getLocation(),
-                                 /*Foundnonskip*/false,
-                                 /*FoundElse*/false);
+    SkipExcludedConditionalBlock(HashToken, DirectiveTok.getLocation(),
+                                 /*Foundnonskip*/ false,
+                                 /*FoundElse*/ false);
   }
 }
 
 /// HandleIfDirective - Implements the \#if directive.
 ///
 void Preprocessor::HandleIfDirective(Token &IfToken,
+                                     const Token &HashToken,
                                      bool ReadAnyTokensBeforeDirective) {
   ++NumIf;
 
@@ -2720,8 +2727,9 @@ void Preprocessor::HandleIfDirective(Token &IfToken,
                                    /*foundnonskip*/true, /*foundelse*/false);
   } else {
     // No, skip the contents of this block.
-    SkipExcludedConditionalBlock(IfToken.getLocation(), /*Foundnonskip*/false,
-                                 /*FoundElse*/false);
+    SkipExcludedConditionalBlock(HashToken, IfToken.getLocation(),
+                                 /*Foundnonskip*/ false,
+                                 /*FoundElse*/ false);
   }
 }
 
@@ -2753,7 +2761,7 @@ void Preprocessor::HandleEndifDirective(Token &EndifToken) {
 
 /// HandleElseDirective - Implements the \#else directive.
 ///
-void Preprocessor::HandleElseDirective(Token &Result) {
+void Preprocessor::HandleElseDirective(Token &Result, const Token &HashToken) {
   ++NumElse;
 
   // #else directive in a non-skipping conditional... start skipping.
@@ -2784,13 +2792,14 @@ void Preprocessor::HandleElseDirective(Token &Result) {
   }
 
   // Finally, skip the rest of the contents of this block.
-  SkipExcludedConditionalBlock(CI.IfLoc, /*Foundnonskip*/true,
-                               /*FoundElse*/true, Result.getLocation());
+  SkipExcludedConditionalBlock(HashToken, CI.IfLoc, /*Foundnonskip*/ true,
+                               /*FoundElse*/ true, Result.getLocation());
 }
 
 /// HandleElifDirective - Implements the \#elif directive.
 ///
-void Preprocessor::HandleElifDirective(Token &ElifToken) {
+void Preprocessor::HandleElifDirective(Token &ElifToken,
+                                       const Token &HashToken) {
   ++NumElse;
 
   // #elif directive in a non-skipping conditional... start skipping.
@@ -2827,7 +2836,7 @@ void Preprocessor::HandleElifDirective(Token &ElifToken) {
   }
 
   // Finally, skip the rest of the contents of this block.
-  SkipExcludedConditionalBlock(CI.IfLoc, /*Foundnonskip*/true,
-                               /*FoundElse*/CI.FoundElse,
+  SkipExcludedConditionalBlock(HashToken, CI.IfLoc, /*Foundnonskip*/ true,
+                               /*FoundElse*/ CI.FoundElse,
                                ElifToken.getLocation());
 }
diff --git a/clang/lib/Lex/PreprocessingRecord.cpp b/clang/lib/Lex/PreprocessingRecord.cpp
index 03c4cbe589d..954b569bb02 100644
--- a/clang/lib/Lex/PreprocessingRecord.cpp
+++ b/clang/lib/Lex/PreprocessingRecord.cpp
@@ -400,8 +400,9 @@ void PreprocessingRecord::Defined(const Token &MacroNameTok,
                       MacroNameTok.getLocation());
 }
 
-void PreprocessingRecord::SourceRangeSkipped(SourceRange Range) {
-  SkippedRanges.push_back(Range);
+void PreprocessingRecord::SourceRangeSkipped(SourceRange Range,
+                                             SourceLocation EndifLoc) {
+  SkippedRanges.emplace_back(Range.getBegin(), EndifLoc);
 }
 
 void PreprocessingRecord::MacroExpands(const Token &Id,
diff --git a/clang/test/CoverageMapping/preprocessor.c b/clang/test/CoverageMapping/preprocessor.c
index bd82b3939ed..cce8b67999c 100644
--- a/clang/test/CoverageMapping/preprocessor.c
+++ b/clang/test/CoverageMapping/preprocessor.c
@@ -3,36 +3,69 @@
                  // CHECK: func
 void func() {    // CHECK: File 0, [[@LINE]]:13 -> [[@LINE+5]]:2 = #0
   int i = 0;
-#ifdef MACRO     // CHECK-NEXT: Skipped,File 0, [[@LINE]]:2 -> [[@LINE+2]]:2 = 0
+#ifdef MACRO     // CHECK-NEXT: Skipped,File 0, [[@LINE]]:1 -> [[@LINE+3]]:1 = 0
   int x = i;
 #endif
 }
 
-#if 0
-  int g = 0;
-
-  void bar() { }
-#endif
-
                  // CHECK: main
 int main() {     // CHECK-NEXT: File 0, [[@LINE]]:12 -> {{[0-9]+}}:2 = #0
   int i = 0;
-#if 0            // CHECK-NEXT: Skipped,File 0, [[@LINE]]:2 -> [[@LINE+4]]:2 = 0
+#  if 0            // CHECK-NEXT: Skipped,File 0, [[@LINE]]:1 -> [[@LINE+5]]:1 = 0
   if(i == 0) {
     i = 1;
   }
-#endif
+#  endif // Mark me skipped!
 
 #if 1
                  // CHECK-NEXT: File 0, [[@LINE+1]]:6 -> [[@LINE+1]]:12 = #0
   if(i == 0) {   // CHECK-NEXT: File 0, [[@LINE]]:14 -> [[@LINE+2]]:4 = #1
     i = 1;
   }
-#else            // CHECK-NEXT: Skipped,File 0, [[@LINE]]:2 -> [[@LINE+5]]:2 = 0
+#else            // CHECK-NEXT: Skipped,File 0, [[@LINE]]:1 -> [[@LINE+6]]:1 = 0
   if(i == 1) {
     i = 0;
   }
 }
 #endif
+
+  // CHECK-NEXT: Skipped,File 0, [[@LINE+1]]:1 -> [[@LINE+5]]:1
+#\
+  if 0
+#\
+  endif // also skipped
+
+#if 1
+  // CHECK-NEXT: Skipped,File 0, [[@LINE+1]]:1 -> [[@LINE+4]]:1
+#\
+  elif 0
+#endif
+
+#if 1
+  // CHECK-NEXT: Skipped,File 0, [[@LINE+1]]:1 -> [[@LINE+4]]:1
+#\
+  else
+#endif
+
+  // CHECK-NEXT: Skipped,File 0, [[@LINE+1]]:1 -> [[@LINE+5]]:1
+#\
+  ifdef NOT_DEFINED
+#\
+  endif
+
+  // CHECK-NEXT: Skipped,File 0, [[@LINE+1]]:1 -> [[@LINE+5]]:1
+#\
+  ifndef __FILE__
+#\
+  endif
+
+  // CHECK-NEXT: Skipped,File 0, [[@LINE+1]]:1 -> [[@LINE+7]]:1
+#\
+  ifdef NOT_DEFINED
+#\
+  \
+   \
+    endif // also skipped
+
   return 0;
 }
diff --git a/clang/test/Index/skipped-ranges.c b/clang/test/Index/skipped-ranges.c
index bd16fb3856b..f6c6c7f638e 100644
--- a/clang/test/Index/skipped-ranges.c
+++ b/clang/test/Index/skipped-ranges.c
@@ -20,6 +20,6 @@ int probably_hot = 1;
 #endif // cool
 
 // RUN: env CINDEXTEST_SHOW_SKIPPED_RANGES=1 c-index-test -test-annotate-tokens=%s:1:1:16:1 %s | FileCheck %s
-// CHECK: Skipping: [5:2 - 6:7]
-// CHECK: Skipping: [8:2 - 12:7]
-// CHECK: Skipping: [14:2 - 20:7]
+// CHECK: Skipping: [5:1 - 6:7]
+// CHECK: Skipping: [8:1 - 12:7]
+// CHECK: Skipping: [14:1 - 20:7]
diff --git a/clang/tools/libclang/Indexing.cpp b/clang/tools/libclang/Indexing.cpp
index 5312b7c0169..2a136242ef5 100644
--- a/clang/tools/libclang/Indexing.cpp
+++ b/clang/tools/libclang/Indexing.cpp
@@ -272,7 +272,8 @@ public:
   /// SourceRangeSkipped - This hook is called when a source range is skipped.
   /// \param Range The SourceRange that was skipped. The range begins at the
   /// #if/#else directive and ends after the #endif/#else directive.
-  void SourceRangeSkipped(SourceRange Range) override {}
+  void SourceRangeSkipped(SourceRange Range, SourceLocation EndifLoc) override {
+  }
 };
 
 //===----------------------------------------------------------------------===//