[clang][modules] Avoid serializing all diag mappings in non-deterministic order

[benlangmuir]

Summary

When writing a pcm, we serialize diagnostic mappings in order to
accurately reproduce the diagnostic environment inside any headers from
that module. However, the diagnostic state mapping table contains
entries for every diagnostic ID ever accessed, while we only want to
serialize the ones that are actually modified from their default value.
Futher, we need to serialize them in a deterministic order.

rdar://111477511

[steven_wu]

clang/lib/Serialization/ASTWriter.cpp

3016–3034

Is pragma in this context refer to #pragma diagnostics push/pop? Do we have test to cover those to be deterministic?

[benlangmuir]

Extended test to include diagnostic pragmas

[benlangmuir]

clang/lib/Serialization/ASTWriter.cpp

3016–3034

Is pragma in this context refer to #pragma diagnostics push/pop?

Yeah, this is diagnostic pragmas.

Do we have test to cover those to be deterministic?

I extended the current test to include some pragmas as well. It's harder to check them exhaustively because the encoded format gets more complex, but I checked the specific mappings are in order and the diff part of the test should help catch any other non-determinism there could be.

[steven_wu]

LGTM

clang/include/clang/Basic/DiagnosticIDs.h

static DiagnosticMapping deserialize(unsigned Bits) {
     Result.IsUser = (Bits >> 7) & 1;
     Result.IsPragma = (Bits >> 6) & 1;
     Result.HasNoWarningAsError = (Bits >> 5) & 1;
     Result.HasNoErrorAsFatal = (Bits >> 4) & 1;
     Result.WasUpgradedFromWarning = (Bits >> 3) & 1;
     Result.Severity = Bits & 0x7;
     return Result;
   }
+
+  bool operator==(DiagnosticMapping Other) const {
+    return serialize() == Other.serialize();
+  }
 };
 
 /// 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:
   /// The level of the diagnostic, after it has been through mapping.
public:
   /// This only works on builtin diagnostics, not custom ones, and is not
   /// legal to call on NOTEs.
   static bool isBuiltinWarningOrExtension(unsigned DiagID);
 
   /// Return true if the specified diagnostic is mapped to errors by
   /// default.
   static bool isDefaultMappingAsError(unsigned DiagID);
 
+  /// Get the default mapping for this diagnostic.
+  static DiagnosticMapping getDefaultMapping(unsigned DiagID);
+
   /// Determine whether the given built-in diagnostic ID is a Note.
   static bool isBuiltinNote(unsigned DiagID);
 
   /// Determine whether the given built-in diagnostic ID is for an
   /// extension of some sort.
   static bool isBuiltinExtensionDiag(unsigned DiagID) {
     bool ignored;
     return isBuiltinExtensionDiag(DiagID, ignored);

clang/lib/Basic/Diagnostic.cpp

 }
 
 void DiagnosticsEngine::ReportDelayed() {
   unsigned ID = DelayedDiagID;
   DelayedDiagID = 0;
   Report(ID) << DelayedDiagArg1 << DelayedDiagArg2 << DelayedDiagArg3;
 }
 
+DiagnosticMapping &
+DiagnosticsEngine::DiagState::getOrAddMapping(diag::kind Diag) {
+  std::pair<iterator, bool> Result =
+      DiagMap.insert(std::make_pair(Diag, DiagnosticMapping()));
+
+  // Initialize the entry if we added it.
+  if (Result.second)
+    Result.first->second = DiagnosticIDs::getDefaultMapping(Diag);
+
+  return Result.first->second;
+}
+
 void DiagnosticsEngine::DiagStateMap::appendFirst(DiagState *State) {
   assert(Files.empty() && "not first");
   FirstDiagState = CurDiagState = State;
   CurDiagStateLoc = SourceLocation();
 }
 
 void DiagnosticsEngine::DiagStateMap::append(SourceManager &SrcMgr,
                                              SourceLocation Loc,

clang/lib/Basic/DiagnosticIDs.cpp

#undef CATEGORY
   // If the diag id doesn't match we found a different diag, abort. This can
   // happen when this function is called with an ID that points into a hole in
   // the diagID space.
   if (Found->DiagID != DiagID)
     return nullptr;
   return Found;
 }
 
