Do not validate pch when -fno-validate-pch is set

[yaxunl]

Summary

There is a bug causing pch to be validated even though -fno-validate-pch is set. This patch fixes it.

ASTReader relies on ASTReaderListener to initialize SuggestedPredefines, which is required for compilations using PCH. Before this change, PCHValidator is the default ASTReaderListener. After this change, when -fno-validate-pch is set, PCHValidator is disabled, but we need a replacement ASTReaderListener to initialize SuggestedPredefines. Class SimpleASTReaderListener is implemented for this purpose.

This change only affects -fno-validate-pch. There is no functional change if -fno-validate-pch is not set.

If -fno-validate-pch is not set, conflicts in predefined macros between pch and current compiler instance causes error.

If -fno-validate-pch is set, predefine macros in current compiler override those in pch so that compilation can continue.

[yaxunl]

The bug is due to PCHValidator is added as Listener, which causes pch is validated even though DisableValidation is true (when -fno-validate-pch is on). The fix is not add PCHValidator as listener when DisableValidation is true.

The bug is annoying for using pch. When pch is generated with default optimization, the generated pch cannot be used to compile program with -O0.

Tom/Alexey, can you help review the code? Thanks.

[bader]

Could you a regression test, please?

[yaxunl]

Implemented a simple AST reader listener to replace PCHValidator. Added a test.

[Anastasia]

LGTM!

[rsmith]

It's pretty gross that we're computing SuggestedPredefines in the ASTReaderListener, but sure, this seems OK for now.

cfe/trunk/include/clang/Serialization/ASTReader.h

bool ReadHeaderSearchOptions(const HeaderSearchOptions &HSOpts,
                                StringRef SpecificModuleCachePath,
                                bool Complain) override;
   void ReadCounter(const serialization::ModuleFile &M, unsigned Value) override;
 
 private:
   void Error(const char *Msg);
 };
 
