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.

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@300025 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/include/clang/Basic/Diagnostic.h b/include/clang/Basic/Diagnostic.h
index ffce99bf9f7d08b3f9ff77d5a67326578148c5c9..a8e11bcb892783fdc2f37b1f7e5de6d24565f314 100644 (file)
--- a/include/clang/Basic/Diagnostic.h
+++ b/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/include/clang/Basic/DiagnosticIDs.h b/include/clang/Basic/DiagnosticIDs.h
index fcd04a0be187d3c91d47f3c94fc79cccc0207ba7..f5f70cb5e7d3c05698b01c7290201a04f8861e94 100644 (file)
--- a/include/clang/Basic/DiagnosticIDs.h
+++ b/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/lib/Basic/Diagnostic.cpp b/lib/Basic/Diagnostic.cpp
index 0c27ae4638d5b9b93c8655575e08815aa8c33c9b..350d5477751c629e868dfb29b0cc6b5fdf705eb7 100644 (file)
--- a/lib/Basic/Diagnostic.cpp
+++ b/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/lib/Serialization/ASTReader.cpp b/lib/Serialization/ASTReader.cpp
index e09af28059b390fe54ae1a0b588050f8fb26b25a..d6bacfe3e096a6b37c58d514377c8ea961ddb23c 100644 (file)
--- a/lib/Serialization/ASTReader.cpp
+++ b/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/lib/Serialization/ASTWriter.cpp b/lib/Serialization/ASTWriter.cpp
index ffcad78f295a667d632b068daa4ab0e4e13cb2ee..7dd2ca302ca6bf3c28cd6a1ad3cc5157b168c3a5 100644 (file)
--- a/lib/Serialization/ASTWriter.cpp
+++ b/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/test/Modules/Inputs/implicit-built-Werror-using-W/convert.h b/test/Modules/Inputs/implicit-built-Werror-using-W/convert.h
new file mode 100644 (file)
index 0000000..0ed02bc
--- /dev/null
+++ b/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/test/Modules/Inputs/implicit-built-Werror-using-W/module.modulemap b/test/Modules/Inputs/implicit-built-Werror-using-W/module.modulemap
new file mode 100644 (file)
index 0000000..825eb86
--- /dev/null
+++ b/test/Modules/Inputs/implicit-built-Werror-using-W/module.modulemap
@@ -0,0 +1,3 @@
+module convert {
+  header "convert.h"
+}

diff --git a/test/Modules/implicit-built-Werror-using-W.cpp b/test/Modules/implicit-built-Werror-using-W.cpp
new file mode 100644 (file)
index 0000000..9fb7a6b
--- /dev/null
+++ b/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