-static DiagnosticMapping GetDefaultDiagMapping(unsigned DiagID) {
+DiagnosticMapping DiagnosticIDs::getDefaultMapping(unsigned DiagID) {
   DiagnosticMapping Info = DiagnosticMapping::Make(
       diag::Severity::Fatal, /*IsUser=*/false, /*IsPragma=*/false);
 
   if (const StaticDiagInfoRec *StaticInfo = GetDiagInfo(DiagID)) {
     Info.setSeverity((diag::Severity)StaticInfo->DefaultSeverity);
 
     if (StaticInfo->WarnNoWerror) {
       assert(Info.getSeverity() == diag::Severity::Warning &&
struct StaticDiagCategoryRec {
     uint8_t NameLen;
 
     StringRef getName() const {
       return StringRef(NameStr, NameLen);
     }
   };
 }
 
-// Unfortunately, the split between DiagnosticIDs and Diagnostic is not
-// particularly clean, but for now we just implement this method here so we can
-// access GetDefaultDiagMapping.
-DiagnosticMapping &
-DiagnosticsEngine::DiagState::getOrAddMapping(diag::kind Diag) {
-  std::pair<iterator, bool> Result =
-      DiagMap.insert(std::make_pair(Diag, DiagnosticMapping()));
-
-  // Initialize the entry if we added it.
-  if (Result.second)
-    Result.first->second = GetDefaultDiagMapping(Diag);
-
-  return Result.first->second;
-}
-
 static const StaticDiagCategoryRec CategoryNameTable[] = {
 #define GET_CATEGORY_TABLE
 #define CATEGORY(X, ENUM) { X, STR_SIZE(X, uint8_t) },
 #include "clang/Basic/DiagnosticGroups.inc"
 #undef GET_CATEGORY_TABLE
   { nullptr, 0 }
 };
 

 ///
 bool DiagnosticIDs::isBuiltinExtensionDiag(unsigned DiagID,
                                         bool &EnabledByDefault) {
   if (DiagID >= diag::DIAG_UPPER_LIMIT ||
       getBuiltinDiagClass(DiagID) != CLASS_EXTENSION)
     return false;
 
   EnabledByDefault =
-      GetDefaultDiagMapping(DiagID).getSeverity() != diag::Severity::Ignored;
+      getDefaultMapping(DiagID).getSeverity() != diag::Severity::Ignored;
   return true;
 }
 
 bool DiagnosticIDs::isDefaultMappingAsError(unsigned DiagID) {
   if (DiagID >= diag::DIAG_UPPER_LIMIT)
     return false;
 
-  return GetDefaultDiagMapping(DiagID).getSeverity() >= diag::Severity::Error;
+  return getDefaultMapping(DiagID).getSeverity() >= diag::Severity::Error;
 }
 
 /// getDescription - Given a diagnostic ID, return a description of the
 /// issue.
 StringRef DiagnosticIDs::getDescription(unsigned DiagID) const {
   if (const StaticDiagInfoRec *Info = GetDiagInfo(DiagID))
     return Info->getDescription();
   assert(CustomDiagInfo && "Invalid CustomDiagInfo");

clang/lib/Serialization/ASTWriter.cpp

void ASTWriter::WritePragmaDiagnosticMappings(const DiagnosticsEngine &Diag,
 
   auto AddDiagState = [&](const DiagnosticsEngine::DiagState *State,
                           bool IncludeNonPragmaStates) {
     // Ensure that the diagnostic state wasn't modified since it was created.
     // We will not correctly round-trip this information otherwise.
     assert(Flags == EncodeDiagStateFlags(State) &&
            "diag state flags vary in single AST file");
 
+    // If we ever serialize non-pragma mappings outside the initial state, the
+    // code below will need to consider more than getDefaultMapping.
+    assert(!IncludeNonPragmaStates ||
+           State == Diag.DiagStatesByLoc.FirstDiagState);
+
     unsigned &DiagStateID = DiagStateIDMap[State];
     Record.push_back(DiagStateID);
 
     if (DiagStateID == 0) {
       DiagStateID = ++CurrID;
+      SmallVector<std::pair<unsigned, DiagnosticMapping>> Mappings;
 
       // 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(I.second.serialize());
-        }
-      }
+        // Maybe skip non-pragmas.
+        if (!I.second.isPragma() && !IncludeNonPragmaStates)
+          continue;
+        // Skip default mappings. We have a mapping for every diagnostic ever
+        // emitted, regardless of whether it was customized.
+        if (!I.second.isPragma() &&
+            I.second == DiagnosticIDs::getDefaultMapping(I.first))
+          continue;
+        Mappings.push_back(I);
+      }
+
+      // Sort by diag::kind for deterministic output.
+      llvm::sort(Mappings, [](const auto &LHS, const auto &RHS) {
+        return LHS.first < RHS.first;
+      });
+
+      for (const auto &I : Mappings) {
       // Update the placeholder.
       Record[SizeIdx] = (Record.size() - SizeIdx) / 2;
     }
   };
 
   AddDiagState(Diag.DiagStatesByLoc.FirstDiagState, isModule);
 
   // Reserve a spot for the number of locations with state transitions.

clang/test/Modules/diag-mappings.c

+// Test that diagnostic mappings are emitted only when needed and in order of
+// diagnostic ID rather than non-deterministically. This test passes 3
+// -W options and expects exactly 3 mappings to be emitted in the pcm. The -W
+// options are chosen to be far apart in ID (see DiagnosticIDs.h) so we can
+// check they are ordered. We also intentionally trigger several other warnings
+// inside the module and ensure they do not show up in the pcm as mappings.
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps \
+// RUN:   -fmodules-cache-path=%t/cache -triple x86_64-apple-macosx10.11.0 \
+// RUN:   %t/main.m -fdisable-module-hash \
+// RUN:   -Werror=stack-protector -Werror=empty-translation-unit -Werror=float-equal
+
+// RUN: mv %t/cache/A.pcm %t/A1.pcm
+
+// RUN: llvm-bcanalyzer --dump --disable-histogram %t/A1.pcm | FileCheck %s
+
+// CHECK: <DIAG_PRAGMA_MAPPINGS
+
+// == Initial mappings
+// Number of mappings = 3
+// CHECK-SAME: op2=3
+// Common diag id is < 1000 (see DiagnosticIDs.h)
+// CHECK-SAME: op3=[[STACK_PROT:[0-9][0-9]?[0-9]?]] op4=
+// Parse diag id is somewhere in 1000..2999, leaving room for changes
+// CHECK-SAME: op5=[[EMPTY_TU:[12][0-9][0-9][0-9]]] op6=
+// Sema diag id is > 2000
+// CHECK-SAME: op7=[[FLOAT_EQ:[2-9][0-9][0-9][0-9]]] op8=
+
+// == Pragmas:
+// Each pragma creates a mapping table; and each copies the previous table. The
+// initial mappings are copied as well, but are not serialized since they have
+// isPragma=false.
+
+// == ignored "-Wfloat-equal"
+// CHECK-SAME: op{{[0-9]+}}=1
+// CHECK-SAME: op{{[0-9]+}}=[[FLOAT_EQ]] op{{[0-9]+}}=
+
+// == ignored "-Wstack-protector"
+// CHECK-SAME: op{{[0-9]+}}=2
+// CHECK-SAME: op{{[0-9]+}}=[[STACK_PROT]] op{{[0-9]+}}=
+// CHECK-SAME: op{{[0-9]+}}=[[FLOAT_EQ]] op{{[0-9]+}}=
+
+// == warning "-Wempty-translation-unit"
+// CHECK-SAME: op{{[0-9]+}}=3
+// CHECK-SAME: op{{[0-9]+}}=[[STACK_PROT]] op{{[0-9]+}}=
+// CHECK-SAME: op{{[0-9]+}}=[[EMPTY_TU]] op{{[0-9]+}}=
+// CHECK-SAME: op{{[0-9]+}}=[[FLOAT_EQ]] op{{[0-9]+}}=
+
+// == warning "-Wstack-protector"
+// CHECK-SAME: op{{[0-9]+}}=3
+// CHECK-SAME: op{{[0-9]+}}=[[STACK_PROT]] op{{[0-9]+}}=
+// CHECK-SAME: op{{[0-9]+}}=[[EMPTY_TU]] op{{[0-9]+}}=
+// CHECK-SAME: op{{[0-9]+}}=[[FLOAT_EQ]] op{{[0-9]+}}=
+
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps \
+// RUN:   -fmodules-cache-path=%t/cache -triple x86_64-apple-macosx10.11.0 \
+// RUN:   %t/main.m -fdisable-module-hash \
+// RUN:   -Werror=stack-protector -Werror=empty-translation-unit -Werror=float-equal
+
+// RUN: diff %t/cache/A.pcm %t/A1.pcm
+
+//--- module.modulemap
+module A { header "a.h" }
+
+//--- a.h
+// Lex warning
+#warning "w"
+
+static inline void f() {
+// Parse warning
+  ;
+}
+
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wfloat-equal"
+#pragma clang diagnostic ignored "-Wstack-protector"
+
+static inline void g() {
+// Sema warning
+  int x;
+}
+
+#pragma clang diagnostic push
+#pragma clang diagnostic warning "-Wempty-translation-unit"
+#pragma clang diagnostic warning "-Wstack-protector"
+
+#pragma clang diagnostic pop
+#pragma clang diagnostic pop
+
+//--- main.m
+#import "a.h"