Revert r351209 (which was a revert of r350891) with a fix.

The test case had a parse error that was causing the condition string to be misreported. We now have better fallback code for error cases.

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@351470 91177308-0d34-0410-b5e6-96231b3b80d8

4 files changed, 198 insertions, 122 deletions

diff --git a/include/clang/Lex/Preprocessor.h b/include/clang/Lex/Preprocessor.h
index 64ddb5307f..36cb718570 100644
--- a/include/clang/Lex/Preprocessor.h
+++ b/include/clang/Lex/Preprocessor.h
@@ -1816,8 +1816,8 @@ public:
   void CheckEndOfDirective(const char *DirType, bool EnableMacros = false);
 
   /// Read and discard all tokens remaining on the current line until
-  /// the tok::eod token is found.
-  void DiscardUntilEndOfDirective();
+  /// the tok::eod token is found. Returns the range of the skipped tokens.
+  SourceRange DiscardUntilEndOfDirective();
 
   /// Returns true if the preprocessor has seen a use of
   /// __DATE__ or __TIME__ in the file so far.
@@ -1982,6 +1982,9 @@ private:
 
     /// True if the expression contained identifiers that were undefined.
     bool IncludedUndefinedIds;
+
+    /// The source range for the expression.
+    SourceRange ExprRange;
   };
 
   /// Evaluate an integer constant expression that may occur after a
diff --git a/lib/Lex/PPDirectives.cpp b/lib/Lex/PPDirectives.cpp
index d62a3513c7..3bee026b04 100644
--- a/lib/Lex/PPDirectives.cpp
+++ b/lib/Lex/PPDirectives.cpp
@@ -76,18 +76,24 @@ Preprocessor::AllocateVisibilityMacroDirective(SourceLocation Loc,
                                                bool isPublic) {
   return new (BP) VisibilityMacroDirective(Loc, isPublic);
 }
-
-/// Read and discard all tokens remaining on the current line until
-/// the tok::eod token is found.
-void Preprocessor::DiscardUntilEndOfDirective() {
-  Token Tmp;
-  do {
-    LexUnexpandedToken(Tmp);
-    assert(Tmp.isNot(tok::eof) && "EOF seen while discarding directive tokens");
-  } while (Tmp.isNot(tok::eod));
-}
-
-/// Enumerates possible cases of #define/#undef a reserved identifier.
+

+/// Read and discard all tokens remaining on the current line until

+/// the tok::eod token is found.

+SourceRange Preprocessor::DiscardUntilEndOfDirective() {

+  Token Tmp;

+  SourceRange Res;

+

+  LexUnexpandedToken(Tmp);

+  Res.setBegin(Tmp.getLocation());

+  while (Tmp.isNot(tok::eod)) {

+    assert(Tmp.isNot(tok::eof) && "EOF seen while discarding directive tokens");

+    LexUnexpandedToken(Tmp);

+  }

+  Res.setEnd(Tmp.getLocation());

+  return Res;

+}

+