+/// \brief ASTReaderListenter implementation to set SuggestedPredefines of
+/// ASTReader which is required to use a pch file. This is the replacement
+/// of PCHValidator or SimplePCHValidator when using a pch file without
+/// validating it.
+class SimpleASTReaderListener : public ASTReaderListener {
+  Preprocessor &PP;
+
+public:
+  SimpleASTReaderListener(Preprocessor &PP)
+    : PP(PP) {}
+
+  bool ReadPreprocessorOptions(const PreprocessorOptions &PPOpts, bool Complain,
+                               std::string &SuggestedPredefines) override;
+};
+
 namespace serialization {
 
 class ReadMethodPoolVisitor;
 
 namespace reader {
   class ASTIdentifierLookupTrait;
   /// \brief The on-disk hash table(s) used for DeclContext name lookup.
   struct DeclContextLookupTable;

cfe/trunk/lib/Serialization/ASTReader.cpp

for (unsigned I = 0, N = PPOpts.Macros.size(); I != N; ++I) {
     Macros[MacroName] = std::make_pair(MacroBody, false);
   }
 }
 
 /// \brief Check the preprocessor options deserialized from the control block
 /// against the preprocessor options in an existing preprocessor.
 ///
 /// \param Diags If non-null, produce diagnostics for any mismatches incurred.
+/// \param Validate If true, validate preprocessor options. If false, allow
+///        macros defined by \p ExistingPPOpts to override those defined by
+///        \p PPOpts in SuggestedPredefines.
 static bool checkPreprocessorOptions(const PreprocessorOptions &PPOpts,
                                      const PreprocessorOptions &ExistingPPOpts,
                                      DiagnosticsEngine *Diags,
                                      FileManager &FileMgr,
                                      std::string &SuggestedPredefines,
-                                     const LangOptions &LangOpts) {
+                                     const LangOptions &LangOpts,
+                                     bool Validate = true) {
   // Check macro definitions.
   MacroDefinitionsMap ASTFileMacros;
   collectMacroDefinitions(PPOpts, ASTFileMacros);
   MacroDefinitionsMap ExistingMacros;
   SmallVector<StringRef, 4> ExistingMacroNames;
   collectMacroDefinitions(ExistingPPOpts, ExistingMacros, &ExistingMacroNames);
 
   for (unsigned I = 0, N = ExistingMacroNames.size(); I != N; ++I) {
     // Dig out the macro definition in the existing preprocessor options.
     StringRef MacroName = ExistingMacroNames[I];
     std::pair<StringRef, bool> Existing = ExistingMacros[MacroName];
 
     // Check whether we know anything about this macro name or not.
     llvm::StringMap<std::pair<StringRef, bool /*IsUndef*/> >::iterator Known
       = ASTFileMacros.find(MacroName);
-    if (Known == ASTFileMacros.end()) {
+    if (!Validate || Known == ASTFileMacros.end()) {
       // FIXME: Check whether this identifier was referenced anywhere in the
       // AST file. If so, we should reject the AST file. Unfortunately, this
       // information isn't in the control block. What shall we do about it?
 
       if (Existing.second) {
         SuggestedPredefines += "#undef ";
         SuggestedPredefines += MacroName.str();
         SuggestedPredefines += '\n';
for (unsigned I = 0, N = ExistingMacroNames.size(); I != N; ++I) {
     if (Diags) {
       Diags->Report(diag::err_pch_macro_def_conflict)
         << MacroName << Known->second.first << Existing.first;
     }
     return true;
   }
 
   // Check whether we're using predefines.
-  if (PPOpts.UsePredefines != ExistingPPOpts.UsePredefines) {
+  if (PPOpts.UsePredefines != ExistingPPOpts.UsePredefines && Validate) {
     if (Diags) {
       Diags->Report(diag::err_pch_undef) << ExistingPPOpts.UsePredefines;
     }
     return true;
   }
 
   // Detailed record is important since it is used for the module cache hash.
   if (LangOpts.Modules &&
-      PPOpts.DetailedRecord != ExistingPPOpts.DetailedRecord) {
+      PPOpts.DetailedRecord != ExistingPPOpts.DetailedRecord && Validate) {
     if (Diags) {
       Diags->Report(diag::err_pch_pp_detailed_record) << PPOpts.DetailedRecord;
     }
     return true;
   }
 
   // Compute the #include and #include_macros lines we need.
   for (unsigned I = 0, N = ExistingPPOpts.Includes.size(); I != N; ++I) {
bool PCHValidator::ReadPreprocessorOptions(const PreprocessorOptions &PPOpts,
 
   return checkPreprocessorOptions(PPOpts, ExistingPPOpts,
                                   Complain? &Reader.Diags : nullptr,
                                   PP.getFileManager(),
                                   SuggestedPredefines,
                                   PP.getLangOpts());
 }
 
+bool SimpleASTReaderListener::ReadPreprocessorOptions(
+                                  const PreprocessorOptions &PPOpts,
+                                  bool Complain,
+                                  std::string &SuggestedPredefines) {
+  return checkPreprocessorOptions(PPOpts,
+                                  PP.getPreprocessorOpts(),
+                                  nullptr,
+                                  PP.getFileManager(),
+                                  SuggestedPredefines,
+                                  PP.getLangOpts(),
+                                  false);
+}
+
 /// Check the header search options deserialized from the control block
 /// against the header search options in an existing preprocessor.
 ///
 /// \param Diags If non-null, produce diagnostics for any mismatches incurred.
 static bool checkHeaderSearchOptions(const HeaderSearchOptions &HSOpts,
                                      StringRef SpecificModuleCachePath,
                                      StringRef ExistingModuleCachePath,
                                      DiagnosticsEngine *Diags,
ASTReader::ASTReader(
   Preprocessor &PP, ASTContext &Context,
   const PCHContainerReader &PCHContainerRdr,
   ArrayRef<IntrusiveRefCntPtr<ModuleFileExtension>> Extensions,
   StringRef isysroot, bool DisableValidation,
   bool AllowASTWithCompilerErrors,
   bool AllowConfigurationMismatch, bool ValidateSystemInputs,
   bool UseGlobalIndex,
   std::unique_ptr<llvm::Timer> ReadTimer)
-    : Listener(new PCHValidator(PP, *this)), DeserializationListener(nullptr),
+    : Listener(DisableValidation ?
+        cast<ASTReaderListener>(new SimpleASTReaderListener(PP)) :
+        cast<ASTReaderListener>(new PCHValidator(PP, *this))),
+      DeserializationListener(nullptr),
       OwnsDeserializationListener(false), SourceMgr(PP.getSourceManager()),
       FileMgr(PP.getFileManager()), PCHContainerRdr(PCHContainerRdr),
       Diags(PP.getDiagnostics()), SemaObj(nullptr), PP(PP), Context(Context),
       Consumer(nullptr), ModuleMgr(PP.getFileManager(), PCHContainerRdr),
       DummyIdResolver(PP),
       ReadTimer(std::move(ReadTimer)),
       PragmaMSStructState(-1),
       PragmaMSPointersToMembersState(-1),

cfe/trunk/test/PCH/no-validate-pch.cl

+// RUN: %clang_cc1 -emit-pch -D TWO=2 -D X=4 -o %t %s -triple spir-unknown-unknown
+// RUN: %clang_cc1 -include-pch %t -D THREE=3 -D X=5 -O0 -U__OPTIMIZE__ -fno-validate-pch %s -triple spir-unknown-unknown 2>&1 | FileCheck %s
+// RUN: not %clang_cc1 -include-pch %t -D THREE=3 -D X=5 -D VALIDATE -O0 -fsyntax-only %s -triple spir-unknown-unknown 2>&1 | FileCheck --check-prefix=CHECK-VAL %s
+
+#ifndef HEADER
+#define HEADER
+// Header.
+
+#define ONE 1
+
+#else
+// Using the header.
+
+// CHECK: warning: 'X' macro redefined
+// CHECK: #define X 5
+// CHECK: note: previous definition is here
+// CHECK: #define X 4
+
+// CHECK-VAL: error: __OPTIMIZE__ predefined macro was enabled in PCH file but is currently disabled
+// CHECK-VAL: error: definition of macro 'X' differs between the precompiled header ('4') and the command line ('5')
+
+void test(void) {
+  int a = ONE;
+  int b = TWO;
+  int c = THREE;
+
+#ifndef VALIDATE
+#if X != 5
+#error Definition of X is not overridden!
+#endif
+
+#ifdef __OPTIMIZE__
+#error Optimization is not off!
+#endif
+#endif
+
+}
+
+#endif