r232248 - [modules] Teach the AST reader to handle the case of importing a module

Chandler Carruth chandlerc at gmail.com
Fri Mar 13 21:47:43 PDT 2015


Author: chandlerc
Date: Fri Mar 13 23:47:43 2015
New Revision: 232248

URL: http://llvm.org/viewvc/llvm-project?rev=232248&view=rev
Log:
[modules] Teach the AST reader to handle the case of importing a module
with a subset of the existing target CPU features or mismatched CPU
names.

While we can't check that the CPU name used to build the module will end
up being able to codegen correctly for the translation unit, we actually
check that the imported features are a subset of the existing features.

While here, rewrite the code to use std::set_difference and have it
diagnose all of the differences found.

Test case added which walks the set relationships and ensures we
diagnose all the right cases and accept the others.

No functional change for implicit modules here, just better diagnostics.

Added:
    cfe/trunk/test/Modules/Inputs/merge-target-features/
    cfe/trunk/test/Modules/Inputs/merge-target-features/foo.h
    cfe/trunk/test/Modules/Inputs/merge-target-features/module.modulemap
    cfe/trunk/test/Modules/merge-target-features.cpp
Modified:
    cfe/trunk/include/clang/Serialization/ASTReader.h
    cfe/trunk/lib/Frontend/ASTUnit.cpp
    cfe/trunk/lib/Frontend/FrontendActions.cpp
    cfe/trunk/lib/Serialization/ASTReader.cpp

Modified: cfe/trunk/include/clang/Serialization/ASTReader.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Serialization/ASTReader.h?rev=232248&r1=232247&r2=232248&view=diff
==============================================================================
--- cfe/trunk/include/clang/Serialization/ASTReader.h (original)
+++ cfe/trunk/include/clang/Serialization/ASTReader.h Fri Mar 13 23:47:43 2015
@@ -124,8 +124,8 @@ public:
   ///
   /// \returns true to indicate the target options are invalid, or false
   /// otherwise.
