[AMDGPU] llvm-objdump: Skip amd_kernel_code_t only at the begining of kernel symbol.

[SamWot]

Summary

This change fix bug in AMDGPU disassembly. Previously, presence of symbols other than kernel symbols caused objdump to skip begining of those symbols.

[Bigcheese]

I find it odd that we need all this machinery just to keep track of the type of a symbol. Why not just make `AllSymbols``` map to a SymbolRef an extract it from there?

[SamWot]

In D21966#473780, @Bigcheese wrote:

I find it odd that we need all this machinery just to keep track of the type of a symbol. Why not just make `AllSymbols``` map to a SymbolRef an extract it from there?

Problem here is that SymbolRef::Type isn't same as ELF type. SymbolRef::Type contains only some common types between ELF, COFF and Mach-O file formats. So SymbolRef::Type does not content types that AMDGPU uses (STT_AMDGPU_HSA_KERNEL).

[Bigcheese]

In D21966#473994, @SamWot wrote:

In D21966#473780, @Bigcheese wrote:

I find it odd that we need all this machinery just to keep track of the type of a symbol. Why not just make `AllSymbols``` map to a SymbolRef an extract it from there?

Problem here is that SymbolRef::Type isn't same as ELF type. SymbolRef::Type contains only some common types between ELF, COFF and Mach-O file formats. So SymbolRef::Type does not content types that AMDGPU uses (STT_AMDGPU_HSA_KERNEL).

Yes, but I'm pretty sure you can go from a SymbolRef to a Elf_Sym *.

[SamWot]

In D21966#474213, @Bigcheese wrote:

In D21966#473994, @SamWot wrote:

In D21966#473780, @Bigcheese wrote:

I find it odd that we need all this machinery just to keep track of the type of a symbol. Why not just make `AllSymbols``` map to a SymbolRef an extract it from there?

Problem here is that SymbolRef::Type isn't same as ELF type. SymbolRef::Type contains only some common types between ELF, COFF and Mach-O file formats. So SymbolRef::Type does not content types that AMDGPU uses (STT_AMDGPU_HSA_KERNEL).

Yes, but I'm pretty sure you can go from a SymbolRef to a Elf_Sym *.

Yes, I think this solution might be better, but it will include rewriting big part of DisassembleObject() that will involve changes in code for other back-ends.
I'm not sure that my change is worth such changes. But If you think it's worth then I can do it.

[SamWot]

In D21966#474988, @SamWot wrote:

In D21966#474213, @Bigcheese wrote:

In D21966#473994, @SamWot wrote:

In D21966#473780, @Bigcheese wrote:

I find it odd that we need all this machinery just to keep track of the type of a symbol. Why not just make `AllSymbols``` map to a SymbolRef an extract it from there?

Problem here is that SymbolRef::Type isn't same as ELF type. SymbolRef::Type contains only some common types between ELF, COFF and Mach-O file formats. So SymbolRef::Type does not content types that AMDGPU uses (STT_AMDGPU_HSA_KERNEL).

Yes, but I'm pretty sure you can go from a SymbolRef to a Elf_Sym *.

Yes, I think this solution might be better, but it will include rewriting big part of DisassembleObject() that will involve changes in code for other back-ends.
I'm not sure that my change is worth such changes. But If you think it's worth then I can do it.

Hi Michael,
What do you think about this changes? Are they really needed?
Actually, I took another look at DisassembleObject(). Changing AllSymbols map to SymbolRef would require even more changes than I thought earlier. Problem is with COFFObjectFile. It have export table and its contents (class ExportDirectoryEntryRef) are currently stored inside AllSymbols. But if we want to change AllSymbols then we would not be able to store ExportDirectoryEntryRef in AllSymbols. To solve this would requier either some greater machinery or changes in COFFObjectFile architecture.

[SamWot]

Ping

[vpykhtin]

In D21966#474213, @Bigcheese wrote:

In D21966#473994, @SamWot wrote:

In D21966#473780, @Bigcheese wrote:

I find it odd that we need all this machinery just to keep track of the type of a symbol. Why not just make `AllSymbols``` map to a SymbolRef an extract it from there?

