From: Chandler Carruth Date: Sat, 14 Mar 2015 04:47:43 +0000 (+0000) Subject: [modules] Teach the AST reader to handle the case of importing a module X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=b80f819ba523fdc43c2f7ac4a4e3fecb594d17e8;p=clang [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. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@232248 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/include/clang/Serialization/ASTReader.h b/include/clang/Serialization/ASTReader.h index d926c2176a..55fe0ef597 100644 --- a/include/clang/Serialization/ASTReader.h +++ b/include/clang/Serialization/ASTReader.h @@ -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 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 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, diff --git a/lib/Frontend/ASTUnit.cpp b/lib/Frontend/ASTUnit.cpp index d153ca245c..bd9961edc2 100644 --- a/lib/Frontend/ASTUnit.cpp +++ b/lib/Frontend/ASTUnit.cpp @@ -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; diff --git a/lib/Frontend/FrontendActions.cpp b/lib/Frontend/FrontendActions.cpp index a55a325785..8d600a10f8 100644 --- a/lib/Frontend/FrontendActions.cpp +++ b/lib/Frontend/FrontendActions.cpp @@ -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"; diff --git a/lib/Serialization/ASTReader.cpp b/lib/Serialization/ASTReader.cpp index 2e4e891431..68d57569b7 100644 --- a/lib/Serialization/ASTReader.cpp +++ b/lib/Serialization/ASTReader.cpp @@ -89,11 +89,13 @@ ChainedASTReaderListener::ReadLanguageOptions(const LangOptions &LangOpts, 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 DiagOpts, bool Complain) { @@ -232,7 +234,8 @@ static bool checkLanguageOptions(const LangOptions &LangOpts, /// \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 TargetOptions &TargetOpts, 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 TargetOptions &TargetOpts, 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 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 LangOptions &LangOpts, } 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 &F, 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(StringRef Filename, 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(const RecordData &Record, 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 RecordData &Record, 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, diff --git a/test/Modules/Inputs/merge-target-features/foo.h b/test/Modules/Inputs/merge-target-features/foo.h new file mode 100644 index 0000000000..1c8b3f6282 --- /dev/null +++ b/test/Modules/Inputs/merge-target-features/foo.h @@ -0,0 +1,8 @@ +#ifndef FOO_H +#define FOO_H + +int foo(int x) { + return x + 42; +} + +#endif // FOO_H diff --git a/test/Modules/Inputs/merge-target-features/module.modulemap b/test/Modules/Inputs/merge-target-features/module.modulemap new file mode 100644 index 0000000000..f2e9932e4a --- /dev/null +++ b/test/Modules/Inputs/merge-target-features/module.modulemap @@ -0,0 +1 @@ +module foo { header "Inputs/merge-target-features/foo.h" export * } diff --git a/test/Modules/merge-target-features.cpp b/test/Modules/merge-target-features.cpp new file mode 100644 index 0000000000..ccf3aab3d0 --- /dev/null +++ b/test/Modules/merge-target-features.cpp @@ -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); +}