+/// Enumerates possible cases of #define/#undef a reserved identifier.

 enum MacroDiag {
   MD_NoWarn,        //> Not a reserved identifier
   MD_KeywordDef,    //> Macro hides keyword, enabled by default
@@ -535,25 +541,25 @@ void Preprocessor::SkipExcludedConditionalBlock(SourceLocation HashTokenLoc,
 
         // If this is in a skipping block or if we're already handled this #if
         // block, don't bother parsing the condition.
-        if (CondInfo.WasSkipping || CondInfo.FoundNonSkip) {
-          DiscardUntilEndOfDirective();
-        } else {
-          const SourceLocation CondBegin = CurPPLexer->getSourceLocation();
-          // Restore the value of LexingRawMode so that identifiers are
-          // looked up, etc, inside the #elif expression.
-          assert(CurPPLexer->LexingRawMode && "We have to be skipping here!");
-          CurPPLexer->LexingRawMode = false;
-          IdentifierInfo *IfNDefMacro = nullptr;
-          const bool CondValue = EvaluateDirectiveExpression(IfNDefMacro).Conditional;
-          CurPPLexer->LexingRawMode = true;
-          if (Callbacks) {
-            const SourceLocation CondEnd = CurPPLexer->getSourceLocation();
-            Callbacks->Elif(Tok.getLocation(),
-                            SourceRange(CondBegin, CondEnd),
-                            (CondValue ? PPCallbacks::CVK_True : PPCallbacks::CVK_False), CondInfo.IfLoc);
-          }
-          // If this condition is true, enter it!
-          if (CondValue) {
+        if (CondInfo.WasSkipping || CondInfo.FoundNonSkip) {

+          DiscardUntilEndOfDirective();

+        } else {

+          // Restore the value of LexingRawMode so that identifiers are

+          // looked up, etc, inside the #elif expression.

+          assert(CurPPLexer->LexingRawMode && "We have to be skipping here!");

+          CurPPLexer->LexingRawMode = false;

+          IdentifierInfo *IfNDefMacro = nullptr;

+          DirectiveEvalResult DER = EvaluateDirectiveExpression(IfNDefMacro);

+          const bool CondValue = DER.Conditional;

+          CurPPLexer->LexingRawMode = true;

+          if (Callbacks) {

+            Callbacks->Elif(

+                Tok.getLocation(), DER.ExprRange,

+                (CondValue ? PPCallbacks::CVK_True : PPCallbacks::CVK_False),

+                CondInfo.IfLoc);

+          }

+          // If this condition is true, enter it!

+          if (CondValue) {

             CondInfo.FoundNonSkip = true;
             break;
           }
@@ -1113,25 +1119,30 @@ void Preprocessor::HandleLineDirective() {
   // If the StrTok is "eod", then it wasn't present.  Otherwise, it must be a
   // string followed by eod.
   if (StrTok.is(tok::eod))
-    ; // ok
-  else if (StrTok.isNot(tok::string_literal)) {
-    Diag(StrTok, diag::err_pp_line_invalid_filename);
-    return DiscardUntilEndOfDirective();
-  } else if (StrTok.hasUDSuffix()) {
-    Diag(StrTok, diag::err_invalid_string_udl);
-    return DiscardUntilEndOfDirective();
-  } else {
-    // Parse and validate the string, converting it into a unique ID.
-    StringLiteralParser Literal(StrTok, *this);
-    assert(Literal.isAscii() && "Didn't allow wide strings in");
-    if (Literal.hadError)
-      return DiscardUntilEndOfDirective();
-    if (Literal.Pascal) {
-      Diag(StrTok, diag::err_pp_linemarker_invalid_filename);
-      return DiscardUntilEndOfDirective();
-    }
-    FilenameID = SourceMgr.getLineTableFilenameID(Literal.GetString());
-
+    ; // ok

+  else if (StrTok.isNot(tok::string_literal)) {

+    Diag(StrTok, diag::err_pp_line_invalid_filename);

+    DiscardUntilEndOfDirective();

+    return;

+  } else if (StrTok.hasUDSuffix()) {

+    Diag(StrTok, diag::err_invalid_string_udl);

+    DiscardUntilEndOfDirective();

+    return;

+  } else {

+    // Parse and validate the string, converting it into a unique ID.

+    StringLiteralParser Literal(StrTok, *this);

+    assert(Literal.isAscii() && "Didn't allow wide strings in");

+    if (Literal.hadError) {

+      DiscardUntilEndOfDirective();

+      return;

+    }

+    if (Literal.Pascal) {

+      Diag(StrTok, diag::err_pp_linemarker_invalid_filename);

+      DiscardUntilEndOfDirective();

+      return;

+    }

+    FilenameID = SourceMgr.getLineTableFilenameID(Literal.GetString());

+

     // Verify that there is nothing after the string, other than EOD.  Because
     // of C99 6.10.4p5, macros that expand to empty tokens are ok.
     CheckEndOfDirective("line", true);
@@ -1258,25 +1269,30 @@ void Preprocessor::HandleDigitDirective(Token &DigitTok) {
   // string followed by eod.
   if (StrTok.is(tok::eod)) {
     // Treat this like "#line NN", which doesn't change file characteristics.
-    FileKind = SourceMgr.getFileCharacteristic(DigitTok.getLocation());
-  } else if (StrTok.isNot(tok::string_literal)) {
-    Diag(StrTok, diag::err_pp_linemarker_invalid_filename);
-    return DiscardUntilEndOfDirective();
-  } else if (StrTok.hasUDSuffix()) {
-    Diag(StrTok, diag::err_invalid_string_udl);
-    return DiscardUntilEndOfDirective();
-  } else {
-    // Parse and validate the string, converting it into a unique ID.
-    StringLiteralParser Literal(StrTok, *this);
-    assert(Literal.isAscii() && "Didn't allow wide strings in");
-    if (Literal.hadError)
-      return DiscardUntilEndOfDirective();
-    if (Literal.Pascal) {
-      Diag(StrTok, diag::err_pp_linemarker_invalid_filename);
-      return DiscardUntilEndOfDirective();
-    }
-    FilenameID = SourceMgr.getLineTableFilenameID(Literal.GetString());
-
+    FileKind = SourceMgr.getFileCharacteristic(DigitTok.getLocation());

+  } else if (StrTok.isNot(tok::string_literal)) {

+    Diag(StrTok, diag::err_pp_linemarker_invalid_filename);

+    DiscardUntilEndOfDirective();

+    return;

+  } else if (StrTok.hasUDSuffix()) {

+    Diag(StrTok, diag::err_invalid_string_udl);

+    DiscardUntilEndOfDirective();

+    return;

+  } else {

+    // Parse and validate the string, converting it into a unique ID.

+    StringLiteralParser Literal(StrTok, *this);

+    assert(Literal.isAscii() && "Didn't allow wide strings in");

+    if (Literal.hadError) {

+      DiscardUntilEndOfDirective();

+      return;

+    }

+    if (Literal.Pascal) {

+      Diag(StrTok, diag::err_pp_linemarker_invalid_filename);

+      DiscardUntilEndOfDirective();

+      return;

+    }

+    FilenameID = SourceMgr.getLineTableFilenameID(Literal.GetString());

+

     // If a filename was present, read any flags that are present.
     if (ReadLineMarkerFlags(IsFileEntry, IsFileExit, FileKind, *this))
       return;
@@ -1340,13 +1356,14 @@ void Preprocessor::HandleIdentSCCSDirective(Token &Tok) {
       DiscardUntilEndOfDirective();
     return;
   }
-
-  if (StrTok.hasUDSuffix()) {
-    Diag(StrTok, diag::err_invalid_string_udl);
-    return DiscardUntilEndOfDirective();
-  }
-
-  // Verify that there is nothing after the string, other than EOD.
+

+  if (StrTok.hasUDSuffix()) {

+    Diag(StrTok, diag::err_invalid_string_udl);

+    DiscardUntilEndOfDirective();

+    return;

+  }

+

+  // Verify that there is nothing after the string, other than EOD.

   CheckEndOfDirective("ident");
 
   if (Callbacks) {
@@ -2788,31 +2805,29 @@ void Preprocessor::HandleIfDirective(Token &IfToken,
                                      const Token &HashToken,
                                      bool ReadAnyTokensBeforeDirective) {
   ++NumIf;
-
-  // Parse and evaluate the conditional expression.
-  IdentifierInfo *IfNDefMacro = nullptr;
-  const SourceLocation ConditionalBegin = CurPPLexer->getSourceLocation();
-  const DirectiveEvalResult DER = EvaluateDirectiveExpression(IfNDefMacro);
-  const bool ConditionalTrue = DER.Conditional;
-  const SourceLocation ConditionalEnd = CurPPLexer->getSourceLocation();
-
-  // If this condition is equivalent to #ifndef X, and if this is the first
-  // directive seen, handle it for the multiple-include optimization.
+

+  // Parse and evaluate the conditional expression.

+  IdentifierInfo *IfNDefMacro = nullptr;

+  const DirectiveEvalResult DER = EvaluateDirectiveExpression(IfNDefMacro);

+  const bool ConditionalTrue = DER.Conditional;

+

+  // If this condition is equivalent to #ifndef X, and if this is the first

+  // directive seen, handle it for the multiple-include optimization.

   if (CurPPLexer->getConditionalStackDepth() == 0) {
     if (!ReadAnyTokensBeforeDirective && IfNDefMacro && ConditionalTrue)
       // FIXME: Pass in the location of the macro name, not the 'if' token.
       CurPPLexer->MIOpt.EnterTopLevelIfndef(IfNDefMacro, IfToken.getLocation());
     else
       CurPPLexer->MIOpt.EnterTopLevelConditional();
-  }
-
-  if (Callbacks)
-    Callbacks->If(IfToken.getLocation(),
-                  SourceRange(ConditionalBegin, ConditionalEnd),
-                  (ConditionalTrue ? PPCallbacks::CVK_True : PPCallbacks::CVK_False));
-
-  // Should we include the stuff contained by this directive?
-  if (PPOpts->SingleFileParseMode && DER.IncludedUndefinedIds) {
+  }

+

+  if (Callbacks)

+    Callbacks->If(

+        IfToken.getLocation(), DER.ExprRange,

+        (ConditionalTrue ? PPCallbacks::CVK_True : PPCallbacks::CVK_False));

+

+  // Should we include the stuff contained by this directive?

+  if (PPOpts->SingleFileParseMode && DER.IncludedUndefinedIds) {

     // In 'single-file-parse mode' undefined identifiers trigger parsing of all
     // the directive blocks.
     CurPPLexer->pushConditionalLevel(IfToken.getLocation(), /*wasskip*/false,
@@ -2899,15 +2914,13 @@ void Preprocessor::HandleElifDirective(Token &ElifToken,
                                        const Token &HashToken) {
   ++NumElse;
 
-  // #elif directive in a non-skipping conditional... start skipping.
-  // We don't care what the condition is, because we will always skip it (since
-  // the block immediately before it was included).
-  const SourceLocation ConditionalBegin = CurPPLexer->getSourceLocation();
-  DiscardUntilEndOfDirective();
-  const SourceLocation ConditionalEnd = CurPPLexer->getSourceLocation();
-
-  PPConditionalInfo CI;
-  if (CurPPLexer->popConditionalLevel(CI)) {
+  // #elif directive in a non-skipping conditional... start skipping.

+  // We don't care what the condition is, because we will always skip it (since

+  // the block immediately before it was included).

+  SourceRange ConditionRange = DiscardUntilEndOfDirective();

+

+  PPConditionalInfo CI;

+  if (CurPPLexer->popConditionalLevel(CI)) {

     Diag(ElifToken, diag::pp_err_elif_without_if);
     return;
   }
@@ -2917,14 +2930,13 @@ void Preprocessor::HandleElifDirective(Token &ElifToken,
     CurPPLexer->MIOpt.EnterTopLevelConditional();
 
   // If this is a #elif with a #else before it, report the error.
-  if (CI.FoundElse) Diag(ElifToken, diag::pp_err_elif_after_else);
-
-  if (Callbacks)
-    Callbacks->Elif(ElifToken.getLocation(),
-                    SourceRange(ConditionalBegin, ConditionalEnd),
-                    PPCallbacks::CVK_NotEvaluated, CI.IfLoc);
-
-  if (PPOpts->SingleFileParseMode && !CI.FoundNonSkip) {
+  if (CI.FoundElse) Diag(ElifToken, diag::pp_err_elif_after_else);

+

+  if (Callbacks)

+    Callbacks->Elif(ElifToken.getLocation(), ConditionRange,

+                    PPCallbacks::CVK_NotEvaluated, CI.IfLoc);

+

+  if (PPOpts->SingleFileParseMode && !CI.FoundNonSkip) {

     // In 'single-file-parse mode' undefined identifiers trigger parsing of all
     // the directive blocks.
     CurPPLexer->pushConditionalLevel(ElifToken.getLocation(), /*wasskip*/false,
diff --git a/lib/Lex/PPExpressions.cpp b/lib/Lex/PPExpressions.cpp
index ac01efad9b..01ded9c1e2 100644
--- a/lib/Lex/PPExpressions.cpp
+++ b/lib/Lex/PPExpressions.cpp
@@ -152,8 +152,8 @@ static bool EvaluateDefined(PPValue &Result, Token &PeekTok, DefinedTracker &DT,
       return true;
     }
     // Consume the ).
-    Result.setEnd(PeekTok.getLocation());
     PP.LexNonComment(PeekTok);
+    Result.setEnd(PeekTok.getLocation());
   } else {
     // Consume identifier.
     Result.setEnd(PeekTok.getLocation());
@@ -842,14 +842,22 @@ Preprocessor::EvaluateDirectiveExpression(IdentifierInfo *&IfNDefMacro) {
 
   PPValue ResVal(BitWidth);
   DefinedTracker DT;
+  SourceLocation ExprStartLoc = SourceMgr.getExpansionLoc(Tok.getLocation());
   if (EvaluateValue(ResVal, Tok, DT, true, *this)) {
     // Parse error, skip the rest of the macro line.
+    SourceRange ConditionRange = ExprStartLoc;
     if (Tok.isNot(tok::eod))
-      DiscardUntilEndOfDirective();
+      ConditionRange = DiscardUntilEndOfDirective();
 
     // Restore 'DisableMacroExpansion'.
     DisableMacroExpansion = DisableMacroExpansionAtStartOfDirective;
-    return {false, DT.IncludedUndefinedIds};
+
+    // We cannot trust the source range from the value because there was a
+    // parse error. Track the range manually -- the end of the directive is the
+    // end of the condition range.
+    return {false,
+            DT.IncludedUndefinedIds,
+            {ExprStartLoc, ConditionRange.getEnd()}};
   }
 
   // If we are at the end of the expression after just parsing a value, there
@@ -863,7 +871,7 @@ Preprocessor::EvaluateDirectiveExpression(IdentifierInfo *&IfNDefMacro) {
 
     // Restore 'DisableMacroExpansion'.
     DisableMacroExpansion = DisableMacroExpansionAtStartOfDirective;
-    return {ResVal.Val != 0, DT.IncludedUndefinedIds};
+    return {ResVal.Val != 0, DT.IncludedUndefinedIds, ResVal.getRange()};
   }
 
   // Otherwise, we must have a binary operator (e.g. "#if 1 < 2"), so parse the
@@ -876,7 +884,7 @@ Preprocessor::EvaluateDirectiveExpression(IdentifierInfo *&IfNDefMacro) {
 
     // Restore 'DisableMacroExpansion'.
     DisableMacroExpansion = DisableMacroExpansionAtStartOfDirective;
-    return {false, DT.IncludedUndefinedIds};
+    return {false, DT.IncludedUndefinedIds, ResVal.getRange()};
   }
 
   // If we aren't at the tok::eod token, something bad happened, like an extra
@@ -888,5 +896,5 @@ Preprocessor::EvaluateDirectiveExpression(IdentifierInfo *&IfNDefMacro) {
 
   // Restore 'DisableMacroExpansion'.
   DisableMacroExpansion = DisableMacroExpansionAtStartOfDirective;
-  return {ResVal.Val != 0, DT.IncludedUndefinedIds};
+  return {ResVal.Val != 0, DT.IncludedUndefinedIds, ResVal.getRange()};
 }
diff --git a/unittests/Lex/PPCallbacksTest.cpp b/unittests/Lex/PPCallbacksTest.cpp
index 838e033e3d..e19b99cd6d 100644
--- a/unittests/Lex/PPCallbacksTest.cpp
+++ b/unittests/Lex/PPCallbacksTest.cpp
@@ -431,16 +431,69 @@ TEST_F(PPCallbacksTest, OpenCLExtensionPragmaDisabled) {
 }
 
 TEST_F(PPCallbacksTest, DirectiveExprRanges) {
+  const auto &Results1 = DirectiveExprRange("#if FLUZZY_FLOOF\n#endif\n");
+  EXPECT_EQ(Results1.size(), 1);
+  EXPECT_EQ(
+      GetSourceStringToEnd(CharSourceRange(Results1[0].ConditionRange, false)),
+      "FLUZZY_FLOOF");
+
+  const auto &Results2 = DirectiveExprRange("#if 1 + 4 < 7\n#endif\n");
+  EXPECT_EQ(Results2.size(), 1);
+  EXPECT_EQ(
+      GetSourceStringToEnd(CharSourceRange(Results2[0].ConditionRange, false)),
+      "1 + 4 < 7");
+
+  const auto &Results3 = DirectiveExprRange("#if 1 + \\\n  2\n#endif\n");
+  EXPECT_EQ(Results3.size(), 1);
+  EXPECT_EQ(
+      GetSourceStringToEnd(CharSourceRange(Results3[0].ConditionRange, false)),
+      "1 + \\\n  2");
+
+  const auto &Results4 = DirectiveExprRange("#if 0\n#elif FLOOFY\n#endif\n");
+  EXPECT_EQ(Results4.size(), 2);
+  EXPECT_EQ(
+      GetSourceStringToEnd(CharSourceRange(Results4[0].ConditionRange, false)),
+      "0");
+  EXPECT_EQ(
+      GetSourceStringToEnd(CharSourceRange(Results4[1].ConditionRange, false)),
+      "FLOOFY");
+
+  const auto &Results5 = DirectiveExprRange("#if 1\n#elif FLOOFY\n#endif\n");
+  EXPECT_EQ(Results5.size(), 2);
+  EXPECT_EQ(
+      GetSourceStringToEnd(CharSourceRange(Results5[0].ConditionRange, false)),
+      "1");
+  EXPECT_EQ(
+      GetSourceStringToEnd(CharSourceRange(Results5[1].ConditionRange, false)),
+      "FLOOFY");
+
+  const auto &Results6 =
+      DirectiveExprRange("#if defined(FLUZZY_FLOOF)\n#endif\n");
+  EXPECT_EQ(Results6.size(), 1);
+  EXPECT_EQ(
+      GetSourceStringToEnd(CharSourceRange(Results6[0].ConditionRange, false)),
+      "defined(FLUZZY_FLOOF)");
+
+  const auto &Results7 =
+      DirectiveExprRange("#if 1\n#elif defined(FLOOFY)\n#endif\n");
+  EXPECT_EQ(Results7.size(), 2);
+  EXPECT_EQ(
+      GetSourceStringToEnd(CharSourceRange(Results7[0].ConditionRange, false)),
+      "1");
+  EXPECT_EQ(
+      GetSourceStringToEnd(CharSourceRange(Results7[1].ConditionRange, false)),
+      "defined(FLOOFY)");
+
   const auto &Results8 =
       DirectiveExprRange("#define FLOOFY 0\n#if __FILE__ > FLOOFY\n#endif\n");
   EXPECT_EQ(Results8.size(), 1U);
   EXPECT_EQ(
       GetSourceStringToEnd(CharSourceRange(Results8[0].ConditionRange, false)),
-      " __FILE__ > FLOOFY\n#");
+      "__FILE__ > FLOOFY");
   EXPECT_EQ(
       Lexer::getSourceText(CharSourceRange(Results8[0].ConditionRange, false),
                            SourceMgr, LangOpts),
-      " __FILE__ > FLOOFY\n");
+      "__FILE__ > FLOOFY");
 }
 
 } // namespace