[NFC] Refactor the tuple of symbol information with structure for llvm-objdump

[daltenty]

llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h

23

Can we give a more precise type for this field? It only seems to take on ELF::STT enum values.

[DiggerLin]

llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h

23

in the ELF , the Type is means symbol type(in all the info in the SymbolInfoTy should be symbol related. ) , it is only ELF object file use the data member now. We will use the Type data member to store "Store Mapping class" of symbol in next patch, it is a refactor patch , I prefer to keep the Name "Type" here, we can change the name to other in the next patch.

[daltenty]

llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h

23

Sorry, to clarify I'm proposing changing the type of the Type field, not the name (it's confusing to talk about the type of a field named Type unfortunately).

Specifically I mean naming th ELF unnamed enum involved in BinaryFormat/ELF.h and specifying it here (an enum class would be nice but that may not be possible).

Something like:

struct SymbolInfoTy {
uint64_t  Addr;
StringRef Name;
Elf::STType  Type;
}

Provided the enum was named to STType. We would then introduce a union in the follow on patch.

[DiggerLin]

llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h

23

change to type Elf::STTType and I need to added a new include header file BinaryFormat/ELF.h

and the Type "Elf::STType" will be change soon to "int16_t" in later patch.

[daltenty]

LGTM (with note about later follow on)

llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h

23

Summary of offline discussion: This would probably necessitate revamping the enums in elf.h quite a bit, so this probably is best done as a latter follow on patch.

[jasonliu]

llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h

25

Any reason why a constructor is needed? It could be an aggregate and initialize with {}?

[jasonliu]

llvm/tools/llvm-objdump/llvm-objdump.cpp

344–352

Prefer this to be a friend function inside of struct SymbolInfoTy, as this operator could be used in other compilation unit.

Also, the function would be easier to reason about and maintain if we write it as:
{

return std::tie(P1.Addr, P1.Name, P1.Type) < std::tie(P2.Addr, P2.Name, P2.Type);

}

[Xiangling_L]

llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h

12

Since you add the header of StringRef, then forward declaration of class StringRef can be removed on line 33?

llvm/tools/llvm-objdump/llvm-objdump.cpp

345

Based on Jason's comment, maybe you can further simplify the function as:

bool operator<(const SymbolInfoTy& ST) const {
          return std::tie(Addr, Name, Type) < std::tie(ST.Addr, ST.Name, ST.Type);
        }

[jasonliu]

llvm/tools/llvm-objdump/llvm-objdump.cpp

345

Re @Xiangling_L
I still think a hidden friend function is a better idea than a member function (based on the syntax you are using):
https://www.justsoftwaresolutions.co.uk/cplusplus/hidden-friends.html

[jasonliu]

llvm/tools/llvm-objdump/llvm-objdump.cpp

345

Another article talks about overloading operator function, hopefully it would help: https://www.learncpp.com/cpp-tutorial/94-overloading-operators-using-member-functions/

[DiggerLin]

llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h

12

good finding . I will delete is in another patch

25

yes, I think we need it in the code

Symbols.insert(
        Symbols.begin(),
         SymbolInfoTy(SectionAddr, SectionName,
                        Section.isText() ? ELF::STT_FUNC : ELF::STT_OBJECT))

llvm/tools/llvm-objdump/llvm-objdump.cpp

345

1. If you’re overloading a binary operator that does not modify its left operand (e.g. operator+), do so as a normal function (preferred) or friend function.

2. , before the patch, it use tuple which compare address, Name, Type, but the type always is STT_NOTYPE for xcoffobject . The type value(storage mapping class) will be change in later patch , if we compare the type , it will cause a lot of xcoff dissemble test case to change. I do not use the Type data member as compare value.

3. the function std::tie generate a tuple, as we talk about we do not use tuple, why we use a tie ?

[Xiangling_L]

llvm/tools/llvm-objdump/llvm-objdump.cpp

345

Thank you for providing those articles, they are really helpful. And I agree that there are extra benefits comparing hidden friend function than member function in this case.

[jasonliu]

llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h

25

Okk.

This line does not have proper indentation I think. If we are going to fix other issues mentioned below, I would prefer to have it fix as well.

llvm/tools/llvm-objdump/llvm-objdump.cpp

345

