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

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

Differential Revision: https://reviews.llvm.org/D154016

Author: benlangmuir

clang/include/clang/Basic/DiagnosticIDs.h

@@ -159,6 +159,10 @@ class DiagnosticMapping {
     Result.Severity = Bits & 0x7;
     return Result;
   }
+
+  bool operator==(DiagnosticMapping Other) const {
+    return serialize() == Other.serialize();
+  }
 };
 
 /// Used for handling and querying diagnostic IDs.
@@ -208,6 +212,9 @@ class DiagnosticIDs : public RefCountedBase<DiagnosticIDs> {
   /// 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);
 

clang/lib/Basic/Diagnostic.cpp

@@ -160,6 +160,18 @@ void DiagnosticsEngine::ReportDelayed() {
   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;

clang/lib/Basic/DiagnosticIDs.cpp

@@ -256,7 +256,7 @@ CATEGORY(REFACTORING, ANALYSIS)
   return Found;
 }
 
-static DiagnosticMapping GetDefaultDiagMapping(unsigned DiagID) {
+DiagnosticMapping DiagnosticIDs::getDefaultMapping(unsigned DiagID) {
   DiagnosticMapping Info = DiagnosticMapping::Make(
       diag::Severity::Fatal, /*IsUser=*/false, /*IsPragma=*/false);
 
@@ -293,21 +293,6 @@ namespace {
   };
 }
 
-// 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) },
@@ -449,15 +434,15 @@ bool DiagnosticIDs::isBuiltinExtensionDiag(unsigned DiagID,
     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

clang/lib/Serialization/ASTWriter.cpp

@@ -2997,20 +2997,41 @@ void ASTWriter::WritePragmaDiagnosticMappings(const DiagnosticsEngine &Diag,
     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) {
+        Record.push_back(I.first);
+        Record.push_back(I.second.serialize());
       }
       // Update the placeholder.
       Record[SizeIdx] = (Record.size() - SizeIdx) / 2;

clang/test/Modules/diag-mappings.c

@@ -0,0 +1,94 @@
+// 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"