Address Feedback Nr.1

Author: Alex B

llvm/include/llvm/DebugInfo/GSYM/FunctionInfo.h

@@ -187,13 +187,17 @@ struct FunctionInfo {
   ///
   /// \param Addr The address to lookup.
   ///
+  /// \param MergedFuncsData A pointer to an optional DataExtractor that, if
+  /// non-null, will be set to the raw data of the MergedFunctionInfo, if
+  /// present.
+  ///
   /// \returns An LookupResult or an error describing the issue that was
   /// encountered during decoding. An error should only be returned if the
   /// address is not contained in the FunctionInfo or if the data is corrupted.
-  static llvm::Expected<LookupResult> lookup(DataExtractor &Data,
-                                             const GsymReader &GR,
-                                             uint64_t FuncAddr,
-                                             uint64_t Addr);
+  static llvm::Expected<LookupResult>
+  lookup(DataExtractor &Data, const GsymReader &GR, uint64_t FuncAddr,
+         uint64_t Addr,
+         std::optional<DataExtractor> *MergedFuncsData = nullptr);
 
   uint64_t startAddress() const { return Range.start(); }
   uint64_t endAddress() const { return Range.end(); }

llvm/include/llvm/DebugInfo/GSYM/GsymReader.h

@@ -127,10 +127,17 @@ class GsymReader {
   /// is much faster for lookups.
   ///
   /// \param Addr A virtual address from the orignal object file to lookup.
+  ///
+  /// \param MergedFuncsData A pointer to an optional DataExtractor that, if
+  /// non-null, will be set to the raw data of the MergedFunctionInfo, if
+  /// present.
+  ///
   /// \returns An expected LookupResult that contains only the information
   /// needed for the current address, or an error object that indicates reason
   /// for failing to lookup the address.
-  llvm::Expected<LookupResult> lookup(uint64_t Addr) const;
+  llvm::Expected<LookupResult>
+  lookup(uint64_t Addr,
+         std::optional<DataExtractor> *MergedFuncsData = nullptr) const;
 
   /// Lookup all merged functions for a given address.
   ///
@@ -139,6 +146,7 @@ class GsymReader {
   /// with the primary LookupResult.
   ///
   /// \param Addr The address to lookup.
+  ///
   /// \returns A vector of LookupResult objects, where the first element is the
   /// primary result, followed by results for any merged functions
   llvm::Expected<std::vector<LookupResult>> lookupAll(uint64_t Addr) const;

llvm/include/llvm/DebugInfo/GSYM/LookupResult.h

@@ -11,7 +11,6 @@
 
 #include "llvm/ADT/AddressRanges.h"
 #include "llvm/ADT/StringRef.h"
-#include "llvm/Support/DataExtractor.h"
 #include <inttypes.h>
 #include <vector>
 
@@ -51,17 +50,6 @@ struct LookupResult {
   /// array, and the concrete function will appear at the end of the array.
   SourceLocations Locations;
   std::string getSourceFile(uint32_t Index) const;
-
-  /// Optional DataExtractor containing the merged functions data.
-  /// This is only populated during lookups if merged function information
-  /// was present. This is an optimization to avoid parsing the
-  /// MergedFunctionsInfo data unless needed.
-  std::optional<DataExtractor> MergedFunctionsData;
-
-  /// The binary data used to decode the FunctionInfo from which this
-  /// LookupResult was created. This can be used to re-decode the entire
-  /// FunctionInfo if desired.
-  std::optional<DataExtractor> FunctionInfoData;
 };
 
 inline bool operator==(const LookupResult &LHS, const LookupResult &RHS) {

llvm/lib/DebugInfo/GSYM/FunctionInfo.cpp

@@ -235,13 +235,12 @@ llvm::Expected<uint64_t> FunctionInfo::encode(FileWriter &Out,
   return FuncInfoOffset;
 }
 
-llvm::Expected<LookupResult> FunctionInfo::lookup(DataExtractor &Data,
-                                                  const GsymReader &GR,
-                                                  uint64_t FuncAddr,
-                                                  uint64_t Addr) {
+llvm::Expected<LookupResult>
+FunctionInfo::lookup(DataExtractor &Data, const GsymReader &GR,
+                     uint64_t FuncAddr, uint64_t Addr,
+                     std::optional<DataExtractor> *MergedFuncsData) {
   LookupResult LR;
   LR.LookupAddr = Addr;
-  LR.FunctionInfoData = Data;
   uint64_t Offset = 0;
   LR.FuncRange = {FuncAddr, FuncAddr + Data.getU32(&Offset)};
   uint32_t NameOffset = Data.getU32(&Offset);
@@ -292,7 +291,8 @@ llvm::Expected<LookupResult> FunctionInfo::lookup(DataExtractor &Data,
 
       case InfoType::MergedFunctionsInfo:
         // Store the merged functions data for later parsing, if needed.
-        LR.MergedFunctionsData = InfoData;
+        if (MergedFuncsData)
+          *MergedFuncsData = InfoData;
         break;
 
       case InfoType::InlineInfo:

llvm/lib/DebugInfo/GSYM/GsymReader.cpp

@@ -334,36 +334,39 @@ GsymReader::getFunctionInfoAtIndex(uint64_t Idx) const {
     return ExpectedData.takeError();
 }
 
-llvm::Expected<LookupResult> GsymReader::lookup(uint64_t Addr) const {
+llvm::Expected<LookupResult>
+GsymReader::lookup(uint64_t Addr,
+                   std::optional<DataExtractor> *MergedFunctionsData) const {
   uint64_t FuncStartAddr = 0;
   if (auto ExpectedData = getFunctionInfoDataForAddress(Addr, FuncStartAddr))
-    return FunctionInfo::lookup(*ExpectedData, *this, FuncStartAddr, Addr);
+    return FunctionInfo::lookup(*ExpectedData, *this, FuncStartAddr, Addr,
+                                MergedFunctionsData);
   else
     return ExpectedData.takeError();
 }
 
 llvm::Expected<std::vector<LookupResult>>
 GsymReader::lookupAll(uint64_t Addr) const {
   std::vector<LookupResult> Results;
+  std::optional<DataExtractor> MergedFunctionsData;
 
-  // First perform a lookup to get the primary function info result
-  auto MainResult = lookup(Addr);
+  // First perform a lookup to get the primary function info result.
+  auto MainResult = lookup(Addr, &MergedFunctionsData);
   if (!MainResult)
     return MainResult.takeError();
 
-  // Add the main result as the first entry
+  // Add the main result as the first entry.
   Results.push_back(std::move(*MainResult));
 
-  // Now process any merged functions data that was found during the lookup
-  if (MainResult->MergedFunctionsData) {
-    // Get data extractors for each merged function
+  // Now process any merged functions data that was found during the lookup.
+  if (MergedFunctionsData) {
+    // Get data extractors for each merged function.
     auto ExpectedMergedFuncExtractors =
-        MergedFunctionsInfo::getFuncsDataExtractors(
-            *MainResult->MergedFunctionsData);
+        MergedFunctionsInfo::getFuncsDataExtractors(*MergedFunctionsData);
     if (!ExpectedMergedFuncExtractors)
       return ExpectedMergedFuncExtractors.takeError();
 
-    // Process each merged function data
+    // Process each merged function data.
     for (DataExtractor &MergedData : *ExpectedMergedFuncExtractors) {
       if (auto FI = FunctionInfo::lookup(MergedData, *this,
                                          MainResult->FuncRange.start(), Addr)) {

llvm/test/tools/llvm-gsymutil/ARM_AArch64/macho-merged-funcs-dwarf.yaml

@@ -69,9 +69,9 @@
 # RUN: llvm-gsymutil --verify %t.keep.gSYM --address 0x248 | FileCheck --check-prefix=CHECK-NORMAL-LOOKUP %s
 
 # CHECK-MERGED-LOOKUP: Found 3 functions at address 0x0000000000000248:
-# CHECK-MERGED-LOOKUP:    0x0000000000000248: my_func_01 @ /tmp/test_gsym_yaml/out/file_01.cpp:5
-# CHECK-MERGED-LOOKUP:    0x0000000000000248: my_func_02 @ /tmp/test_gsym_yaml/out/file_02.cpp:5
-# CHECK-MERGED-LOOKUP:    0x0000000000000248: my_func_03 @ /tmp/test_gsym_yaml/out/file_03.cpp:5
+# CHECK-MERGED-LOOKUP-NEXT:       0x0000000000000248: my_func_02 @ /tmp/test_gsym_yaml/out/file_02.cpp:5
+# CHECK-MERGED-LOOKUP-NEXT-NEXT:  0x0000000000000248: my_func_01 @ /tmp/test_gsym_yaml/out/file_01.cpp:5
+# CHECK-MERGED-LOOKUP-NEXT-NEXT:  0x0000000000000248: my_func_03 @ /tmp/test_gsym_yaml/out/file_03.cpp:5
 
 # CHECK-NORMAL-LOOKUP: 0x0000000000000248: my_func_01 @ /tmp/test_gsym_yaml/out/file_01.cpp:5
 

llvm/tools/llvm-gsymutil/llvm-gsymutil.cpp

@@ -507,61 +507,40 @@ static llvm::Error convertFileToGSYM(OutputAggregator &Out) {
   return Error::success();
 }
 
-static void doLookupMergedFunctions(GsymReader &Gsym, uint64_t Addr,
-                                    raw_ostream &OS) {
-  if (auto Results = Gsym.lookupAll(Addr)) {
-    OS << "Found " << Results->size() << " functions at address " << HEX64(Addr)
-       << ":\n";
-    for (size_t i = 0; i < Results->size(); ++i) {
-      if (Verbose) {
-        if (auto FI = FunctionInfo::decode(*Results->at(i).FunctionInfoData,
-                                           Results->at(i).FuncRange.start())) {
-          OS << "FunctionInfo for " << HEX64(Addr) << ":\n";
-          Gsym.dump(OS, *FI);
-          OS << "\nLookupResults for " << HEX64(Addr) << ":\n";
-        }
-      }
-
-      // Print the primary function lookup result
-      OS << "   " << Results->at(i);
-
-      if (i != Results->size() - 1)
-        OS << "\n";
-    }
-  } else {
-    if (Verbose)
-      OS << "\nLookupResult for " << HEX64(Addr) << ":\n";
+static void doLookup(GsymReader &Gsym, uint64_t Addr, raw_ostream &OS) {
+  auto logError = [Addr, &OS](Error E) {
     OS << HEX64(Addr) << ": ";
-    logAllUnhandledErrors(Results.takeError(), OS, "error: ");
-  }
-  if (Verbose)
-    OS << "\n";
-}
+    logAllUnhandledErrors(std::move(E), OS, "error: ");
+  };
 
-static void doLookup(GsymReader &Gsym, uint64_t Addr, raw_ostream &OS) {
   if (UseMergedFunctions) {
-    doLookupMergedFunctions(Gsym, Addr, OS);
-    return;
-  }
-
-  if (auto Result = Gsym.lookup(Addr)) {
-    // If verbose is enabled, dump the full function info for the address.
-    if (Verbose) {
-      if (auto FI = Gsym.getFunctionInfo(Addr)) {
-        OS << "FunctionInfo for " << HEX64(Addr) << ":\n";
-        Gsym.dump(OS, *FI);
-        OS << "\nLookupResult for " << HEX64(Addr) << ":\n";
+    if (auto Results = Gsym.lookupAll(Addr)) {
+      OS << "Found " << Results->size() << " functions at address "
+         << HEX64(Addr) << ":\n";
+      for (size_t i = 0; i < Results->size(); ++i) {
+        OS << "   " << Results->at(i);
+
+        if (i != Results->size() - 1)
+          OS << "\n";
       }
     }
-    OS << Result.get();
-  } else {
-    if (Verbose)
-      OS << "\nLookupResult for " << HEX64(Addr) << ":\n";
-    OS << HEX64(Addr) << ": ";
-    logAllUnhandledErrors(Result.takeError(), OS, "error: ");
+  } else { /* UseMergedFunctions == false */
+    if (auto Result = Gsym.lookup(Addr)) {
+      OS << Result.get();
+    } else {
+      logError(Result.takeError());
+      return;
+    }
   }
-  if (Verbose)
+
+  if (Verbose) {
+    if (auto FI = Gsym.getFunctionInfo(Addr)) {
+      OS << "FunctionInfo for " << HEX64(Addr) << ":\n";
+      Gsym.dump(OS, *FI);
+      OS << "\nLookupResult for " << HEX64(Addr) << ":\n";
+    }
     OS << "\n";
+  }
 }
 
 int llvm_gsymutil_main(int argc, char **argv, const llvm::ToolContext &) {