1. Friend function is a normal function too. Declaring it as private inside the class will help with the lookup speed for the function. And my biggest complain is that this is a static function only available in this translation unit.
2. You don't have to compare it if you don't want. I write the Type out because it's the original behavior. And if it's subject to change later, I'm fine with that. But for this patch, that comparison should still holds.
3. We don't want tuple because we need to use std::get<?> everywhere. And this case, by making it a tuple only in comparison, it's easier to reason about what the order is for comparison. And easier to maintain as well, as most of time, people know they only need to add one more parameter to the tuple if they add a new data member to this structure.

[jasonliu]

llvm/tools/llvm-objdump/llvm-objdump.cpp

345

2. You don't have to compare it if you don't want. I write the Type out because it's the original behavior. And if it's subject to change later, I'm fine with that. But for this patch, that comparison should still holds.

Also, since this is an NFC patch, we are not suppose to change any functional behavior right?

[leonardchan]

This comment was removed by leonardchan.

[leonardchan]

Actually, disregard my last comment. I think I might've made a mistake on the bisect. Sorry!

llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h

 //===- llvm/MC/MCDisassembler.h - Disassembler interface --------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_MC_MCDISASSEMBLER_MCDISASSEMBLER_H
 #define LLVM_MC_MCDISASSEMBLER_MCDISASSEMBLER_H
 
+#include "llvm/ADT/StringRef.h"
 #include "llvm/MC/MCDisassembler/MCSymbolizer.h"
 #include <cstdint>
 #include <memory>
