[clang] [InstallAPI] Simplify & improve symbol printing for diagnostics (PR #85894)
via cfe-commits
cfe-commits at lists.llvm.org
Tue Mar 19 21:47:47 PDT 2024
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang
Author: Cyndy Ishida (cyndyishida)
<details>
<summary>Changes</summary>
* Defer mangling of symbols until an error is ready to report
* Pass around fewer parameters when reporting
---
Full diff: https://github.com/llvm/llvm-project/pull/85894.diff
4 Files Affected:
- (modified) clang/include/clang/InstallAPI/DylibVerifier.h (+4)
- (modified) clang/include/clang/InstallAPI/MachO.h (+2)
- (modified) clang/lib/InstallAPI/DylibVerifier.cpp (+65-69)
- (modified) clang/test/InstallAPI/diagnostics-cpp.test (+2-2)
``````````diff
diff --git a/clang/include/clang/InstallAPI/DylibVerifier.h b/clang/include/clang/InstallAPI/DylibVerifier.h
index 8269715c7f2345..bbfa8711313e47 100644
--- a/clang/include/clang/InstallAPI/DylibVerifier.h
+++ b/clang/include/clang/InstallAPI/DylibVerifier.h
@@ -128,6 +128,10 @@ class DylibVerifier {
/// Find matching dylib slice for target triple that is being parsed.
void assignSlice(const Target &T);
+ /// Gather annotations for symbol for error reporting.
+ std::string getAnnotatedName(const Record *R, SymbolContext &SymCtx,
+ bool ValidSourceLoc = true);
+
// Symbols in dylib.
llvm::MachO::Records Dylib;
diff --git a/clang/include/clang/InstallAPI/MachO.h b/clang/include/clang/InstallAPI/MachO.h
index a77766511fa3e5..f0dea8bbd24ccd 100644
--- a/clang/include/clang/InstallAPI/MachO.h
+++ b/clang/include/clang/InstallAPI/MachO.h
@@ -26,11 +26,13 @@
using SymbolFlags = llvm::MachO::SymbolFlags;
using RecordLinkage = llvm::MachO::RecordLinkage;
using Record = llvm::MachO::Record;
+using EncodeKind = llvm::MachO::EncodeKind;
using GlobalRecord = llvm::MachO::GlobalRecord;
using ObjCContainerRecord = llvm::MachO::ObjCContainerRecord;
using ObjCInterfaceRecord = llvm::MachO::ObjCInterfaceRecord;
using ObjCCategoryRecord = llvm::MachO::ObjCCategoryRecord;
using ObjCIVarRecord = llvm::MachO::ObjCIVarRecord;
+using ObjCIFSymbolKind = llvm::MachO::ObjCIFSymbolKind;
using Records = llvm::MachO::Records;
using RecordsSlice = llvm::MachO::RecordsSlice;
using BinaryAttrs = llvm::MachO::RecordsSlice::BinaryAttrs;
diff --git a/clang/lib/InstallAPI/DylibVerifier.cpp b/clang/lib/InstallAPI/DylibVerifier.cpp
index 700763b3fee0db..24e0d0addf2f46 100644
--- a/clang/lib/InstallAPI/DylibVerifier.cpp
+++ b/clang/lib/InstallAPI/DylibVerifier.cpp
@@ -10,9 +10,6 @@ namespace installapi {
/// Metadata stored about a mapping of a declaration to a symbol.
struct DylibVerifier::SymbolContext {
- // Name to use for printing in diagnostics.
- std::string PrettyPrintName{""};
-
// Name to use for all querying and verification
// purposes.
std::string SymbolName{""};
@@ -30,11 +27,35 @@ struct DylibVerifier::SymbolContext {
bool Inlined = false;
};
-static std::string
-getAnnotatedName(const Record *R, EncodeKind Kind, StringRef Name,
- bool ValidSourceLoc = true,
- ObjCIFSymbolKind ObjCIF = ObjCIFSymbolKind::None) {
- assert(!Name.empty() && "Need symbol name for printing");
+static bool isCppMangled(StringRef Name) {
+ // InstallAPI currently only supports itanium manglings.
+ return (Name.starts_with("_Z") || Name.starts_with("__Z") ||
+ Name.starts_with("___Z"));
+}
+
+static std::string demangle(StringRef Name) {
+ // InstallAPI currently only supports itanium manglings.
+ if (!isCppMangled(Name))
+ return Name.str();
+ char *Result = llvm::itaniumDemangle(Name);
+ if (!Result)
+ return Name.str();
+
+ std::string Demangled(Result);
+ free(Result);
+ return Demangled;
+}
+
+std::string DylibVerifier::getAnnotatedName(const Record *R,
+ SymbolContext &SymCtx,
+ bool ValidSourceLoc) {
+ assert(!SymCtx.SymbolName.empty() && "Expected symbol name");
+
+ const StringRef SymbolName = SymCtx.SymbolName;
+ std::string PrettyName =
+ (Demangle && (SymCtx.Kind == EncodeKind::GlobalSymbol))
+ ? demangle(SymbolName)
+ : SymbolName.str();
std::string Annotation;
if (R->isWeakDefined())
@@ -45,15 +66,16 @@ getAnnotatedName(const Record *R, EncodeKind Kind, StringRef Name,
Annotation += "(tlv) ";
// Check if symbol represents only part of a @interface declaration.
- const bool IsAnnotatedObjCClass = ((ObjCIF != ObjCIFSymbolKind::None) &&
- (ObjCIF <= ObjCIFSymbolKind::EHType));
+ const bool IsAnnotatedObjCClass =
+ ((SymCtx.ObjCIFKind != ObjCIFSymbolKind::None) &&
+ (SymCtx.ObjCIFKind <= ObjCIFSymbolKind::EHType));
if (IsAnnotatedObjCClass) {
- if (ObjCIF == ObjCIFSymbolKind::EHType)
+ if (SymCtx.ObjCIFKind == ObjCIFSymbolKind::EHType)
Annotation += "Exception Type of ";
- if (ObjCIF == ObjCIFSymbolKind::MetaClass)
+ if (SymCtx.ObjCIFKind == ObjCIFSymbolKind::MetaClass)
Annotation += "Metaclass of ";
- if (ObjCIF == ObjCIFSymbolKind::Class)
+ if (SymCtx.ObjCIFKind == ObjCIFSymbolKind::Class)
Annotation += "Class of ";
}
@@ -61,42 +83,30 @@ getAnnotatedName(const Record *R, EncodeKind Kind, StringRef Name,
// tied to it. This can only ever happen when the location has to come from
// debug info.
if (ValidSourceLoc) {
- if ((Kind == EncodeKind::GlobalSymbol) && Name.starts_with("_"))
- return Annotation + Name.drop_front(1).str();
- return Annotation + Name.str();
+ StringRef PrettyNameRef(PrettyName);
+ if ((SymCtx.Kind == EncodeKind::GlobalSymbol) &&
+ !isCppMangled(SymbolName) && PrettyNameRef.starts_with("_"))
+ return Annotation + PrettyNameRef.drop_front(1).str();
+ return Annotation + PrettyName;
}
if (IsAnnotatedObjCClass)
- return Annotation + Name.str();
+ return Annotation + PrettyName;
- switch (Kind) {
+ switch (SymCtx.Kind) {
case EncodeKind::GlobalSymbol:
- return Annotation + Name.str();
+ return Annotation + PrettyName;
case EncodeKind::ObjectiveCInstanceVariable:
- return Annotation + "(ObjC IVar) " + Name.str();
+ return Annotation + "(ObjC IVar) " + PrettyName;
case EncodeKind::ObjectiveCClass:
- return Annotation + "(ObjC Class) " + Name.str();
+ return Annotation + "(ObjC Class) " + PrettyName;
case EncodeKind::ObjectiveCClassEHType:
- return Annotation + "(ObjC Class EH) " + Name.str();
+ return Annotation + "(ObjC Class EH) " + PrettyName;
}
llvm_unreachable("unexpected case for EncodeKind");
}
-static std::string demangle(StringRef Name) {
- // InstallAPI currently only supports itanium manglings.
- if (!(Name.starts_with("_Z") || Name.starts_with("__Z") ||
- Name.starts_with("___Z")))
- return Name.str();
- char *Result = llvm::itaniumDemangle(Name);
- if (!Result)
- return Name.str();
-
- std::string Demangled(Result);
- free(Result);
- return Demangled;
-}
-
static DylibVerifier::Result updateResult(const DylibVerifier::Result Prev,
const DylibVerifier::Result Curr) {
if (Prev == Curr)
@@ -193,19 +203,18 @@ bool DylibVerifier::compareObjCInterfaceSymbols(const Record *R,
// The decl represents a complete ObjCInterface, but the symbols in the
// dylib do not. Determine which symbol is missing. To keep older projects
// building, treat this as a warning.
- if (!DR->isExportedSymbol(ObjCIFSymbolKind::Class))
+ if (!DR->isExportedSymbol(ObjCIFSymbolKind::Class)) {
+ SymCtx.ObjCIFKind = ObjCIFSymbolKind::Class;
PrintDiagnostic(DR->getLinkageForSymbol(ObjCIFSymbolKind::Class), R,
- getAnnotatedName(R, SymCtx.Kind, SymCtx.PrettyPrintName,
- /*ValidSourceLoc=*/true,
- ObjCIFSymbolKind::Class),
+ getAnnotatedName(R, SymCtx),
/*PrintAsWarning=*/true);
-
- if (!DR->isExportedSymbol(ObjCIFSymbolKind::MetaClass))
+ }
+ if (!DR->isExportedSymbol(ObjCIFSymbolKind::MetaClass)) {
+ SymCtx.ObjCIFKind = ObjCIFSymbolKind::MetaClass;
PrintDiagnostic(DR->getLinkageForSymbol(ObjCIFSymbolKind::MetaClass), R,
- getAnnotatedName(R, SymCtx.Kind, SymCtx.PrettyPrintName,
- /*ValidSourceLoc=*/true,
- ObjCIFSymbolKind::MetaClass),
+ getAnnotatedName(R, SymCtx),
/*PrintAsWarning=*/true);
+ }
return true;
}
@@ -221,7 +230,7 @@ bool DylibVerifier::compareObjCInterfaceSymbols(const Record *R,
// At this point that means there was not a matching class symbol
// to represent the one discovered as a declaration.
PrintDiagnostic(DR->getLinkageForSymbol(SymCtx.ObjCIFKind), R,
- SymCtx.PrettyPrintName);
+ SymCtx.SymbolName);
return false;
}
@@ -234,7 +243,7 @@ DylibVerifier::Result DylibVerifier::compareVisibility(const Record *R,
Ctx.emitDiag([&]() {
Ctx.Diag->Report(SymCtx.FA->D->getLocation(),
diag::err_library_missing_symbol)
- << SymCtx.PrettyPrintName;
+ << getAnnotatedName(R, SymCtx);
});
return Result::Invalid;
}
@@ -242,7 +251,7 @@ DylibVerifier::Result DylibVerifier::compareVisibility(const Record *R,
Ctx.emitDiag([&]() {
Ctx.Diag->Report(SymCtx.FA->D->getLocation(),
diag::err_library_hidden_symbol)
- << SymCtx.PrettyPrintName;
+ << getAnnotatedName(R, SymCtx);
});
return Result::Invalid;
}
@@ -269,7 +278,7 @@ DylibVerifier::Result DylibVerifier::compareVisibility(const Record *R,
}
Ctx.emitDiag([&]() {
Ctx.Diag->Report(SymCtx.FA->D->getLocation(), ID)
- << SymCtx.PrettyPrintName;
+ << getAnnotatedName(R, SymCtx);
});
return Outcome;
}
@@ -293,14 +302,14 @@ DylibVerifier::Result DylibVerifier::compareAvailability(const Record *R,
Ctx.emitDiag([&]() {
Ctx.Diag->Report(SymCtx.FA->D->getLocation(),
diag::warn_header_availability_mismatch)
- << SymCtx.PrettyPrintName << IsDeclAvailable << IsDeclAvailable;
+ << getAnnotatedName(R, SymCtx) << IsDeclAvailable << IsDeclAvailable;
});
return Result::Ignore;
case VerificationMode::Pedantic:
Ctx.emitDiag([&]() {
Ctx.Diag->Report(SymCtx.FA->D->getLocation(),
diag::err_header_availability_mismatch)
- << SymCtx.PrettyPrintName << IsDeclAvailable << IsDeclAvailable;
+ << getAnnotatedName(R, SymCtx) << IsDeclAvailable << IsDeclAvailable;
});
return Result::Invalid;
case VerificationMode::ErrorsOnly:
@@ -313,15 +322,11 @@ DylibVerifier::Result DylibVerifier::compareAvailability(const Record *R,
bool DylibVerifier::compareSymbolFlags(const Record *R, SymbolContext &SymCtx,
const Record *DR) {
- std::string DisplayName =
- Demangle ? demangle(DR->getName()) : DR->getName().str();
-
if (DR->isThreadLocalValue() && !R->isThreadLocalValue()) {
Ctx.emitDiag([&]() {
Ctx.Diag->Report(SymCtx.FA->D->getLocation(),
diag::err_dylib_symbol_flags_mismatch)
- << getAnnotatedName(DR, SymCtx.Kind, DisplayName)
- << DR->isThreadLocalValue();
+ << getAnnotatedName(DR, SymCtx) << DR->isThreadLocalValue();
});
return false;
}
@@ -329,7 +334,7 @@ bool DylibVerifier::compareSymbolFlags(const Record *R, SymbolContext &SymCtx,
Ctx.emitDiag([&]() {
SymCtx.FA->D->getLocation(),
Ctx.Diag->Report(diag::err_header_symbol_flags_mismatch)
- << SymCtx.PrettyPrintName << R->isThreadLocalValue();
+ << getAnnotatedName(DR, SymCtx) << R->isThreadLocalValue();
});
return false;
}
@@ -338,8 +343,7 @@ bool DylibVerifier::compareSymbolFlags(const Record *R, SymbolContext &SymCtx,
Ctx.emitDiag([&]() {
Ctx.Diag->Report(SymCtx.FA->D->getLocation(),
diag::err_dylib_symbol_flags_mismatch)
- << getAnnotatedName(DR, SymCtx.Kind, DisplayName)
- << R->isWeakDefined();
+ << getAnnotatedName(DR, SymCtx) << R->isWeakDefined();
});
return false;
}
@@ -347,7 +351,7 @@ bool DylibVerifier::compareSymbolFlags(const Record *R, SymbolContext &SymCtx,
Ctx.emitDiag([&]() {
Ctx.Diag->Report(SymCtx.FA->D->getLocation(),
diag::err_header_symbol_flags_mismatch)
- << SymCtx.PrettyPrintName << R->isWeakDefined();
+ << getAnnotatedName(R, SymCtx) << R->isWeakDefined();
});
return false;
}
@@ -457,10 +461,7 @@ DylibVerifier::Result DylibVerifier::verify(ObjCIVarRecord *R,
std::string FullName =
ObjCIVarRecord::createScopedName(SuperClass, R->getName());
- SymbolContext SymCtx{
- getAnnotatedName(R, EncodeKind::ObjectiveCInstanceVariable,
- Demangle ? demangle(FullName) : FullName),
- FullName, EncodeKind::ObjectiveCInstanceVariable, FA};
+ SymbolContext SymCtx{FullName, EncodeKind::ObjectiveCInstanceVariable, FA};
return verifyImpl(R, SymCtx);
}
@@ -485,11 +486,8 @@ DylibVerifier::Result DylibVerifier::verify(ObjCInterfaceRecord *R,
SymCtx.SymbolName = R->getName();
SymCtx.ObjCIFKind = assignObjCIFSymbolKind(R);
- std::string DisplayName =
- Demangle ? demangle(SymCtx.SymbolName) : SymCtx.SymbolName;
SymCtx.Kind = R->hasExceptionAttribute() ? EncodeKind::ObjectiveCClassEHType
: EncodeKind::ObjectiveCClass;
- SymCtx.PrettyPrintName = getAnnotatedName(R, SymCtx.Kind, DisplayName);
SymCtx.FA = FA;
return verifyImpl(R, SymCtx);
@@ -504,8 +502,6 @@ DylibVerifier::Result DylibVerifier::verify(GlobalRecord *R,
SimpleSymbol Sym = parseSymbol(R->getName());
SymbolContext SymCtx;
SymCtx.SymbolName = Sym.Name;
- SymCtx.PrettyPrintName =
- getAnnotatedName(R, Sym.Kind, Demangle ? demangle(Sym.Name) : Sym.Name);
SymCtx.Kind = Sym.Kind;
SymCtx.FA = FA;
SymCtx.Inlined = R->isInlined();
diff --git a/clang/test/InstallAPI/diagnostics-cpp.test b/clang/test/InstallAPI/diagnostics-cpp.test
index 9319a7a61d4839..65888653750722 100644
--- a/clang/test/InstallAPI/diagnostics-cpp.test
+++ b/clang/test/InstallAPI/diagnostics-cpp.test
@@ -12,8 +12,8 @@
// RUN: --verify-mode=Pedantic -o %t/output.tbd --demangle 2> %t/errors.log
// RUN: FileCheck -input-file %t/errors.log %s
-CHECK: warning: violations found for arm64-apple-macos13
-CHECK: CPP.h:5:7: error: declaration has external linkage, but symbol has internal linkage in dynamic library 'vtable for Bar'
+CHECK: warning: violations found for arm64-apple-macos13
+CHECK: CPP.h:5:7: error: declaration has external linkage, but symbol has internal linkage in dynamic library 'vtable for Bar'
CHECK-NEXT: class Bar : Foo {
CHECK-NEXT: ^
CHECK-NEXT: CPP.h:5:7: error: declaration has external linkage, but symbol has internal linkage in dynamic library 'typeinfo for Bar'
``````````
</details>
https://github.com/llvm/llvm-project/pull/85894
More information about the cfe-commits
mailing list