Modules: Simulate diagnostic settings for implicit modules

[dexonsmith]

Summary

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.

This commit:

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?

[dexonsmith]

Ping!

[rsmith]

LGTM

• 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?

For the PCH and preamble cases, PCHValidator should check that the diagnostic mappings are the same prior to applying the diagnostic pragmas, so there should never be a case where a warning mapping is upgraded to error/fatal error and wasn't when the PCH was built (or vice versa) except when building with -fno-validate-pch. Even in that case, the emergent behavior here seems mostly OK (you imagine what the PCH would have looked like if it were built with the current compilation's warning flags), except...

If you build a PCH that contains #pragma clang diagnostic warning "-Wfoo" and then use it from a -Werror=foo compilation, it looks like we won't notice that we need to upgrade the warning to an error when replaying the PCH's pragma mappings. This is a corner case of a corner case, though.

• 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...

IIRC we only get backreferences from pragma push/pop (and CurDiagState), and I think the push/pop cases will always have the same upgrade behavior (you can't push inside a module and pop outside it, for instance, so the starting state for the push and pop should be consistent).

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?

I don't think so, and putting CurDiagState last seems better in general (it keeps the states in something much more like source order). I think I only put it second for convenience (so I didn't need to check whether I'd just read the last state in order to handle it differently).

clang/include/clang/Basic/DiagnosticIDs.h

119–120

This could do with a documentation comment. Something like "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."

[dexonsmith]

Thanks for the review! Somehow I missed it until now (I think I must have an "ignore rsmith" filter set up), but I should find a chance to rebase and commit tomorrow or Monday.

[dexonsmith]

And committed in:
r297770 c0c27f3 Modules: Optimize bitcode encoding of diagnostic state
r300021 5e5be8e Serialization: Skip check in WritePragmaDiagnosticMappings, NFC
r300024 91f051f Serialization: Emit the final diagnostic state last, almost NFC
r300025 d3992c5 Serialization: Simulate -Werror settings in implicit modules

Thanks again for the review.

clang/include/clang/Basic/Diagnostic.h

private:
   public:
     typedef llvm::DenseMap<unsigned, DiagnosticMapping>::iterator iterator;
     typedef llvm::DenseMap<unsigned, DiagnosticMapping>::const_iterator
     const_iterator;
 
     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);
 
     const_iterator begin() const { return DiagMap.begin(); }
     const_iterator end() const { return DiagMap.end(); }
   };
 
   /// \brief Keeps and automatically disposes all DiagStates that we create.

clang/include/clang/Basic/DiagnosticIDs.h

#undef DIAG
   }
 
 class DiagnosticMapping {
   unsigned Severity : 3;
   unsigned IsUser : 1;
   unsigned IsPragma : 1;
   unsigned HasNoWarningAsError : 1;
   unsigned HasNoErrorAsFatal : 1;
+  unsigned WasUpgradedFromWarning : 1;
 
 public:
   static DiagnosticMapping Make(diag::Severity Severity, bool IsUser,
                                 bool IsPragma) {
     DiagnosticMapping Result;
     Result.Severity = (unsigned)Severity;
     Result.IsUser = IsUser;
     Result.IsPragma = IsPragma;
     Result.HasNoWarningAsError = 0;
     Result.HasNoErrorAsFatal = 0;
+    Result.WasUpgradedFromWarning = 0;
     return Result;
   }
 
   diag::Severity getSeverity() const { return (diag::Severity)Severity; }
   void setSeverity(diag::Severity Value) { Severity = (unsigned)Value; }
 
   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.
 ///
 /// Can be used and shared by multiple Diagnostics for multiple translation units.
 class DiagnosticIDs : public RefCountedBase<DiagnosticIDs> {
 public:
   /// \brief The level of the diagnostic, after it has been through mapping.

clang/lib/Basic/Diagnostic.cpp

void DiagnosticsEngine::setSeverity(diag::kind Diag, diag::Severity Map,
   assert(Diag < diag::DIAG_UPPER_LIMIT &&
          "Can only map builtin diagnostics");
   assert((Diags->isBuiltinWarningOrExtension(Diag) ||
           (Map == diag::Severity::Fatal || Map == diag::Severity::Error)) &&
          "Cannot map errors into warnings!");
   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()) &&
       DiagStatesByLoc.getCurDiagState()) {
     // FIXME: This is theoretically wrong: if the current state is shared with
     // some other location (via push/pop) we will change the state for that
     // other location as well. This cannot currently happen, as we can't update
     // the diagnostic state at the same location at which we pop.

clang/lib/Serialization/ASTReader.cpp

auto ReadDiagState =
       Diag.DiagStates.push_back(BasedOn);
       DiagState *NewState = &Diag.DiagStates.back();
       DiagStates.push_back(NewState);
       unsigned Size = Record[Idx++];
       assert(Idx + Size * 2 <= Record.size() &&
              "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(
+    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);
 
     if (!F.isModule()) {
       Diag.DiagStatesByLoc.CurDiagState = CurState;
       Diag.DiagStatesByLoc.CurDiagStateLoc = CurStateLoc;
 

clang/lib/Serialization/ASTWriter.cpp

if (DiagStateID == 0) {
       DiagStateID = ++CurrID;
 
       // Add a placeholder for the number of mappings.
       auto SizeIdx = Record.size();
       Record.emplace_back();
       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.
       Record[SizeIdx] = (Record.size() - SizeIdx) / 2;
     }
   };
 
   AddDiagState(Diag.DiagStatesByLoc.FirstDiagState, isModule);

clang/test/Modules/Inputs/implicit-built-Werror-using-W/convert.h

+#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

clang/test/Modules/Inputs/implicit-built-Werror-using-W/module.modulemap

+module convert {
+  header "convert.h"
+}

clang/test/Modules/implicit-built-Werror-using-W.cpp

+// 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