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

diff --git a/clang/include/clang/Basic/Diagnostic.h b/clang/include/clang/Basic/Diagnostic.h
index ffce99b..a8e11bc 100644 (file)
--- 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 (file)
--- 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 (file)
--- 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 (file)
--- 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 (file)
--- 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 (file)
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 <class T> 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 (file)
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 (file)
index 0000000..9fb7a6b
--- /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 <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