[clang] 1ede7b4 - [clang][modules] Avoid serializing all diag mappings in non-deterministic order
Ben Langmuir via cfe-commits
cfe-commits at lists.llvm.org
Thu Jun 29 13:17:42 PDT 2023
Author: Ben Langmuir
Date: 2023-06-29T13:17:24-07:00
New Revision: 1ede7b47493f8d8548202e5f4e4a150c11e070fd
URL: https://github.com/llvm/llvm-project/commit/1ede7b47493f8d8548202e5f4e4a150c11e070fd
DIFF: https://github.com/llvm/llvm-project/commit/1ede7b47493f8d8548202e5f4e4a150c11e070fd.diff
LOG: [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
Added:
clang/test/Modules/diag-mappings.c
Modified:
clang/include/clang/Basic/DiagnosticIDs.h
clang/lib/Basic/Diagnostic.cpp
clang/lib/Basic/DiagnosticIDs.cpp
clang/lib/Serialization/ASTWriter.cpp
Removed:
################################################################################
diff --git a/clang/include/clang/Basic/DiagnosticIDs.h b/clang/include/clang/Basic/DiagnosticIDs.h
index a59ff25fe20ebb..bf4995175ef196 100644
--- a/clang/include/clang/Basic/DiagnosticIDs.h
+++ b/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);
diff --git a/clang/lib/Basic/Diagnostic.cpp b/clang/lib/Basic/Diagnostic.cpp
index b2106b0eb66118..7a54d27ef9d8ee 100644
--- a/clang/lib/Basic/Diagnostic.cpp
+++ b/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;
diff --git a/clang/lib/Basic/DiagnosticIDs.cpp b/clang/lib/Basic/DiagnosticIDs.cpp
index ac08e98a278db7..e5667d57f8cff1 100644
--- a/clang/lib/Basic/DiagnosticIDs.cpp
+++ b/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,7 +434,7 @@ bool DiagnosticIDs::isBuiltinExtensionDiag(unsigned DiagID,
return false;
EnabledByDefault =
- GetDefaultDiagMapping(DiagID).getSeverity() != diag::Severity::Ignored;
+ getDefaultMapping(DiagID).getSeverity() != diag::Severity::Ignored;
return true;
}
@@ -457,7 +442,7 @@ 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
diff --git a/clang/lib/Serialization/ASTWriter.cpp b/clang/lib/Serialization/ASTWriter.cpp
index ca0db085d94cc9..2f73eb526641a5 100644
--- a/clang/lib/Serialization/ASTWriter.cpp
+++ b/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;
diff --git a/clang/test/Modules/diag-mappings.c b/clang/test/Modules/diag-mappings.c
new file mode 100644
index 00000000000000..ddc8380a862e9b
--- /dev/null
+++ b/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"
More information about the cfe-commits
mailing list