DebugInfo/Symbolize: Don't differentiate function/data symbolization

[MaskRay]

Summary

Before d08bd13ac8a560c4645e17e192ca07e1bdcd2895, only SymbolRef::ST_Function
symbols were used for .symtab symbolization. That commit added a "DATA" mode
to llvm-symbolizer which used SymbolRef::ST_Data symbols for symbolization.

Since function and data symbols have different addresses, we don't need to
differentiate the two modes. This patch unifies the two modes to simplify code.

"DATA" is used by compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_libcdep.cpp.
check-hwasan and check-tsan have runtime tests.

[jhenderson]

Let's not rely on testing in compiler-rt to test llvm-symbolizer behaviour. Tests should be kept to the modules where the behaviour in question is. In this case, either DebugInfo or tools/llvm-symbolizer (probably the former in this case). I don't think it's hard to add a test case for this at all - all it needs to show is that a DATA directive can symbolize a function as data and CODE symbolizes an object as code.

[jhenderson]

To be clear, the rest of the change is fine. I just disagree with the testing strategy.

[MaskRay]

Add a test

[jhenderson]

LGTM, thanks!

[MaskRay]

Rebase

[dmajor]

coff-dwarf.test started failing after this commit: http://lab.llvm.org:8011/#/builders/109/builds/8398

[MaskRay]

In D96322#2558898, @dmajor wrote:

coff-dwarf.test started failing after this commit: http://lab.llvm.org:8011/#/builders/109/builds/8398

The code used non-stable llvm::sort. Fixed by 0fd7c31a098efdfaa5a57dbac6e9c0921b00a999

llvm/lib/DebugInfo/Symbolize/SymbolizableObjectFile.h

public:
   // Returns the preferred base of the module, i.e. where the loader would place
   // it in memory assuming there were no conflicts.
   uint64_t getModulePreferredBase() const override;
 
 private:
   bool shouldOverrideWithSymbolTable(FunctionNameKind FNKind,
                                      bool UseSymbolTable) const;
 
-  bool getNameFromSymbolTable(object::SymbolRef::Type Type, uint64_t Address,
-                              std::string &Name, uint64_t &Addr, uint64_t &Size,
+  bool getNameFromSymbolTable(uint64_t Address, std::string &Name,
+                              uint64_t &Addr, uint64_t &Size,
                               std::string &FileName) const;
   // For big-endian PowerPC64 ELF, OpdAddress is the address of the .opd
   // (function descriptor) section and OpdExtractor refers to its contents.
   Error addSymbol(const object::SymbolRef &Symbol, uint64_t SymbolSize,
                   DataExtractor *OpdExtractor = nullptr,
                   uint64_t OpdAddress = 0);
   Error addCoffExportSymbols(const object::COFFObjectFile *CoffObj);
 
struct SymbolDesc {
     // Non-zero if this is an ELF local symbol. See the comment in
     // getNameFromSymbolTable.
     uint32_t ELFLocalSymIdx;
 
     bool operator<(const SymbolDesc &RHS) const {
       return Addr != RHS.Addr ? Addr < RHS.Addr : Size < RHS.Size;
     }
   };
-  std::vector<SymbolDesc> Functions;
-  std::vector<SymbolDesc> Objects;
+  std::vector<SymbolDesc> Symbols;
   // (index, filename) pairs of ELF STT_FILE symbols.
   std::vector<std::pair<uint32_t, StringRef>> FileSymbols;
 
   SymbolizableObjectFile(const object::ObjectFile *Obj,
                          std::unique_ptr<DIContext> DICtx,
                          bool UntagAddresses);
 };
 
 } // end namespace symbolize
 
 } // end namespace llvm
 
 #endif // LLVM_LIB_DEBUGINFO_SYMBOLIZE_SYMBOLIZABLEOBJECTFILE_H

llvm/lib/DebugInfo/Symbolize/SymbolizableObjectFile.cpp

