Skip to content

[InstallAPI] Add support for parsing dSYMs #86852

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Mar 29, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 17 additions & 3 deletions clang/include/clang/InstallAPI/DylibVerifier.h
Original file line number Diff line number Diff line change
Expand Up @@ -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 };
Expand All @@ -54,7 +55,7 @@ class DylibVerifier : llvm::MachO::RecordVisitor {
DiagnosticsEngine *Diag = nullptr;

// Handle diagnostics reporting for target level violations.
void emitDiag(llvm::function_ref<void()> Report);
void emitDiag(llvm::function_ref<void()> Report, RecordLoc *Loc = nullptr);

VerifierContext() = default;
VerifierContext(DiagnosticsEngine *Diag) : Diag(Diag) {}
Expand All @@ -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<SymbolSet>()), Ctx(VerifierContext{Diag}) {}
DSYMPath(DSYMPath), Exports(std::make_unique<SymbolSet>()),
Ctx(VerifierContext{Diag}) {}

Result verify(GlobalRecord *R, const FrontendAttrs *FA);
Result verify(ObjCInterfaceRecord *R, const FrontendAttrs *FA);
Expand Down Expand Up @@ -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;

Expand All @@ -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<SymbolSet> Exports = std::make_unique<SymbolSet>();

// Track current state of verification while traversing AST.
VerifierContext Ctx;

// Track DWARF provided source location for dylibs.
DWARFContext *DWARFCtx = nullptr;
};

} // namespace installapi
Expand Down
1 change: 1 addition & 0 deletions clang/include/clang/InstallAPI/MachO.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
1 change: 1 addition & 0 deletions clang/lib/InstallAPI/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
set(LLVM_LINK_COMPONENTS
Support
TextAPI
TextAPIBinaryReader
Demangle
Core
)
Expand Down
71 changes: 54 additions & 17 deletions clang/lib/InstallAPI/DylibVerifier.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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") ||
Expand Down Expand Up @@ -511,14 +520,16 @@ DylibVerifier::Result DylibVerifier::verify(GlobalRecord *R,
return verifyImpl(R, SymCtx);
}

void DylibVerifier::VerifierContext::emitDiag(
llvm::function_ref<void()> Report) {
void DylibVerifier::VerifierContext::emitDiag(llvm::function_ref<void()> 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();
}
Expand Down Expand Up @@ -561,37 +572,49 @@ 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;
}

// 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;
}
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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<RecordsSlice> Slice : Dylib) {
Expand Down
43 changes: 43 additions & 0 deletions clang/test/InstallAPI/diagnostics-dsym.test
Original file line number Diff line number Diff line change
@@ -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 \
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JDevlieghere Is there a better/known practice for generating full dylibs with debug info for tests? Hopefully, PR CI will let me know, but I'm a little worried about different linker semantics.
e.g. Xcode's ld requires all userspace dylibs link against libSystem.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the linker on the linux bot doesn't support building Darwin anyway

/usr/bin/ld: unrecognised emulation mode: llvm
Supported emulations: elf_x86_64 elf32_x86_64 elf_i386 elf_iamcu elf_l1om elf_k1om i386pep i386pe
clang: error: linker command failed with exit code 1 (use -v to see invocation)

Perhaps I'll require Darwin env to run the test and then I won't need to introduce a dsymutil dependency at all since it can come from xcode.

; 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
}

2 changes: 2 additions & 0 deletions clang/tools/clang-installapi/InstallAPIOpts.td
Original file line number Diff line number Diff line change
Expand Up @@ -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<"<path>">, HelpText<"Specify dSYM path for enriched diagnostics.">;

// Additional input options.
def extra_project_header : Separate<["-"], "extra-project-header">,
Expand Down
6 changes: 5 additions & 1 deletion clang/tools/clang-installapi/Options.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,9 @@ Options::processAndFilterOutInstallAPIOptions(ArrayRef<const char *> 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) {
Expand Down Expand Up @@ -522,7 +525,8 @@ InstallAPIContext Options::createContext() {
}

Ctx.Verifier = std::make_unique<DylibVerifier>(
std::move(*Slices), Diags, DriverOpts.VerifyMode, DriverOpts.Demangle);
std::move(*Slices), Diags, DriverOpts.VerifyMode, DriverOpts.Demangle,
DriverOpts.DSYMPath);
return Ctx;
}

Expand Down
3 changes: 3 additions & 0 deletions clang/tools/clang-installapi/Options.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
9 changes: 9 additions & 0 deletions llvm/include/llvm/TextAPI/DylibReader.h
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -43,6 +44,14 @@ Expected<Records> readFile(MemoryBufferRef Buffer, const ParseOption &Opt);
/// \param Buffer Data that points to dylib.
Expected<std::unique_ptr<InterfaceFile>> get(MemoryBufferRef Buffer);

using SymbolToSourceLocMap = llvm::StringMap<RecordLoc>;
/// 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
17 changes: 17 additions & 0 deletions llvm/include/llvm/TextAPI/Record.h
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
1 change: 1 addition & 0 deletions llvm/lib/TextAPI/BinaryReader/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ add_llvm_component_library(LLVMTextAPIBinaryReader
DylibReader.cpp

LINK_COMPONENTS
DebugInfoDWARF
Support
Object
TextAPI
Expand Down
Loading