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) const;
+  bool getNameFromSymbolTable(uint64_t Address, std::string &Name,
+                              uint64_t &Addr, uint64_t &Size) 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);
 
   /// Search for the first occurence of specified Address in ObjectFile.
   uint64_t getModuleSectionIndexForAddress(uint64_t Address) const;
 
   const object::ObjectFile *Module;
   std::unique_ptr<DIContext> DebugInfoContext;
   bool UntagAddresses;
 
   struct SymbolDesc {
     uint64_t Addr;
     // If size is 0, assume that symbol occupies the whole memory range up to
     // the following symbol.
     uint64_t Size;
 
     bool operator<(const SymbolDesc &RHS) const {
       return Addr != RHS.Addr ? Addr < RHS.Addr : Size < RHS.Size;
     }
   };
-  std::vector<std::pair<SymbolDesc, StringRef>> Functions;
-  std::vector<std::pair<SymbolDesc, StringRef>> Objects;
+  std::vector<std::pair<SymbolDesc, StringRef>> Symbols;
 
   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<std::pair<SymbolDesc, StringRef>> &Fs = res->Functions,
-                                                &Os = res->Objects;
-  auto Uniquify = [](std::vector<std::pair<SymbolDesc, StringRef>> &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->first.Addr == I->first.Addr) {
-      }
-      *J++ = I[-1];
-    }
-    S.erase(J, S.end());
-  };
-  Uniquify(Fs);
-  Uniquify(Os);
+  std::vector<std::pair<SymbolDesc, StringRef>> &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(
   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;
     SymbolDesc SD = {SymbolStart, SymbolSize};
-    Functions.emplace_back(SD, Export.Name);
+    Symbols.emplace_back(SD, Export.Name);
   }
   return Error::success();
 }
 
 Error SymbolizableObjectFile::addSymbol(const SymbolRef &Symbol,
                                         uint64_t SymbolSize,
                                         DataExtractor *OpdExtractor,
                                         uint64_t OpdAddress) {
Error SymbolizableObjectFile::addSymbol(const SymbolRef &Symbol,
   Expected<StringRef> SymbolNameOrErr = Symbol.getName();
   if (!SymbolNameOrErr)
     return SymbolNameOrErr.takeError();
   StringRef SymbolName = *SymbolNameOrErr;
   // Mach-O symbol table names have leading underscore, skip it.
   if (Module->isMachO() && !SymbolName.empty() && SymbolName[0] == '_')
     SymbolName = SymbolName.drop_front();
 
-  SymbolDesc SD = {SymbolAddress, SymbolSize};
-
-  // 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.emplace_back(SD, SymbolName);
-  else
-    Functions.emplace_back(SD, SymbolName);
+  Symbols.emplace_back(SymbolDesc{SymbolAddress, SymbolSize}, SymbolName);
   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,
+bool SymbolizableObjectFile::getNameFromSymbolTable(uint64_t Address,
                                                     std::string &Name,
                                                     uint64_t &Addr,
                                                     uint64_t &Size) const {
-  const auto &Symbols = Type == SymbolRef::ST_Function ? Functions : Objects;
   std::pair<SymbolDesc, StringRef> SD{{Address, UINT64_C(-1)}, StringRef()};
   auto SymbolIterator = llvm::upper_bound(Symbols, SD);
   if (SymbolIterator == Symbols.begin())
     return false;
   --SymbolIterator;
   if (SymbolIterator->first.Size != 0 &&
       SymbolIterator->first.Addr + SymbolIterator->first.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;
     uint64_t Start, Size;
-    if (getNameFromSymbolTable(SymbolRef::ST_Function, ModuleOffset.Address,
-                               FunctionName, Start, Size)) {
+    if (getNameFromSymbolTable(ModuleOffset.Address, FunctionName, Start,
+                               Size)) {
       LineInfo.FunctionName = FunctionName;
     }
   }
   return LineInfo;
 }
 
 DIInliningInfo SymbolizableObjectFile::symbolizeInlinedCode(
     object::SectionedAddress ModuleOffset,
     DILineInfoSpecifier LineInfoSpecifier, bool UseSymbolTable) const {
   if (ModuleOffset.SectionIndex == object::SectionedAddress::UndefSection)
     ModuleOffset.SectionIndex =
         getModuleSectionIndexForAddress(ModuleOffset.Address);
   DIInliningInfo InlinedContext = DebugInfoContext->getInliningInfoForAddress(
       ModuleOffset, LineInfoSpecifier);
 
   // 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;
     uint64_t Start, Size;
-    if (getNameFromSymbolTable(SymbolRef::ST_Function, ModuleOffset.Address,
-                               FunctionName, Start, Size)) {
+    if (getNameFromSymbolTable(ModuleOffset.Address, FunctionName, Start,
+                               Size)) {
       InlinedContext.getMutableFrame(InlinedContext.getNumberOfFrames() - 1)
           ->FunctionName = FunctionName;
     }
   }
 
   return InlinedContext;
 }
 
 DIGlobal SymbolizableObjectFile::symbolizeData(
     object::SectionedAddress ModuleOffset) const {
   DIGlobal Res;
-  getNameFromSymbolTable(SymbolRef::ST_Data, ModuleOffset.Address, Res.Name,
-                         Res.Start, Res.Size);
+  getNameFromSymbolTable(ModuleOffset.Address, Res.Name, Res.Start, Res.Size);
   return Res;
 }
 
 std::vector<DILocal> SymbolizableObjectFile::symbolizeFrame(
     object::SectionedAddress ModuleOffset) const {
   if (ModuleOffset.SectionIndex == object::SectionedAddress::UndefSection)
     ModuleOffset.SectionIndex =
         getModuleSectionIndexForAddress(ModuleOffset.Address);