Skip to content

Commit 9f16859

Browse files
cyndyishidacyndyishida
authored
[InstallAPI] Simplify & improve symbol printing for diagnostics (llvm#85894)
* Defer mangling of symbols until an error is ready to report * Pass around fewer parameters when reporting
1 parent f872043 commit 9f16859

File tree

4 files changed

+73
-71
lines changed

4 files changed

+73
-71
lines changed

‎clang/include/clang/InstallAPI/DylibVerifier.h

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -128,6 +128,10 @@ class DylibVerifier {
128128
/// Find matching dylib slice for target triple that is being parsed.
129129
void assignSlice(const Target &T);
130130

131+
/// Gather annotations for symbol for error reporting.
132+
std::string getAnnotatedName(const Record *R, SymbolContext &SymCtx,
133+
bool ValidSourceLoc = true);
134+
131135
// Symbols in dylib.
132136
llvm::MachO::Records Dylib;
133137

‎clang/include/clang/InstallAPI/MachO.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,11 +26,13 @@
2626
using SymbolFlags = llvm::MachO::SymbolFlags;
2727
using RecordLinkage = llvm::MachO::RecordLinkage;
2828
using Record = llvm::MachO::Record;
29+
using EncodeKind = llvm::MachO::EncodeKind;
2930
using GlobalRecord = llvm::MachO::GlobalRecord;
3031
using ObjCContainerRecord = llvm::MachO::ObjCContainerRecord;
3132
using ObjCInterfaceRecord = llvm::MachO::ObjCInterfaceRecord;
3233
using ObjCCategoryRecord = llvm::MachO::ObjCCategoryRecord;
3334
using ObjCIVarRecord = llvm::MachO::ObjCIVarRecord;
35+
using ObjCIFSymbolKind = llvm::MachO::ObjCIFSymbolKind;
3436
using Records = llvm::MachO::Records;
3537
using RecordsSlice = llvm::MachO::RecordsSlice;
3638
using BinaryAttrs = llvm::MachO::RecordsSlice::BinaryAttrs;

‎clang/lib/InstallAPI/DylibVerifier.cpp

Lines changed: 65 additions & 69 deletions
Original file line numberDiff line numberDiff line change
@@ -10,9 +10,6 @@ namespace installapi {
1010

1111
/// Metadata stored about a mapping of a declaration to a symbol.
1212
struct DylibVerifier::SymbolContext {
13-
// Name to use for printing in diagnostics.
14-
std::string PrettyPrintName{""};
15-
1613
// Name to use for all querying and verification
1714
// purposes.
1815
std::string SymbolName{""};
@@ -30,11 +27,35 @@ struct DylibVerifier::SymbolContext {
3027
bool Inlined = false;
3128
};
3229

33-
static std::string
34-
getAnnotatedName(const Record *R, EncodeKind Kind, StringRef Name,
35-
bool ValidSourceLoc = true,
36-
ObjCIFSymbolKind ObjCIF = ObjCIFSymbolKind::None) {
37-
assert(!Name.empty() && "Need symbol name for printing");
30+
static bool isCppMangled(StringRef Name) {
31+
// InstallAPI currently only supports itanium manglings.
32+
return (Name.starts_with("_Z") || Name.starts_with("__Z") ||
33+
Name.starts_with("___Z"));
34+
}
35+
36+
static std::string demangle(StringRef Name) {
37+
// InstallAPI currently only supports itanium manglings.
38+
if (!isCppMangled(Name))
39+
return Name.str();
40+
char *Result = llvm::itaniumDemangle(Name);
41+
if (!Result)
42+
return Name.str();
43+
44+
std::string Demangled(Result);
45+
free(Result);
46+
return Demangled;
47+
}
48+
49+
std::string DylibVerifier::getAnnotatedName(const Record *R,
50+
SymbolContext &SymCtx,
51+
bool ValidSourceLoc) {
52+
assert(!SymCtx.SymbolName.empty() && "Expected symbol name");
53+
54+
const StringRef SymbolName = SymCtx.SymbolName;
55+
std::string PrettyName =
56+
(Demangle && (SymCtx.Kind == EncodeKind::GlobalSymbol))
57+
? demangle(SymbolName)
58+
: SymbolName.str();
3859

3960
std::string Annotation;
4061
if (R->isWeakDefined())
@@ -45,58 +66,47 @@ getAnnotatedName(const Record *R, EncodeKind Kind, StringRef Name,
4566
Annotation += "(tlv) ";
4667

4768
// Check if symbol represents only part of a @interface declaration.
48-
const bool IsAnnotatedObjCClass = ((ObjCIF != ObjCIFSymbolKind::None) &&
49-
(ObjCIF <= ObjCIFSymbolKind::EHType));
69+
const bool IsAnnotatedObjCClass =
70+
((SymCtx.ObjCIFKind != ObjCIFSymbolKind::None) &&
71+
(SymCtx.ObjCIFKind <= ObjCIFSymbolKind::EHType));
5072

5173
if (IsAnnotatedObjCClass) {
52-
if (ObjCIF == ObjCIFSymbolKind::EHType)
74+
if (SymCtx.ObjCIFKind == ObjCIFSymbolKind::EHType)
5375
Annotation += "Exception Type of ";
54-
if (ObjCIF == ObjCIFSymbolKind::MetaClass)
76+
if (SymCtx.ObjCIFKind == ObjCIFSymbolKind::MetaClass)
5577
Annotation += "Metaclass of ";
56-
if (ObjCIF == ObjCIFSymbolKind::Class)
78+
if (SymCtx.ObjCIFKind == ObjCIFSymbolKind::Class)
5779
Annotation += "Class of ";
5880
}
5981

6082
// Only print symbol type prefix or leading "_" if there is no source location
6183
// tied to it. This can only ever happen when the location has to come from
6284
// debug info.
6385
if (ValidSourceLoc) {
64-
if ((Kind == EncodeKind::GlobalSymbol) && Name.starts_with("_"))
65-
return Annotation + Name.drop_front(1).str();
66-
return Annotation + Name.str();
86+
StringRef PrettyNameRef(PrettyName);
87+
if ((SymCtx.Kind == EncodeKind::GlobalSymbol) &&
88+
!isCppMangled(SymbolName) && PrettyNameRef.starts_with("_"))
89+
return Annotation + PrettyNameRef.drop_front(1).str();
90+
return Annotation + PrettyName;
6791
}
6892

6993
if (IsAnnotatedObjCClass)
70-
return Annotation + Name.str();
94+
return Annotation + PrettyName;
7195

72-
switch (Kind) {
96+
switch (SymCtx.Kind) {
7397
case EncodeKind::GlobalSymbol:
74-
return Annotation + Name.str();
98+
return Annotation + PrettyName;
7599
case EncodeKind::ObjectiveCInstanceVariable:
76-
return Annotation + "(ObjC IVar) " + Name.str();
100+
return Annotation + "(ObjC IVar) " + PrettyName;
77101
case EncodeKind::ObjectiveCClass:
78-
return Annotation + "(ObjC Class) " + Name.str();
102+
return Annotation + "(ObjC Class) " + PrettyName;
79103
case EncodeKind::ObjectiveCClassEHType:
80-
return Annotation + "(ObjC Class EH) " + Name.str();
104+
return Annotation + "(ObjC Class EH) " + PrettyName;
81105
}
82106

83107
llvm_unreachable("unexpected case for EncodeKind");
84108
}
85109

86-
static std::string demangle(StringRef Name) {
87-
// InstallAPI currently only supports itanium manglings.
88-
if (!(Name.starts_with("_Z") || Name.starts_with("__Z") ||
89-
Name.starts_with("___Z")))
90-
return Name.str();
91-
char *Result = llvm::itaniumDemangle(Name);
92-
if (!Result)
93-
return Name.str();
94-
95-
std::string Demangled(Result);
96-
free(Result);
97-
return Demangled;
98-
}
99-
100110
static DylibVerifier::Result updateResult(const DylibVerifier::Result Prev,
101111
const DylibVerifier::Result Curr) {
102112
if (Prev == Curr)
@@ -193,19 +203,18 @@ bool DylibVerifier::compareObjCInterfaceSymbols(const Record *R,
193203
// The decl represents a complete ObjCInterface, but the symbols in the
194204
// dylib do not. Determine which symbol is missing. To keep older projects
195205
// building, treat this as a warning.
196-
if (!DR->isExportedSymbol(ObjCIFSymbolKind::Class))
206+
if (!DR->isExportedSymbol(ObjCIFSymbolKind::Class)) {
207+
SymCtx.ObjCIFKind = ObjCIFSymbolKind::Class;
197208
PrintDiagnostic(DR->getLinkageForSymbol(ObjCIFSymbolKind::Class), R,
198-
getAnnotatedName(R, SymCtx.Kind, SymCtx.PrettyPrintName,
199-
/*ValidSourceLoc=*/true,
200-
ObjCIFSymbolKind::Class),
209+
getAnnotatedName(R, SymCtx),
201210
/*PrintAsWarning=*/true);
202-
203-
if (!DR->isExportedSymbol(ObjCIFSymbolKind::MetaClass))
211+
}
212+
if (!DR->isExportedSymbol(ObjCIFSymbolKind::MetaClass)) {
213+
SymCtx.ObjCIFKind = ObjCIFSymbolKind::MetaClass;
204214
PrintDiagnostic(DR->getLinkageForSymbol(ObjCIFSymbolKind::MetaClass), R,
205-
getAnnotatedName(R, SymCtx.Kind, SymCtx.PrettyPrintName,
206-
/*ValidSourceLoc=*/true,
207-
ObjCIFSymbolKind::MetaClass),
215+
getAnnotatedName(R, SymCtx),
208216
/*PrintAsWarning=*/true);
217+
}
209218
return true;
210219
}
211220

@@ -221,7 +230,7 @@ bool DylibVerifier::compareObjCInterfaceSymbols(const Record *R,
221230
// At this point that means there was not a matching class symbol
222231
// to represent the one discovered as a declaration.
223232
PrintDiagnostic(DR->getLinkageForSymbol(SymCtx.ObjCIFKind), R,
224-
SymCtx.PrettyPrintName);
233+
SymCtx.SymbolName);
225234
return false;
226235
}
227236

@@ -234,15 +243,15 @@ DylibVerifier::Result DylibVerifier::compareVisibility(const Record *R,
234243
Ctx.emitDiag([&]() {
235244
Ctx.Diag->Report(SymCtx.FA->D->getLocation(),
236245
diag::err_library_missing_symbol)
237-
<< SymCtx.PrettyPrintName;
246+
<< getAnnotatedName(R, SymCtx);
238247
});
239248
return Result::Invalid;
240249
}
241250
if (DR->isInternal()) {
242251
Ctx.emitDiag([&]() {
243252
Ctx.Diag->Report(SymCtx.FA->D->getLocation(),
244253
diag::err_library_hidden_symbol)
245-
<< SymCtx.PrettyPrintName;
254+
<< getAnnotatedName(R, SymCtx);
246255
});
247256
return Result::Invalid;
248257
}
@@ -269,7 +278,7 @@ DylibVerifier::Result DylibVerifier::compareVisibility(const Record *R,
269278
}
270279
Ctx.emitDiag([&]() {
271280
Ctx.Diag->Report(SymCtx.FA->D->getLocation(), ID)
272-
<< SymCtx.PrettyPrintName;
281+
<< getAnnotatedName(R, SymCtx);
273282
});
274283
return Outcome;
275284
}
@@ -293,14 +302,14 @@ DylibVerifier::Result DylibVerifier::compareAvailability(const Record *R,
293302
Ctx.emitDiag([&]() {
294303
Ctx.Diag->Report(SymCtx.FA->D->getLocation(),
295304
diag::warn_header_availability_mismatch)
296-
<< SymCtx.PrettyPrintName << IsDeclAvailable << IsDeclAvailable;
305+
<< getAnnotatedName(R, SymCtx) << IsDeclAvailable << IsDeclAvailable;
297306
});
298307
return Result::Ignore;
299308
case VerificationMode::Pedantic:
300309
Ctx.emitDiag([&]() {
301310
Ctx.Diag->Report(SymCtx.FA->D->getLocation(),
302311
diag::err_header_availability_mismatch)
303-
<< SymCtx.PrettyPrintName << IsDeclAvailable << IsDeclAvailable;
312+
<< getAnnotatedName(R, SymCtx) << IsDeclAvailable << IsDeclAvailable;
304313
});
305314
return Result::Invalid;
306315
case VerificationMode::ErrorsOnly:
@@ -313,23 +322,19 @@ DylibVerifier::Result DylibVerifier::compareAvailability(const Record *R,
313322

314323
bool DylibVerifier::compareSymbolFlags(const Record *R, SymbolContext &SymCtx,
315324
const Record *DR) {
316-
std::string DisplayName =
317-
Demangle ? demangle(DR->getName()) : DR->getName().str();
318-
319325
if (DR->isThreadLocalValue() && !R->isThreadLocalValue()) {
320326
Ctx.emitDiag([&]() {
321327
Ctx.Diag->Report(SymCtx.FA->D->getLocation(),
322328
diag::err_dylib_symbol_flags_mismatch)
323-
<< getAnnotatedName(DR, SymCtx.Kind, DisplayName)
324-
<< DR->isThreadLocalValue();
329+
<< getAnnotatedName(DR, SymCtx) << DR->isThreadLocalValue();
325330
});
326331
return false;
327332
}
328333
if (!DR->isThreadLocalValue() && R->isThreadLocalValue()) {
329334
Ctx.emitDiag([&]() {
330335
SymCtx.FA->D->getLocation(),
331336
Ctx.Diag->Report(diag::err_header_symbol_flags_mismatch)
332-
<< SymCtx.PrettyPrintName << R->isThreadLocalValue();
337+
<< getAnnotatedName(DR, SymCtx) << R->isThreadLocalValue();
333338
});
334339
return false;
335340
}
@@ -338,16 +343,15 @@ bool DylibVerifier::compareSymbolFlags(const Record *R, SymbolContext &SymCtx,
338343
Ctx.emitDiag([&]() {
339344
Ctx.Diag->Report(SymCtx.FA->D->getLocation(),
340345
diag::err_dylib_symbol_flags_mismatch)
341-
<< getAnnotatedName(DR, SymCtx.Kind, DisplayName)
342-
<< R->isWeakDefined();
346+
<< getAnnotatedName(DR, SymCtx) << R->isWeakDefined();
343347
});
344348
return false;
345349
}
346350
if (!DR->isWeakDefined() && R->isWeakDefined()) {
347351
Ctx.emitDiag([&]() {
348352
Ctx.Diag->Report(SymCtx.FA->D->getLocation(),
349353
diag::err_header_symbol_flags_mismatch)
350-
<< SymCtx.PrettyPrintName << R->isWeakDefined();
354+
<< getAnnotatedName(R, SymCtx) << R->isWeakDefined();
351355
});
352356
return false;
353357
}
@@ -457,10 +461,7 @@ DylibVerifier::Result DylibVerifier::verify(ObjCIVarRecord *R,
457461

458462
std::string FullName =
459463
ObjCIVarRecord::createScopedName(SuperClass, R->getName());
460-
SymbolContext SymCtx{
461-
getAnnotatedName(R, EncodeKind::ObjectiveCInstanceVariable,
462-
Demangle ? demangle(FullName) : FullName),
463-
FullName, EncodeKind::ObjectiveCInstanceVariable, FA};
464+
SymbolContext SymCtx{FullName, EncodeKind::ObjectiveCInstanceVariable, FA};
464465
return verifyImpl(R, SymCtx);
465466
}
466467

@@ -485,11 +486,8 @@ DylibVerifier::Result DylibVerifier::verify(ObjCInterfaceRecord *R,
485486
SymCtx.SymbolName = R->getName();
486487
SymCtx.ObjCIFKind = assignObjCIFSymbolKind(R);
487488

488-
std::string DisplayName =
489-
Demangle ? demangle(SymCtx.SymbolName) : SymCtx.SymbolName;
490489
SymCtx.Kind = R->hasExceptionAttribute() ? EncodeKind::ObjectiveCClassEHType
491490
: EncodeKind::ObjectiveCClass;
492-
SymCtx.PrettyPrintName = getAnnotatedName(R, SymCtx.Kind, DisplayName);
493491
SymCtx.FA = FA;
494492

495493
return verifyImpl(R, SymCtx);
@@ -504,8 +502,6 @@ DylibVerifier::Result DylibVerifier::verify(GlobalRecord *R,
504502
SimpleSymbol Sym = parseSymbol(R->getName());
505503
SymbolContext SymCtx;
506504
SymCtx.SymbolName = Sym.Name;
507-
SymCtx.PrettyPrintName =
508-
getAnnotatedName(R, Sym.Kind, Demangle ? demangle(Sym.Name) : Sym.Name);
509505
SymCtx.Kind = Sym.Kind;
510506
SymCtx.FA = FA;
511507
SymCtx.Inlined = R->isInlined();

‎clang/test/InstallAPI/diagnostics-cpp.test

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,8 +12,8 @@
1212
// RUN: --verify-mode=Pedantic -o %t/output.tbd --demangle 2> %t/errors.log
1313
// RUN: FileCheck -input-file %t/errors.log %s
1414

15-
CHECK: warning: violations found for arm64-apple-macos13
16-
CHECK: CPP.h:5:7: error: declaration has external linkage, but symbol has internal linkage in dynamic library 'vtable for Bar'
15+
CHECK: warning: violations found for arm64-apple-macos13
16+
CHECK: CPP.h:5:7: error: declaration has external linkage, but symbol has internal linkage in dynamic library 'vtable for Bar'
1717
CHECK-NEXT: class Bar : Foo {
1818
CHECK-NEXT: ^
1919
CHECK-NEXT: CPP.h:5:7: error: declaration has external linkage, but symbol has internal linkage in dynamic library 'typeinfo for Bar'

0 commit comments

Comments
 (0)