From 900f817591d0f6871b324aef655cbda926d26052 Mon Sep 17 00:00:00 2001 From: "Duncan P. N. Exon Smith" Date: Wed, 12 Apr 2017 03:58:58 +0000 Subject: [PATCH] Serialization: Simulate -Werror settings in implicit modules r293123 started serializing diagnostic pragma state for modules. This makes the serialization work properly for implicit modules. An implicit module build (using Clang's internal build system) uses the same PCM file location for different `-Werror` levels. E.g., if a TU has `-Werror=format` and tries to load a PCM built without `-Werror=format`, a new PCM will be built in its place (and the new PCM should have the same signature, since r297655). In the other direction, if a TU does not have `-Werror=format` and tries to load a PCM built with `-Werror=format`, it should "just work". The idea is to evolve the PCM toward the strictest -Werror flags that anyone tries. r293123 started serializing the diagnostic pragma state for each PCM. Since this encodes the -Werror settings at module-build time, it breaks the implicit build model. This commit filters the diagnostic state in order to simulate the current compilation's diagnostic settings. Firstly, it ignores the module's serialized first diagnostic state, replacing it with the state from this compilation's command-line. Secondly, if a pragma warning was upgraded to error/fatal when generating the PCM (e.g., due to `-Werror` on the command-line), it checks whether it should still be upgraded in its current context. llvm-svn: 300025 --- clang/include/clang/Basic/Diagnostic.h | 3 ++ clang/include/clang/Basic/DiagnosticIDs.h | 24 +++++++++++ clang/lib/Basic/Diagnostic.cpp | 6 ++- clang/lib/Serialization/ASTReader.cpp | 50 +++++++++++++++++++--- clang/lib/Serialization/ASTWriter.cpp | 7 ++- .../Inputs/implicit-built-Werror-using-W/convert.h | 8 ++++ .../implicit-built-Werror-using-W/module.modulemap | 3 ++ .../test/Modules/implicit-built-Werror-using-W.cpp | 42 ++++++++++++++++++ 8 files changed, 134 insertions(+), 9 deletions(-) create mode 100644 clang/test/Modules/Inputs/implicit-built-Werror-using-W/convert.h create mode 100644 clang/test/Modules/Inputs/implicit-built-Werror-using-W/module.modulemap create mode 100644 clang/test/Modules/implicit-built-Werror-using-W.cpp diff --git a/clang/include/clang/Basic/Diagnostic.h b/clang/include/clang/Basic/Diagnostic.h index ffce99b..a8e11bc 100644 --- a/clang/include/clang/Basic/Diagnostic.h +++ b/clang/include/clang/Basic/Diagnostic.h @@ -223,6 +223,9 @@ private: void setMapping(diag::kind Diag, DiagnosticMapping Info) { DiagMap[Diag] = Info; } + DiagnosticMapping lookupMapping(diag::kind Diag) const { + return DiagMap.lookup(Diag); + } DiagnosticMapping &getOrAddMapping(diag::kind Diag); diff --git a/clang/include/clang/Basic/DiagnosticIDs.h b/clang/include/clang/Basic/DiagnosticIDs.h index fcd04a0..f5f70cb 100644 --- a/clang/include/clang/Basic/DiagnosticIDs.h +++ b/clang/include/clang/Basic/DiagnosticIDs.h @@ -84,6 +84,7 @@ class DiagnosticMapping { unsigned IsPragma : 1; unsigned HasNoWarningAsError : 1; unsigned HasNoErrorAsFatal : 1; + unsigned WasUpgradedFromWarning : 1; public: static DiagnosticMapping Make(diag::Severity Severity, bool IsUser, @@ -94,6 +95,7 @@ public: Result.IsPragma = IsPragma; Result.HasNoWarningAsError = 0; Result.HasNoErrorAsFatal = 0; + Result.WasUpgradedFromWarning = 0; return Result; } @@ -103,11 +105,33 @@ public: bool isUser() const { return IsUser; } bool isPragma() const { return IsPragma; } + bool isErrorOrFatal() const { + return getSeverity() == diag::Severity::Error || + getSeverity() == diag::Severity::Fatal; + } + bool hasNoWarningAsError() const { return HasNoWarningAsError; } void setNoWarningAsError(bool Value) { HasNoWarningAsError = Value; } bool hasNoErrorAsFatal() const { return HasNoErrorAsFatal; } void setNoErrorAsFatal(bool Value) { HasNoErrorAsFatal = Value; } + + /// Whether this mapping attempted to map the diagnostic to a warning, but + /// was overruled because the diagnostic was already mapped to an error or + /// fatal error. + bool wasUpgradedFromWarning() const { return WasUpgradedFromWarning; } + void setUpgradedFromWarning(bool Value) { WasUpgradedFromWarning = Value; } + + /// Serialize the bits that aren't based on context. + unsigned serializeBits() const { + return (WasUpgradedFromWarning << 3) | Severity; + } + static diag::Severity deserializeSeverity(unsigned Bits) { + return (diag::Severity)(Bits & 0x7); + } + static bool deserializeUpgradedFromWarning(unsigned Bits) { + return Bits >> 3; + } }; /// \brief Used for handling and querying diagnostic IDs. diff --git a/clang/lib/Basic/Diagnostic.cpp b/clang/lib/Basic/Diagnostic.cpp index 0c27ae4..350d547 100644 --- a/clang/lib/Basic/Diagnostic.cpp +++ b/clang/lib/Basic/Diagnostic.cpp @@ -258,13 +258,17 @@ void DiagnosticsEngine::setSeverity(diag::kind Diag, diag::Severity Map, assert((L.isInvalid() || SourceMgr) && "No SourceMgr for valid location"); // Don't allow a mapping to a warning override an error/fatal mapping. + bool WasUpgradedFromWarning = false; if (Map == diag::Severity::Warning) { DiagnosticMapping &Info = GetCurDiagState()->getOrAddMapping(Diag); if (Info.getSeverity() == diag::Severity::Error || - Info.getSeverity() == diag::Severity::Fatal) + Info.getSeverity() == diag::Severity::Fatal) { Map = Info.getSeverity(); + WasUpgradedFromWarning = true; + } } DiagnosticMapping Mapping = makeUserMapping(Map, L); + Mapping.setUpgradedFromWarning(WasUpgradedFromWarning); // Common case; setting all the diagnostics of a group in one place. if ((L.isInvalid() || L == DiagStatesByLoc.getCurDiagStateLoc()) && diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp index e09af28..d6bacfe 100644 --- a/clang/lib/Serialization/ASTReader.cpp +++ b/clang/lib/Serialization/ASTReader.cpp @@ -5525,18 +5525,54 @@ void ASTReader::ReadPragmaDiagnosticMappings(DiagnosticsEngine &Diag) { "Invalid data, not enough diag/map pairs"); while (Size--) { unsigned DiagID = Record[Idx++]; - diag::Severity Map = (diag::Severity)Record[Idx++]; - DiagnosticMapping Mapping = Diag.makeUserMapping(Map, Loc); - if (Mapping.isPragma() || IncludeNonPragmaStates) - NewState->setMapping(DiagID, Mapping); + unsigned SeverityAndUpgradedFromWarning = Record[Idx++]; + bool WasUpgradedFromWarning = + DiagnosticMapping::deserializeUpgradedFromWarning( + SeverityAndUpgradedFromWarning); + DiagnosticMapping NewMapping = + Diag.makeUserMapping(DiagnosticMapping::deserializeSeverity( + SeverityAndUpgradedFromWarning), + Loc); + if (!NewMapping.isPragma() && !IncludeNonPragmaStates) + continue; + + DiagnosticMapping &Mapping = NewState->getOrAddMapping(DiagID); + + // If this mapping was specified as a warning but the severity was + // upgraded due to diagnostic settings, simulate the current diagnostic + // settings (and use a warning). + if (WasUpgradedFromWarning && !Mapping.isErrorOrFatal()) { + Mapping = Diag.makeUserMapping(diag::Severity::Warning, Loc); + continue; + } + + // Use the deserialized mapping verbatim. + Mapping = NewMapping; + Mapping.setUpgradedFromWarning(WasUpgradedFromWarning); } return NewState; }; // Read the first state. - auto *FirstState = ReadDiagState( - F.isModule() ? DiagState() : *Diag.DiagStatesByLoc.CurDiagState, - SourceLocation(), F.isModule()); + DiagState *FirstState; + if (F.Kind == MK_ImplicitModule) { + // Implicitly-built modules are reused with different diagnostic + // settings. Use the initial diagnostic state from Diag to simulate this + // compilation's diagnostic settings. + FirstState = Diag.DiagStatesByLoc.FirstDiagState; + DiagStates.push_back(FirstState); + + // Skip the initial diagnostic state from the serialized module. + assert(Record[0] == 0 && + "Invalid data, unexpected backref in initial state"); + Idx = 2 + Record[1] * 2; + assert(Idx < Record.size() && + "Invalid data, not enough state change pairs in initial state"); + } else { + FirstState = ReadDiagState( + F.isModule() ? DiagState() : *Diag.DiagStatesByLoc.CurDiagState, + SourceLocation(), F.isModule()); + } // Read the state transitions. unsigned NumLocations = Record[Idx++]; diff --git a/clang/lib/Serialization/ASTWriter.cpp b/clang/lib/Serialization/ASTWriter.cpp index ffcad78..7dd2ca3 100644 --- a/clang/lib/Serialization/ASTWriter.cpp +++ b/clang/lib/Serialization/ASTWriter.cpp @@ -2879,7 +2879,7 @@ void ASTWriter::WritePragmaDiagnosticMappings(const DiagnosticsEngine &Diag, for (const auto &I : *State) { if (I.second.isPragma() || IncludeNonPragmaStates) { Record.push_back(I.first); - Record.push_back((unsigned)I.second.getSeverity()); + Record.push_back(I.second.serializeBits()); } } // Update the placeholder. @@ -2913,6 +2913,11 @@ void ASTWriter::WritePragmaDiagnosticMappings(const DiagnosticsEngine &Diag, Record[NumLocationsIdx] = NumLocations; // Emit CurDiagStateLoc. Do it last in order to match source order. + // + // This also protects against a hypothetical corner case with simulating + // -Werror settings for implicit modules in the ASTReader, where reading + // CurDiagState out of context could change whether warning pragmas are + // treated as errors. AddSourceLocation(Diag.DiagStatesByLoc.CurDiagStateLoc, Record); AddDiagState(Diag.DiagStatesByLoc.CurDiagState, false); diff --git a/clang/test/Modules/Inputs/implicit-built-Werror-using-W/convert.h b/clang/test/Modules/Inputs/implicit-built-Werror-using-W/convert.h new file mode 100644 index 0000000..0ed02bc --- /dev/null +++ b/clang/test/Modules/Inputs/implicit-built-Werror-using-W/convert.h @@ -0,0 +1,8 @@ +#ifdef USE_PRAGMA +#pragma clang diagnostic push +#pragma clang diagnostic warning "-Wshorten-64-to-32" +#endif +template int convert(T V) { return V; } +#ifdef USE_PRAGMA +#pragma clang diagnostic pop +#endif diff --git a/clang/test/Modules/Inputs/implicit-built-Werror-using-W/module.modulemap b/clang/test/Modules/Inputs/implicit-built-Werror-using-W/module.modulemap new file mode 100644 index 0000000..825eb86 --- /dev/null +++ b/clang/test/Modules/Inputs/implicit-built-Werror-using-W/module.modulemap @@ -0,0 +1,3 @@ +module convert { + header "convert.h" +} diff --git a/clang/test/Modules/implicit-built-Werror-using-W.cpp b/clang/test/Modules/implicit-built-Werror-using-W.cpp new file mode 100644 index 0000000..9fb7a6bf0 --- /dev/null +++ b/clang/test/Modules/implicit-built-Werror-using-W.cpp @@ -0,0 +1,42 @@ +// Check that implicit modules builds give correct diagnostics, even when +// reusing a module built with strong -Werror flags. +// +// Clear the caches. +// RUN: rm -rf %t.cache %t-pragma.cache +// +// Build with -Werror, then with -W, and then with neither. +// RUN: not %clang_cc1 -triple x86_64-apple-darwin16 -fsyntax-only -fmodules \ +// RUN: -Werror=shorten-64-to-32 \ +// RUN: -I%S/Inputs/implicit-built-Werror-using-W -fimplicit-module-maps \ +// RUN: -fmodules-cache-path=%t.cache -x c++ %s 2>&1 \ +// RUN: | FileCheck %s -check-prefix=CHECK-ERROR +// RUN: %clang_cc1 -triple x86_64-apple-darwin16 -fsyntax-only -fmodules \ +// RUN: -Wshorten-64-to-32 \ +// RUN: -I%S/Inputs/implicit-built-Werror-using-W -fimplicit-module-maps \ +// RUN: -fdisable-module-hash \ +// RUN: -fmodules-cache-path=%t.cache -x c++ %s 2>&1 \ +// RUN: | FileCheck %s -check-prefix=CHECK-WARN +// RUN: %clang_cc1 -triple x86_64-apple-darwin16 -fsyntax-only -fmodules \ +// RUN: -I%S/Inputs/implicit-built-Werror-using-W -fimplicit-module-maps \ +// RUN: -fmodules-cache-path=%t.cache -x c++ %s 2>&1 \ +// RUN: | FileCheck %s -allow-empty +// +// In the presence of a warning pragma, build with -Werror and then without. +// RUN: not %clang_cc1 -triple x86_64-apple-darwin16 -fsyntax-only -fmodules \ +// RUN: -DUSE_PRAGMA -Werror=shorten-64-to-32 \ +// RUN: -I%S/Inputs/implicit-built-Werror-using-W -fimplicit-module-maps \ +// RUN: -fmodules-cache-path=%t-pragma.cache -x c++ %s 2>&1 \ +// RUN: | FileCheck %s -check-prefix=CHECK-ERROR +// RUN: %clang_cc1 -triple x86_64-apple-darwin16 -fsyntax-only -fmodules \ +// RUN: -DUSE_PRAGMA \ +// RUN: -I%S/Inputs/implicit-built-Werror-using-W -fimplicit-module-maps \ +// RUN: -fmodules-cache-path=%t-pragma.cache -x c++ %s 2>&1 \ +// RUN: | FileCheck %s -check-prefix=CHECK-WARN +#include + +long long foo() { return convert(0); } + +// CHECK-ERROR: error: implicit conversion +// CHECK-WARN: warning: implicit conversion +// CHECK-NOT: error: implicit conversion +// CHECK-NOT: warning: implicit conversion -- 2.7.4