Address feedback nr.2

Author: Alex B

llvm/include/llvm/DebugInfo/GSYM/CallSiteInfo.h

@@ -32,6 +32,7 @@ struct FunctionsYAML;
 
 namespace gsym {
 class FileWriter;
+class GsymCreator;
 struct FunctionInfo;
 struct CallSiteInfo {
   enum Flags : uint8_t {
@@ -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;
@@ -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.
   ///
@@ -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.
   ///
@@ -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);
@@ -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);

llvm/include/llvm/DebugInfo/GSYM/GsymCreator.h

@@ -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

llvm/lib/DebugInfo/GSYM/CallSiteInfo.cpp

@@ -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"
@@ -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
@@ -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;
 
@@ -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);
@@ -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;
 };
@@ -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);
@@ -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
@@ -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();
@@ -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);
 
@@ -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";

llvm/lib/DebugInfo/GSYM/FunctionInfo.cpp

@@ -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();

llvm/lib/DebugInfo/GSYM/GsymCreator.cpp

@@ -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);
 }
 
@@ -385,6 +385,12 @@ uint32_t GsymCreator::insertString(StringRef S, bool Copy) {
   return StrOff;
 }
 
+StringRef GsymCreator::getString(uint32_t offset) {
+  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));