SymbolizableObjectFile::create(const object::ObjectFile *Obj,
   // If this is a COFF object and we didn't find any symbols, try the export
   // table.
   if (Symbols.empty()) {
     if (auto *CoffObj = dyn_cast<COFFObjectFile>(Obj))
       if (Error E = res->addCoffExportSymbols(CoffObj))
         return std::move(E);
   }
 
-  std::vector<SymbolDesc> &Fs = res->Functions, &Os = res->Objects;
-  auto Uniquify = [](std::vector<SymbolDesc> &S) {
-    // Sort by (Addr,Size,Name). If several SymbolDescs share the same Addr,
-    // pick the one with the largest Size. This helps us avoid symbols with no
-    // size information (Size=0).
-    llvm::sort(S);
-    auto I = S.begin(), E = S.end(), J = S.begin();
-    while (I != E) {
-      auto OI = I;
-      while (++I != E && OI->Addr == I->Addr) {
-      }
-      *J++ = I[-1];
-    }
-    S.erase(J, S.end());
-  };
-  Uniquify(Fs);
-  Uniquify(Os);
+  std::vector<SymbolDesc> &SS = res->Symbols;
+  llvm::sort(SS);
+  auto I = SS.begin(), E = SS.end(), J = SS.begin();
+  SS.erase(J, SS.end());
 
   return std::move(res);
 }
 
 SymbolizableObjectFile::SymbolizableObjectFile(const ObjectFile *Obj,
                                                std::unique_ptr<DIContext> DICtx,
                                                bool UntagAddresses)
     : Module(Obj), DebugInfoContext(std::move(DICtx)),
Error SymbolizableObjectFile::addCoffExportSymbols(
   // FIXME: This assumes all exports are functions.
   uint64_t ImageBase = CoffObj->getImageBase();
   for (auto I = ExportSyms.begin(), E = ExportSyms.end(); I != E; ++I) {
     OffsetNamePair &Export = *I;
     // FIXME: The last export has a one byte size now.
     uint32_t NextOffset = I != E ? I->Offset : Export.Offset + 1;
     uint64_t SymbolStart = ImageBase + Export.Offset;
     uint64_t SymbolSize = NextOffset - Export.Offset;
-    Functions.push_back({SymbolStart, SymbolSize, Export.Name, 0});
+    Symbols.push_back({SymbolStart, SymbolSize, Export.Name, 0});
   }
   return Error::success();
 }
 
 Error SymbolizableObjectFile::addSymbol(const SymbolRef &Symbol,
                                         uint64_t SymbolSize,
                                         DataExtractor *OpdExtractor,
                                         uint64_t OpdAddress) {
if (OpdExtractor->isValidOffsetForAddress(OpdOffset))
       SymbolAddress = OpdExtractor->getAddress(&OpdOffset);
   }
   // Mach-O symbol table names have leading underscore, skip it.
   if (Module->isMachO() && !SymbolName.empty() && SymbolName[0] == '_')
     SymbolName = SymbolName.drop_front();
 
   if (Obj.isELF() && ELFSymbolRef(Symbol).getBinding() != ELF::STB_LOCAL)
     ELFSymIdx = 0;
