[PATCH] D81717: [llvm/Object] Reimplment basic_symbol_iterator in TapiFile

[Jonas Devlieghere via Phabricator via llvm-commits]

JDevlieghere created this revision.
JDevlieghere added a reviewer: cishida.
Herald added subscribers: dexonsmith, hiraditya.
Herald added a project: LLVM.
cishida accepted this revision.
cishida added a comment.
This revision is now accepted and ready to land.

Thanks for fixing this! one nit but LGTM



================
Comment at: llvm/lib/Object/TapiFile.cpp:81
 Error TapiFile::printSymbolName(raw_ostream &OS, DataRefImpl DRI) const {
-  const auto *Sym = reinterpret_cast<const Symbol *>(DRI.p);
-  OS << Sym->Prefix << Sym->Name;
+  assert(DRI.d.a < Symbols.size());
+  const Symbol &Sym = Symbols[DRI.d.a];
----------------
nit: maybe add a message to notify an out of bounds access? 


================
Comment at: llvm/lib/Object/TapiFile.cpp:88
 Expected<uint32_t> TapiFile::getSymbolFlags(DataRefImpl DRI) const {
-  const auto *Sym = reinterpret_cast<const Symbol *>(DRI.p);
-  return Sym->Flags;
+  assert(DRI.d.a < Symbols.size());
+  return Symbols[DRI.d.a].Flags;
----------------
same here


Use indices into the Symbols vector instead of dereferencing `std::vector::end()`.

NFC modulo the Windows failure: http://lab.llvm.org:8011/builders/llvm-clang-x86_64-expensive-checks-win/builds/24024


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D81717

Files:
  llvm/lib/Object/TapiFile.cpp


Index: llvm/lib/Object/TapiFile.cpp
===================================================================
--- llvm/lib/Object/TapiFile.cpp
+++ llvm/lib/Object/TapiFile.cpp
@@ -75,30 +75,28 @@
 
 TapiFile::~TapiFile() = default;
 
-void TapiFile::moveSymbolNext(DataRefImpl &DRI) const {
-  const auto *Sym = reinterpret_cast<const Symbol *>(DRI.p);
-  DRI.p = reinterpret_cast<uintptr_t>(++Sym);
-}
+void TapiFile::moveSymbolNext(DataRefImpl &DRI) const { DRI.d.a++; }
 
 Error TapiFile::printSymbolName(raw_ostream &OS, DataRefImpl DRI) const {
-  const auto *Sym = reinterpret_cast<const Symbol *>(DRI.p);
-  OS << Sym->Prefix << Sym->Name;
+  assert(DRI.d.a < Symbols.size());
+  const Symbol &Sym = Symbols[DRI.d.a];
+  OS << Sym.Prefix << Sym.Name;
   return Error::success();
 }
 
 Expected<uint32_t> TapiFile::getSymbolFlags(DataRefImpl DRI) const {
-  const auto *Sym = reinterpret_cast<const Symbol *>(DRI.p);
-  return Sym->Flags;
+  assert(DRI.d.a < Symbols.size());
+  return Symbols[DRI.d.a].Flags;
 }
 
 basic_symbol_iterator TapiFile::symbol_begin() const {
   DataRefImpl DRI;
-  DRI.p = reinterpret_cast<uintptr_t>(&*Symbols.begin());
+  DRI.d.a = 0;
   return BasicSymbolRef{DRI, this};
 }
 
 basic_symbol_iterator TapiFile::symbol_end() const {
   DataRefImpl DRI;
-  DRI.p = reinterpret_cast<uintptr_t>(&*Symbols.end());
+  DRI.d.a = Symbols.size();
   return BasicSymbolRef{DRI, this};
 }


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D81717.270310.patch
Type: text/x-patch
Size: 1408 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200612/f9c43223/attachment.bin>