[GSYM] Callsites: Add data format support and loading from YAML #109781
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
alx32
force-pushed
the
09_gsym_call_sites
branch
from
2fda9a1 to
72855ae
Compare
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
alx32
force-pushed
the
09_gsym_call_sites
branch
from
72855ae to
ac48f2c
Compare
|
@llvm/pr-subscribers-debuginfo Author: None (alx32) ChangesThis PR adds support in the gSYM format for call site information and adds support for loading call sites from a YAML file. The support for YAML input is mostly for testing purposes - so we have a way to test the functionality. Note that this data is not currently used in the gSYM tooling - the logic to use call sites will be added in a later PR. The reason why we need call site information in gSYM files is so that we can support better call stack function disambiguation in the case where multiple functions have been merged due to optimization (linker ICF). When resolving a merged function on the callstack, we can use the call site information of the calling function to narrow down the actual function that is being called, from the set of all merged functions. See this RFC for more details on this change. Patch is 101.35 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/109781.diff 15 Files Affected:
diff --git a/llvm/include/llvm/DebugInfo/GSYM/CallSiteInfo.h b/llvm/include/llvm/DebugInfo/GSYM/CallSiteInfo.h
new file mode 100644
index 00000000000000..45257f0e11578e
--- /dev/null
+++ b/llvm/include/llvm/DebugInfo/GSYM/CallSiteInfo.h
@@ -0,0 +1,224 @@
+//===- CallSiteInfo.h -------------------------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_DEBUGINFO_GSYM_CALLSITEINFO_H
+#define LLVM_DEBUGINFO_GSYM_CALLSITEINFO_H
+
+#include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/StringSet.h"
+#include "llvm/DebugInfo/GSYM/ExtractRanges.h"
+#include "llvm/Support/YAMLParser.h"
+#include <string>
+#include <unordered_map>
+#include <vector>
+
+namespace llvm {
+class DataExtractor;
+class raw_ostream;
+class StringTableBuilder;
+class CachedHashStringRef;
+
+namespace yaml {
+struct CallSiteYAML;
+struct FunctionYAML;
+struct FunctionsYAML;
+} // namespace yaml
+
+namespace gsym {
+class FileWriter;
+struct FunctionInfo;
+struct CallSiteInfo {
+public:
+ enum Flags : uint8_t {
+ None = 0,
+ // This flag specifies that the call site can only call a function within
+ // the same link unit as the call site.
+ InternalCall = 1 << 0,
+ // This flag specifies that the call site can only call a function outside
+ // the link unit that the call site is in.
+ ExternalCall = 1 << 1,
+ };
+
+ /// The return address of the call site.
+ uint64_t ReturnAddress;
+
+ /// Offsets into the string table for function names regex patterns.
+ std::vector<uint32_t> MatchRegex;
+
+ /// Bitwise OR of CallSiteInfo::Flags values
+ uint8_t Flags;
+
+ /// Decode a CallSiteInfo object from a binary data stream.
+ ///
+ /// \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);
+
+ /// Encode this CallSiteInfo object into a FileWriter stream.
+ ///
+ /// \param O The binary stream to write the data to.
+ /// \returns An error object that indicates success or failure.
+ llvm::Error encode(FileWriter &O) const;
+};
+
+struct CallSiteInfoCollection {
+public:
+ std::vector<CallSiteInfo> CallSites;
+
+ void clear() { CallSites.clear(); }
+
+ /// Query if a CallSiteInfoCollection object is valid.
+ ///
+ /// \returns True if the collection is not empty.
+ bool isValid() const { return !CallSites.empty(); }
+
+ /// 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);
+
+ /// Encode this CallSiteInfoCollection object into a FileWriter stream.
+ ///
+ /// \param O The binary stream to write the data to.
+ /// \returns An error object that indicates success or failure.
+ 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.
+ ///
+ /// 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);
+
+ /// Reads the content of the YAML file specified by `YAMLFile` into
+ /// `yamlContent`.
+ ///
+ /// \param YAMLFile A StringRef representing the path to the YAML file.
+ /// \param Buffer The memory buffer containing the YAML content.
+ ///
+ /// \returns An `llvm::Error` indicating success or describing any issues
+ /// encountered while reading the file.
+ llvm::Error readYAMLFile(StringRef YAMLFile,
+ std::unique_ptr<llvm::MemoryBuffer> &Buffer);
+
+ /// Parses the YAML content and populates `functionsYAML` with the parsed
+ /// data.
+ ///
+ /// \param Buffer The memory buffer containing the YAML content.
+ /// \param functionsYAML A reference to an llvm::yaml::FunctionsYAML object to
+ /// be populated.
+ ///
+ /// \returns An `llvm::Error` indicating success or describing any issues
+ /// encountered during parsing.
+ llvm::Error parseYAML(llvm::MemoryBuffer &Buffer,
+ llvm::yaml::FunctionsYAML &functionsYAML);
+
+ /// 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 An unordered_map mapping function names (std::string) to their
+ /// corresponding FunctionInfo pointers.
+ std::unordered_map<std::string, FunctionInfo *>
+ buildFunctionMap(std::vector<FunctionInfo> &Funcs);
+
+ /// Processes the parsed YAML functions and updates the `FuncMap` accordingly.
+ ///
+ /// \param functionsYAML A constant reference to an llvm::yaml::FunctionsYAML
+ /// object containing parsed YAML data.
+ /// \param FuncMap A reference to an unordered_map mapping function names to
+ /// FunctionInfo pointers.
+ /// \param YAMLFile A StringRef representing the name of the YAML file (used
+ /// for error messages).
+ ///
+ /// \returns An `llvm::Error` indicating success or describing any issues
+ /// encountered during processing.
+ llvm::Error
+ processYAMLFunctions(const llvm::yaml::FunctionsYAML &functionsYAML,
+ std::unordered_map<std::string, FunctionInfo *> &FuncMap,
+ StringRef YAMLFile);
+
+ /// 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;
+};
+
+raw_ostream &operator<<(raw_ostream &OS, const CallSiteInfo &CSI);
+raw_ostream &operator<<(raw_ostream &OS, const CallSiteInfoCollection &CSIC);
+
+} // namespace gsym
+} // namespace llvm
+
+#endif // LLVM_DEBUGINFO_GSYM_CALLSITEINFO_H
diff --git a/llvm/include/llvm/DebugInfo/GSYM/FunctionInfo.h b/llvm/include/llvm/DebugInfo/GSYM/FunctionInfo.h
index 71209b6b5c9cd1..fd4ac3164c686d 100644
--- a/llvm/include/llvm/DebugInfo/GSYM/FunctionInfo.h
+++ b/llvm/include/llvm/DebugInfo/GSYM/FunctionInfo.h
@@ -10,6 +10,7 @@
#define LLVM_DEBUGINFO_GSYM_FUNCTIONINFO_H
#include "llvm/ADT/SmallString.h"
+#include "llvm/DebugInfo/GSYM/CallSiteInfo.h"
#include "llvm/DebugInfo/GSYM/ExtractRanges.h"
#include "llvm/DebugInfo/GSYM/InlineInfo.h"
#include "llvm/DebugInfo/GSYM/LineTable.h"
@@ -63,7 +64,9 @@ class GsymReader;
/// enum InfoType {
/// EndOfList = 0u,
/// LineTableInfo = 1u,
-/// InlineInfo = 2u
+/// InlineInfo = 2u,
+/// MergedFunctionsInfo = 3u,
+/// CallSiteInfo = 4u
/// };
///
/// This stream of tuples is terminated by a "InfoType" whose value is
@@ -73,7 +76,7 @@ class GsymReader;
/// clients to still parse the format and skip over any data that they don't
/// understand or want to parse.
///
-/// So the function information encoding essientially looks like:
+/// So the function information encoding essentially looks like:
///
/// struct {
/// uint32_t Size;
@@ -92,6 +95,7 @@ struct FunctionInfo {
std::optional<LineTable> OptLineTable;
std::optional<InlineInfo> Inline;
std::optional<MergedFunctionsInfo> MergedFunctions;
+ std::optional<CallSiteInfoCollection> CallSites;
/// If we encode a FunctionInfo during segmenting so we know its size, we can
/// cache that encoding here so we don't need to re-encode it when saving the
/// GSYM file.
@@ -107,7 +111,7 @@ struct FunctionInfo {
/// debug info, we might end up with multiple FunctionInfo objects for the
/// same range and we need to be able to tell which one is the better object
/// to use.
- bool hasRichInfo() const { return OptLineTable || Inline; }
+ bool hasRichInfo() const { return OptLineTable || Inline || CallSites; }
/// Query if a FunctionInfo object is valid.
///
diff --git a/llvm/include/llvm/DebugInfo/GSYM/GsymCreator.h b/llvm/include/llvm/DebugInfo/GSYM/GsymCreator.h
index 48808fb7b71e18..9e5b3c1f8d92de 100644
--- a/llvm/include/llvm/DebugInfo/GSYM/GsymCreator.h
+++ b/llvm/include/llvm/DebugInfo/GSYM/GsymCreator.h
@@ -352,13 +352,22 @@ class GsymCreator {
/// \param FI The function info object to emplace into our functions list.
void addFunctionInfo(FunctionInfo &&FI);
+ /// Load call site information from a YAML file.
+ ///
+ /// This function reads call site information from a specified YAML file and
+ /// adds it to the GSYM data.
+ ///
+ /// \param YAMLFile The path to the YAML file containing call site
+ /// information.
+ llvm::Error loadCallSitesFromYAML(StringRef YAMLFile);
+
/// Organize merged FunctionInfo's
///
/// This method processes the list of function infos (Funcs) to identify and
/// group functions with overlapping address ranges.
///
/// \param Out Output stream to report information about how merged
- /// FunctionInfo's were handeled.
+ /// FunctionInfo's were handled.
void prepareMergedFunctions(OutputAggregator &Out);
/// Finalize the data in the GSYM creator prior to saving the data out.
diff --git a/llvm/include/llvm/DebugInfo/GSYM/GsymReader.h b/llvm/include/llvm/DebugInfo/GSYM/GsymReader.h
index 89f8c043b91519..72b7f3e7bfc42e 100644
--- a/llvm/include/llvm/DebugInfo/GSYM/GsymReader.h
+++ b/llvm/include/llvm/DebugInfo/GSYM/GsymReader.h
@@ -181,6 +181,26 @@ class GsymReader {
/// \param MFI The object to dump.
void dump(raw_ostream &OS, const MergedFunctionsInfo &MFI);
+ /// Dump a CallSiteInfo object.
+ ///
+ /// This function will output the details of a CallSiteInfo object in a
+ /// human-readable format.
+ ///
+ /// \param OS The output stream to dump to.
+ ///
+ /// \param CSI The CallSiteInfo object to dump.
+ void dump(raw_ostream &OS, const CallSiteInfo &CSI);
+
+ /// Dump a CallSiteInfoCollection object.
+ ///
+ /// This function will iterate over a collection of CallSiteInfo objects and
+ /// dump each one.
+ ///
+ /// \param OS The output stream to dump to.
+ ///
+ /// \param CSIC The CallSiteInfoCollection object to dump.
+ void dump(raw_ostream &OS, const CallSiteInfoCollection &CSIC);
+
/// Dump a LineTable object.
///
/// This function will convert any string table indexes and file indexes
diff --git a/llvm/lib/DebugInfo/GSYM/CMakeLists.txt b/llvm/lib/DebugInfo/GSYM/CMakeLists.txt
index be90bfdaa7fd2b..c27d648db62f61 100644
--- a/llvm/lib/DebugInfo/GSYM/CMakeLists.txt
+++ b/llvm/lib/DebugInfo/GSYM/CMakeLists.txt
@@ -8,6 +8,7 @@ add_llvm_component_library(LLVMDebugInfoGSYM
InlineInfo.cpp
LineTable.cpp
LookupResult.cpp
+ CallSiteInfo.cpp
MergedFunctionsInfo.cpp
ObjectFileTransformer.cpp
ExtractRanges.cpp
diff --git a/llvm/lib/DebugInfo/GSYM/CallSiteInfo.cpp b/llvm/lib/DebugInfo/GSYM/CallSiteInfo.cpp
new file mode 100644
index 00000000000000..4ed3d3f67a44fd
--- /dev/null
+++ b/llvm/lib/DebugInfo/GSYM/CallSiteInfo.cpp
@@ -0,0 +1,293 @@
+//===- CallSiteInfo.cpp ----------------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "llvm/DebugInfo/GSYM/CallSiteInfo.h"
+#include "llvm/ADT/CachedHashString.h"
+#include "llvm/DebugInfo/GSYM/FileWriter.h"
+#include "llvm/DebugInfo/GSYM/FunctionInfo.h"
+#include "llvm/MC/StringTableBuilder.h"
+#include "llvm/Support/DataExtractor.h"
+#include "llvm/Support/YAMLParser.h"
+#include "llvm/Support/YAMLTraits.h"
+#include "llvm/Support/raw_ostream.h"
+#include <fstream>
+#include <string>
+#include <unordered_map>
+#include <vector>
+
+using namespace llvm;
+using namespace gsym;
+
+llvm::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();
+}
+
+llvm::Expected<CallSiteInfo>
+CallSiteInfo::decode(DataExtractor &Data, uint64_t &Offset, uint64_t BaseAddr) {
+ CallSiteInfo CSI;
+
+ // Read ReturnAddress
+ if (!Data.isValidOffsetForDataOfSize(Offset, sizeof(uint64_t)))
+ return createStringError(std::errc::io_error,
+ "0x%8.8" PRIx64 ": missing ReturnAddress", Offset);
+ CSI.ReturnAddress = Data.getU64(&Offset);
+
+ // Read Flags
+ if (!Data.isValidOffsetForDataOfSize(Offset, sizeof(uint8_t)))
+ return createStringError(std::errc::io_error,
+ "0x%8.8" PRIx64 ": missing Flags", Offset);
+ CSI.Flags = Data.getU8(&Offset);
+
+ // Read number of MatchRegex entries
+ if (!Data.isValidOffsetForDataOfSize(Offset, sizeof(uint32_t)))
+ return createStringError(std::errc::io_error,
+ "0x%8.8" PRIx64 ": missing MatchRegex count",
+ Offset);
+ uint32_t NumEntries = Data.getU32(&Offset);
+
+ CSI.MatchRegex.reserve(NumEntries);
+ for (uint32_t i = 0; i < NumEntries; ++i) {
+ if (!Data.isValidOffsetForDataOfSize(Offset, sizeof(uint32_t)))
+ return createStringError(std::errc::io_error,
+ "0x%8.8" PRIx64 ": missing MatchRegex entry",
+ Offset);
+ uint32_t Entry = Data.getU32(&Offset);
+ CSI.MatchRegex.push_back(Entry);
+ }
+
+ return CSI;
+}
+
+llvm::Error CallSiteInfoCollection::encode(FileWriter &O) const {
+ O.writeU32(CallSites.size());
+ for (const CallSiteInfo &CSI : CallSites) {
+ if (llvm::Error Err = CSI.encode(O))
+ return Err;
+ }
+ return llvm::Error::success();
+}
+
+llvm::Expected<CallSiteInfoCollection>
+CallSiteInfoCollection::decode(DataExtractor &Data, uint64_t BaseAddr) {
+ CallSiteInfoCollection CSC;
+ uint64_t Offset = 0;
+
+ // Read number of CallSiteInfo entries
+ if (!Data.isValidOffsetForDataOfSize(Offset, sizeof(uint32_t)))
+ return createStringError(std::errc::io_error,
+ "0x%8.8" PRIx64 ": missing CallSiteInfo count",
+ Offset);
+ uint32_t NumCallSites = Data.getU32(&Offset);
+
+ CSC.CallSites.reserve(NumCallSites);
+ for (uint32_t i = 0; i < NumCallSites; ++i) {
+ llvm::Expected<CallSiteInfo> ECSI =
+ CallSiteInfo::decode(Data, Offset, BaseAddr);
+ if (!ECSI)
+ return ECSI.takeError();
+ CSC.CallSites.emplace_back(*ECSI);
+ }
+
+ return CSC;
+}
+
+/// Structures necessary for reading CallSiteInfo from YAML.
+namespace llvm {
+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;
+ std::vector<std::string> match_regex;
+ std::vector<std::string> flags;
+};
+
+struct FunctionYAML {
+ std::string name;
+ std::vector<CallSiteYAML> callsites;
+};
+
+struct FunctionsYAML {
+ std::vector<FunctionYAML> functions;
+};
+
+template <> struct MappingTraits<CallSiteYAML> {
+ static void mapping(IO &io, CallSiteYAML &callsite) {
+ io.mapRequired("return_offset", callsite.return_offset);
+ io.mapRequired("match_regex", callsite.match_regex);
+ io.mapOptional("flags", callsite.flags);
+ }
+};
+
+template <> struct MappingTraits<FunctionYAML> {
+ static void mapping(IO &io, FunctionYAML &func) {
+ io.mapRequired("name", func.name);
+ io.mapOptional("callsites", func.callsites);
+ }
+};
+
+template <> struct MappingTraits<FunctionsYAML> {
+ static void mapping(IO &io, FunctionsYAML &functionsYAML) {
+ io.mapRequired("functions", functionsYAML.functions);
+ }
+};
+
+} // namespace yaml
+} // namespace llvm
+
+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) {
+ std::unique_ptr<llvm::MemoryBuffer> Buffer;
+ // Step 1: Read YAML file
+ if (auto Err = readYAMLFile(YAMLFile, Buffer))
+ return Err;
+
+ // Step 2: Parse YAML content
+ llvm::yaml::FunctionsYAML functionsYAML;
+ if (auto Err = parseYAML(*Buffer, functionsYAML))
+ return Err;
+
+ // Step 3: Build function map from Funcs
+ auto FuncMap = buildFunctionMap(Funcs);
+
+ // Step 4: Process parsed YAML functions and update FuncMap
+ return processYAMLFunctions(functionsYAML, FuncMap, YAMLFile);
+}
+
+llvm::Error
+CallSiteInfoLoader::readYAMLFile(StringRef YAMLFile,
+ std::unique_ptr<llvm::MemoryBuffer> &Buffer) {
+ auto BufferOrError = llvm::MemoryBuffer::getFile(YAMLFile);
+ if (!BufferOrError)
+ return errorCodeToError(BufferOrError.getError());
+ Buffer = std::move(*BufferOrError);
+ return llvm::Error::success();
+}
+
+llvm::Error
+CallSiteInfoLoader::parseYAML(llvm::MemoryBuffer &Buffer,
+ llvm::yaml::FunctionsYAML &functionsYAML) {
+ // Use the MemoryBufferRef constructor
+ llvm::yaml::Input yin(Buffer.getMemBufferRef());
+ yin >> functionsYAML...
[truncated]
|
kyulee-com
reviewed
Sep 26, 2024
kyulee-com
reviewed
Sep 27, 2024
kyulee-com
reviewed
Oct 3, 2024
| @@ -385,6 +385,12 @@ uint32_t GsymCreator::insertString(StringRef S, bool Copy) { | |||
| return StrOff; | |||
| } | |||
|
|
|||
| StringRef GsymCreator::getString(uint32_t offset) { | |||
There was a problem hiding this comment.
Can we return an optional value like std::optional? The release build ignores asserts. I'd make it explicit for its use.
There was a problem hiding this comment.
This would mean that the GSYM is corrupt - do you mean we should return optional and emit an error ?
There was a problem hiding this comment.
@kyulee-com could you clarify what you meant here ?
There was a problem hiding this comment.
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();
There was a problem hiding this comment.
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.
clayborg
requested changes
Oct 15, 2024
There was a problem hiding this comment.
The main concern is adding data that contains full addresses anywhere in the FunctionInfo encodings. If we can avoid that and encode offsets, that will be good, but it only works is addresses are relative to the start address of the function's address range. We have no relocations in FunctionInfo prior to merge function info or call site info and I would like to keep it that way if possible.
kyulee-com
reviewed
Nov 14, 2024
| @@ -385,6 +385,12 @@ uint32_t GsymCreator::insertString(StringRef S, bool Copy) { | |||
| return StrOff; | |||
| } | |||
|
|
|||
| StringRef GsymCreator::getString(uint32_t offset) { | |||
There was a problem hiding this comment.
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();
|
Is the failure real? |
They were, should be fixed now. |
added 2 commits
November 14, 2024 11:03
ellishg
reviewed
Nov 14, 2024
| @@ -385,6 +385,12 @@ uint32_t GsymCreator::insertString(StringRef S, bool Copy) { | |||
| return StrOff; | |||
| } | |||
|
|
|||
| StringRef GsymCreator::getString(uint32_t offset) { | |||
There was a problem hiding this comment.
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.
kyulee-com
approved these changes
Nov 15, 2024
There was a problem hiding this comment.
Overall, it looks good to me. Since this is primarily for testing a new feature, I won't go into every single detail. However, I'll leave it to others, especially @clayborg, to approve this approach, too.
clayborg
requested changes
Nov 22, 2024
clayborg
approved these changes
Nov 26, 2024
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/146/builds/1698 Here is the relevant piece of the build log for the reference |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds support in the gSYM format for call site information and adds support for loading call sites from a YAML file. The support for YAML input is mostly for testing purposes - so we have a way to test the functionality.
Note that this data is not currently used in the gSYM tooling - the logic to use call sites will be added in a later PR.
The reason why we need call site information in gSYM files is so that we can support better call stack function disambiguation in the case where multiple functions have been merged due to optimization (linker ICF). When resolving a merged function on the callstack, we can use the call site information of the calling function to narrow down the actual function that is being called, from the set of all merged functions.
See this RFC for more details on this change.