From a172a34fb1b047c946366af60eb7e5da2ac32f4c Mon Sep 17 00:00:00 2001 From: Ben Langmuir Date: Tue, 29 Apr 2014 00:36:53 +0000 Subject: [PATCH] Check -Werror options during module validation MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit This patch checks whether the diagnostic options that could lead to errors (principally -Werror) are consistent between when a module was built and when it is loaded. If there are new -Werror flags, then the module is rebuilt. In order to canonicalize the options we do this check at the level of the constructed DiagnosticsEngine, which contains the final set of diag to diagnostic level mappings. Currently we only rebuild with the new diagnostic options, but we intend to refine this in the future to include the union of the new and old flags, since we know the old ones did not cause errors. System modules are only rebuilt when -Wsystem-headers is enabled. One oddity is that unlike checking language options, we don’t perform this diagnostic option checking when loading from a precompiled header. The reason for this is that the compiler cannot rebuild the PCH, so anything that requires it to be rebuilt effectively leaks into the build system. And in this case, that would mean the build system understanding the complex relationship between diagnostic options and the underlying diagnostic mappings, which is unreasonable. Skipping the check is safe, because these options do not affect the generated AST. You simply won’t get new build errors due to changed -Werror options automatically, which is also true for non-module cases. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@207477 91177308-0d34-0410-b5e6-96231b3b80d8 --- include/clang/Basic/Diagnostic.h | 9 ++ .../Basic/DiagnosticSerializationKinds.td | 2 + include/clang/Serialization/ASTReader.h | 9 +- lib/Frontend/FrontendActions.cpp | 19 +++ lib/Serialization/ASTReader.cpp | 131 ++++++++++++++++-- test/Modules/Werror-Wsystem-headers.m | 23 +++ test/Modules/Werror.m | 75 ++++++++++ test/Modules/module_file_info.m | 7 +- test/Modules/resolution-change.m | 3 + 9 files changed, 266 insertions(+), 12 deletions(-) create mode 100644 test/Modules/Werror-Wsystem-headers.m create mode 100644 test/Modules/Werror.m diff --git a/include/clang/Basic/Diagnostic.h b/include/clang/Basic/Diagnostic.h index f7f84440a5..76b2eac9b5 100644 --- a/include/clang/Basic/Diagnostic.h +++ b/include/clang/Basic/Diagnostic.h @@ -21,6 +21,7 @@ #include "llvm/ADT/ArrayRef.h" #include "llvm/ADT/DenseMap.h" #include "llvm/ADT/IntrusiveRefCntPtr.h" +#include "llvm/ADT/iterator_range.h" #include #include @@ -364,6 +365,14 @@ public: /// \brief Retrieve the diagnostic options. DiagnosticOptions &getDiagnosticOptions() const { return *DiagOpts; } + typedef llvm::iterator_range diag_mapping_range; + + /// \brief Get the current set of diagnostic mappings. + diag_mapping_range getDiagnosticMappings() const { + const DiagState &DS = *GetCurDiagState(); + return diag_mapping_range(DS.begin(), DS.end()); + } + DiagnosticConsumer *getClient() { return Client; } const DiagnosticConsumer *getClient() const { return Client; } diff --git a/include/clang/Basic/DiagnosticSerializationKinds.td b/include/clang/Basic/DiagnosticSerializationKinds.td index db1ca43327..be9d2bdbd2 100644 --- a/include/clang/Basic/DiagnosticSerializationKinds.td +++ b/include/clang/Basic/DiagnosticSerializationKinds.td @@ -39,6 +39,8 @@ def err_pch_langopt_mismatch : Error<"%0 was %select{disabled|enabled}1 in " "PCH file but is currently %select{disabled|enabled}2">; def err_pch_langopt_value_mismatch : Error< "%0 differs in PCH file vs. current file">; +def err_pch_diagopt_mismatch : Error<"%0 is currently enabled, but was not in " + "the PCH file">; def err_pch_version_too_old : Error< "PCH file uses an older PCH format that is no longer supported">; diff --git a/include/clang/Serialization/ASTReader.h b/include/clang/Serialization/ASTReader.h index 0d2bfc6797..32bec0b2d7 100644 --- a/include/clang/Serialization/ASTReader.h +++ b/include/clang/Serialization/ASTReader.h @@ -133,8 +133,9 @@ public: /// /// \returns true to indicate the diagnostic options are invalid, or false /// otherwise. - virtual bool ReadDiagnosticOptions(const DiagnosticOptions &DiagOpts, - bool Complain) { + virtual bool + ReadDiagnosticOptions(IntrusiveRefCntPtr DiagOpts, + bool Complain) { return false; } @@ -211,7 +212,7 @@ public: bool ReadLanguageOptions(const LangOptions &LangOpts, bool Complain) override; bool ReadTargetOptions(const TargetOptions &TargetOpts, bool Complain) override; - bool ReadDiagnosticOptions(const DiagnosticOptions &DiagOpts, + bool ReadDiagnosticOptions(IntrusiveRefCntPtr DiagOpts, bool Complain) override; bool ReadFileSystemOptions(const FileSystemOptions &FSOpts, bool Complain) override; @@ -244,6 +245,8 @@ public: bool Complain) override; bool ReadTargetOptions(const TargetOptions &TargetOpts, bool Complain) override; + bool ReadDiagnosticOptions(IntrusiveRefCntPtr DiagOpts, + bool Complain) override; bool ReadPreprocessorOptions(const PreprocessorOptions &PPOpts, bool Complain, std::string &SuggestedPredefines) override; void ReadCounter(const serialization::ModuleFile &M, unsigned Value) override; diff --git a/lib/Frontend/FrontendActions.cpp b/lib/Frontend/FrontendActions.cpp index 3d65ae32c0..a8df7fd1cb 100644 --- a/lib/Frontend/FrontendActions.cpp +++ b/lib/Frontend/FrontendActions.cpp @@ -459,6 +459,25 @@ namespace { return false; } + virtual bool + ReadDiagnosticOptions(IntrusiveRefCntPtr DiagOpts, + bool Complain) override { + Out.indent(2) << "Diagnostic options:\n"; +#define DIAGOPT(Name, Bits, Default) DUMP_BOOLEAN(DiagOpts->Name, #Name); +#define ENUM_DIAGOPT(Name, Type, Bits, Default) \ + Out.indent(4) << #Name << ": " << DiagOpts->get##Name() << "\n"; +#define VALUE_DIAGOPT(Name, Bits, Default) \ + Out.indent(4) << #Name << ": " << DiagOpts->Name << "\n"; +#include "clang/Basic/DiagnosticOptions.def" + + Out.indent(4) << "Warning options:\n"; + for (const std::string &Warning : DiagOpts->Warnings) { + Out.indent(6) << "-W" << Warning << "\n"; + } + + return false; + } + bool ReadHeaderSearchOptions(const HeaderSearchOptions &HSOpts, bool Complain) override { Out.indent(2) << "Header search options:\n"; diff --git a/lib/Serialization/ASTReader.cpp b/lib/Serialization/ASTReader.cpp index 834917de1d..c6aec4592b 100644 --- a/lib/Serialization/ASTReader.cpp +++ b/lib/Serialization/ASTReader.cpp @@ -29,6 +29,7 @@ #include "clang/Basic/TargetOptions.h" #include "clang/Basic/Version.h" #include "clang/Basic/VersionTuple.h" +#include "clang/Frontend/Utils.h" #include "clang/Lex/HeaderSearch.h" #include "clang/Lex/HeaderSearchOptions.h" #include "clang/Lex/MacroInfo.h" @@ -90,7 +91,7 @@ ChainedASTReaderListener::ReadTargetOptions(const TargetOptions &TargetOpts, Second->ReadTargetOptions(TargetOpts, Complain); } bool ChainedASTReaderListener::ReadDiagnosticOptions( - const DiagnosticOptions &DiagOpts, bool Complain) { + IntrusiveRefCntPtr DiagOpts, bool Complain) { return First->ReadDiagnosticOptions(DiagOpts, Complain) || Second->ReadDiagnosticOptions(DiagOpts, Complain); } @@ -291,6 +292,120 @@ namespace { DeclsMap; } +static bool checkDiagnosticGroupMappings(DiagnosticsEngine &StoredDiags, + DiagnosticsEngine &Diags, + bool Complain) { + typedef DiagnosticsEngine::Level Level; + + // Check current mappings for new -Werror mappings, and the stored mappings + // for cases that were explicitly mapped to *not* be errors that are now + // errors because of options like -Werror. + DiagnosticsEngine *MappingSources[] = { &Diags, &StoredDiags }; + + for (DiagnosticsEngine *MappingSource : MappingSources) { + for (auto DiagIDMappingPair : MappingSource->getDiagnosticMappings()) { + diag::kind DiagID = DiagIDMappingPair.first; + Level CurLevel = Diags.getDiagnosticLevel(DiagID, SourceLocation()); + if (CurLevel < DiagnosticsEngine::Error) + continue; // not significant + Level StoredLevel = + StoredDiags.getDiagnosticLevel(DiagID, SourceLocation()); + if (StoredLevel < DiagnosticsEngine::Error) { + if (Complain) + Diags.Report(diag::err_pch_diagopt_mismatch) << "-Werror=" + + Diags.getDiagnosticIDs()->getWarningOptionForDiag(DiagID).str(); + return true; + } + } + } + + return false; +} + +static DiagnosticsEngine::ExtensionHandling +isExtHandlingFromDiagsError(DiagnosticsEngine &Diags) { + DiagnosticsEngine::ExtensionHandling Ext = + Diags.getExtensionHandlingBehavior(); + if (Ext == DiagnosticsEngine::Ext_Warn && Diags.getWarningsAsErrors()) + Ext = DiagnosticsEngine::Ext_Error; + return Ext; +} + +static bool checkDiagnosticMappings(DiagnosticsEngine &StoredDiags, + DiagnosticsEngine &Diags, + bool IsSystem, bool Complain) { + // Top-level options + if (IsSystem) { + if (Diags.getSuppressSystemWarnings()) + return false; + // If -Wsystem-headers was not enabled before, be conservative + if (StoredDiags.getSuppressSystemWarnings()) { + if (Complain) + Diags.Report(diag::err_pch_diagopt_mismatch) << "-Wsystem-headers"; + return true; + } + } + + if (Diags.getWarningsAsErrors() && !StoredDiags.getWarningsAsErrors()) { + if (Complain) + Diags.Report(diag::err_pch_diagopt_mismatch) << "-Werror"; + return true; + } + + if (Diags.getWarningsAsErrors() && Diags.getEnableAllWarnings() && + !StoredDiags.getEnableAllWarnings()) { + if (Complain) + Diags.Report(diag::err_pch_diagopt_mismatch) << "-Weverything -Werror"; + return true; + } + + if (isExtHandlingFromDiagsError(Diags) && + !isExtHandlingFromDiagsError(StoredDiags)) { + if (Complain) + Diags.Report(diag::err_pch_diagopt_mismatch) << "-pedantic-errors"; + return true; + } + + return checkDiagnosticGroupMappings(StoredDiags, Diags, Complain); +} + +bool PCHValidator::ReadDiagnosticOptions( + IntrusiveRefCntPtr DiagOpts, bool Complain) { + DiagnosticsEngine &ExistingDiags = PP.getDiagnostics(); + IntrusiveRefCntPtr DiagIDs(ExistingDiags.getDiagnosticIDs()); + IntrusiveRefCntPtr Diags( + new DiagnosticsEngine(DiagIDs, DiagOpts.getPtr())); + // This should never fail, because we would have processed these options + // before writing them to an ASTFile. + ProcessWarningOptions(*Diags, *DiagOpts, /*Report*/false); + + ModuleManager &ModuleMgr = Reader.getModuleManager(); + assert(ModuleMgr.size() >= 1 && "what ASTFile is this then"); + + // If the original import came from a file explicitly generated by the user, + // don't check the diagnostic mappings. + // FIXME: currently this is approximated by checking whether this is not a + // module import. + // Note: ModuleMgr.rbegin() may not be the current module, but it must be in + // the transitive closure of its imports, since unrelated modules cannot be + // imported until after this module finishes validation. + ModuleFile *TopImport = *ModuleMgr.rbegin(); + while (!TopImport->ImportedBy.empty()) + TopImport = TopImport->ImportedBy[0]; + if (TopImport->Kind != MK_Module) + return false; + + StringRef ModuleName = TopImport->ModuleName; + assert(!ModuleName.empty() && "diagnostic options read before module name"); + + Module *M = PP.getHeaderSearchInfo().lookupModule(ModuleName); + assert(M && "missing module"); + + // FIXME: if the diagnostics are incompatible, save a DiagnosticOptions that + // contains the union of their flags. + return checkDiagnosticMappings(*Diags, ExistingDiags, M->IsSystem, Complain); +} + /// \brief Collect the macro definitions provided by the given preprocessor /// options. static void collectMacroDefinitions(const PreprocessorOptions &PPOpts, @@ -2268,11 +2383,11 @@ ASTReader::ReadControlBlock(ModuleFile &F, } case DIAGNOSTIC_OPTIONS: { - bool Complain = (ClientLoadCapabilities & ARR_ConfigurationMismatch)==0; + bool Complain = (ClientLoadCapabilities & ARR_OutOfDate)==0; if (Listener && &F == *ModuleMgr.begin() && ParseDiagnosticOptions(Record, Complain, *Listener) && - !DisableValidation && !AllowConfigurationMismatch) - return ConfigurationMismatch; + !DisableValidation) + return OutOfDate; break; } @@ -4481,15 +4596,15 @@ bool ASTReader::ParseTargetOptions(const RecordData &Record, bool ASTReader::ParseDiagnosticOptions(const RecordData &Record, bool Complain, ASTReaderListener &Listener) { - DiagnosticOptions DiagOpts; + IntrusiveRefCntPtr DiagOpts(new DiagnosticOptions); unsigned Idx = 0; -#define DIAGOPT(Name, Bits, Default) DiagOpts.Name = Record[Idx++]; +#define DIAGOPT(Name, Bits, Default) DiagOpts->Name = Record[Idx++]; #define ENUM_DIAGOPT(Name, Type, Bits, Default) \ - DiagOpts.set##Name(static_cast(Record[Idx++])); + DiagOpts->set##Name(static_cast(Record[Idx++])); #include "clang/Basic/DiagnosticOptions.def" for (unsigned N = Record[Idx++]; N; --N) { - DiagOpts.Warnings.push_back(ReadString(Record, Idx)); + DiagOpts->Warnings.push_back(ReadString(Record, Idx)); } return Listener.ReadDiagnosticOptions(DiagOpts, Complain); diff --git a/test/Modules/Werror-Wsystem-headers.m b/test/Modules/Werror-Wsystem-headers.m new file mode 100644 index 0000000000..b3ca77e960 --- /dev/null +++ b/test/Modules/Werror-Wsystem-headers.m @@ -0,0 +1,23 @@ +// REQUIRES: shell +// RUN: rm -rf %t +// RUN: rm -rf %t-saved +// RUN: mkdir %t-saved + +// Initial module build +// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -fdisable-module-hash \ +// RUN: -isysroot %S/Inputs/System -fsyntax-only %s -verify +// RUN: cp %t/cstd.pcm %t-saved/cstd.pcm + +// Even with -Werror don't rebuild a system module +// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -fdisable-module-hash \ +// RUN: -isysroot %S/Inputs/System -fsyntax-only %s -verify -Werror +// RUN: diff %t/cstd.pcm %t-saved/cstd.pcm + +// Unless -Wsystem-headers is on +// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -fdisable-module-hash \ +// RUN: -isysroot %S/Inputs/System -fsyntax-only %s -verify \ +// RUN: -Werror=unused -Wsystem-headers +// RUN: not diff %t/cstd.pcm %t-saved/cstd.pcm + +// expected-no-diagnostics +@import cstd; diff --git a/test/Modules/Werror.m b/test/Modules/Werror.m new file mode 100644 index 0000000000..94a98a5a19 --- /dev/null +++ b/test/Modules/Werror.m @@ -0,0 +1,75 @@ +// REQUIRES: shell +// RUN: rm -rf %t +// RUN: rm -rf %t-saved +// RUN: mkdir -p %t-saved + +// Initial module build (-Werror=header-guard) +// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -fdisable-module-hash \ +// RUN: -F %S/Inputs -fsyntax-only %s -verify -Wno-incomplete-umbrella \ +// RUN: -Werror=header-guard +// RUN: cp %t/Module.pcm %t-saved/Module.pcm + +// Building with looser -Werror options does not rebuild +// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -fdisable-module-hash \ +// RUN: -F %S/Inputs -fsyntax-only %s -verify -Wno-incomplete-umbrella +// RUN: diff %t/Module.pcm %t-saved/Module.pcm + +// Make the build more restricted (-Werror) +// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -fdisable-module-hash \ +// RUN: -F %S/Inputs -fsyntax-only %s -verify -Wno-incomplete-umbrella \ +// RUN: -Werror -Wno-incomplete-umbrella +// RUN: not diff %t/Module.pcm %t-saved/Module.pcm +// RUN: cp %t/Module.pcm %t-saved/Module.pcm + +// Ensure -Werror=header-guard is less strict than -Werror +// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -fdisable-module-hash \ +// RUN: -F %S/Inputs -fsyntax-only %s -verify -Wno-incomplete-umbrella \ +// RUN: -Werror=header-guard -Wno-incomplete-umbrella +// RUN: diff %t/Module.pcm %t-saved/Module.pcm + +// But -Werror=unused is not, because some of its diags are DefaultIgnore +// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -fdisable-module-hash \ +// RUN: -F %S/Inputs -fsyntax-only %s -verify -Wno-incomplete-umbrella \ +// RUN: -Werror=unused +// RUN: not diff %t/Module.pcm %t-saved/Module.pcm +// RUN: cp %t/Module.pcm %t-saved/Module.pcm + +// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -fdisable-module-hash \ +// RUN: -F %S/Inputs -fsyntax-only %s -verify -Wno-incomplete-umbrella \ +// RUN: -Werror -Wno-incomplete-umbrella + +// FIXME: when rebuilding the module, take the union of the diagnostic options +// so that we don't need to rebuild here +// RUN-DISABLED: diff %t/Module.pcm %t-saved/Module.pcm + +// -Wno-everything, -Werror +// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -fdisable-module-hash \ +// RUN: -F %S/Inputs -fsyntax-only %s -verify -Wno-incomplete-umbrella \ +// RUN: -Wno-everything -Wall -Werror +// RUN: cp %t/Module.pcm %t-saved/Module.pcm +// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -fdisable-module-hash \ +// RUN: -F %S/Inputs -fsyntax-only %s -verify -Wno-incomplete-umbrella \ +// RUN: -Wall -Werror +// RUN: not diff %t/Module.pcm %t-saved/Module.pcm + +// -pedantic, -Werror is not compatible with -Wall -Werror +// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -fdisable-module-hash \ +// RUN: -F %S/Inputs -fsyntax-only %s -verify -Wno-incomplete-umbrella \ +// RUN: -Werror -pedantic +// RUN: not diff %t/Module.pcm %t-saved/Module.pcm +// RUN: cp %t/Module.pcm %t-saved/Module.pcm + +// -pedantic-errors is less strict that -pedantic, -Werror +// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -fdisable-module-hash \ +// RUN: -F %S/Inputs -fsyntax-only %s -verify -Wno-incomplete-umbrella \ +// RUN: -pedantic-errors +// RUN: diff %t/Module.pcm %t-saved/Module.pcm + +// -Wsystem-headers does not affect non-system modules +// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -fdisable-module-hash \ +// RUN: -F %S/Inputs -fsyntax-only %s -verify -Wno-incomplete-umbrella \ +// RUN: -pedantic-errors -Wsystem-headers +// RUN: diff %t/Module.pcm %t-saved/Module.pcm + +// expected-no-diagnostics +@import Module; diff --git a/test/Modules/module_file_info.m b/test/Modules/module_file_info.m index 9f1ff2221b..13e086ee48 100644 --- a/test/Modules/module_file_info.m +++ b/test/Modules/module_file_info.m @@ -2,7 +2,7 @@ @import DependsOnModule; // RUN: rm -rf %t -// RUN: %clang_cc1 -w -fmodules -fdisable-module-hash -fmodules-cache-path=%t -F %S/Inputs -DBLARG -DWIBBLE=WOBBLE %s +// RUN: %clang_cc1 -w -Wunused -fmodules -fdisable-module-hash -fmodules-cache-path=%t -F %S/Inputs -DBLARG -DWIBBLE=WOBBLE %s // RUN: %clang_cc1 -module-file-info %t/DependsOnModule.pcm | FileCheck %s // CHECK: Generated by this Clang: @@ -21,6 +21,11 @@ // CHECK: ABI: // CHECK: Linker version: +// CHECK: Diagnostic options: +// CHECK: IgnoreWarnings: Yes +// CHECK: Warning options: +// CHECK: -Wunused + // CHECK: Header search options: // CHECK: System root [-isysroot=]: '/' // CHECK: Use builtin include directories [-nobuiltininc]: Yes diff --git a/test/Modules/resolution-change.m b/test/Modules/resolution-change.m index a69014c6ae..011782eec2 100644 --- a/test/Modules/resolution-change.m +++ b/test/Modules/resolution-change.m @@ -6,6 +6,9 @@ // Use the PCH with the same header search options; should be fine // RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I %S/Inputs/modules-with-same-name/DependsOnA -I %S/Inputs/modules-with-same-name/path1/A -include-pch %t-A.pch %s -fsyntax-only -Werror +// Different -W options are ok +// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I %S/Inputs/modules-with-same-name/DependsOnA -I %S/Inputs/modules-with-same-name/path1/A -include-pch %t-A.pch %s -fsyntax-only -Werror -Wauto-import + // Use the PCH with no way to resolve DependsOnA // RUN: not %clang_cc1 -fmodules -fmodules-cache-path=%t -include-pch %t-A.pch %s -fsyntax-only 2>&1 | FileCheck -check-prefix=CHECK-NODOA %s // CHECK-NODOA: module 'DependsOnA' imported by AST file '{{.*A.pch}}' not found -- 2.50.1