[preamble] Also record the "skipping" state of the preprocessor

[erikjv]

Summary

When a preamble ends in a conditional preprocessor block that is being
skipped, the preprocessor needs to continue skipping that block when
the preamble is used.

This fixes PR34570.

[ilya-biryukov]

LGTM. Only a few code style-related comments.

include/clang/Lex/Preprocessor.h

102

Maybe rename to HashTokenLoc for consistencty with IfTokenLoc?

337

Maybe move closer to other fields to avoid mixing data and functions?

1863

Maybe rename to HashTokenLoc for consistencty with IfTokenLoc?

lib/Lex/PPDirectives.cpp

353

Maybe rename to HashTokenLoc for consistencty with IfTokenLoc?

[ilya-biryukov]

Also, one more important thing. I'll unaccept revision for now, before getting feedback on this one.

include/clang/Lex/Preprocessor.h

337

Maybe initialize SkipInfo with default values? Or even better: forbid default constructor and put it into llvm::Optional?

It's a public member, so I guess there's a huge potential some code will eventually get into undefined behavior when using it.

[ilya-biryukov]

LGTM.

[nik]

This fixes the reported bug for me :)

There is another related issue that is not addressed by this change. I've reported it as

[preamble] Skipped ranges vanish after reparse (#ifdef with #include)
https://bugs.llvm.org/show_bug.cgi?id=34971

[cameron314]

@nik, pretty sure this fix I submitted ages ago fixes that. Been using it in production a couple years. Would be nice if someone reviewed it so we could finally upstream it.

[erikjv]

Submitted as r317308.

include/clang/Lex/Preprocessor.h

 ///
 /// Lexers know only about tokens within a single source file, and don't
 /// know anything about preprocessor-level issues like the \#include stack,
 /// token expansion, etc.
 class Preprocessor {
   friend class VariadicMacroScopeGuard;
   std::shared_ptr<PreprocessorOptions> PPOpts;
   DiagnosticsEngine        *Diags;
   LangOptions       &LangOpts;
   const TargetInfo  *Target;
   const TargetInfo  *AuxTarget;
   FileManager       &FileMgr;
   SourceManager     &SourceMgr;
   MemoryBufferCache &PCMCache;
   std::unique_ptr<ScratchBuffer> ScratchBuf;
   HeaderSearch      &HeaderInfo;
   ModuleLoader      &TheModuleLoader;
class Preprocessor {
 
   /// \brief The number of bytes that we will initially skip when entering the
   /// main file, along with a flag that indicates whether skipping this number
   /// of bytes will place the lexer at the start of a line.
   ///
   /// This is used when loading a precompiled preamble.
   std::pair<int, bool> SkipMainFilePreamble;
 
+public:
+  struct PreambleSkipInfo {
+    PreambleSkipInfo(SourceLocation HashTokenLoc, SourceLocation IfTokenLoc,
+                     bool FoundNonSkipPortion, bool FoundElse,
+                     SourceLocation ElseLoc)
+        : HashTokenLoc(HashTokenLoc), IfTokenLoc(IfTokenLoc),
+          FoundNonSkipPortion(FoundNonSkipPortion), FoundElse(FoundElse),
+          ElseLoc(ElseLoc) {}
+
+    SourceLocation HashTokenLoc;
+    SourceLocation IfTokenLoc;
+    bool FoundNonSkipPortion;
+    bool FoundElse;
+    SourceLocation ElseLoc;
+  };
+
+private:
   class PreambleConditionalStackStore {
     enum State {
       Off = 0,
       Recording = 1,
       Replaying = 2,
     };
 
   public:
void setStack(ArrayRef<PPConditionalInfo> s) {
       if (!isRecording() && !isReplaying())
         return;
       ConditionalStack.clear();
       ConditionalStack.append(s.begin(), s.end());
     }
 
     bool hasRecordedPreamble() const { return !ConditionalStack.empty(); }
 
+    bool reachedEOFWhileSkipping() const { return SkipInfo.hasValue(); }
+
+    void clearSkipInfo() { SkipInfo.reset(); }
+
+    llvm::Optional<PreambleSkipInfo> SkipInfo;
+
   private:
     SmallVector<PPConditionalInfo, 4> ConditionalStack;
     State ConditionalStackState;
   } PreambleConditionalStack;
 
   /// \brief The current top of the stack that we're lexing from if
   /// not expanding a macro and we are lexing directly from source code.
   ///
private:
   /// We just read a \#if or related directive and decided that the
   /// subsequent tokens are in the \#if'd out portion of the
   /// file.  Lex the rest of the file, until we see an \#endif.  If \p
   /// FoundNonSkipPortion is true, then we have already emitted code for part of
   /// this \#if directive, so \#else/\#elif blocks should never be entered. If
   /// \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(const Token &HashToken,
+  void SkipExcludedConditionalBlock(SourceLocation HashTokenLoc,
                                     SourceLocation IfTokenLoc,
                                     bool FoundNonSkipPortion, bool FoundElse,
                                     SourceLocation ElseLoc = SourceLocation());
 
   /// \brief A fast PTH version of SkipExcludedConditionalBlock.
   void PTHSkipExcludedConditionalBlock();
 
   /// Information about the result for evaluating an expression for a
public:
   ArrayRef<PPConditionalInfo> getPreambleConditionalStack() const {
       return PreambleConditionalStack.getStack();
   }
 
   void setRecordedPreambleConditionalStack(ArrayRef<PPConditionalInfo> s) {
     PreambleConditionalStack.setStack(s);
   }
 
-  void setReplayablePreambleConditionalStack(ArrayRef<PPConditionalInfo> s) {
+  void setReplayablePreambleConditionalStack(ArrayRef<PPConditionalInfo> s,
+                                             llvm::Optional<PreambleSkipInfo> SkipInfo) {
     PreambleConditionalStack.startReplaying();
     PreambleConditionalStack.setStack(s);
+    PreambleConditionalStack.SkipInfo = SkipInfo;
+  }
+
+  llvm::Optional<PreambleSkipInfo> getPreambleSkipInfo() const {
+    return PreambleConditionalStack.SkipInfo;
   }
 
 private:
   /// \brief After processing predefined file, initialize the conditional stack from
   /// the preamble.
   void replayPreambleConditionalStack();
 
   // Macro handling.

lib/Lex/PPDirectives.cpp

 /// SkipExcludedConditionalBlock - We just read a \#if or related directive and
 /// decided that the subsequent tokens are in the \#if'd out portion of the
 /// file.  Lex the rest of the file, until we see an \#endif.  If
 /// FoundNonSkipPortion is true, then we have already emitted code for part of
 /// this \#if directive, so \#else/\#elif blocks should never be entered.
 /// 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(const Token &HashToken,
+void Preprocessor::SkipExcludedConditionalBlock(SourceLocation HashTokenLoc,
                                                 SourceLocation IfTokenLoc,
                                                 bool FoundNonSkipPortion,
                                                 bool FoundElse,
                                                 SourceLocation ElseLoc) {
   ++NumSkipped;
   assert(!CurTokenLexer && CurPPLexer && "Lexing a macro, not a file?");
 
-  CurPPLexer->pushConditionalLevel(IfTokenLoc, /*isSkipping*/false,
+  if (PreambleConditionalStack.reachedEOFWhileSkipping())
+    PreambleConditionalStack.clearSkipInfo();
+  else
+    CurPPLexer->pushConditionalLevel(IfTokenLoc, /*isSkipping*/ false,
                                      FoundNonSkipPortion, FoundElse);
 
   if (CurPTHLexer) {
     PTHSkipExcludedConditionalBlock();
     return;
   }
 
   // Enter raw mode to disable identifier lookup (and thus macro expansion),
   // disabling warnings, etc.
if (Tok.is(tok::code_completion)) {
       continue;
     }
 
     // If this is the end of the buffer, we have an error.
     if (Tok.is(tok::eof)) {
       // We don't emit errors for unterminated conditionals here,
       // Lexer::LexEndOfFile can do that propertly.
       // Just return and let the caller lex after this #include.
+      if (PreambleConditionalStack.isRecording())
+        PreambleConditionalStack.SkipInfo.emplace(
+            HashTokenLoc, IfTokenLoc, FoundNonSkipPortion, FoundElse, ElseLoc);
       break;
     }
 
     // If this token is not a preprocessor directive, just skip it.
     if (Tok.isNot(tok::hash) || !Tok.isAtStartOfLine())
       continue;
 
     // We just parsed a # character at the start of a line, so we're in
void Preprocessor::SkipExcludedConditionalBlock(SourceLocation HashTokenLoc,
 
   // Finally, if we are out of the conditional (saw an #endif or ran off the end
   // of the file, just stop skipping and return to lexing whatever came after
   // the #if block.
   CurPPLexer->LexingRawMode = false;
 
   if (Callbacks)
     Callbacks->SourceRangeSkipped(
-        SourceRange(HashToken.getLocation(), CurPPLexer->getSourceLocation()),
+        SourceRange(HashTokenLoc, CurPPLexer->getSourceLocation()),
         Tok.getLocation());
 }
 
 void Preprocessor::PTHSkipExcludedConditionalBlock() {
   while (true) {
     assert(CurPTHLexer);
     assert(CurPTHLexer->LexingRawMode == false);
 
void Preprocessor::HandleIfdefDirective(Token &Result,
 
   Token MacroNameTok;
   ReadMacroName(MacroNameTok);
 
   // Error reading macro name?  If so, diagnostic already issued.
   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(HashToken, DirectiveTok.getLocation(),
+    SkipExcludedConditionalBlock(HashToken.getLocation(),
+                                 DirectiveTok.getLocation(),
                                  /*Foundnonskip*/ false, /*FoundElse*/ false);
     return;
   }
 
   // Check to see if this is the last token on the #if[n]def line.
   CheckEndOfDirective(isIfndef ? "ifndef" : "ifdef");
 
   IdentifierInfo *MII = MacroNameTok.getIdentifierInfo();
CurPPLexer->pushConditionalLevel(DirectiveTok.getLocation(),
                                      /*foundelse*/false);
   } else if (!MI == isIfndef) {
     // Yes, remember that we are inside a conditional, then lex the next token.
     CurPPLexer->pushConditionalLevel(DirectiveTok.getLocation(),
                                      /*wasskip*/false, /*foundnonskip*/true,
                                      /*foundelse*/false);
   } else {
     // No, skip the contents of this block.
-    SkipExcludedConditionalBlock(HashToken, DirectiveTok.getLocation(),
+    SkipExcludedConditionalBlock(HashToken.getLocation(),
+                                 DirectiveTok.getLocation(),
                                  /*Foundnonskip*/ false,
                                  /*FoundElse*/ false);
   }
 }
 
 /// HandleIfDirective - Implements the \#if directive.
 ///
 void Preprocessor::HandleIfDirective(Token &IfToken,
if (PPOpts->SingleFileParseMode && DER.IncludedUndefinedIds) {
     CurPPLexer->pushConditionalLevel(IfToken.getLocation(), /*wasskip*/false,
                                      /*foundnonskip*/false, /*foundelse*/false);
   } else if (ConditionalTrue) {
     // Yes, remember that we are inside a conditional, then lex the next token.
     CurPPLexer->pushConditionalLevel(IfToken.getLocation(), /*wasskip*/false,
                                    /*foundnonskip*/true, /*foundelse*/false);
   } else {
     // No, skip the contents of this block.
-    SkipExcludedConditionalBlock(HashToken, IfToken.getLocation(),
+    SkipExcludedConditionalBlock(HashToken.getLocation(), IfToken.getLocation(),
                                  /*Foundnonskip*/ false,
                                  /*FoundElse*/ false);
   }
 }
 
 /// HandleEndifDirective - Implements the \#endif directive.
 ///
 void Preprocessor::HandleEndifDirective(Token &EndifToken) {
if (PPOpts->SingleFileParseMode && !CI.FoundNonSkip) {
     // In 'single-file-parse mode' undefined identifiers trigger parsing of all
     // the directive blocks.
     CurPPLexer->pushConditionalLevel(CI.IfLoc, /*wasskip*/false,
                                      /*foundnonskip*/false, /*foundelse*/true);
     return;
   }
 
   // Finally, skip the rest of the contents of this block.
-  SkipExcludedConditionalBlock(HashToken, CI.IfLoc, /*Foundnonskip*/ true,
+  SkipExcludedConditionalBlock(HashToken.getLocation(), CI.IfLoc,
+                               /*Foundnonskip*/ true,
                                /*FoundElse*/ true, Result.getLocation());
 }
 
 /// HandleElifDirective - Implements the \#elif directive.
 ///
 void Preprocessor::HandleElifDirective(Token &ElifToken,
                                        const Token &HashToken) {
   ++NumElse;
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,
                                      /*foundnonskip*/false, /*foundelse*/false);
     return;
   }
 
   // Finally, skip the rest of the contents of this block.
-  SkipExcludedConditionalBlock(HashToken, CI.IfLoc, /*Foundnonskip*/ true,
-                               /*FoundElse*/ CI.FoundElse,
-                               ElifToken.getLocation());
+  SkipExcludedConditionalBlock(
+      HashToken.getLocation(), CI.IfLoc, /*Foundnonskip*/ true,
+      /*FoundElse*/ CI.FoundElse, ElifToken.getLocation());
 }

lib/Lex/Preprocessor.cpp

 
 void Preprocessor::replayPreambleConditionalStack() {
   // Restore the conditional stack from the preamble, if there is one.
   if (PreambleConditionalStack.isReplaying()) {
     assert(CurPPLexer &&
            "CurPPLexer is null when calling replayPreambleConditionalStack.");
     CurPPLexer->setConditionalLevels(PreambleConditionalStack.getStack());
     PreambleConditionalStack.doneReplaying();
+    if (PreambleConditionalStack.reachedEOFWhileSkipping())
+      SkipExcludedConditionalBlock(
+          PreambleConditionalStack.SkipInfo->HashTokenLoc,
+          PreambleConditionalStack.SkipInfo->IfTokenLoc,
+          PreambleConditionalStack.SkipInfo->FoundNonSkipPortion,
+          PreambleConditionalStack.SkipInfo->FoundElse,
+          PreambleConditionalStack.SkipInfo->ElseLoc);
   }
 }
 
 void Preprocessor::EndSourceFile() {
   // Notify the client that we reached the end of the source file.
   if (Callbacks)
     Callbacks->EndOfMainFile();
 }

lib/Serialization/ASTReader.cpp

case REFERENCED_SELECTOR_POOL:
           ReferencedSelectorsData.push_back(ReadSourceLocation(F, Record, Idx).
                                               getRawEncoding());
         }
       }
       break;
 
     case PP_CONDITIONAL_STACK:
       if (!Record.empty()) {
+        unsigned Idx = 0, End = Record.size() - 1;
+        bool ReachedEOFWhileSkipping = Record[Idx++];
+        llvm::Optional<Preprocessor::PreambleSkipInfo> SkipInfo;
+        if (ReachedEOFWhileSkipping) {
+          SourceLocation HashToken = ReadSourceLocation(F, Record, Idx);
+          SourceLocation IfTokenLoc = ReadSourceLocation(F, Record, Idx);
+          bool FoundNonSkipPortion = Record[Idx++];
+          bool FoundElse = Record[Idx++];
+          SourceLocation ElseLoc = ReadSourceLocation(F, Record, Idx);
+          SkipInfo.emplace(HashToken, IfTokenLoc, FoundNonSkipPortion,
+                           FoundElse, ElseLoc);
+        }
         SmallVector<PPConditionalInfo, 4> ConditionalStack;
-        for (unsigned Idx = 0, N = Record.size() - 1; Idx < N; /* in loop */) {
+        while (Idx < End) {
           auto Loc = ReadSourceLocation(F, Record, Idx);
           bool WasSkipping = Record[Idx++];
           bool FoundNonSkip = Record[Idx++];
           bool FoundElse = Record[Idx++];
           ConditionalStack.push_back(
               {Loc, WasSkipping, FoundNonSkip, FoundElse});
         }
-        PP.setReplayablePreambleConditionalStack(ConditionalStack);
+        PP.setReplayablePreambleConditionalStack(ConditionalStack, SkipInfo);
       }
       break;
 
     case PP_COUNTER_VALUE:
       if (!Record.empty() && Listener)
         Listener->ReadCounter(F, Record[0]);
       break;
 

lib/Serialization/ASTWriter.cpp

void ASTWriter::WritePreprocessor(const Preprocessor &PP, bool IsModule) {
   // If the preprocessor __COUNTER__ value has been bumped, remember it.
   if (PP.getCounterValue() != 0) {
     RecordData::value_type Record[] = {PP.getCounterValue()};
     Stream.EmitRecord(PP_COUNTER_VALUE, Record);
   }
 
   if (PP.isRecordingPreamble() && PP.hasRecordedPreamble()) {
     assert(!IsModule);
+    auto SkipInfo = PP.getPreambleSkipInfo();
+    if (SkipInfo.hasValue()) {
+      Record.push_back(true);
+      AddSourceLocation(SkipInfo->HashTokenLoc, Record);
+      AddSourceLocation(SkipInfo->IfTokenLoc, Record);
+      Record.push_back(SkipInfo->FoundNonSkipPortion);
+      Record.push_back(SkipInfo->FoundElse);
+      AddSourceLocation(SkipInfo->ElseLoc, Record);
+    } else {
+      Record.push_back(false);
+    }
     for (const auto &Cond : PP.getPreambleConditionalStack()) {
       AddSourceLocation(Cond.IfLoc, Record);
       Record.push_back(Cond.WasSkipping);
       Record.push_back(Cond.FoundNonSkip);
       Record.push_back(Cond.FoundElse);
     }
     Stream.EmitRecord(PP_CONDITIONAL_STACK, Record);
     Record.clear();

test/Index/preamble-conditionals-inverted.cpp

 // RUN: env CINDEXTEST_EDITING=1 c-index-test -test-load-source-reparse 5 \
 // RUN:                                       local -std=c++14 %s 2>&1 \
 // RUN: | FileCheck %s --implicit-check-not "error:"
 #ifdef FOO_H
 
-void foo();
+void foo() {}
 
 #endif
+
+int foo() { return 0; }

test/Index/preamble-conditionals-skipping.cpp

+// RUN: env CINDEXTEST_EDITING=1 c-index-test -test-load-source-reparse 5 \
+// RUN:                                       local -std=c++14 %s 2>&1 \
+// RUN: | FileCheck %s --implicit-check-not "error:"
+
+#ifdef MYCPLUSPLUS
+extern "C" {
+#endif
+
+#ifdef MYCPLUSPLUS
+}
+#endif
+
+int main()
+{
+    return 0;
+}