Problem here is that SymbolRef::Type isn't same as ELF type. SymbolRef::Type contains only some common types between ELF, COFF and Mach-O file formats. So SymbolRef::Type does not content types that AMDGPU uses (STT_AMDGPU_HSA_KERNEL).

Yes, but I'm pretty sure you can go from a SymbolRef to a Elf_Sym *.

For what I see "AllSymbols" is a level of indirection to be able to unify symbol name processing with COFF export table, which contains actually no symbols. One of the decision can be to add some generic "type" field to "AllSymbols" and initialize it where applicable removing the need in additional map machinery.

[SamWot]

In D21966#499160, @vpykhtin wrote:

In D21966#474213, @Bigcheese wrote:

In D21966#473994, @SamWot wrote:

In D21966#473780, @Bigcheese wrote:

I find it odd that we need all this machinery just to keep track of the type of a symbol. Why not just make `AllSymbols``` map to a SymbolRef an extract it from there?

Problem here is that SymbolRef::Type isn't same as ELF type. SymbolRef::Type contains only some common types between ELF, COFF and Mach-O file formats. So SymbolRef::Type does not content types that AMDGPU uses (STT_AMDGPU_HSA_KERNEL).

Yes, but I'm pretty sure you can go from a SymbolRef to a Elf_Sym *.

For what I see "AllSymbols" is a level of indirection to be able to unify symbol name processing with COFF export table, which contains actually no symbols. One of the decision can be to add some generic "type" field to "AllSymbols" and initialize it where applicable removing the need in additional map machinery.

I thought of this but decided that different map whould be better for several reasons:

• It is better if code needed only for AMDGPU would be separeted from general code used by everybody.
• Another thing is that adding another field to AllSymbols would increase it's size for all back-end. Even for those that don't use it.

[SamWot]

Removed extra map. Added symbol type to AllSymbols.

[SamWot]

Ping

[Bigcheese]

lgtm. Sorry for taking so long on the review.

llvm/trunk/test/Object/AMDGPU/objdump.s

 // RUN: llvm-mc -arch=amdgcn -mcpu=tonga %s -filetype=obj | llvm-objdump -disassemble -arch-name=amdgcn -mcpu=tonga - | FileCheck %s
 
   .text
 
   .amdgpu_hsa_kernel hello_world
 hello_world:
   .amd_kernel_code_t
   .end_amd_kernel_code_t
 
   s_mov_b32 m0, 0x10000
   s_load_dwordx2 s[0:1], s[4:5], 0x8
   s_waitcnt lgkmcnt(0)
   s_add_u32 s0, s7, s0
+BB0:
   v_add_i32_e32 v1, vcc, s0, v1
+BB1:
   s_movk_i32 s0, 0x483
   v_cmp_ge_i32_e32 vcc, s0, v0
   s_and_saveexec_b64 s[0:1], vcc
   v_lshlrev_b32_e32 v4, 2, v0
+BB3:
   s_cbranch_execz 21
   s_mov_b64 s[2:3], exec
   s_mov_b64 s[10:11], exec
   v_mov_b32_e32 v3, v0
         s_endpgm
 
   .amdgpu_hsa_kernel hello_world2
 hello_world2:
   .amd_kernel_code_t
   .end_amd_kernel_code_t
 
   s_and_saveexec_b64 s[0:1], vcc
   s_cbranch_execz 85
   s_load_dwordx4 s[8:11], s[4:5], 0x40
+BB5:
   v_ashrrev_i32_e32 v77, 31, v76
   v_lshlrev_b64 v[10:11], 2, v[76:77]
   s_waitcnt lgkmcnt(0)
   v_add_i32_e32 v10, vcc, s8, v10
   v_mov_b32_e32 v6, s9
   v_addc_u32_e32 v11, vcc, v6, v11, vcc
   flat_load_dword v0, v[10:11]
   v_lshlrev_b32_e32 v6, 5, v8
   v_lshlrev_b32_e32 v7, 2, v7
         s_endpgm
 
 // CHECK:  file format ELF64-amdgpu-hsacobj
 // CHECK:  Disassembly of section .text:
 // CHECK:  hello_world:
 // CHECK:  s_mov_b32 m0, 0x10000                                      // 000000000100: BEFC00FF 00010000
 // CHECK:  s_load_dwordx2 s[0:1], s[4:5], 0x8                         // 000000000108: C0060002 00000008
 // CHECK:  s_waitcnt lgkmcnt(0)                                       // 000000000110: BF8C007F
 // CHECK:  s_add_u32 s0, s7, s0                                       // 000000000114: 80000007