+#include <vector>
 
 namespace llvm {
 
+struct SymbolInfoTy {
+  uint64_t  Addr;
+  StringRef Name;
+  uint8_t   Type;
+  bool operator< (const SymbolInfoTy& other) {
+    return Addr < other.Addr;
+  }
+       SymbolInfoTy(uint64_t Addr, StringRef Name, uint8_t Type):
+        Addr(Addr),Name(Name),Type(Type) {};
+};
+
+using SectionSymbolsTy = std::vector<SymbolInfoTy>;
+
+
 template <typename T> class ArrayRef;
 class StringRef;
 class MCContext;
 class MCInst;
 class MCSubtargetInfo;
 class raw_ostream;
 
 /// Superclass for all disassemblers. Consumes a memory region and provides an

llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp

 // AMDGPUSymbolizer
 //===----------------------------------------------------------------------===//
 
 // Try to find symbol name for specified label
 bool AMDGPUSymbolizer::tryAddingSymbolicOperand(MCInst &Inst,
                                 raw_ostream &/*cStream*/, int64_t Value,
                                 uint64_t /*Address*/, bool IsBranch,
                                 uint64_t /*Offset*/, uint64_t /*InstSize*/) {
-  using SymbolInfoTy = std::tuple<uint64_t, StringRef, uint8_t>;
-  using SectionSymbolsTy = std::vector<SymbolInfoTy>;
 
   if (!IsBranch) {
     return false;
   }
 
   auto *Symbols = static_cast<SectionSymbolsTy *>(DisInfo);
   if (!Symbols)
     return false;
 
   auto Result = std::find_if(Symbols->begin(), Symbols->end(),
                              [Value](const SymbolInfoTy& Val) {
-                                return std::get<0>(Val) == static_cast<uint64_t>(Value)
-                                    && std::get<2>(Val) == ELF::STT_NOTYPE;
+                                return Val.Addr == static_cast<uint64_t>(Value)
+                                    && Val.Type == ELF::STT_NOTYPE;
                              });
   if (Result != Symbols->end()) {
-    auto *Sym = Ctx.getOrCreateSymbol(std::get<1>(*Result));
+    auto *Sym = Ctx.getOrCreateSymbol(Result->Name);
     const auto *Add = MCSymbolRefExpr::create(Sym, Ctx);
     Inst.addOperand(MCOperand::createExpr(Add));
     return true;
   }
   return false;
 }
 
 void AMDGPUSymbolizer::tryAddingPcLoadReferenceComment(raw_ostream &cStream,

llvm/tools/llvm-objdump/llvm-objdump.cpp

 
 static cl::extrahelp
     HelpResponse("\nPass @FILE as argument to read options from FILE.\n");
 
 static StringSet<> DisasmFuncsSet;
 StringSet<> FoundSectionSet;
 static StringRef ToolName;
 
-typedef std::vector<std::tuple<uint64_t, StringRef, uint8_t>> SectionSymbolsTy;
+static bool operator<(const SymbolInfoTy& P1 ,const SymbolInfoTy& P2) {
+  if (P1.Addr < P2.Addr)
+    return true;
+
+  if (P1.Addr == P2.Addr)
+    return P1.Name < P2.Name;
+
+  return false;
+}
 
 namespace {
 struct FilterResult {
   // True if the section should not be skipped.
   bool Keep;
 
   // True if the index counter should be incremented, even if the section should
   // be skipped. For example, sections may be skipped if they are not included
for (const SectionRef &Section : ToolSectionFilter(*Obj)) {
     if (!SectSize)
       continue;
 
     // Get the list of all the symbols in this section.
     SectionSymbolsTy &Symbols = AllSymbols[Section];
     std::vector<MappingSymbolPair> MappingSymbols;
     if (hasMappingSymbols(Obj)) {
       for (const auto &Symb : Symbols) {
-        uint64_t Address = std::get<0>(Symb);
-        StringRef Name = std::get<1>(Symb);
+        uint64_t Address = Symb.Addr;
+        StringRef Name = Symb.Name;
         if (Name.startswith("$d"))
           MappingSymbols.emplace_back(Address - SectionAddr, 'd');
         if (Name.startswith("$x"))
           MappingSymbols.emplace_back(Address - SectionAddr, 'x');
         if (Name.startswith("$a"))
           MappingSymbols.emplace_back(Address - SectionAddr, 'a');
         if (Name.startswith("$t"))
           MappingSymbols.emplace_back(Address - SectionAddr, 't');
for (const SectionRef &Section : ToolSectionFilter(*Obj)) {
     StringRef SegmentName = "";
     if (MachO) {
       DataRefImpl DR = Section.getRawDataRefImpl();
       SegmentName = MachO->getSectionFinalSegmentName(DR);
     }
 
     StringRef SectionName = unwrapOrError(Section.getName(), Obj->getFileName());
     // If the section has no symbol at the start, just insert a dummy one.
-    if (Symbols.empty() || std::get<0>(Symbols[0]) != 0) {
+    if (Symbols.empty() || Symbols[0].Addr != 0) {
       Symbols.insert(
           Symbols.begin(),
-          std::make_tuple(SectionAddr, SectionName,
+           SymbolInfoTy(SectionAddr, SectionName,
                           Section.isText() ? ELF::STT_FUNC : ELF::STT_OBJECT));
     }
 
     SmallString<40> Comments;
     raw_svector_ostream CommentStream(Comments);
 
     ArrayRef<uint8_t> Bytes = arrayRefFromStringRef(
         unwrapOrError(Section.getContents(), Obj->getFileName()));
 
     uint64_t VMAAdjustment = 0;
     if (shouldAdjustVA(Section))
       VMAAdjustment = AdjustVMA;
 
     uint64_t Size;
     uint64_t Index;
     bool PrintedSection = false;
     std::vector<RelocationRef> Rels = RelocMap[Section];
     std::vector<RelocationRef>::const_iterator RelCur = Rels.begin();
     std::vector<RelocationRef>::const_iterator RelEnd = Rels.end();
     // Disassemble symbol by symbol.
     for (unsigned SI = 0, SE = Symbols.size(); SI != SE; ++SI) {
-      std::string SymbolName = std::get<1>(Symbols[SI]).str();
+      std::string SymbolName = Symbols[SI].Name.str();
       if (Demangle)
         SymbolName = demangle(SymbolName);
 
       // Skip if --disassemble-functions is not empty and the symbol is not in
       // the list.
       if (!DisasmFuncsSet.empty() && !DisasmFuncsSet.count(SymbolName))
         continue;
 
-      uint64_t Start = std::get<0>(Symbols[SI]);
+      uint64_t Start = Symbols[SI].Addr;
       if (Start < SectionAddr || StopAddress <= Start)
         continue;
       else
         FoundDisasmFuncsSet.insert(SymbolName);
 
       // The end is the section end, the beginning of the next symbol, or
       // --stop-address.
       uint64_t End = std::min<uint64_t>(SectionAddr + SectSize, StopAddress);
       if (SI + 1 < SE)
-        End = std::min(End, std::get<0>(Symbols[SI + 1]));
+        End = std::min(End, Symbols[SI + 1].Addr);
       if (Start >= End || End <= StartAddress)
         continue;
       Start -= SectionAddr;
       End -= SectionAddr;
 
       if (!PrintedSection) {
         PrintedSection = true;
         outs() << "\nDisassembly of section ";
         if (!SegmentName.empty())
           outs() << SegmentName << ",";
         outs() << SectionName << ":\n";
       }
 
       if (Obj->isELF() && Obj->getArch() == Triple::amdgcn) {
-        if (std::get<2>(Symbols[SI]) == ELF::STT_AMDGPU_HSA_KERNEL) {
+        if (Symbols[SI].Type == ELF::STT_AMDGPU_HSA_KERNEL) {
           // skip amd_kernel_code_t at the begining of kernel symbol (256 bytes)
           Start += 256;
         }
         if (SI == SE - 1 ||
-            std::get<2>(Symbols[SI + 1]) == ELF::STT_AMDGPU_HSA_KERNEL) {
+            Symbols[SI + 1].Type == ELF::STT_AMDGPU_HSA_KERNEL) {
           // cut trailing zeroes at the end of kernel
           // cut up to 256 bytes
           const uint64_t EndAlign = 256;
           const auto Limit = End - (std::min)(EndAlign, End - Start);
           while (End > Limit &&
             *reinterpret_cast<const support::ulittle32_t*>(&Bytes[End - 4]) == 0)
             End -= 4;
         }
for (unsigned SI = 0, SE = Symbols.size(); SI != SE; ++SI) {
       Index = Start;
       if (SectionAddr < StartAddress)
         Index = std::max<uint64_t>(Index, StartAddress - SectionAddr);
 
       // If there is a data/common symbol inside an ELF text section and we are
       // only disassembling text (applicable all architectures), we are in a
       // situation where we must print the data and not disassemble it.
       if (Obj->isELF() && !DisassembleAll && Section.isText()) {
-        uint8_t SymTy = std::get<2>(Symbols[SI]);
+        uint8_t SymTy = Symbols[SI].Type;
         if (SymTy == ELF::STT_OBJECT || SymTy == ELF::STT_COMMON) {
           dumpELFData(SectionAddr, Index, End, Bytes);
           Index = End;
         }
       }
 
       bool CheckARMELFData = hasMappingSymbols(Obj) &&
-                             std::get<2>(Symbols[SI]) != ELF::STT_OBJECT &&
+                             Symbols[SI].Type != ELF::STT_OBJECT &&
                              !DisassembleAll;
       while (Index < End) {
         // ARM and AArch64 ELF binaries can interleave data and text in the
         // same section. We rely on the markers introduced to understand what
         // we need to dump. If the data marker is within a function, it is
         // denoted as a word/short etc.
         if (CheckARMELFData &&
             getMappingSymbolKind(MappingSymbols, Index) == 'd') {
for (unsigned SI = 0, SE = Symbols.size(); SI != SE; ++SI) {
               }
             }
 
             // Find the last symbol in the section whose offset is less than
             // or equal to the target. If there isn't a section that contains
             // the target, find the nearest preceding absolute symbol.
             auto TargetSym = partition_point(
                 *TargetSectionSymbols,
-                [=](const std::tuple<uint64_t, StringRef, uint8_t> &O) {
-                  return std::get<0>(O) <= Target;
+                [=](const SymbolInfoTy &O) {
+                  return O.Addr <= Target;
                 });
             if (TargetSym == TargetSectionSymbols->begin()) {
               TargetSectionSymbols = &AbsoluteSymbols;
               TargetSym = partition_point(
                   AbsoluteSymbols,
-                  [=](const std::tuple<uint64_t, StringRef, uint8_t> &O) {
-                    return std::get<0>(O) <= Target;
+                  [=](const SymbolInfoTy &O) {
+                    return O.Addr <= Target;
                   });
             }
             if (TargetSym != TargetSectionSymbols->begin()) {
               --TargetSym;
-              uint64_t TargetAddress = std::get<0>(*TargetSym);
-              StringRef TargetName = std::get<1>(*TargetSym);
+              uint64_t TargetAddress = TargetSym->Addr;
+              StringRef TargetName = TargetSym->Name;
               outs() << " <" << TargetName;
               uint64_t Disp = Target - TargetAddress;
               if (Disp)
                 outs() << "+0x" << Twine::utohexstr(Disp);
               outs() << '>';
             }
           }
         }