Skip to content

[GSYM] Callsites: Add data format support and loading from YAML #109781

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.

Sign up for GitHub

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 12 commits into from
Nov 27, 2024
Merged

[GSYM] Callsites: Add data format support and loading from YAML #109781

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
ac48f2c
[GSYM] Callsites: Add data format support and loading from YAML
Sep 24, 2024
fe23779
Address Feedback nr.1
Sep 26, 2024
cd7c30a
Address feedback nr.2
Sep 27, 2024
087cdea
Address Feedback Nr 2.1
Sep 27, 2024
d7cff1b
Address [Some] Feedback Nr 3 - Rest pending YAML format
Oct 17, 2024
cc1d314
Switch to relative return offsets
Nov 13, 2024
32f7987
Fix lit test
Nov 14, 2024
584fdf9
Address Feedback Nr 4
Nov 14, 2024
d5bd6d5
Fix Formatting
Nov 14, 2024
fda812c
Address Feedback Nr 5
Nov 14, 2024
ae391a2
Address Feedback Nr.6
Nov 26, 2024
4e7d719
Address Feedback Nr.7
Nov 26, 2024
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
72 changes: 13 additions & 59 deletions llvm/include/llvm/DebugInfo/GSYM/CallSiteInfo.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ struct FunctionsYAML;