+// CHECK:  BB0:
 // CHECK:  v_add_i32_e32 v1, vcc, s0, v1                              // 000000000118: 32020200
+// CHECK:  BB1:
 // CHECK:  s_movk_i32 s0, 0x483                                       // 00000000011C: B0000483
 // CHECK:  v_cmp_ge_i32_e32 vcc, s0, v0                               // 000000000120: 7D8C0000
 // CHECK:  s_and_saveexec_b64 s[0:1], vcc                             // 000000000124: BE80206A
 // CHECK:  v_lshlrev_b32_e32 v4, 2, v0                                // 000000000128: 24080082
+// CHECK:  BB3:
 // CHECK:  s_cbranch_execz 21                                         // 00000000012C: BF880015
 // CHECK:  s_mov_b64 s[2:3], exec                                     // 000000000130: BE82017E
 // CHECK:  s_mov_b64 s[10:11], exec                                   // 000000000134: BE8A017E
 // CHECK:  v_mov_b32_e32 v3, v0                                       // 000000000138: 7E060300
 // CHECK:  s_endpgm                                                   // 00000000013C: BF810000
 
 // CHECK:  hello_world2:
 // CHECK:  s_and_saveexec_b64 s[0:1], vcc                             // 000000000240: BE80206A
 // CHECK:  s_cbranch_execz 85                                         // 000000000244: BF880055
 // CHECK:  s_load_dwordx4 s[8:11], s[4:5], 0x40                       // 000000000248: C00A0202 00000040
+// CHECK:  BB5:
 // CHECK:  v_ashrrev_i32_e32 v77, 31, v76                             // 000000000250: 229A989F
 // CHECK:  v_lshlrev_b64 v[10:11], 2, v[76:77]                        // 000000000254: D28F000A 00029882
 // CHECK:  s_waitcnt lgkmcnt(0)                                       // 00000000025C: BF8C007F
 // CHECK:  v_add_i32_e32 v10, vcc, s8, v10                            // 000000000260: 32141408
 // CHECK:  v_mov_b32_e32 v6, s9                                       // 000000000264: 7E0C0209
 // CHECK:  v_addc_u32_e32 v11, vcc, v6, v11, vcc                      // 000000000268: 38161706
 // CHECK:  flat_load_dword v0, v[10:11]                               // 00000000026C: DC500000 0000000A
 // CHECK:  v_lshlrev_b32_e32 v6, 5, v8                                // 000000000274: 240C1085
 // CHECK:  v_lshlrev_b32_e32 v7, 2, v7                                // 000000000278: 240E0E82
 // CHECK:  s_endpgm                                                   // 00000000027C: BF810000

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

if (Type == MachO::X86_64_RELOC_UNSIGNED && Rel.d.a > 0) {
       if (PrevType == MachO::X86_64_RELOC_SUBTRACTOR)
         return true;
     }
   }
 
   return false;
 }
 
