diff --git a/clang/include/clang/InstallAPI/DylibVerifier.h b/clang/include/clang/InstallAPI/DylibVerifier.h index 49de24763f1f9..22cdc234486cf 100644 --- a/clang/include/clang/InstallAPI/DylibVerifier.h +++ b/clang/include/clang/InstallAPI/DylibVerifier.h @@ -31,6 +31,7 @@ enum class VerificationMode { class DylibVerifier : llvm::MachO::RecordVisitor { private: struct SymbolContext; + struct DWARFContext; public: enum class Result { NoVerify, Ignore, Valid, Invalid }; @@ -54,7 +55,7 @@ class DylibVerifier : llvm::MachO::RecordVisitor { DiagnosticsEngine *Diag = nullptr; // Handle diagnostics reporting for target level violations. - void emitDiag(llvm::function_ref Report); + void emitDiag(llvm::function_ref Report, RecordLoc *Loc = nullptr); VerifierContext() = default; VerifierContext(DiagnosticsEngine *Diag) : Diag(Diag) {} @@ -63,9 +64,10 @@ class DylibVerifier : llvm::MachO::RecordVisitor { DylibVerifier() = default; DylibVerifier(llvm::MachO::Records &&Dylib, DiagnosticsEngine *Diag, - VerificationMode Mode, bool Demangle) + VerificationMode Mode, bool Demangle, StringRef DSYMPath) : Dylib(std::move(Dylib)), Mode(Mode), Demangle(Demangle), - Exports(std::make_unique()), Ctx(VerifierContext{Diag}) {} + DSYMPath(DSYMPath), Exports(std::make_unique()), + Ctx(VerifierContext{Diag}) {} Result verify(GlobalRecord *R, const FrontendAttrs *FA); Result verify(ObjCInterfaceRecord *R, const FrontendAttrs *FA); @@ -143,6 +145,12 @@ class DylibVerifier : llvm::MachO::RecordVisitor { 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; @@ -152,11 +160,17 @@ class DylibVerifier : llvm::MachO::RecordVisitor { // 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 4961c596fd68a..827220dbf39fb 100644 --- a/clang/include/clang/InstallAPI/MachO.h +++ b/clang/include/clang/InstallAPI/MachO.h @@ -34,6 +34,7 @@ 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 894db699578f2..e0bc8d969ecb3 100644 --- a/clang/lib/InstallAPI/CMakeLists.txt +++ b/clang/lib/InstallAPI/CMakeLists.txt @@ -1,6 +1,7 @@ set(LLVM_LINK_COMPONENTS Support TextAPI + TextAPIBinaryReader Demangle Core ) diff --git a/clang/lib/InstallAPI/DylibVerifier.cpp b/clang/lib/InstallAPI/DylibVerifier.cpp index ba25e4183a9b8..c0eda1d81b9b9 100644 --- a/clang/lib/InstallAPI/DylibVerifier.cpp +++ b/clang/lib/InstallAPI/DylibVerifier.cpp @@ -10,6 +10,7 @@ #include "clang/InstallAPI/FrontendRecords.h" #include "clang/InstallAPI/InstallAPIDiagnostic.h" #include "llvm/Demangle/Demangle.h" +#include "llvm/TextAPI/DylibReader.h" using namespace llvm::MachO; @@ -35,6 +36,14 @@ 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") || @@ -511,14 +520,16 @@ DylibVerifier::Result DylibVerifier::verify(GlobalRecord *R, return verifyImpl(R, SymCtx); } -void DylibVerifier::VerifierContext::emitDiag( - llvm::function_ref Report) { +void DylibVerifier::VerifierContext::emitDiag(llvm::function_ref Report, + RecordLoc *Loc) { 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(); } @@ -561,26 +572,36 @@ void DylibVerifier::visitSymbolInDylib(const Record &R, SymbolContext &SymCtx) { return; } - // All checks at this point classify as some kind of violation that should be - // reported. + 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); // Regardless of verification mode, error out on mismatched special linker // symbols. - if (SymbolName.starts_with("$ld$")) { - Ctx.emitDiag([&]() { - Ctx.Diag->Report(diag::err_header_symbol_missing) - << getAnnotatedName(&R, SymCtx, /*ValidSourceLoc=*/false); - }); + if (IsLinkerSymbol) { + Ctx.emitDiag( + [&]() { + Ctx.Diag->Report(diag::err_header_symbol_missing) + << getAnnotatedName(&R, SymCtx, Loc.isValid()); + }, + &Loc); 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, /*ValidSourceLoc=*/false); - }); + Ctx.emitDiag( + [&]() { + Ctx.Diag->Report(diag::err_header_symbol_missing) + << getAnnotatedName(&R, SymCtx, Loc.isValid()); + }, + &Loc); updateState(Result::Invalid); return; } @@ -588,10 +609,12 @@ 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, /*ValidSourceLoc=*/false); - }); + Ctx.emitDiag( + [&]() { + Ctx.Diag->Report(diag::warn_header_symbol_missing) + << getAnnotatedName(&R, SymCtx, Loc.isValid()); + }, + &Loc); updateState(Result::Ignore); return; } @@ -622,6 +645,18 @@ 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; @@ -655,6 +690,8 @@ 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 new file mode 100644 index 0000000000000..ee2c8b32df29c --- /dev/null +++ b/clang/test/InstallAPI/diagnostics-dsym.test @@ -0,0 +1,43 @@ +; 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 71532c9cf24d1..010f2507a1d1f 100644 --- a/clang/tools/clang-installapi/InstallAPIOpts.td +++ b/clang/tools/clang-installapi/InstallAPIOpts.td @@ -29,6 +29,8 @@ 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 8e4a1b019fd81..c4f39b7c84174 100644 --- a/clang/tools/clang-installapi/Options.cpp +++ b/clang/tools/clang-installapi/Options.cpp @@ -241,6 +241,9 @@ 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) { @@ -522,7 +525,8 @@ InstallAPIContext Options::createContext() { } Ctx.Verifier = std::make_unique( - std::move(*Slices), Diags, DriverOpts.VerifyMode, DriverOpts.Demangle); + std::move(*Slices), Diags, DriverOpts.VerifyMode, DriverOpts.Demangle, + DriverOpts.DSYMPath); return Ctx; } diff --git a/clang/tools/clang-installapi/Options.h b/clang/tools/clang-installapi/Options.h index 3671e4c8274bd..82e04b49d1259 100644 --- a/clang/tools/clang-installapi/Options.h +++ b/clang/tools/clang-installapi/Options.h @@ -67,6 +67,9 @@ 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 b556fbf6832a9..6861d3cb1591b 100644 --- a/llvm/include/llvm/TextAPI/DylibReader.h +++ b/llvm/include/llvm/TextAPI/DylibReader.h @@ -13,6 +13,7 @@ #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" @@ -43,6 +44,14 @@ 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 ef152ce433877..7d721988ec3da 100644 --- a/llvm/include/llvm/TextAPI/Record.h +++ b/llvm/include/llvm/TextAPI/Record.h @@ -27,6 +27,23 @@ 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 cbdf7b2c96969..c4535310d91c1 100644 --- a/llvm/lib/TextAPI/BinaryReader/CMakeLists.txt +++ b/llvm/lib/TextAPI/BinaryReader/CMakeLists.txt @@ -2,6 +2,7 @@ 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 2e36d4a8b98ce..f92a2d19a63fc 100644 --- a/llvm/lib/TextAPI/BinaryReader/DylibReader.cpp +++ b/llvm/lib/TextAPI/BinaryReader/DylibReader.cpp @@ -12,7 +12,8 @@ #include "llvm/TextAPI/DylibReader.h" #include "llvm/ADT/STLExtras.h" -#include "llvm/ADT/StringMap.h" +#include "llvm/DebugInfo/DWARF/DWARFCompileUnit.h" +#include "llvm/DebugInfo/DWARF/DWARFContext.h" #include "llvm/Object/Binary.h" #include "llvm/Object/MachOUniversal.h" #include "llvm/Support/Endian.h" @@ -432,3 +433,111 @@ 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(); +}