namespace gsym {
class FileWriter;
class GsymCreator;
struct FunctionInfo;
struct CallSiteInfo {
enum Flags : uint8_t {
Expand All @@ -45,7 +46,7 @@ struct CallSiteInfo {
};

/// The return address of the call site.
uint64_t ReturnAddress;
uint64_t ReturnAddress = 0;

/// Offsets into the string table for function names regex patterns.
std::vector<uint32_t> MatchRegex;
Expand All @@ -57,12 +58,9 @@ struct CallSiteInfo {
///
/// \param Data The binary stream to read the data from.
/// \param Offset The current offset within the data stream.
/// \param BaseAddr The base address for decoding (unused here but included
/// for consistency).
///
/// \returns A CallSiteInfo or an error describing the issue.
static llvm::Expected<CallSiteInfo>
decode(DataExtractor &Data, uint64_t &Offset, uint64_t BaseAddr);
static llvm::Expected<CallSiteInfo> decode(DataExtractor &Data,
uint64_t &Offset);

/// Encode this CallSiteInfo object into a FileWriter stream.
///
Expand All @@ -77,12 +75,8 @@ struct CallSiteInfoCollection {
/// Decode a CallSiteInfoCollection object from a binary data stream.
///
/// \param Data The binary stream to read the data from.
/// \param BaseAddr The base address for decoding (unused here but included
/// for consistency).
///
/// \returns A CallSiteInfoCollection or an error describing the issue.
static llvm::Expected<CallSiteInfoCollection> decode(DataExtractor &Data,
uint64_t BaseAddr);
static llvm::Expected<CallSiteInfoCollection> decode(DataExtractor &Data);

/// Encode this CallSiteInfoCollection object into a FileWriter stream.
///
Expand All @@ -91,67 +85,34 @@ struct CallSiteInfoCollection {
llvm::Error encode(FileWriter &O) const;
};

bool operator==(const CallSiteInfoCollection &LHS,
const CallSiteInfoCollection &RHS);

bool operator==(const CallSiteInfo &LHS, const CallSiteInfo &RHS);

class CallSiteInfoLoader {
public:
/// Constructor that initializes the CallSiteInfoLoader with necessary data
/// structures.
///
/// \param StringOffsetMap A reference to a DenseMap that maps existing string
/// offsets to CachedHashStringRef. \param StrTab A reference to a
/// StringTableBuilder used for managing looking up and creating new strings.
/// \param StringStorage A reference to a StringSet for storing the data for
/// generated strings.
CallSiteInfoLoader(DenseMap<uint64_t, CachedHashStringRef> &StringOffsetMap,
StringTableBuilder &StrTab, StringSet<> &StringStorage)
: StringOffsetMap(StringOffsetMap), StrTab(StrTab),
StringStorage(StringStorage) {}

/// Loads call site information from a YAML file and populates the provided
/// FunctionInfo vector.
///
/// offsets to CachedHashStringRef.
/// \param StrTab A reference to a StringTableBuilder used for managing
/// looking up and creating new strings. \param StringStorage A reference to a
/// StringSet for storing the data for generated strings.
CallSiteInfoLoader(GsymCreator &GCreator) : GCreator(GCreator) {}

/// This method reads the specified YAML file, parses its content, and updates
/// the `Funcs` vector with call site information based on the YAML data.
///
/// \param Funcs A reference to a vector of FunctionInfo objects to be
/// populated.
/// \param YAMLFile A StringRef representing the path to the YAML
/// file to be loaded.
///
/// \returns An `llvm::Error` indicating success or describing any issues
/// encountered during the loading process.
llvm::Error loadYAML(std::vector<FunctionInfo> &Funcs, StringRef YAMLFile);

private:
/// Retrieves an existing string from the StringOffsetMap using the provided
/// offset.
///
/// \param offset A 32-bit unsigned integer representing the offset of the
/// string.
///
/// \returns A StringRef corresponding to the string for the given offset.
///
/// \note This method asserts that the offset exists in the StringOffsetMap.
StringRef stringFromOffset(uint32_t offset) const;

/// Obtains the offset corresponding to a given string in the StrTab. If the
/// string does not already exist, it is created.
///
/// \param str A StringRef representing the string for which the offset is
/// requested.
///
/// \returns A 32-bit unsigned integer representing the offset of the string.
uint32_t offsetFromString(StringRef str);

/// Builds a map from function names to FunctionInfo pointers based on the
/// provided `Funcs` vector.
///
/// \param Funcs A reference to a vector of FunctionInfo objects.
///
/// \returns A StringMap mapping function names (StringRef) to their
/// corresponding FunctionInfo pointers.
StringMap<FunctionInfo *> buildFunctionMap(std::vector<FunctionInfo> &Funcs);
Expand All @@ -162,21 +123,14 @@ class CallSiteInfoLoader {
/// object containing parsed YAML data.
/// \param FuncMap A reference to a StringMap mapping function names to
/// FunctionInfo pointers.
///
/// \returns An `llvm::Error` indicating success or describing any issues
/// encountered during processing.
llvm::Error
processYAMLFunctions(const llvm::yaml::FunctionsYAML &functionsYAML,
StringMap<FunctionInfo *> &FuncMap);

/// Map of existing string offsets to CachedHashStringRef.
DenseMap<uint64_t, CachedHashStringRef> &StringOffsetMap;

/// The gSYM string table builder.
StringTableBuilder &StrTab;

/// The gSYM string storage - we store generated strings here.
StringSet<> &StringStorage;
/// Reference to the parent Gsym Creator object.
GsymCreator &GCreator;
};

raw_ostream &operator<<(raw_ostream &OS, const CallSiteInfo &CSI);
Expand Down
9 changes: 9 additions & 0 deletions llvm/include/llvm/DebugInfo/GSYM/GsymCreator.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -329,6 +329,15 @@ class GsymCreator {
/// \returns The unique 32 bit offset into the string table.
uint32_t insertString(StringRef S, bool Copy = true);

/// Retrieve a string fromt he GSYM string table given its offset.
///
/// The offset is assumed to be a valid offset into the string table.
/// otherwise an assert will be triggered.
///
/// \param offset The offset of the string to retrieve, previously returned by
/// insertString. \returns The string at the given offset in the string table.
StringRef getString(uint32_t offset);

/// Insert a file into this GSYM creator.
///
/// Inserts a file by adding a FileEntry into the "Files" member variable if
Expand Down
92 changes: 40 additions & 52 deletions llvm/lib/DebugInfo/GSYM/CallSiteInfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#include "llvm/ADT/CachedHashString.h"
#include "llvm/DebugInfo/GSYM/FileWriter.h"
#include "llvm/DebugInfo/GSYM/FunctionInfo.h"
#include "llvm/DebugInfo/GSYM/GsymCreator.h"
#include "llvm/MC/StringTableBuilder.h"
#include "llvm/Support/DataExtractor.h"
#include "llvm/Support/YAMLParser.h"
Expand All @@ -23,17 +24,17 @@
using namespace llvm;
using namespace gsym;

llvm::Error CallSiteInfo::encode(FileWriter &O) const {
Error CallSiteInfo::encode(FileWriter &O) const {
O.writeU64(ReturnAddress);
O.writeU8(Flags);
O.writeU32(MatchRegex.size());
for (uint32_t Entry : MatchRegex)
O.writeU32(Entry);
return llvm::Error::success();
return Error::success();
}

llvm::Expected<CallSiteInfo>
CallSiteInfo::decode(DataExtractor &Data, uint64_t &Offset, uint64_t BaseAddr) {
Expected<CallSiteInfo> CallSiteInfo::decode(DataExtractor &Data,
uint64_t &Offset) {
CallSiteInfo CSI;

// Read ReturnAddress
Expand Down Expand Up @@ -68,17 +69,17 @@ CallSiteInfo::decode(DataExtractor &Data, uint64_t &Offset, uint64_t BaseAddr) {
return CSI;
}

llvm::Error CallSiteInfoCollection::encode(FileWriter &O) const {
Error CallSiteInfoCollection::encode(FileWriter &O) const {
O.writeU32(CallSites.size());
for (const CallSiteInfo &CSI : CallSites) {
if (llvm::Error Err = CSI.encode(O))
if (Error Err = CSI.encode(O))
return Err;
}
return llvm::Error::success();
return Error::success();
}

llvm::Expected<CallSiteInfoCollection>
CallSiteInfoCollection::decode(DataExtractor &Data, uint64_t BaseAddr) {
Expected<CallSiteInfoCollection>
CallSiteInfoCollection::decode(DataExtractor &Data) {
CallSiteInfoCollection CSC;
uint64_t Offset = 0;

Expand All @@ -91,8 +92,7 @@ CallSiteInfoCollection::decode(DataExtractor &Data, uint64_t BaseAddr) {

CSC.CallSites.reserve(NumCallSites);
for (uint32_t i = 0; i < NumCallSites; ++i) {
llvm::Expected<CallSiteInfo> ECSI =
CallSiteInfo::decode(Data, Offset, BaseAddr);
Expected<CallSiteInfo> ECSI = CallSiteInfo::decode(Data, Offset);
if (!ECSI)
return ECSI.takeError();
CSC.CallSites.emplace_back(*ECSI);
Expand All @@ -108,7 +108,7 @@ namespace yaml {
struct CallSiteYAML {
// The offset of the return address of the call site - relative to the start
// of the function.
llvm::yaml::Hex64 return_offset;
Hex64 return_offset;
std::vector<std::string> match_regex;
std::vector<std::string> flags;
};
Expand Down Expand Up @@ -149,34 +149,22 @@ template <> struct MappingTraits<FunctionsYAML> {
LLVM_YAML_IS_SEQUENCE_VECTOR(CallSiteYAML)
LLVM_YAML_IS_SEQUENCE_VECTOR(FunctionYAML)

// Implementation of CallSiteInfoLoader
StringRef CallSiteInfoLoader::stringFromOffset(uint32_t offset) const {
assert(StringOffsetMap.count(offset) &&
"expected function name offset to already be in StringOffsetMap");
return StringOffsetMap.find(offset)->second.val();
}

uint32_t CallSiteInfoLoader::offsetFromString(StringRef str) {
return StrTab.add(StringStorage.insert(str).first->getKey());
}

llvm::Error CallSiteInfoLoader::loadYAML(std::vector<FunctionInfo> &Funcs,
StringRef YAMLFile) {
Error CallSiteInfoLoader::loadYAML(std::vector<FunctionInfo> &Funcs,
StringRef YAMLFile) {
// Step 1: Read YAML file
auto BufferOrError = llvm::MemoryBuffer::getFile(YAMLFile);
auto BufferOrError = MemoryBuffer::getFile(YAMLFile);
if (!BufferOrError)
return errorCodeToError(BufferOrError.getError());

std::unique_ptr<llvm::MemoryBuffer> Buffer = std::move(*BufferOrError);
std::unique_ptr<MemoryBuffer> Buffer = std::move(*BufferOrError);

// Step 2: Parse YAML content
llvm::yaml::FunctionsYAML functionsYAML;
llvm::yaml::Input yin(Buffer->getMemBufferRef());
yaml::FunctionsYAML functionsYAML;
yaml::Input yin(Buffer->getMemBufferRef());
yin >> functionsYAML;
if (yin.error()) {
return llvm::createStringError(yin.error(), "Error parsing YAML file: %s\n",
Buffer->getBufferIdentifier().str().c_str());
}
if (yin.error())
return createStringError(yin.error(), "Error parsing YAML file: %s\n",
Buffer->getBufferIdentifier().str().c_str());

// Step 3: Build function map from Funcs
auto FuncMap = buildFunctionMap(Funcs);
Expand All @@ -189,7 +177,7 @@ StringMap<FunctionInfo *>
CallSiteInfoLoader::buildFunctionMap(std::vector<FunctionInfo> &Funcs) {
StringMap<FunctionInfo *> FuncMap;
auto insertFunc = [&](auto &Function) {
std::string FuncName = stringFromOffset(Function.Name).str();
StringRef FuncName = GCreator.getString(Function.Name);
// If the function name is already in the map, don't update it. This way we
// preferentially use the first encountered function. Since symbols are
// loaded from dSYM first, we end up preferring keeping track of symbols
Expand All @@ -208,19 +196,19 @@ CallSiteInfoLoader::buildFunctionMap(std::vector<FunctionInfo> &Funcs) {
return FuncMap;
}

llvm::Error CallSiteInfoLoader::processYAMLFunctions(
const llvm::yaml::FunctionsYAML &functionsYAML,
Error CallSiteInfoLoader::processYAMLFunctions(
const yaml::FunctionsYAML &functionsYAML,
StringMap<FunctionInfo *> &FuncMap) {
// For each function in the YAML file
for (const auto &FuncYAML : functionsYAML.functions) {
auto it = FuncMap.find(FuncYAML.name);
if (it == FuncMap.end()) {
return llvm::createStringError(
auto It = FuncMap.find(FuncYAML.name);
if (It == FuncMap.end())
return createStringError(
std::errc::invalid_argument,
"Can't find function '%s' specified in callsite YAML\n",
FuncYAML.name.c_str());
}
FunctionInfo *FuncInfo = it->second;

FunctionInfo *FuncInfo = It->second;
// Create a CallSiteInfoCollection if not already present
if (!FuncInfo->CallSites)
FuncInfo->CallSites = CallSiteInfoCollection();
Expand All @@ -229,30 +217,30 @@ llvm::Error CallSiteInfoLoader::processYAMLFunctions(
// Since YAML has specifies relative return offsets, add the function
// start address to make the offset absolute.
CSI.ReturnAddress = FuncInfo->Range.start() + CallSiteYAML.return_offset;
for (const auto &Regex : CallSiteYAML.match_regex)
CSI.MatchRegex.push_back(offsetFromString(Regex));
for (const auto &Regex : CallSiteYAML.match_regex) {
uint32_t StrOffset = GCreator.insertString(Regex);
CSI.MatchRegex.push_back(StrOffset);
}

// Initialize flags to None
CSI.Flags = CallSiteInfo::None;
// Parse flags and combine them
for (const auto &FlagStr : CallSiteYAML.flags) {
if (FlagStr == "InternalCall") {
CSI.Flags |= static_cast<uint8_t>(CallSiteInfo::InternalCall);
} else if (FlagStr == "ExternalCall") {
CSI.Flags |= static_cast<uint8_t>(CallSiteInfo::ExternalCall);
} else {
return llvm::createStringError(std::errc::invalid_argument,
"Unknown flag in callsite YAML: %s\n",
FlagStr.c_str());
return createStringError(std::errc::invalid_argument,
"Unknown flag in callsite YAML: %s\n",
FlagStr.c_str());
}
}
FuncInfo->CallSites->CallSites.push_back(CSI);
}
}
return llvm::Error::success();
return Error::success();
}

raw_ostream &llvm::gsym::operator<<(raw_ostream &OS, const CallSiteInfo &CSI) {
raw_ostream &gsym::operator<<(raw_ostream &OS, const CallSiteInfo &CSI) {
OS << " Return=" << HEX64(CSI.ReturnAddress);
OS << " Flags=" << HEX8(CSI.Flags);

Expand All @@ -265,8 +253,8 @@ raw_ostream &llvm::gsym::operator<<(raw_ostream &OS, const CallSiteInfo &CSI) {
return OS;
}

raw_ostream &llvm::gsym::operator<<(raw_ostream &OS,
const CallSiteInfoCollection &CSIC) {
raw_ostream &gsym::operator<<(raw_ostream &OS,
const CallSiteInfoCollection &CSIC) {
for (const auto &CS : CSIC.CallSites) {
OS << CS;
OS << "\n";
Expand Down
2 changes: 1 addition & 1 deletion llvm/lib/DebugInfo/GSYM/FunctionInfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ llvm::Expected<FunctionInfo> FunctionInfo::decode(DataExtractor &Data,

case InfoType::CallSiteInfo:
if (Expected<llvm::gsym::CallSiteInfoCollection> CI =
llvm::gsym::CallSiteInfoCollection::decode(InfoData, BaseAddr))
llvm::gsym::CallSiteInfoCollection::decode(InfoData))
FI.CallSites = std::move(CI.get());
else
return CI.takeError();
Expand Down
8 changes: 7 additions & 1 deletion llvm/lib/DebugInfo/GSYM/GsymCreator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,7 @@ llvm::Error GsymCreator::encode(FileWriter &O) const {

llvm::Error GsymCreator::loadCallSitesFromYAML(StringRef YAMLFile) {
// Use the loader to load call site information from the YAML file.
CallSiteInfoLoader Loader(StringOffsetMap, StrTab, StringStorage);
CallSiteInfoLoader Loader(*this);
return Loader.loadYAML(Funcs, YAMLFile);
}

Expand Down Expand Up @@ -385,6 +385,12 @@ uint32_t GsymCreator::insertString(StringRef S, bool Copy) {
return StrOff;
}

StringRef GsymCreator::getString(uint32_t offset) {
Copy link
Contributor

@kyulee-com kyulee-com Oct 3, 2024

Choose a reason for hiding this comment

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

Can we return an optional value like std::optional? The release build ignores asserts. I'd make it explicit for its use.

Copy link
Contributor Author

@alx32 alx32 Oct 17, 2024

Choose a reason for hiding this comment

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

This would mean that the GSYM is corrupt - do you mean we should return optional and emit an error ?

Copy link
Contributor Author

@alx32 alx32 Nov 13, 2024

Choose a reason for hiding this comment

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

@kyulee-com could you clarify what you meant here ?

Copy link
Contributor

@kyulee-com kyulee-com Nov 14, 2024

Choose a reason for hiding this comment

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

I'm fine with not using optional value.
As for the assertion below, you could do this way.

  auto I = StringOffsetMap.find(offset);
  assert(I != StringOffsetMap.end() && 
         "GsymCreator::getString expects a valid offset as parameter.");
  return I->second.val();

Copy link
Contributor

@ellishg ellishg Nov 14, 2024

Choose a reason for hiding this comment

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

If you use StringOffsetMap->at(offset) then it will abort if the key doesn't exist. However you don't get to pass a custom error message.

assert(StringOffsetMap.count(offset) &&
"GsymCreator::getString expects a valid offset as parameter.");
return StringOffsetMap.find(offset)->second.val();
}

void GsymCreator::addFunctionInfo(FunctionInfo &&FI) {
std::lock_guard<std::mutex> Guard(Mutex);
Funcs.emplace_back(std::move(FI));
Expand Down
Loading
Loading