+static uint8_t getElfSymbolType(const ObjectFile *Obj, const SymbolRef &Sym) {
+  assert(Obj->isELF());
+  if (auto *Elf32LEObj = dyn_cast<ELF32LEObjectFile>(Obj))
+    return Elf32LEObj->getSymbol(Sym.getRawDataRefImpl())->getType();
+  if (auto *Elf64LEObj = dyn_cast<ELF64LEObjectFile>(Obj))
+    return Elf64LEObj->getSymbol(Sym.getRawDataRefImpl())->getType();
+  if (auto *Elf32BEObj = dyn_cast<ELF32BEObjectFile>(Obj))
+    return Elf32BEObj->getSymbol(Sym.getRawDataRefImpl())->getType();
+  if (auto *Elf64BEObj = cast<ELF64BEObjectFile>(Obj))
+    return Elf64BEObj->getSymbol(Sym.getRawDataRefImpl())->getType();
+  llvm_unreachable("Unsupported binary format");
+}
+
 static void DisassembleObject(const ObjectFile *Obj, bool InlineRelocs) {
   const Target *TheTarget = getTarget(Obj);
 
   // Package up features to be passed to target/subtarget
   SubtargetFeatures Features = Obj->getFeatures();
   if (MAttrs.size()) {
     for (unsigned i = 0; i != MAttrs.size(); ++i)
       Features.AddFeature(MAttrs[i]);
static void DisassembleObject(const ObjectFile *Obj, bool InlineRelocs) {
   for (const SectionRef &Section : ToolSectionFilter(*Obj)) {
     section_iterator Sec2 = Section.getRelocatedSection();
     if (Sec2 != Obj->section_end())
       SectionRelocMap[*Sec2].push_back(Section);
   }
 
   // Create a mapping from virtual address to symbol name.  This is used to
   // pretty print the symbols while disassembling.
-  typedef std::vector<std::pair<uint64_t, StringRef>> SectionSymbolsTy;
+  typedef std::vector<std::tuple<uint64_t, StringRef, uint8_t>> SectionSymbolsTy;
   std::map<SectionRef, SectionSymbolsTy> AllSymbols;
   for (const SymbolRef &Symbol : Obj->symbols()) {
     Expected<uint64_t> AddressOrErr = Symbol.getAddress();
     error(errorToErrorCode(AddressOrErr.takeError()));
     uint64_t Address = *AddressOrErr;
 
     Expected<StringRef> Name = Symbol.getName();
     error(errorToErrorCode(Name.takeError()));
     if (Name->empty())
       continue;
 
     Expected<section_iterator> SectionOrErr = Symbol.getSection();
     error(errorToErrorCode(SectionOrErr.takeError()));
     section_iterator SecI = *SectionOrErr;
     if (SecI == Obj->section_end())
       continue;
     
-    AllSymbols[*SecI].emplace_back(Address, *Name);
+    // For AMDGPU we need to track symbol types
+    uint8_t SymbolType = ELF::STT_NOTYPE;
+    if (Obj->isELF() && Obj->getArch() == Triple::amdgcn) {
+      SymbolType = getElfSymbolType(Obj, Symbol);
+    }
+
+    AllSymbols[*SecI].emplace_back(Address, *Name, SymbolType);
+
   }
 
   // Create a mapping from virtual address to section.
   std::vector<std::pair<uint64_t, SectionRef>> SectionAddresses;
   for (SectionRef Sec : Obj->sections())
     SectionAddresses.emplace_back(Sec.getAddress(), Sec);
   array_pod_sort(SectionAddresses.begin(), SectionAddresses.end());
 
for (const auto &ExportEntry : COFFObj->export_directories()) {
             return LHS < RHS.first;
           });
       if (Sec != SectionAddresses.begin())
         --Sec;
       else
         Sec = SectionAddresses.end();
 
       if (Sec != SectionAddresses.end())
-        AllSymbols[Sec->second].emplace_back(VA, Name);
+        AllSymbols[Sec->second].emplace_back(VA, Name, ELF::STT_NOTYPE);
     }
   }
 
   // Sort all the symbols, this allows us to use a simple binary search to find
   // a symbol near an address.
   for (std::pair<const SectionRef, SectionSymbolsTy> &SecSyms : AllSymbols)
     array_pod_sort(SecSyms.second.begin(), SecSyms.second.end());
 
   for (const SectionRef &Section : ToolSectionFilter(*Obj)) {
     if (!DisassembleAll && (!Section.isText() || Section.isVirtual()))
       continue;
 
     uint64_t SectionAddr = Section.getAddress();
     uint64_t SectSize = Section.getSize();
     if (!SectSize)
       continue;
 
     // Get the list of all the symbols in this section.
     SectionSymbolsTy &Symbols = AllSymbols[Section];
     std::vector<uint64_t> DataMappingSymsAddr;
     std::vector<uint64_t> TextMappingSymsAddr;
     if (Obj->isELF() && Obj->getArch() == Triple::aarch64) {
       for (const auto &Symb : Symbols) {
-        uint64_t Address = Symb.first;
-        StringRef Name = Symb.second;
+        uint64_t Address = std::get<0>(Symb);
+        StringRef Name = std::get<1>(Symb);
         if (Name.startswith("$d"))
           DataMappingSymsAddr.push_back(Address - SectionAddr);
         if (Name.startswith("$x"))
           TextMappingSymsAddr.push_back(Address - SectionAddr);
       }
     }
 
     std::sort(DataMappingSymsAddr.begin(), DataMappingSymsAddr.end());
for (const SectionRef &Section : ToolSectionFilter(*Obj)) {
     StringRef name;
     error(Section.getName(name));
     outs() << "Disassembly of section ";
     if (!SegmentName.empty())
       outs() << SegmentName << ",";
     outs() << name << ':';
 
     // If the section has no symbol at the start, just insert a dummy one.
-    if (Symbols.empty() || Symbols[0].first != 0)
-      Symbols.insert(Symbols.begin(), std::make_pair(SectionAddr, name));
+    if (Symbols.empty() || std::get<0>(Symbols[0]) != 0) {
+      Symbols.insert(Symbols.begin(), std::make_tuple(SectionAddr, name, ELF::STT_NOTYPE));
+    }
 
     SmallString<40> Comments;
     raw_svector_ostream CommentStream(Comments);
 
     StringRef BytesStr;
     error(Section.getContents(BytesStr));
     ArrayRef<uint8_t> Bytes(reinterpret_cast<const uint8_t *>(BytesStr.data()),
                             BytesStr.size());
 
     uint64_t Size;
     uint64_t Index;
 
     std::vector<RelocationRef>::const_iterator rel_cur = Rels.begin();
     std::vector<RelocationRef>::const_iterator rel_end = Rels.end();
     // Disassemble symbol by symbol.
     for (unsigned si = 0, se = Symbols.size(); si != se; ++si) {
-
-      uint64_t Start = Symbols[si].first - SectionAddr;
+      uint64_t Start = std::get<0>(Symbols[si]) - SectionAddr;
       // The end is either the section end or the beginning of the next
       // symbol.
       uint64_t End =
-          (si == se - 1) ? SectSize : Symbols[si + 1].first - SectionAddr;
+          (si == se - 1) ? SectSize : std::get<0>(Symbols[si + 1]) - SectionAddr;
       // Don't try to disassemble beyond the end of section contents.
       if (End > SectSize)
         End = SectSize;
       // If this symbol has the same address as the next symbol, then skip it.
       if (Start >= End)
         continue;
 
       if (Obj->isELF() && Obj->getArch() == Triple::amdgcn) {
         // make size 4 bytes folded
         End = Start + ((End - Start) & ~0x3ull);
-        Start += 256; // add sizeof(amd_kernel_code_t)
-        // cut trailing zeroes - up to 256 bytes (align)
+        if (std::get<2>(Symbols[si]) == 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) {
+          // 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;
         }
+      }
 
-      outs() << '\n' << Symbols[si].second << ":\n";
+      outs() << '\n' << std::get<1>(Symbols[si]) << ":\n";
 
 #ifndef NDEBUG
       raw_ostream &DebugOut = DebugFlag ? dbgs() : nulls();
 #else
       raw_ostream &DebugOut = nulls();
 #endif
 
       for (Index = Start; Index < End; Index += Size) {
#endif
             }
 
             // Find the first symbol in the section whose offset is less than
             // or equal to the target.
             if (TargetSectionSymbols) {
               auto TargetSym = std::upper_bound(
                   TargetSectionSymbols->begin(), TargetSectionSymbols->end(),
                   Target, [](uint64_t LHS,
-                              const std::pair<uint64_t, StringRef> &RHS) {
-                    return LHS < RHS.first;
+                             const std::tuple<uint64_t, StringRef, uint8_t> &RHS) {
+                    return LHS < std::get<0>(RHS);
                   });
               if (TargetSym != TargetSectionSymbols->begin()) {
                 --TargetSym;
                 uint64_t TargetAddress = std::get<0>(*TargetSym);
                 StringRef TargetName = std::get<1>(*TargetSym);
                 outs() << " <" << TargetName;
                 uint64_t Disp = Target - TargetAddress;
                 if (Disp)