-  virtual bool ReadTargetOptions(const TargetOptions &TargetOpts,
-                                 bool Complain) {
+  virtual bool ReadTargetOptions(const TargetOptions &TargetOpts, bool Complain,
+                                 bool AllowCompatibleDifferences) {
     return false;
   }
 
@@ -223,8 +223,8 @@ public:
   void ReadModuleMapFile(StringRef ModuleMapPath) override;
   bool ReadLanguageOptions(const LangOptions &LangOpts, bool Complain,
                            bool AllowCompatibleDifferences) override;
-  bool ReadTargetOptions(const TargetOptions &TargetOpts,
-                         bool Complain) override;
+  bool ReadTargetOptions(const TargetOptions &TargetOpts, bool Complain,
+                         bool AllowCompatibleDifferences) override;
   bool ReadDiagnosticOptions(IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts,
                              bool Complain) override;
   bool ReadFileSystemOptions(const FileSystemOptions &FSOpts,
@@ -257,8 +257,8 @@ public:
 
   bool ReadLanguageOptions(const LangOptions &LangOpts, bool Complain,
                            bool AllowCompatibleDifferences) override;
-  bool ReadTargetOptions(const TargetOptions &TargetOpts,
-                         bool Complain) override;
+  bool ReadTargetOptions(const TargetOptions &TargetOpts, bool Complain,
+                         bool AllowCompatibleDifferences) override;
   bool ReadDiagnosticOptions(IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts,
                              bool Complain) override;
   bool ReadPreprocessorOptions(const PreprocessorOptions &PPOpts, bool Complain,
@@ -1140,7 +1140,8 @@ private:
                                    ASTReaderListener &Listener,
                                    bool AllowCompatibleDifferences);
   static bool ParseTargetOptions(const RecordData &Record, bool Complain,
-                                 ASTReaderListener &Listener);
+                                 ASTReaderListener &Listener,
+                                 bool AllowCompatibleDifferences);
   static bool ParseDiagnosticOptions(const RecordData &Record, bool Complain,
                                      ASTReaderListener &Listener);
   static bool ParseFileSystemOptions(const RecordData &Record, bool Complain,

Modified: cfe/trunk/lib/Frontend/ASTUnit.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/ASTUnit.cpp?rev=232248&r1=232247&r2=232248&view=diff
==============================================================================
--- cfe/trunk/lib/Frontend/ASTUnit.cpp (original)
+++ cfe/trunk/lib/Frontend/ASTUnit.cpp Fri Mar 13 23:47:43 2015
@@ -513,8 +513,8 @@ public:
     return false;
   }
 
-  bool ReadTargetOptions(const TargetOptions &TargetOpts,
-                         bool Complain) override {
+  bool ReadTargetOptions(const TargetOptions &TargetOpts, bool Complain,
+                         bool AllowCompatibleDifferences) override {
     // If we've already initialized the target, don't do it again.
     if (Target)
       return false;

Modified: cfe/trunk/lib/Frontend/FrontendActions.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/FrontendActions.cpp?rev=232248&r1=232247&r2=232248&view=diff
==============================================================================
--- cfe/trunk/lib/Frontend/FrontendActions.cpp (original)
+++ cfe/trunk/lib/Frontend/FrontendActions.cpp Fri Mar 13 23:47:43 2015
@@ -462,8 +462,8 @@ namespace {
       return false;
     }
 
-    bool ReadTargetOptions(const TargetOptions &TargetOpts,
-                           bool Complain) override {
+    bool ReadTargetOptions(const TargetOptions &TargetOpts, bool Complain,
+                           bool AllowCompatibleDifferences) override {
       Out.indent(2) << "Target options:\n";
       Out.indent(4) << "  Triple: " << TargetOpts.Triple << "\n";
       Out.indent(4) << "  CPU: " << TargetOpts.CPU << "\n";

Modified: cfe/trunk/lib/Serialization/ASTReader.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ASTReader.cpp?rev=232248&r1=232247&r2=232248&view=diff
==============================================================================
--- cfe/trunk/lib/Serialization/ASTReader.cpp (original)
+++ cfe/trunk/lib/Serialization/ASTReader.cpp Fri Mar 13 23:47:43 2015
@@ -89,11 +89,13 @@ ChainedASTReaderListener::ReadLanguageOp
          Second->ReadLanguageOptions(LangOpts, Complain,
                                      AllowCompatibleDifferences);
 }
-bool
-ChainedASTReaderListener::ReadTargetOptions(const TargetOptions &TargetOpts,
-                                            bool Complain) {
-  return First->ReadTargetOptions(TargetOpts, Complain) ||
-         Second->ReadTargetOptions(TargetOpts, Complain);
+bool ChainedASTReaderListener::ReadTargetOptions(
+    const TargetOptions &TargetOpts, bool Complain,
+    bool AllowCompatibleDifferences) {
+  return First->ReadTargetOptions(TargetOpts, Complain,
+                                  AllowCompatibleDifferences) ||
+         Second->ReadTargetOptions(TargetOpts, Complain,
+                                   AllowCompatibleDifferences);
 }
 bool ChainedASTReaderListener::ReadDiagnosticOptions(
     IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts, bool Complain) {
@@ -232,7 +234,8 @@ static bool checkLanguageOptions(const L
 /// \returns true if the target options mis-match, false otherwise.
 static bool checkTargetOptions(const TargetOptions &TargetOpts,
                                const TargetOptions &ExistingTargetOpts,
-                               DiagnosticsEngine *Diags) {
+                               DiagnosticsEngine *Diags,
+                               bool AllowCompatibleDifferences = true) {
 #define CHECK_TARGET_OPT(Field, Name)                             \
   if (TargetOpts.Field != ExistingTargetOpts.Field) {             \
     if (Diags)                                                    \
@@ -241,9 +244,16 @@ static bool checkTargetOptions(const Tar
     return true;                                                  \
   }
 
+  // The triple and ABI must match exactly.
   CHECK_TARGET_OPT(Triple, "target");
-  CHECK_TARGET_OPT(CPU, "target CPU");
   CHECK_TARGET_OPT(ABI, "target ABI");
+
+  // We can tolerate different CPUs in many cases, notably when one CPU
+  // supports a strict superset of another. When allowing compatible
+  // differences skip this check.
+  if (!AllowCompatibleDifferences)
+    CHECK_TARGET_OPT(CPU, "target CPU");
+
 #undef CHECK_TARGET_OPT
 
   // Compare feature sets.
@@ -255,43 +265,31 @@ static bool checkTargetOptions(const Tar
   std::sort(ExistingFeatures.begin(), ExistingFeatures.end());
   std::sort(ReadFeatures.begin(), ReadFeatures.end());
 
-  unsigned ExistingIdx = 0, ExistingN = ExistingFeatures.size();
-  unsigned ReadIdx = 0, ReadN = ReadFeatures.size();
-  while (ExistingIdx < ExistingN && ReadIdx < ReadN) {
-    if (ExistingFeatures[ExistingIdx] == ReadFeatures[ReadIdx]) {
-      ++ExistingIdx;
-      ++ReadIdx;
-      continue;
-    }
-
-    if (ReadFeatures[ReadIdx] < ExistingFeatures[ExistingIdx]) {
-      if (Diags)
-        Diags->Report(diag::err_pch_targetopt_feature_mismatch)
-          << false << ReadFeatures[ReadIdx];
-      return true;
-    }
-
-    if (Diags)
-      Diags->Report(diag::err_pch_targetopt_feature_mismatch)
-        << true << ExistingFeatures[ExistingIdx];
-    return true;
-  }
+  // We compute the set difference in both directions explicitly so that we can
+  // diagnose the differences differently.
+  SmallVector<StringRef, 4> UnmatchedExistingFeatures, UnmatchedReadFeatures;
+  std::set_difference(
+      ExistingFeatures.begin(), ExistingFeatures.end(), ReadFeatures.begin(),
+      ReadFeatures.end(), std::back_inserter(UnmatchedExistingFeatures));
+  std::set_difference(ReadFeatures.begin(), ReadFeatures.end(),
+                      ExistingFeatures.begin(), ExistingFeatures.end(),
+                      std::back_inserter(UnmatchedReadFeatures));
+
+  // If we are allowing compatible differences and the read feature set is
+  // a strict subset of the existing feature set, there is nothing to diagnose.
+  if (AllowCompatibleDifferences && UnmatchedReadFeatures.empty())
+    return false;
 
-  if (ExistingIdx < ExistingN) {
-    if (Diags)
+  if (Diags) {
+    for (StringRef Feature : UnmatchedReadFeatures)
       Diags->Report(diag::err_pch_targetopt_feature_mismatch)
-        << true << ExistingFeatures[ExistingIdx];
-    return true;
-  }
-
-  if (ReadIdx < ReadN) {
-    if (Diags)
+          << /* is-existing-feature */ false << Feature;
+    for (StringRef Feature : UnmatchedExistingFeatures)
       Diags->Report(diag::err_pch_targetopt_feature_mismatch)
-        << false << ReadFeatures[ReadIdx];
-    return true;
+          << /* is-existing-feature */ true << Feature;
   }
 
-  return false;
+  return !UnmatchedReadFeatures.empty() || !UnmatchedExistingFeatures.empty();
 }
 
 bool
@@ -305,10 +303,12 @@ PCHValidator::ReadLanguageOptions(const
 }
 
 bool PCHValidator::ReadTargetOptions(const TargetOptions &TargetOpts,
-                                     bool Complain) {
+                                     bool Complain,
+                                     bool AllowCompatibleDifferences) {
   const TargetOptions &ExistingTargetOpts = PP.getTargetInfo().getTargetOpts();
   return checkTargetOptions(TargetOpts, ExistingTargetOpts,
-                            Complain? &Reader.Diags : nullptr);
+                            Complain ? &Reader.Diags : nullptr,
+                            AllowCompatibleDifferences);
 }
 
 namespace {
@@ -2476,7 +2476,8 @@ ASTReader::ReadControlBlock(ModuleFile &
     case TARGET_OPTIONS: {
       bool Complain = (ClientLoadCapabilities & ARR_ConfigurationMismatch)==0;
       if (Listener && &F == *ModuleMgr.begin() &&
-          ParseTargetOptions(Record, Complain, *Listener) &&
+          ParseTargetOptions(Record, Complain, *Listener,
+                             AllowCompatibleConfigurationMismatch) &&
           !DisableValidation && !AllowConfigurationMismatch)
         return ConfigurationMismatch;
       break;
@@ -4222,9 +4223,10 @@ namespace {
       return checkLanguageOptions(ExistingLangOpts, LangOpts, nullptr,
                                   AllowCompatibleDifferences);
     }
-    bool ReadTargetOptions(const TargetOptions &TargetOpts,
-                           bool Complain) override {
-      return checkTargetOptions(ExistingTargetOpts, TargetOpts, nullptr);
+    bool ReadTargetOptions(const TargetOptions &TargetOpts, bool Complain,
+                           bool AllowCompatibleDifferences) override {
+      return checkTargetOptions(ExistingTargetOpts, TargetOpts, nullptr,
+                                AllowCompatibleDifferences);
     }
     bool ReadHeaderSearchOptions(const HeaderSearchOptions &HSOpts,
                                  StringRef SpecificModuleCachePath,
@@ -4336,7 +4338,8 @@ bool ASTReader::readASTFileControlBlock(
       break;
 
     case TARGET_OPTIONS:
-      if (ParseTargetOptions(Record, false, Listener))
+      if (ParseTargetOptions(Record, false, Listener,
+                             /*AllowCompatibleConfigurationMismatch*/ false))
         return true;
       break;
 
@@ -4728,9 +4731,9 @@ bool ASTReader::ParseLanguageOptions(con
                                       AllowCompatibleDifferences);
 }
 
-bool ASTReader::ParseTargetOptions(const RecordData &Record,
-                                   bool Complain,
-                                   ASTReaderListener &Listener) {
+bool ASTReader::ParseTargetOptions(const RecordData &Record, bool Complain,
+                                   ASTReaderListener &Listener,
+                                   bool AllowCompatibleDifferences) {
   unsigned Idx = 0;
   TargetOptions TargetOpts;
   TargetOpts.Triple = ReadString(Record, Idx);
@@ -4743,7 +4746,8 @@ bool ASTReader::ParseTargetOptions(const
     TargetOpts.Features.push_back(ReadString(Record, Idx));
   }
 
-  return Listener.ReadTargetOptions(TargetOpts, Complain);
+  return Listener.ReadTargetOptions(TargetOpts, Complain,
+                                    AllowCompatibleDifferences);
 }
 
 bool ASTReader::ParseDiagnosticOptions(const RecordData &Record, bool Complain,

Added: cfe/trunk/test/Modules/Inputs/merge-target-features/foo.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/merge-target-features/foo.h?rev=232248&view=auto
==============================================================================
--- cfe/trunk/test/Modules/Inputs/merge-target-features/foo.h (added)
+++ cfe/trunk/test/Modules/Inputs/merge-target-features/foo.h Fri Mar 13 23:47:43 2015
@@ -0,0 +1,8 @@
+#ifndef FOO_H
+#define FOO_H
+
+int foo(int x) {
+  return x + 42;
+}
+
+#endif // FOO_H

Added: cfe/trunk/test/Modules/Inputs/merge-target-features/module.modulemap
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/merge-target-features/module.modulemap?rev=232248&view=auto
==============================================================================
--- cfe/trunk/test/Modules/Inputs/merge-target-features/module.modulemap (added)
+++ cfe/trunk/test/Modules/Inputs/merge-target-features/module.modulemap Fri Mar 13 23:47:43 2015
@@ -0,0 +1 @@
+module foo { header "Inputs/merge-target-features/foo.h" export * }

Added: cfe/trunk/test/Modules/merge-target-features.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/merge-target-features.cpp?rev=232248&view=auto
==============================================================================
--- cfe/trunk/test/Modules/merge-target-features.cpp (added)
+++ cfe/trunk/test/Modules/merge-target-features.cpp Fri Mar 13 23:47:43 2015
@@ -0,0 +1,66 @@
+// RUN: rm -rf %t
+// RUN: cd %S
+//
+// RUN: %clang_cc1 -fmodules -x c++ -fmodules-cache-path=%t \
+// RUN:   -iquote Inputs/merge-target-features \
+// RUN:   -fno-implicit-modules -fno-modules-implicit-maps \
+// RUN:   -fmodule-map-file-home-is-cwd \
+// RUN:   -emit-module -fmodule-name=foo -o %t/foo.pcm \
+// RUN:   -triple i386-unknown-unknown \
+// RUN:   -target-cpu i386 -target-feature +sse2 \
+// RUN:   Inputs/merge-target-features/module.modulemap
+//
+// RUN: not %clang_cc1 -fmodules -x c++ -fmodules-cache-path=%t \
+// RUN:   -iquote Inputs/merge-target-features \
+// RUN:   -fno-implicit-modules -fno-modules-implicit-maps \
+// RUN:   -fmodule-map-file-home-is-cwd \
+// RUN:   -fmodule-map-file=Inputs/merge-target-features/module.modulemap \
+// RUN:   -fmodule-file=%t/foo.pcm \
+// RUN:   -triple i386-unknown-unknown \
+// RUN:   -target-cpu i386 \
+// RUN:   -fsyntax-only merge-target-features.cpp 2>&1 \
+// RUN:   | FileCheck --check-prefix=SUBSET %s
+// SUBSET: AST file was compiled with the target feature'+sse2' but the current translation unit is not
+//
+// RUN: %clang_cc1 -fmodules -x c++ -fmodules-cache-path=%t \
+// RUN:   -iquote Inputs/merge-target-features \
+// RUN:   -fno-implicit-modules -fno-modules-implicit-maps \
+// RUN:   -fmodule-map-file-home-is-cwd \
+// RUN:   -fmodule-map-file=Inputs/merge-target-features/module.modulemap \
+// RUN:   -fmodule-file=%t/foo.pcm \
+// RUN:   -triple i386-unknown-unknown \
+// RUN:   -target-cpu i386 -target-feature +sse2 \
+// RUN:   -fsyntax-only merge-target-features.cpp 2>&1 \
+// RUN:   | FileCheck --allow-empty --check-prefix=SAME %s
+// SAME-NOT: error:
+//
+// RUN: %clang_cc1 -fmodules -x c++ -fmodules-cache-path=%t \
+// RUN:   -iquote Inputs/merge-target-features \
+// RUN:   -fno-implicit-modules -fno-modules-implicit-maps \
+// RUN:   -fmodule-map-file-home-is-cwd \
+// RUN:   -fmodule-map-file=Inputs/merge-target-features/module.modulemap \
+// RUN:   -fmodule-file=%t/foo.pcm \
+// RUN:   -triple i386-unknown-unknown \
+// RUN:   -target-cpu i386 -target-feature +sse2 -target-feature +sse3 \
+// RUN:   -fsyntax-only merge-target-features.cpp 2>&1 \
+// RUN:   | FileCheck --allow-empty --check-prefix=SUPERSET %s
+// SUPERSET-NOT: error:
+//
+// RUN: not %clang_cc1 -fmodules -x c++ -fmodules-cache-path=%t \
+// RUN:   -iquote Inputs/merge-target-features \
+// RUN:   -fno-implicit-modules -fno-modules-implicit-maps \
+// RUN:   -fmodule-map-file-home-is-cwd \
+// RUN:   -fmodule-map-file=Inputs/merge-target-features/module.modulemap \
+// RUN:   -fmodule-file=%t/foo.pcm \
+// RUN:   -triple i386-unknown-unknown \
+// RUN:   -target-cpu i386 -target-feature +cx16 \
+// RUN:   -fsyntax-only merge-target-features.cpp 2>&1 \
+// RUN:   | FileCheck --check-prefix=MISMATCH %s
+// MISMATCH: AST file was compiled with the target feature'+sse2' but the current translation unit is not
+// MISMATCH: current translation unit was compiled with the target feature'+cx16' but the AST file was not
+
+#include "foo.h"
+
+int test(int x) {
+  return foo(x);
+}





More information about the cfe-commits mailing list