dexonsmith created this revision.
This is a follow-up to r293123 that makes it work with implicit modules.
Some background:
----------------
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 state for each PCM, which broke the
implicit build model.
This commit filters the diagnostic state to match the current compilation's
diagnostic settings.
- Ignore the module's serialized first diagnostic state. Use this TU's state
instead.
- If a pragma warning was upgraded to error/fatal when generating the PCM
(e.g., due to `-Werror` on the command-line), check whether it should still be
upgraded in its current context.
Some feedback I'd like on this approach:
1. The patch-as-is checks whether pragmas should be demoted to warnings for all
AST files, not just implicit modules. I can add a bit of logic to
ReadPragmaDiagnosticMappings that limits it to `F.Kind == MK_ImplicitModule`,
but I'm not sure it's necessary. Maybe it hits when reading PCH files, but no
tests fail, and I'm not sure this is worse... thoughts?
2. If `ReadDiagState` sees a back-reference, it doesn't bother to check whether
pragmas should be demoted; it assumes it should match whatever was done with
the back-reference.
- I think this could be exercised with `-Werror=X` on the command-line and
pragmas modifying -WX (first "ignored", then "error", then "warning"). Perhaps
I should add a FIXME or a comment, but otherwise I think this is okay to miss...
- It could be a back-reference to `CurDiagState`, which current gets
(de)serialized before the pragma mappings. If we instead (de)serialize
`CurDiagState` last, I think this one goes away. Is there a problem with that?
https://reviews.llvm.org/D30954
Files:
clang/include/clang/Basic/Diagnostic.h
clang/include/clang/Basic/DiagnosticIDs.h
clang/lib/Basic/Diagnostic.cpp
clang/lib/Serialization/ASTReader.cpp
clang/lib/Serialization/ASTWriter.cpp
clang/test/Modules/Inputs/implicit-built-Werror-using-W/convert.h
clang/test/Modules/Inputs/implicit-built-Werror-using-W/module.modulemap
clang/test/Modules/implicit-built-Werror-using-W.cpp
Index: clang/test/Modules/implicit-built-Werror-using-W.cpp
===================================================================
--- /dev/null
+++ 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 <convert.h>
+
+long long foo() { return convert<long long>(0); }
+
+// CHECK-ERROR: error: implicit conversion
+// CHECK-WARN: warning: implicit conversion
+// CHECK-NOT: error: implicit conversion
+// CHECK-NOT: warning: implicit conversion
Index: clang/test/Modules/Inputs/implicit-built-Werror-using-W/module.modulemap
===================================================================
--- /dev/null
+++ clang/test/Modules/Inputs/implicit-built-Werror-using-W/module.modulemap
@@ -0,0 +1,3 @@
+module convert {
+ header "convert.h"
+}
Index: clang/test/Modules/Inputs/implicit-built-Werror-using-W/convert.h
===================================================================
--- /dev/null
+++ 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 <class T> int convert(T V) { return V; }
+#ifdef USE_PRAGMA
+#pragma clang diagnostic pop
+#endif
Index: clang/lib/Serialization/ASTWriter.cpp
===================================================================
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -2878,7 +2878,7 @@
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.
Index: clang/lib/Serialization/ASTReader.cpp
===================================================================
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -5468,17 +5468,54 @@
"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;
};
- 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());
+ }
+
SourceLocation CurStateLoc =
ReadSourceLocation(F, F.PragmaDiagMappings[Idx++]);
auto *CurState = ReadDiagState(*FirstState, CurStateLoc, false);
Index: clang/lib/Basic/Diagnostic.cpp
===================================================================
--- clang/lib/Basic/Diagnostic.cpp
+++ clang/lib/Basic/Diagnostic.cpp
@@ -258,13 +258,17 @@
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()) &&
Index: clang/include/clang/Basic/DiagnosticIDs.h
===================================================================
--- clang/include/clang/Basic/DiagnosticIDs.h
+++ clang/include/clang/Basic/DiagnosticIDs.h
@@ -84,6 +84,7 @@
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 @@
Result.IsPragma = IsPragma;
Result.HasNoWarningAsError = 0;
Result.HasNoErrorAsFatal = 0;
+ Result.WasUpgradedFromWarning = 0;
return Result;
}
@@ -103,11 +105,30 @@
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; }
+
+ 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.
Index: clang/include/clang/Basic/Diagnostic.h
===================================================================
--- clang/include/clang/Basic/Diagnostic.h
+++ clang/include/clang/Basic/Diagnostic.h
@@ -223,6 +223,9 @@
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);
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
