Skip to content

Commit

Permalink
[clang][modules] Avoid serializing all diag mappings in non-determini…
Browse files Browse the repository at this point in the history
…stic 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
  • Loading branch information
benlangmuir committed Jun 29, 2023
1 parent 92fbb60 commit 1ede7b4
Show file tree
Hide file tree
Showing 5 changed files with 141 additions and 22 deletions.
7 changes: 7 additions & 0 deletions clang/include/clang/Basic/DiagnosticIDs.h
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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);

Expand Down
12 changes: 12 additions & 0 deletions clang/lib/Basic/Diagnostic.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
21 changes: 3 additions & 18 deletions clang/lib/Basic/DiagnosticIDs.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down Expand Up @@ -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) },
Expand Down Expand Up @@ -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
Expand Down
29 changes: 25 additions & 4 deletions clang/lib/Serialization/ASTWriter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
94 changes: 94 additions & 0 deletions clang/test/Modules/diag-mappings.c
Original file line number Diff line number Diff line change
@@ -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"

0 comments on commit 1ede7b4

Please sign in to comment.