[InstallAPI] Simplify & improve symbol printing for diagnostics #85894
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
cyndyishida
commented
Mar 20, 2024
cyndyishida
commented
- Defer mangling of symbols until an error is ready to report
- Pass around fewer parameters when reporting
* Defer mangling of symbols until error is ready to report. * Pass around less parameters when reporting
cyndyishida
added
the
skip-precommit-approval
|
@llvm/pr-subscribers-clang Author: Cyndy Ishida (cyndyishida) Changes
Full diff: https://github.com/llvm/llvm-project/pull/85894.diff 4 Files Affected:
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'
|
chencha3
pushed a commit
to chencha3/llvm-project
that referenced
this pull request
Mar 23, 2024
…#85894) * Defer mangling of symbols until an error is ready to report * Pass around fewer parameters when reporting
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.