[Preprocessor] Always discard body of #define if we failed to parse it

[ilya-biryukov]

Summary

Preivously we would only discard it if we failed to parse parameter lists.
If we do not consume the body, parser sees tokens inside directive. In
turn, this leads to spurious diagnostics and a crash in TokenBuffer, see
the added tests.

[sammccall]

LG, though I'm obviously not an expert here. Thanks for tracking this down!

clang/lib/Lex/PPDirectives.cpp

2405 ↗

(On Diff #212583)

nit: "if we are still waiting for eod"? (to avoid inverting the sense)

[ilya-biryukov]

• Update a comment

[ilya-biryukov]

@hans, could you please merge rL367530 into the release?

[hans]

In D65517#1609902, @ilya-biryukov wrote:

@hans, could you please merge rL367530 into the release?

Sure! Let's have it bake in trunk for a bit, and then I'll merge it.

[ilya-biryukov]

SG, just wanted to make sure it's on your list ;-)

[hans]

In D65517#1609990, @ilya-biryukov wrote:

SG, just wanted to make sure it's on your list ;-)

Merged in r367681.

[ilya-biryukov]

Thanks!

cfe/trunk/lib/Lex/PPDirectives.cpp

 #include "clang/Lex/ModuleMap.h"
 #include "clang/Lex/PPCallbacks.h"
 #include "clang/Lex/Pragma.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Lex/PreprocessorOptions.h"
 #include "clang/Lex/Token.h"
 #include "clang/Lex/VariadicMacroSupport.h"
 #include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringSwitch.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/AlignOf.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/Path.h"
MacroInfo *Preprocessor::ReadOptionalMacroParameterListAndBody(
 
   Token LastTok = MacroNameTok;
   // Create the new macro.
   MacroInfo *const MI = AllocateMacroInfo(MacroNameTok.getLocation());
 
   Token Tok;
   LexUnexpandedToken(Tok);
 
+  // Ensure we consume the rest of the macro body if errors occur.
+  auto _ = llvm::make_scope_exit([&]() {
+    // The flag indicates if we are still waiting for 'eod'.
+    if (CurLexer->ParsingPreprocessorDirective)
+      DiscardUntilEndOfDirective();
+  });
+
   // Used to un-poison and then re-poison identifiers of the __VA_ARGS__ ilk
   // within their appropriate context.
   VariadicMacroScopeGuard VariadicMacroScopeGuard(*this);
 
   // If this is a function-like macro definition, parse the argument list,
   // marking each of the identifiers as being used as macro arguments.  Also,
   // check other constraints on the first token of the macro body.
   if (Tok.is(tok::eod)) {
     if (ImmediatelyAfterHeaderGuard) {
       // Save this macro information since it may part of a header guard.
       CurPPLexer->MIOpt.SetDefinedMacro(MacroNameTok.getIdentifierInfo(),
                                         MacroNameTok.getLocation());
     }
     // If there is no body to this macro, we have no special handling here.
   } else if (Tok.hasLeadingSpace()) {
     // This is a normal token with leading space.  Clear the leading space
     // marker on the first token to get proper expansion.
     Tok.clearFlag(Token::LeadingSpace);
   } else if (Tok.is(tok::l_paren)) {
     // This is a function-like macro definition.  Read the argument list.
     MI->setIsFunctionLike();
-    if (ReadMacroParameterList(MI, LastTok)) {
-      // Throw away the rest of the line.
-      if (CurPPLexer->ParsingPreprocessorDirective)
-        DiscardUntilEndOfDirective();
+    if (ReadMacroParameterList(MI, LastTok))
       return nullptr;
-    }
 
     // If this is a definition of an ISO C/C++ variadic function-like macro (not
     // using the GNU named varargs extension) inform our variadic scope guard
     // which un-poisons and re-poisons certain identifiers (e.g. __VA_ARGS__)
     // allowed only within the definition of a variadic macro.
 
     if (MI->isC99Varargs()) {
       VariadicMacroScopeGuard.enterScope();

cfe/trunk/test/Preprocessor/stringize_skipped.c

+// RUN: %clang_cc1 -fsyntax-only -verify %s
+// Ensure we see the error from PP and do not see errors from the parser.
+
+// expected-error@+1{{'#' is not followed by a macro parameter}}
+#define INVALID() #B 10+10

cfe/trunk/unittests/Tooling/Syntax/TokensTest.cpp

 )"},
       // Empty files should not crash.
       {R"cpp()cpp", R"(expanded tokens:
   <empty>
 file './input.cpp'
   spelled tokens:
     <empty>
   no mappings.
+)"},
+      // Should not crash on errors inside '#define' directives. Error is that
+      // stringification (#B) does not refer to a macro parameter.
+      {
+          R"cpp(
+a
+#define MACRO() A #B
+)cpp",
+          R"(expanded tokens:
+  a
+file './input.cpp'
+  spelled tokens:
+    a # define MACRO ( ) A # B
+  mappings:
+    ['#'_1, '<eof>'_9) => ['<eof>'_1, '<eof>'_1)
 )"}};
   for (auto &Test : TestCases)
     EXPECT_EQ(collectAndDump(Test.first), Test.second)
         << collectAndDump(Test.first);
 }
 
 TEST_F(TokenCollectorTest, Locations) {
   // Check locations of the tokens.