[PATCH] D53866: [Preamble] Fix preamble for circular #includes

[Nikolai Kosjar via Phabricator via cfe-commits]

nik updated this revision to Diff 175210.
nik added a comment.

> Maybe produce a **fatal** error in the preprocessor? That seems to be the simplest option: the preprocessor is aware it's building the preamble and there's definitely some logic to produce fatal errors in other cases (include not found).
>  A fatal error currently aborts other stages of the compilation pipeline and producing one would make the run of the compiler that tries to produce the preamble fail, giving the empty preamble as a result.

Done. I'm using diag::err_pp_error_opening_file as introducing an new artificial diagnostic error that the user will never see feels wrong.

Note that a preamble is generated for fatal errors like an unresolved #include and I think that's fine. As such, I need a way to propagate the error up to PrecompilePreambleConsumer to avoid writing the preamble. I've done that with an extra flag in Preprocessor. An alternative might be to put it into PreambleConditionalStackStore (as state? and rename that class to something more general?) - what do you think?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D53866/new/

https://reviews.llvm.org/D53866

Files:
  include/clang/Lex/Preprocessor.h
  include/clang/Serialization/ASTWriter.h
  lib/Frontend/PrecompiledPreamble.cpp
  lib/Lex/PPDirectives.cpp
  test/Index/preamble-cyclic-include.cpp


Index: test/Index/preamble-cyclic-include.cpp
===================================================================
--- /dev/null
+++ test/Index/preamble-cyclic-include.cpp
@@ -0,0 +1,9 @@
+// RUN: env CINDEXTEST_EDITING=1 c-index-test -test-annotate-tokens=%s:4:1:8:1 %s 2>&1 | FileCheck %s
+// CHECK-NOT: error: unterminated conditional directive
+// CHECK-NOT: Skipping: [4:1 - 8:7]
+#ifndef A_H
+#define A_H
+#  include "preamble-cyclic-include.cpp"
+int bar();
+#endif
+
Index: lib/Lex/PPDirectives.cpp
===================================================================
--- lib/Lex/PPDirectives.cpp
+++ lib/Lex/PPDirectives.cpp
@@ -1933,6 +1933,20 @@
     return;
   }
 
+  // Check whether it makes sense to continue preamble generation. We can't
+  // generate a consistent preamble with regard to the conditional stack if the
+  // main file is included again as due to the preamble bounds some directives
+  // (e.g. #endif of a header guard) will never be seen. Since this will lead to
+  // confusing errors, abort the preamble generation.
+  if (File && PreambleConditionalStack.isRecording() &&
+      SourceMgr.translateFile(File) == SourceMgr.getMainFileID()) {
+    PreambleGenerationFailed = true;
+    // Generate a fatal error to skip further processing.
+    Diag(FilenameTok.getLocation(), diag::err_pp_error_opening_file) << ""
+                                                                     << "";
+    return;
+  }
+
   // Should we enter the source file? Set to false if either the source file is
   // known to have no effect beyond its effect on module visibility -- that is,
   // if it's got an include guard that is already defined or is a modular header
Index: lib/Frontend/PrecompiledPreamble.cpp
===================================================================
--- lib/Frontend/PrecompiledPreamble.cpp
+++ lib/Frontend/PrecompiledPreamble.cpp
@@ -170,6 +170,9 @@
   }
 
   void HandleTranslationUnit(ASTContext &Ctx) override {
+    if (getPreprocessor().preambleGenerationFailed())
+      return;
+
     PCHGenerator::HandleTranslationUnit(Ctx);
     if (!hasEmittedPCH())
       return;
Index: include/clang/Serialization/ASTWriter.h
===================================================================
--- include/clang/Serialization/ASTWriter.h
+++ include/clang/Serialization/ASTWriter.h
@@ -979,6 +979,7 @@
   ASTWriter &getWriter() { return Writer; }
   const ASTWriter &getWriter() const { return Writer; }
   SmallVectorImpl<char> &getPCH() const { return Buffer->Data; }
+  const Preprocessor &getPreprocessor() const { return PP; }
 
 public:
   PCHGenerator(const Preprocessor &PP, StringRef OutputFile, StringRef isysroot,
Index: include/clang/Lex/Preprocessor.h
===================================================================
--- include/clang/Lex/Preprocessor.h
+++ include/clang/Lex/Preprocessor.h
@@ -388,6 +388,7 @@
     SmallVector<PPConditionalInfo, 4> ConditionalStack;
     State ConditionalStackState = Off;
   } PreambleConditionalStack;
+  bool PreambleGenerationFailed = false;
 
   /// The current top of the stack that we're lexing from if
   /// not expanding a macro and we are lexing directly from source code.
@@ -2159,6 +2160,10 @@
                                                           Module *M,
                                                           SourceLocation MLoc);
 
+  bool preambleGenerationFailed() const {
+    return PreambleGenerationFailed;
+  }
+
   bool isRecordingPreamble() const {
     return PreambleConditionalStack.isRecording();
   }


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D53866.175210.patch
Type: text/x-patch
Size: 3545 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20181126/d9a019c0/attachment-0001.bin>