Author: Cyndy Ishida Date: 2024-03-20T07:26:08-07:00 New Revision: 9f168591f36952d8cca543a5ba67906dc07096ff
URL: https://github.com/llvm/llvm-project/commit/9f168591f36952d8cca543a5ba67906dc07096ff DIFF: https://github.com/llvm/llvm-project/commit/9f168591f36952d8cca543a5ba67906dc07096ff.diff LOG: [InstallAPI] Simplify & improve symbol printing for diagnostics (#85894) * Defer mangling of symbols until an error is ready to report * Pass around fewer parameters when reporting Added: Modified: clang/include/clang/InstallAPI/DylibVerifier.h clang/include/clang/InstallAPI/MachO.h clang/lib/InstallAPI/DylibVerifier.cpp clang/test/InstallAPI/diagnostics-cpp.test Removed: ################################################################################ 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' _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