-  SymbolDesc SD = {SymbolAddress, SymbolSize, SymbolName, ELFSymIdx};
-  // DATA command symbolizes just ST_Data (ELF STT_OBJECT) symbols as an
-  // optimization. Treat everything else (e.g. ELF STT_NOTYPE, STT_FUNC and
-  // STT_GNU_IFUNC) as function symbols which can be used to symbolize
-  // addresses.
-  if (SymbolType == SymbolRef::ST_Data)
-    Objects.push_back(SD);
-  else
-    Functions.push_back(SD);
+  Symbols.push_back({SymbolAddress, SymbolSize, SymbolName, ELFSymIdx});
   return Error::success();
 }
 
 // Return true if this is a 32-bit x86 PE COFF module.
 bool SymbolizableObjectFile::isWin32Module() const {
   auto *CoffObject = dyn_cast<COFFObjectFile>(Module);
   return CoffObject && CoffObject->getMachine() == COFF::IMAGE_FILE_MACHINE_I386;
 }
 
 uint64_t SymbolizableObjectFile::getModulePreferredBase() const {
   if (auto *CoffObject = dyn_cast<COFFObjectFile>(Module))
     return CoffObject->getImageBase();
   return 0;
 }
 
 bool SymbolizableObjectFile::getNameFromSymbolTable(
-    SymbolRef::Type Type, uint64_t Address, std::string &Name, uint64_t &Addr,
-    uint64_t &Size, std::string &FileName) const {
-  const auto &Symbols = Type == SymbolRef::ST_Function ? Functions : Objects;
+    uint64_t Address, std::string &Name, uint64_t &Addr, uint64_t &Size,
+    std::string &FileName) const {
   SymbolDesc SD{Address, UINT64_C(-1), StringRef(), 0};
   auto SymbolIterator = llvm::upper_bound(Symbols, SD);
   if (SymbolIterator == Symbols.begin())
     return false;
   --SymbolIterator;
   if (SymbolIterator->Size != 0 &&
       SymbolIterator->Addr + SymbolIterator->Size <= Address)
     return false;
ModuleOffset.SectionIndex =
         getModuleSectionIndexForAddress(ModuleOffset.Address);
   DILineInfo LineInfo =
       DebugInfoContext->getLineInfoForAddress(ModuleOffset, LineInfoSpecifier);
 
   // Override function name from symbol table if necessary.
   if (shouldOverrideWithSymbolTable(LineInfoSpecifier.FNKind, UseSymbolTable)) {
     std::string FunctionName, FileName;
     uint64_t Start, Size;
-    if (getNameFromSymbolTable(SymbolRef::ST_Function, ModuleOffset.Address,
-                               FunctionName, Start, Size, FileName)) {
+    if (getNameFromSymbolTable(ModuleOffset.Address, FunctionName, Start, Size,
+                               FileName)) {
       LineInfo.FunctionName = FunctionName;
       if (LineInfo.FileName == DILineInfo::BadString && !FileName.empty())
         LineInfo.FileName = FileName;
     }
   }
   return LineInfo;
 }
 
DIInliningInfo SymbolizableObjectFile::symbolizeInlinedCode(
   // Make sure there is at least one frame in context.
   if (InlinedContext.getNumberOfFrames() == 0)
     InlinedContext.addFrame(DILineInfo());
 
   // Override the function name in lower frame with name from symbol table.
   if (shouldOverrideWithSymbolTable(LineInfoSpecifier.FNKind, UseSymbolTable)) {
     std::string FunctionName, FileName;
     uint64_t Start, Size;
-    if (getNameFromSymbolTable(SymbolRef::ST_Function, ModuleOffset.Address,
-                               FunctionName, Start, Size, FileName)) {
+    if (getNameFromSymbolTable(ModuleOffset.Address, FunctionName, Start, Size,
+                               FileName)) {
       DILineInfo *LI = InlinedContext.getMutableFrame(
           InlinedContext.getNumberOfFrames() - 1);
       LI->FunctionName = FunctionName;
       if (LI->FileName == DILineInfo::BadString && !FileName.empty())
         LI->FileName = FileName;
     }
   }
 
   return InlinedContext;
 }
 
 DIGlobal SymbolizableObjectFile::symbolizeData(
     object::SectionedAddress ModuleOffset) const {
   DIGlobal Res;
   std::string FileName;
-  getNameFromSymbolTable(SymbolRef::ST_Data, ModuleOffset.Address, Res.Name,
-                         Res.Start, Res.Size, FileName);
+  getNameFromSymbolTable(ModuleOffset.Address, Res.Name, Res.Start, Res.Size,
+                         FileName);
   return Res;
 }
 
 std::vector<DILocal> SymbolizableObjectFile::symbolizeFrame(
     object::SectionedAddress ModuleOffset) const {
   if (ModuleOffset.SectionIndex == object::SectionedAddress::UndefSection)
     ModuleOffset.SectionIndex =
         getModuleSectionIndexForAddress(ModuleOffset.Address);

llvm/test/DebugInfo/Symbolize/ELF/data-command-symtab.yaml

+## Function and data symbols have different addresses so internally we treat
+## STT_NOTYPE/STT_FUNC/STT_DATA/etc the same. The DATA command may get a function
+## as result. For regular applications, the input addresses are guaranteed to be
+## related to data symbols.
+# RUN: yaml2obj %s -o %t
+# RUN: llvm-symbolizer --obj=%t 'DATA 0x1000' 'DATA 0x2000' 'DATA 0x2002' | FileCheck %s
+
+# CHECK:       func
+# CHECK-NEXT:  4096 1
+# CHECK-EMPTY:
+# CHECK-NEXT:  data
+# CHECK-NEXT:  8192 2
+# CHECK-EMPTY:
+# CHECK-NEXT:  notype
+# CHECK-NEXT:  8194 3
+# CHECK-EMPTY:
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_EXEC
+  Machine: EM_X86_64
+Sections:
+  - Name:    .text
+    Type:    SHT_PROGBITS
+    Flags:   [ SHF_ALLOC, SHF_EXECINSTR ]
+    Address: 0x1000
+    Size:    1
+  - Name:    .data
+    Type:    SHT_PROGBITS
+    Flags:   [ SHF_WRITE, SHF_ALLOC ]
+    Address: 0x2000
+    Size:    5
+Symbols:
+  - Name:    func
+    Type:    STT_FUNC
+    Section: .text
+    Binding: STB_GLOBAL
+    Value:   0x1000
+    Size:    1
+  - Name:    data
+    Type:    STT_OBJECT
+    Section: .data
+    Binding: STB_GLOBAL
+    Value:   0x2000
+    Size:    2
+  - Name:    notype
+    Section: .data
+    Binding: STB_GLOBAL
+    Value:   0x2002
+    Size:    3