llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang Author: Cyndy Ishida (cyndyishida) <details> <summary>Changes</summary> * This completes support for verifying every declaration found in a header is discovered in the dylib. Diagnostics are reported for each class for differences that are representable in TBD files. * This patch also now captures unavailable attributes that depend on target triples. This is needed for proper tbd file generation. --- Patch is 85.46 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/85348.diff 13 Files Affected: - (modified) clang/include/clang/AST/Availability.h (+9-4) - (modified) clang/include/clang/Basic/DiagnosticInstallAPIKinds.td (+23) - (modified) clang/include/clang/InstallAPI/DylibVerifier.h (+51-6) - (modified) clang/include/clang/InstallAPI/Frontend.h (+3) - (modified) clang/include/clang/InstallAPI/MachO.h (+1) - (modified) clang/lib/AST/Availability.cpp (+3-3) - (modified) clang/lib/InstallAPI/DylibVerifier.cpp (+321-5) - (modified) clang/lib/InstallAPI/Visitor.cpp (+1-1) - (added) clang/test/InstallAPI/availability.test (+626) - (added) clang/test/InstallAPI/diagnostics-cpp.test (+461) - (added) clang/test/InstallAPI/hiddens.test (+262) - (modified) clang/tools/clang-installapi/ClangInstallAPI.cpp (+2-4) - (modified) clang/tools/clang-installapi/Options.cpp (+3-1) ``````````diff diff --git a/clang/include/clang/AST/Availability.h b/clang/include/clang/AST/Availability.h index 5cfbaf0cdfbd21..2ccc607d4b63dc 100644 --- a/clang/include/clang/AST/Availability.h +++ b/clang/include/clang/AST/Availability.h @@ -67,6 +67,7 @@ struct AvailabilityInfo { VersionTuple Introduced; VersionTuple Deprecated; VersionTuple Obsoleted; + bool Unavailable = false; bool UnconditionallyDeprecated = false; bool UnconditionallyUnavailable = false; @@ -78,6 +79,9 @@ struct AvailabilityInfo { /// Check if the symbol has been obsoleted. bool isObsoleted() const { return !Obsoleted.empty(); } + /// Check if the symbol is unavailable for the active platform and os version. + bool isUnavailable() const { return Unavailable; } + /// Check if the symbol is unconditionally deprecated. /// /// i.e. \code __attribute__((deprecated)) \endcode @@ -91,9 +95,10 @@ struct AvailabilityInfo { } AvailabilityInfo(StringRef Domain, VersionTuple I, VersionTuple D, - VersionTuple O, bool UD, bool UU) + VersionTuple O, bool U, bool UD, bool UU) : Domain(Domain), Introduced(I), Deprecated(D), Obsoleted(O), - UnconditionallyDeprecated(UD), UnconditionallyUnavailable(UU) {} + Unavailable(U), UnconditionallyDeprecated(UD), + UnconditionallyUnavailable(UU) {} friend bool operator==(const AvailabilityInfo &Lhs, const AvailabilityInfo &Rhs); @@ -105,10 +110,10 @@ struct AvailabilityInfo { inline bool operator==(const AvailabilityInfo &Lhs, const AvailabilityInfo &Rhs) { return std::tie(Lhs.Introduced, Lhs.Deprecated, Lhs.Obsoleted, - Lhs.UnconditionallyDeprecated, + Lhs.Unavailable, Lhs.UnconditionallyDeprecated, Lhs.UnconditionallyUnavailable) == std::tie(Rhs.Introduced, Rhs.Deprecated, Rhs.Obsoleted, - Rhs.UnconditionallyDeprecated, + Rhs.Unavailable, Rhs.UnconditionallyDeprecated, Rhs.UnconditionallyUnavailable); } diff --git a/clang/include/clang/Basic/DiagnosticInstallAPIKinds.td b/clang/include/clang/Basic/DiagnosticInstallAPIKinds.td index 31be4f09cf3a1c..5ed2e23425dc5f 100644 --- a/clang/include/clang/Basic/DiagnosticInstallAPIKinds.td +++ b/clang/include/clang/Basic/DiagnosticInstallAPIKinds.td @@ -17,4 +17,27 @@ def err_no_install_name : Error<"no install name specified: add -install_name <p def err_no_output_file: Error<"no output file specified">; } // end of command line category. +let CategoryName = "Verification" in { +def warn_target: Warning<"violations found for %0">; +def err_library_missing_symbol : Error<"declaration has external linkage, but dynamic library doesn't have symbol '%0'">; +def warn_library_missing_symbol : Warning<"declaration has external linkage, but dynamic library doesn't have symbol '%0'">; +def err_library_hidden_symbol : Error<"declaration has external linkage, but symbol has internal linkage in dynamic library '%0'">; +def warn_library_hidden_symbol : Warning<"declaration has external linkage, but symbol has internal linkage in dynamic library '%0'">; +def warn_header_hidden_symbol : Warning<"symbol exported in dynamic library, but marked hidden in declaration '%0'">; +def err_header_hidden_symbol : Error<"symbol exported in dynamic library, but marked hidden in declaration '%0'">; +def err_header_symbol_missing : Error<"no declaration found for exported symbol '%0' in dynamic library">; +def warn_header_availability_mismatch : Warning<"declaration '%0' is marked %select{available|unavailable}1," + " but symbol is %select{not |}2exported in dynamic library">; +def err_header_availability_mismatch : Error<"declaration '%0' is marked %select{available|unavailable}1," + " but symbol is %select{not |}2exported in dynamic library">; +def warn_dylib_symbol_flags_mismatch : Warning<"dynamic library symbol '%0' is " + "%select{weak defined|thread local}1, but its declaration is not">; +def warn_header_symbol_flags_mismatch : Warning<"declaration '%0' is " + "%select{weak defined|thread local}1, but symbol is not in dynamic library">; +def err_dylib_symbol_flags_mismatch : Error<"dynamic library symbol '%0' is " + "%select{weak defined|thread local}1, but its declaration is not">; +def err_header_symbol_flags_mismatch : Error<"declaration '%0' is " + "%select{weak defined|thread local}1, but symbol is not in dynamic library">; +} // end of Verification category. + } // end of InstallAPI component diff --git a/clang/include/clang/InstallAPI/DylibVerifier.h b/clang/include/clang/InstallAPI/DylibVerifier.h index 72c4743fdf65e0..2094548ced8d5f 100644 --- a/clang/include/clang/InstallAPI/DylibVerifier.h +++ b/clang/include/clang/InstallAPI/DylibVerifier.h @@ -38,19 +38,34 @@ class DylibVerifier { // Current target being verified against the AST. llvm::MachO::Target Target; + // Target specific API from binary. + RecordsSlice *DylibSlice; + // Query state of verification after AST has been traversed. Result FrontendState; // First error for AST traversal, which is tied to the target triple. bool DiscoveredFirstError; + + // Determines what kind of banner to print a violation for. + bool PrintArch = false; + + // Engine for reporting violations. + DiagnosticsEngine *Diag = nullptr; + + // Handle diagnostics reporting for target level violations. + void emitDiag(llvm::function_ref<void()> Report); + + VerifierContext() = default; + VerifierContext(DiagnosticsEngine *Diag) : Diag(Diag) {} }; DylibVerifier() = default; DylibVerifier(llvm::MachO::Records &&Dylib, DiagnosticsEngine *Diag, VerificationMode Mode, bool Demangle) - : Dylib(std::move(Dylib)), Diag(Diag), Mode(Mode), Demangle(Demangle), - Exports(std::make_unique<SymbolSet>()) {} + : Dylib(std::move(Dylib)), Mode(Mode), Demangle(Demangle), + Exports(std::make_unique<SymbolSet>()), Ctx(VerifierContext{Diag}) {} Result verify(GlobalRecord *R, const FrontendAttrs *FA); Result verify(ObjCInterfaceRecord *R, const FrontendAttrs *FA); @@ -66,6 +81,13 @@ class DylibVerifier { /// Get result of verification. Result getState() const { return Ctx.FrontendState; } + /// Set different source managers to the same diagnostics engine. + void setSourceManager(SourceManager &SourceMgr) const { + if (!Ctx.Diag) + return; + Ctx.Diag->setSourceManager(&SourceMgr); + } + private: /// Determine whether to compare declaration to symbol in binary. bool canVerify(); @@ -73,6 +95,29 @@ class DylibVerifier { /// Shared implementation for verifying exported symbols. Result verifyImpl(Record *R, SymbolContext &SymCtx); + /// Check if declaration is marked as obsolete, they are + // expected to result in a symbol mismatch. + bool shouldIgnoreObsolete(const Record *R, SymbolContext &SymCtx, + const Record *DR); + + /// Compare the visibility declarations to the linkage of symbol found in + /// dylib. + Result compareVisibility(const Record *R, SymbolContext &SymCtx, + const Record *DR); + + /// An ObjCInterfaceRecord can represent up to three symbols. When verifying, + // account for this granularity. + bool compareObjCInterfaceSymbols(const Record *R, SymbolContext &SymCtx, + const ObjCInterfaceRecord *DR); + + /// Validate availability annotations against dylib. + Result compareAvailability(const Record *R, SymbolContext &SymCtx, + const Record *DR); + + /// Compare and validate matching symbol flags. + bool compareSymbolFlags(const Record *R, SymbolContext &SymCtx, + const Record *DR); + /// Update result state on each call to `verify`. void updateState(Result State); @@ -80,14 +125,14 @@ class DylibVerifier { void addSymbol(const Record *R, SymbolContext &SymCtx, TargetList &&Targets = {}); + /// Find matching dylib slice for target triple that is being parsed. + void assignSlice(const Target &T); + // Symbols in dylib. llvm::MachO::Records Dylib; - // Engine for reporting violations. - [[maybe_unused]] DiagnosticsEngine *Diag = nullptr; - // Controls what class of violations to report. - [[maybe_unused]] VerificationMode Mode = VerificationMode::Invalid; + VerificationMode Mode = VerificationMode::Invalid; // Attempt to demangle when reporting violations. bool Demangle = false; diff --git a/clang/include/clang/InstallAPI/Frontend.h b/clang/include/clang/InstallAPI/Frontend.h index 660fc8cd69a59d..5cccd891c58093 100644 --- a/clang/include/clang/InstallAPI/Frontend.h +++ b/clang/include/clang/InstallAPI/Frontend.h @@ -17,6 +17,7 @@ #include "clang/Frontend/CompilerInstance.h" #include "clang/Frontend/FrontendActions.h" #include "clang/InstallAPI/Context.h" +#include "clang/InstallAPI/DylibVerifier.h" #include "clang/InstallAPI/Visitor.h" #include "llvm/ADT/Twine.h" #include "llvm/Support/MemoryBuffer.h" @@ -34,6 +35,8 @@ class InstallAPIAction : public ASTFrontendAction { std::unique_ptr<ASTConsumer> CreateASTConsumer(CompilerInstance &CI, StringRef InFile) override { + Ctx.Diags->getClient()->BeginSourceFile(CI.getLangOpts()); + Ctx.Verifier->setSourceManager(CI.getSourceManager()); return std::make_unique<InstallAPIVisitor>( CI.getASTContext(), Ctx, CI.getSourceManager(), CI.getPreprocessor()); } diff --git a/clang/include/clang/InstallAPI/MachO.h b/clang/include/clang/InstallAPI/MachO.h index 6dee6f22420381..a77766511fa3e5 100644 --- a/clang/include/clang/InstallAPI/MachO.h +++ b/clang/include/clang/InstallAPI/MachO.h @@ -32,6 +32,7 @@ using ObjCInterfaceRecord = llvm::MachO::ObjCInterfaceRecord; using ObjCCategoryRecord = llvm::MachO::ObjCCategoryRecord; using ObjCIVarRecord = llvm::MachO::ObjCIVarRecord; using Records = llvm::MachO::Records; +using RecordsSlice = llvm::MachO::RecordsSlice; using BinaryAttrs = llvm::MachO::RecordsSlice::BinaryAttrs; using SymbolSet = llvm::MachO::SymbolSet; using SimpleSymbol = llvm::MachO::SimpleSymbol; diff --git a/clang/lib/AST/Availability.cpp b/clang/lib/AST/Availability.cpp index d0054e37e4dcd2..238359a2dedfcf 100644 --- a/clang/lib/AST/Availability.cpp +++ b/clang/lib/AST/Availability.cpp @@ -28,9 +28,9 @@ AvailabilityInfo AvailabilityInfo::createFromDecl(const Decl *Decl) { for (const auto *A : RD->specific_attrs<AvailabilityAttr>()) { if (A->getPlatform()->getName() != PlatformName) continue; - Availability = - AvailabilityInfo(A->getPlatform()->getName(), A->getIntroduced(), - A->getDeprecated(), A->getObsoleted(), false, false); + Availability = AvailabilityInfo( + A->getPlatform()->getName(), A->getIntroduced(), A->getDeprecated(), + A->getObsoleted(), A->getUnavailable(), false, false); break; } diff --git a/clang/lib/InstallAPI/DylibVerifier.cpp b/clang/lib/InstallAPI/DylibVerifier.cpp index b7dd85d63fa14f..700763b3fee0db 100644 --- a/clang/lib/InstallAPI/DylibVerifier.cpp +++ b/clang/lib/InstallAPI/DylibVerifier.cpp @@ -1,5 +1,6 @@ #include "clang/InstallAPI/DylibVerifier.h" #include "clang/InstallAPI/FrontendRecords.h" +#include "clang/InstallAPI/InstallAPIDiagnostic.h" #include "llvm/Demangle/Demangle.h" using namespace llvm::MachO; @@ -24,6 +25,9 @@ struct DylibVerifier::SymbolContext { // The ObjCInterface symbol type, if applicable. ObjCIFSymbolKind ObjCIFKind = ObjCIFSymbolKind::None; + + // Whether Decl is inlined. + bool Inlined = false; }; static std::string @@ -80,10 +84,11 @@ getAnnotatedName(const Record *R, EncodeKind Kind, StringRef Name, } static std::string demangle(StringRef Name) { - // Itanium encoding requires 1 or 3 leading underscores, followed by 'Z'. - if (!(Name.starts_with("_Z") || Name.starts_with("___Z"))) + // 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.data()); + char *Result = llvm::itaniumDemangle(Name); if (!Result) return Name.str(); @@ -109,6 +114,30 @@ static DylibVerifier::Result updateResult(const DylibVerifier::Result Prev, return Curr; } +// __private_extern__ is a deprecated specifier that clang does not +// respect in all contexts, it should just be considered hidden for InstallAPI. +static bool shouldIgnorePrivateExternAttr(const Decl *D) { + if (const FunctionDecl *FD = cast<FunctionDecl>(D)) + return FD->getStorageClass() == StorageClass::SC_PrivateExtern; + if (const VarDecl *VD = cast<VarDecl>(D)) + return VD->getStorageClass() == StorageClass::SC_PrivateExtern; + + return false; +} + +Record *findRecordFromSlice(const RecordsSlice *Slice, StringRef Name, + EncodeKind Kind) { + switch (Kind) { + case EncodeKind::GlobalSymbol: + return Slice->findGlobal(Name); + case EncodeKind::ObjectiveCInstanceVariable: + return Slice->findObjCIVar(Name.contains('.'), Name); + case EncodeKind::ObjectiveCClass: + case EncodeKind::ObjectiveCClassEHType: + return Slice->findObjCInterface(Name); + } + llvm_unreachable("unexpected end when finding record"); +} void DylibVerifier::updateState(Result State) { Ctx.FrontendState = updateResult(Ctx.FrontendState, State); @@ -122,17 +151,272 @@ void DylibVerifier::addSymbol(const Record *R, SymbolContext &SymCtx, Exports->addGlobal(SymCtx.Kind, SymCtx.SymbolName, R->getFlags(), Targets); } +bool DylibVerifier::shouldIgnoreObsolete(const Record *R, SymbolContext &SymCtx, + const Record *DR) { + return SymCtx.FA->Avail.isObsoleted(); +} + +bool DylibVerifier::compareObjCInterfaceSymbols(const Record *R, + SymbolContext &SymCtx, + const ObjCInterfaceRecord *DR) { + const bool IsDeclVersionComplete = + ((SymCtx.ObjCIFKind & ObjCIFSymbolKind::Class) == + ObjCIFSymbolKind::Class) && + ((SymCtx.ObjCIFKind & ObjCIFSymbolKind::MetaClass) == + ObjCIFSymbolKind::MetaClass); + + const bool IsDylibVersionComplete = DR->isCompleteInterface(); + + // The common case, a complete ObjCInterface. + if (IsDeclVersionComplete && IsDylibVersionComplete) + return true; + + auto PrintDiagnostic = [&](auto SymLinkage, const Record *Record, + StringRef SymName, bool PrintAsWarning = false) { + if (SymLinkage == RecordLinkage::Unknown) + Ctx.emitDiag([&]() { + Ctx.Diag->Report(SymCtx.FA->D->getLocation(), + PrintAsWarning ? diag::warn_library_missing_symbol + : diag::err_library_missing_symbol) + << SymName; + }); + else + Ctx.emitDiag([&]() { + Ctx.Diag->Report(SymCtx.FA->D->getLocation(), + PrintAsWarning ? diag::warn_library_hidden_symbol + : diag::err_library_hidden_symbol) + << SymName; + }); + }; + + if (IsDeclVersionComplete) { + // 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)) + PrintDiagnostic(DR->getLinkageForSymbol(ObjCIFSymbolKind::Class), R, + getAnnotatedName(R, SymCtx.Kind, SymCtx.PrettyPrintName, + /*ValidSourceLoc=*/true, + ObjCIFSymbolKind::Class), + /*PrintAsWarning=*/true); + + if (!DR->isExportedSymbol(ObjCIFSymbolKind::MetaClass)) + PrintDiagnostic(DR->getLinkageForSymbol(ObjCIFSymbolKind::MetaClass), R, + getAnnotatedName(R, SymCtx.Kind, SymCtx.PrettyPrintName, + /*ValidSourceLoc=*/true, + ObjCIFSymbolKind::MetaClass), + /*PrintAsWarning=*/true); + return true; + } + + if (DR->isExportedSymbol(SymCtx.ObjCIFKind)) { + if (!IsDylibVersionComplete) { + // Both the declaration and dylib have a non-complete interface. + SymCtx.Kind = EncodeKind::GlobalSymbol; + SymCtx.SymbolName = R->getName(); + } + return true; + } + + // 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); + return false; +} + +DylibVerifier::Result DylibVerifier::compareVisibility(const Record *R, + SymbolContext &SymCtx, + const Record *DR) { + + if (R->isExported()) { + if (!DR) { + Ctx.emitDiag([&]() { + Ctx.Diag->Report(SymCtx.FA->D->getLocation(), + diag::err_library_missing_symbol) + << SymCtx.PrettyPrintName; + }); + return Result::Invalid; + } + if (DR->isInternal()) { + Ctx.emitDiag([&]() { + Ctx.Diag->Report(SymCtx.FA->D->getLocation(), + diag::err_library_hidden_symbol) + << SymCtx.PrettyPrintName; + }); + return Result::Invalid; + } + } + + // Emit a diagnostic for hidden declarations with external symbols, except + // when theres an inlined attribute. + if ((R->isInternal() && !SymCtx.Inlined) && DR && DR->isExported()) { + + if (Mode == VerificationMode::ErrorsOnly) + return Result::Ignore; + + if (shouldIgnorePrivateExternAttr(SymCtx.FA->D)) + return Result::Ignore; + + unsigned ID; + Result Outcome; + if (Mode == VerificationMode::ErrorsAndWarnings) { + ID = diag::warn_header_hidden_symbol; + Outcome = Result::Ignore; + } else { + ID = diag::err_header_hidden_symbol; + Outcome = Result::Invalid; + } + Ctx.emitDiag([&]() { + Ctx.Diag->Report(SymCtx.FA->D->getLocation(), ID) + << SymCtx.PrettyPrintName; + }); + return Outcome; + } + + if (R->isInternal()) + return Result::Ignore; + + return Result::Valid; +} + +DylibVerifier::Result DylibVerifier::compareAvailability(const Record *R, + SymbolContext &SymCtx, + const Record *DR) { + if (!SymCtx.FA->Avail.isUnavailable()) + return Result::Valid; + + const bool IsDeclAvailable = SymCtx.FA->Avail.isUnavailable(); + + switch (Mode) { + case VerificationMode::ErrorsAndWarnings: + Ctx.emitDiag([&]() { + Ctx.Diag->Report(SymCtx.FA->D->getLocation(), + diag::warn_header_availability_mismatch) + << SymCtx.PrettyPrintName << 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; + }); + return Result::Invalid; + case VerificationMode::ErrorsOnly: + return Result::Ignore; + case VerificationMode::Invalid: + llvm_unreachable("Unexpected verification mode symbol verification"); + } + llvm_unreachable("Unexpected verification mode symbol verification"); +} + +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(); + }); + return false; + } + i... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/85348 _______________________________________________ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
