From 4ec62cd0d0eb38965cac21aca62b6ac01a5b052e Mon Sep 17 00:00:00 2001 From: Mike Hommey Date: Fri, 5 Apr 2024 05:59:24 +0900 Subject: [PATCH 4/4] Revert "[InstallAPI] Add support for parsing dSYMs (#86852)" This reverts commit a4de589d117a4fd52554da3c61ae6eb26c90a0c8. --- .../include/clang/InstallAPI/DylibVerifier.h | 20 +--- clang/include/clang/InstallAPI/MachO.h | 1 - clang/lib/InstallAPI/CMakeLists.txt | 1 - clang/lib/InstallAPI/DylibVerifier.cpp | 71 +++-------- clang/test/InstallAPI/diagnostics-dsym.test | 43 ------- .../tools/clang-installapi/InstallAPIOpts.td | 2 - clang/tools/clang-installapi/Options.cpp | 6 +- clang/tools/clang-installapi/Options.h | 3 - llvm/include/llvm/TextAPI/DylibReader.h | 9 -- llvm/include/llvm/TextAPI/Record.h | 17 --- llvm/lib/TextAPI/BinaryReader/CMakeLists.txt | 1 - llvm/lib/TextAPI/BinaryReader/DylibReader.cpp | 111 +----------------- 12 files changed, 22 insertions(+), 263 deletions(-) delete mode 100644 clang/test/InstallAPI/diagnostics-dsym.test diff --git a/clang/include/clang/InstallAPI/DylibVerifier.h b/clang/include/clang/InstallAPI/DylibVerifier.h index 22cdc234486c..49de24763f1f 100644 --- a/clang/include/clang/InstallAPI/DylibVerifier.h +++ b/clang/include/clang/InstallAPI/DylibVerifier.h @@ -31,7 +31,6 @@ enum class VerificationMode { class DylibVerifier : llvm::MachO::RecordVisitor { private: struct SymbolContext; - struct DWARFContext; public: enum class Result { NoVerify, Ignore, Valid, Invalid }; @@ -55,7 +54,7 @@ public: DiagnosticsEngine *Diag = nullptr; // Handle diagnostics reporting for target level violations. - void emitDiag(llvm::function_ref Report, RecordLoc *Loc = nullptr); + void emitDiag(llvm::function_ref Report); VerifierContext() = default; VerifierContext(DiagnosticsEngine *Diag) : Diag(Diag) {} @@ -64,10 +63,9 @@ public: DylibVerifier() = default; DylibVerifier(llvm::MachO::Records &&Dylib, DiagnosticsEngine *Diag, - VerificationMode Mode, bool Demangle, StringRef DSYMPath) + VerificationMode Mode, bool Demangle) : Dylib(std::move(Dylib)), Mode(Mode), Demangle(Demangle), - DSYMPath(DSYMPath), Exports(std::make_unique()), - Ctx(VerifierContext{Diag}) {} + Exports(std::make_unique()), Ctx(VerifierContext{Diag}) {} Result verify(GlobalRecord *R, const FrontendAttrs *FA); Result verify(ObjCInterfaceRecord *R, const FrontendAttrs *FA); @@ -145,12 +143,6 @@ private: std::string getAnnotatedName(const Record *R, SymbolContext &SymCtx, bool ValidSourceLoc = true); - /// Extract source location for symbol implementations. - /// As this is a relatively expensive operation, it is only used - /// when there is a violation to report and there is not a known declaration - /// in the interface. - void accumulateSrcLocForDylibSymbols(); - // Symbols in dylib. llvm::MachO::Records Dylib; @@ -160,17 +152,11 @@ private: // Attempt to demangle when reporting violations. bool Demangle = false; - // File path to DSYM file. - StringRef DSYMPath; - // Valid symbols in final text file. std::unique_ptr Exports = std::make_unique(); // Track current state of verification while traversing AST. VerifierContext Ctx; - - // Track DWARF provided source location for dylibs. - DWARFContext *DWARFCtx = nullptr; }; } // namespace installapi diff --git a/clang/include/clang/InstallAPI/MachO.h b/clang/include/clang/InstallAPI/MachO.h index 827220dbf39f..4961c596fd68 100644 --- a/clang/include/clang/InstallAPI/MachO.h +++ b/clang/include/clang/InstallAPI/MachO.h @@ -34,7 +34,6 @@ using ObjCCategoryRecord = llvm::MachO::ObjCCategoryRecord; using ObjCIVarRecord = llvm::MachO::ObjCIVarRecord; using ObjCIFSymbolKind = llvm::MachO::ObjCIFSymbolKind; using Records = llvm::MachO::Records; -using RecordLoc = llvm::MachO::RecordLoc; using RecordsSlice = llvm::MachO::RecordsSlice; using BinaryAttrs = llvm::MachO::RecordsSlice::BinaryAttrs; using SymbolSet = llvm::MachO::SymbolSet; diff --git a/clang/lib/InstallAPI/CMakeLists.txt b/clang/lib/InstallAPI/CMakeLists.txt index e0bc8d969ecb..894db699578f 100644 --- a/clang/lib/InstallAPI/CMakeLists.txt +++ b/clang/lib/InstallAPI/CMakeLists.txt @@ -1,7 +1,6 @@ set(LLVM_LINK_COMPONENTS Support TextAPI - TextAPIBinaryReader Demangle Core ) diff --git a/clang/lib/InstallAPI/DylibVerifier.cpp b/clang/lib/InstallAPI/DylibVerifier.cpp index c0eda1d81b9b..ba25e4183a9b 100644 --- a/clang/lib/InstallAPI/DylibVerifier.cpp +++ b/clang/lib/InstallAPI/DylibVerifier.cpp @@ -10,7 +10,6 @@ #include "clang/InstallAPI/FrontendRecords.h" #include "clang/InstallAPI/InstallAPIDiagnostic.h" #include "llvm/Demangle/Demangle.h" -#include "llvm/TextAPI/DylibReader.h" using namespace llvm::MachO; @@ -36,14 +35,6 @@ struct DylibVerifier::SymbolContext { bool Inlined = false; }; -struct DylibVerifier::DWARFContext { - // Track whether DSYM parsing has already been attempted to avoid re-parsing. - bool ParsedDSYM{false}; - - // Lookup table for source locations by symbol name. - DylibReader::SymbolToSourceLocMap SourceLocs{}; -}; - static bool isCppMangled(StringRef Name) { // InstallAPI currently only supports itanium manglings. return (Name.starts_with("_Z") || Name.starts_with("__Z") || @@ -520,16 +511,14 @@ DylibVerifier::Result DylibVerifier::verify(GlobalRecord *R, return verifyImpl(R, SymCtx); } -void DylibVerifier::VerifierContext::emitDiag(llvm::function_ref Report, - RecordLoc *Loc) { +void DylibVerifier::VerifierContext::emitDiag( + llvm::function_ref Report) { if (!DiscoveredFirstError) { Diag->Report(diag::warn_target) << (PrintArch ? getArchitectureName(Target.Arch) : getTargetTripleName(Target)); DiscoveredFirstError = true; } - if (Loc && Loc->isValid()) - llvm::errs() << Loc->File << ":" << Loc->Line << ":" << 0 << ": "; Report(); } @@ -572,36 +561,26 @@ void DylibVerifier::visitSymbolInDylib(const Record &R, SymbolContext &SymCtx) { return; } - const bool IsLinkerSymbol = SymbolName.starts_with("$ld$"); - - // All checks at this point classify as some kind of violation. - // The different verification modes dictate whether they are reported to the - // user. - if (IsLinkerSymbol || (Mode > VerificationMode::ErrorsOnly)) - accumulateSrcLocForDylibSymbols(); - RecordLoc Loc = DWARFCtx->SourceLocs.lookup(SymCtx.SymbolName); + // All checks at this point classify as some kind of violation that should be + // reported. // Regardless of verification mode, error out on mismatched special linker // symbols. - if (IsLinkerSymbol) { - Ctx.emitDiag( - [&]() { - Ctx.Diag->Report(diag::err_header_symbol_missing) - << getAnnotatedName(&R, SymCtx, Loc.isValid()); - }, - &Loc); + if (SymbolName.starts_with("$ld$")) { + Ctx.emitDiag([&]() { + Ctx.Diag->Report(diag::err_header_symbol_missing) + << getAnnotatedName(&R, SymCtx, /*ValidSourceLoc=*/false); + }); updateState(Result::Invalid); return; } // Missing declarations for exported symbols are hard errors on Pedantic mode. if (Mode == VerificationMode::Pedantic) { - Ctx.emitDiag( - [&]() { - Ctx.Diag->Report(diag::err_header_symbol_missing) - << getAnnotatedName(&R, SymCtx, Loc.isValid()); - }, - &Loc); + Ctx.emitDiag([&]() { + Ctx.Diag->Report(diag::err_header_symbol_missing) + << getAnnotatedName(&R, SymCtx, /*ValidSourceLoc=*/false); + }); updateState(Result::Invalid); return; } @@ -609,12 +588,10 @@ void DylibVerifier::visitSymbolInDylib(const Record &R, SymbolContext &SymCtx) { // Missing declarations for exported symbols are warnings on ErrorsAndWarnings // mode. if (Mode == VerificationMode::ErrorsAndWarnings) { - Ctx.emitDiag( - [&]() { - Ctx.Diag->Report(diag::warn_header_symbol_missing) - << getAnnotatedName(&R, SymCtx, Loc.isValid()); - }, - &Loc); + Ctx.emitDiag([&]() { + Ctx.Diag->Report(diag::warn_header_symbol_missing) + << getAnnotatedName(&R, SymCtx, /*ValidSourceLoc=*/false); + }); updateState(Result::Ignore); return; } @@ -645,18 +622,6 @@ void DylibVerifier::visitObjCIVar(const ObjCIVarRecord &R, visitSymbolInDylib(R, SymCtx); } -void DylibVerifier::accumulateSrcLocForDylibSymbols() { - if (DSYMPath.empty()) - return; - - assert(DWARFCtx != nullptr && "Expected an initialized DWARFContext"); - if (DWARFCtx->ParsedDSYM) - return; - DWARFCtx->ParsedDSYM = true; - DWARFCtx->SourceLocs = - DylibReader::accumulateSourceLocFromDSYM(DSYMPath, Ctx.Target); -} - void DylibVerifier::visitObjCInterface(const ObjCInterfaceRecord &R) { if (R.isVerified()) return; @@ -690,8 +655,6 @@ DylibVerifier::Result DylibVerifier::verifyRemainingSymbols() { return Result::NoVerify; assert(!Dylib.empty() && "No binary to verify against"); - DWARFContext DWARFInfo; - DWARFCtx = &DWARFInfo; Ctx.DiscoveredFirstError = false; Ctx.PrintArch = true; for (std::shared_ptr Slice : Dylib) { diff --git a/clang/test/InstallAPI/diagnostics-dsym.test b/clang/test/InstallAPI/diagnostics-dsym.test deleted file mode 100644 index ee2c8b32df29..000000000000 --- a/clang/test/InstallAPI/diagnostics-dsym.test +++ /dev/null @@ -1,43 +0,0 @@ -; REQUIRES: system-darwin - -; RUN: rm -rf %t -; RUN: split-file %s %t - -// Build a simple dylib with debug info. -; RUN: %clang --target=arm64-apple-macos13 -g -dynamiclib %t/foo.c \ -; RUN: -current_version 1 -compatibility_version 1 -L%t/usr/lib \ -; RUN: -save-temps \ -; RUN: -o %t/foo.dylib -install_name %t/foo.dylib -; RUN: dsymutil %t/foo.dylib -o %t/foo.dSYM - -; RUN: not clang-installapi -x c++ --target=arm64-apple-macos13 \ -; RUN: -install_name %t/foo.dylib \ -; RUN: -current_version 1 -compatibility_version 1 \ -; RUN: -o %t/output.tbd \ -; RUN: --verify-against=%t/foo.dylib --dsym=%t/foo.dSYM \ -; RUN: --verify-mode=Pedantic 2>&1 | FileCheck %s - -; CHECK: violations found for arm64 -; CHECK: foo.c:5:0: error: no declaration found for exported symbol 'bar' in dynamic library -; CHECK: foo.c:1:0: error: no declaration found for exported symbol 'foo' in dynamic library - -;--- foo.c -int foo(void) { - return 1; -} -extern char bar; -char bar = 'a'; - -;--- usr/lib/libSystem.tbd -{ - "main_library": { - "install_names": [ - {"name": "/usr/lib/libSystem.B.dylib"} - ], - "target_info": [ - {"target": "arm64-macos"} - ] - }, - "tapi_tbd_version": 5 -} - diff --git a/clang/tools/clang-installapi/InstallAPIOpts.td b/clang/tools/clang-installapi/InstallAPIOpts.td index 010f2507a1d1..71532c9cf24d 100644 --- a/clang/tools/clang-installapi/InstallAPIOpts.td +++ b/clang/tools/clang-installapi/InstallAPIOpts.td @@ -29,8 +29,6 @@ def verify_mode_EQ : Joined<["--"], "verify-mode=">, HelpText<"Specify the severity and extend of the validation. Valid modes are ErrorsOnly, ErrorsAndWarnings, and Pedantic.">; def demangle : Flag<["--", "-"], "demangle">, HelpText<"Demangle symbols when printing warnings and errors">; -def dsym: Joined<["--"], "dsym=">, - MetaVarName<"">, HelpText<"Specify dSYM path for enriched diagnostics.">; // Additional input options. def extra_project_header : Separate<["-"], "extra-project-header">, diff --git a/clang/tools/clang-installapi/Options.cpp b/clang/tools/clang-installapi/Options.cpp index c4f39b7c8417..8e4a1b019fd8 100644 --- a/clang/tools/clang-installapi/Options.cpp +++ b/clang/tools/clang-installapi/Options.cpp @@ -241,9 +241,6 @@ Options::processAndFilterOutInstallAPIOptions(ArrayRef Args) { if (const Arg *A = ParsedArgs.getLastArg(OPT_verify_against)) DriverOpts.DylibToVerify = A->getValue(); - if (const Arg *A = ParsedArgs.getLastArg(OPT_dsym)) - DriverOpts.DSYMPath = A->getValue(); - // Handle exclude & extra header directories or files. auto handleAdditionalInputArgs = [&](PathSeq &Headers, clang::installapi::ID OptID) { @@ -525,8 +522,7 @@ InstallAPIContext Options::createContext() { } Ctx.Verifier = std::make_unique( - std::move(*Slices), Diags, DriverOpts.VerifyMode, DriverOpts.Demangle, - DriverOpts.DSYMPath); + std::move(*Slices), Diags, DriverOpts.VerifyMode, DriverOpts.Demangle); return Ctx; } diff --git a/clang/tools/clang-installapi/Options.h b/clang/tools/clang-installapi/Options.h index 82e04b49d125..3671e4c8274b 100644 --- a/clang/tools/clang-installapi/Options.h +++ b/clang/tools/clang-installapi/Options.h @@ -67,9 +67,6 @@ struct DriverOptions { /// \brief Output path. std::string OutputPath; - /// \brief DSYM path. - std::string DSYMPath; - /// \brief File encoding to print. FileType OutFT = FileType::TBD_V5; diff --git a/llvm/include/llvm/TextAPI/DylibReader.h b/llvm/include/llvm/TextAPI/DylibReader.h index 6861d3cb1591..b556fbf6832a 100644 --- a/llvm/include/llvm/TextAPI/DylibReader.h +++ b/llvm/include/llvm/TextAPI/DylibReader.h @@ -13,7 +13,6 @@ #ifndef LLVM_TEXTAPI_DYLIBREADER_H #define LLVM_TEXTAPI_DYLIBREADER_H -#include "llvm/ADT/StringMap.h" #include "llvm/Support/Error.h" #include "llvm/Support/MemoryBuffer.h" #include "llvm/TextAPI/ArchitectureSet.h" @@ -44,14 +43,6 @@ Expected readFile(MemoryBufferRef Buffer, const ParseOption &Opt); /// \param Buffer Data that points to dylib. Expected> get(MemoryBufferRef Buffer); -using SymbolToSourceLocMap = llvm::StringMap; -/// Get the source location for each symbol from dylib. -/// -/// \param DSYM Path to DSYM file. -/// \param T Requested target slice for dylib. -SymbolToSourceLocMap accumulateSourceLocFromDSYM(const StringRef DSYM, - const Target &T); - } // namespace llvm::MachO::DylibReader #endif // LLVM_TEXTAPI_DYLIBREADER_H diff --git a/llvm/include/llvm/TextAPI/Record.h b/llvm/include/llvm/TextAPI/Record.h index 7d721988ec3d..ef152ce43387 100644 --- a/llvm/include/llvm/TextAPI/Record.h +++ b/llvm/include/llvm/TextAPI/Record.h @@ -27,23 +27,6 @@ LLVM_ENABLE_BITMASK_ENUMS_IN_NAMESPACE(); class RecordsSlice; -// Defines lightweight source location for records. -struct RecordLoc { - RecordLoc() = default; - RecordLoc(std::string File, unsigned Line) - : File(std::move(File)), Line(Line) {} - - /// Whether there is source location tied to the RecordLoc object. - bool isValid() const { return !File.empty(); } - - bool operator==(const RecordLoc &O) const { - return std::tie(File, Line) == std::tie(O.File, O.Line); - } - - const std::string File; - const unsigned Line = 0; -}; - // Defines a list of linkage types. enum class RecordLinkage : uint8_t { // Unknown linkage. diff --git a/llvm/lib/TextAPI/BinaryReader/CMakeLists.txt b/llvm/lib/TextAPI/BinaryReader/CMakeLists.txt index c4535310d91c..cbdf7b2c9696 100644 --- a/llvm/lib/TextAPI/BinaryReader/CMakeLists.txt +++ b/llvm/lib/TextAPI/BinaryReader/CMakeLists.txt @@ -2,7 +2,6 @@ add_llvm_component_library(LLVMTextAPIBinaryReader DylibReader.cpp LINK_COMPONENTS - DebugInfoDWARF Support Object TextAPI diff --git a/llvm/lib/TextAPI/BinaryReader/DylibReader.cpp b/llvm/lib/TextAPI/BinaryReader/DylibReader.cpp index f92a2d19a63f..2e36d4a8b98c 100644 --- a/llvm/lib/TextAPI/BinaryReader/DylibReader.cpp +++ b/llvm/lib/TextAPI/BinaryReader/DylibReader.cpp @@ -12,8 +12,7 @@ #include "llvm/TextAPI/DylibReader.h" #include "llvm/ADT/STLExtras.h" -#include "llvm/DebugInfo/DWARF/DWARFCompileUnit.h" -#include "llvm/DebugInfo/DWARF/DWARFContext.h" +#include "llvm/ADT/StringMap.h" #include "llvm/Object/Binary.h" #include "llvm/Object/MachOUniversal.h" #include "llvm/Support/Endian.h" @@ -433,111 +432,3 @@ DylibReader::get(MemoryBufferRef Buffer) { return convertToInterfaceFile(*SlicesOrErr); } - -static void DWARFErrorHandler(Error Err) { /**/ } - -static SymbolToSourceLocMap -accumulateLocs(MachOObjectFile &Obj, - const std::unique_ptr &DiCtx) { - SymbolToSourceLocMap LocMap; - for (const auto &Symbol : Obj.symbols()) { - Expected FlagsOrErr = Symbol.getFlags(); - if (!FlagsOrErr) { - consumeError(FlagsOrErr.takeError()); - continue; - } - - if (!(*FlagsOrErr & SymbolRef::SF_Exported)) - continue; - - Expected AddressOrErr = Symbol.getAddress(); - if (!AddressOrErr) { - consumeError(AddressOrErr.takeError()); - continue; - } - const uint64_t Address = *AddressOrErr; - - auto TypeOrErr = Symbol.getType(); - if (!TypeOrErr) { - consumeError(TypeOrErr.takeError()); - continue; - } - const bool IsCode = (*TypeOrErr & SymbolRef::ST_Function); - - auto *DWARFCU = IsCode ? DiCtx->getCompileUnitForCodeAddress(Address) - : DiCtx->getCompileUnitForDataAddress(Address); - if (!DWARFCU) - continue; - - const DWARFDie &DIE = IsCode ? DWARFCU->getSubroutineForAddress(Address) - : DWARFCU->getVariableForAddress(Address); - const std::string File = DIE.getDeclFile( - llvm::DILineInfoSpecifier::FileLineInfoKind::AbsoluteFilePath); - const uint64_t Line = DIE.getDeclLine(); - - auto NameOrErr = Symbol.getName(); - if (!NameOrErr) { - consumeError(NameOrErr.takeError()); - continue; - } - auto Name = *NameOrErr; - auto Sym = parseSymbol(Name); - - if (!File.empty() && Line != 0) - LocMap.insert({Sym.Name.str(), RecordLoc(File, Line)}); - } - - return LocMap; -} - -SymbolToSourceLocMap -DylibReader::accumulateSourceLocFromDSYM(const StringRef DSYM, - const Target &T) { - // Find sidecar file. - auto DSYMsOrErr = MachOObjectFile::findDsymObjectMembers(DSYM); - if (!DSYMsOrErr) { - consumeError(DSYMsOrErr.takeError()); - return SymbolToSourceLocMap(); - } - if (DSYMsOrErr->empty()) - return SymbolToSourceLocMap(); - - const StringRef Path = DSYMsOrErr->front(); - auto BufOrErr = MemoryBuffer::getFile(Path); - if (auto Err = BufOrErr.getError()) - return SymbolToSourceLocMap(); - - auto BinOrErr = createBinary(*BufOrErr.get()); - if (!BinOrErr) { - consumeError(BinOrErr.takeError()); - return SymbolToSourceLocMap(); - } - // Handle single arch. - if (auto *Single = dyn_cast(BinOrErr->get())) { - auto DiCtx = DWARFContext::create( - *Single, DWARFContext::ProcessDebugRelocations::Process, nullptr, "", - DWARFErrorHandler, DWARFErrorHandler); - - return accumulateLocs(*Single, DiCtx); - } - // Handle universal companion file. - if (auto *Fat = dyn_cast(BinOrErr->get())) { - auto ObjForArch = Fat->getObjectForArch(getArchitectureName(T.Arch)); - if (!ObjForArch) { - consumeError(ObjForArch.takeError()); - return SymbolToSourceLocMap(); - } - auto MachOOrErr = ObjForArch->getAsObjectFile(); - if (!MachOOrErr) { - consumeError(MachOOrErr.takeError()); - return SymbolToSourceLocMap(); - } - auto &Obj = **MachOOrErr; - auto DiCtx = DWARFContext::create( - Obj, DWARFContext::ProcessDebugRelocations::Process, nullptr, "", - DWARFErrorHandler, DWARFErrorHandler); - - return accumulateLocs(Obj, DiCtx); - } - return SymbolToSourceLocMap(); -} -- 2.44.0.1.g9765aa7075