[AMDGPU] Improve disassembler error handling

[timcorringham]

Summary

llvm-objdump now disassembles unrecognised opcodes as data, using
the .long directive. We treat unrecognised opcodes as being 32 bit
values, so move along 4 bytes rather than the single byte which
previously resulted in a cascade of bogus disassembly following an
unrecognised opcode.

While no solution can always disassemble code that contains
embedded data correctly this provides a significant improvement.

Fixes bug in https://llvm.org/bugs/show_bug.cgi?id=36347

[artem.tamazov]

Adding Dmitry.

[artem.tamazov]

Does this resolve any bugzilla?

[timcorringham]

This implements Bug 36347

[artem.tamazov]

In D44685#1043335, @timcorringham wrote:

This implements Bug 36347

We have to look into it. Meanwhile, could you please update the patch description with bugzilla id so it will get into svn when checked in. Thanks.

[artem.tamazov]

tools/llvm-objdump/llvm-objdump.cpp

611

What if there are less than 4 bytes left? Let's make sure that we are not getting into undefined behavior in such a case.

[timcorringham]

lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp

250

This should return the minimum of 4 or the number of bytes remaining if the disassembly failed.

tools/llvm-objdump/llvm-objdump.cpp

611

Currently there is code in DisassembleObject() - line 1475 - that truncates the section to a multiple of 4 bytes, so we won't encounter the case of having fewer than 4 bytes left here.
However, maybe we should rethink that behaviour as it would be possible for data to be deliberately added to the end of a section that resulted in a non-multiple-of-4 length, and we could cope with that in the disassembly by using .byte directives.

[dp]

tools/llvm-objdump/llvm-objdump.cpp

611

Could you add an assert?

assert(Bytes.size() % sizeof(U32) == 0);

With this assert the assumption about section length will be clear. A relevant comment would be helpful too.

[dp]

Otherwise look fine.

[artem.tamazov]

tools/llvm-objdump/llvm-objdump.cpp

611

I am second to this. Let's make our assumptions (specifically, an assumption that DisassembleObject() truncates the section to a multiple of 4 bytes) expliicit.

[timcorringham]

Added support for disassembly of arbitrary size sections

The text section size is no longer truncated to a multiple
of 4 bytes, and the .byte directive is used to disassemble
any remaining bytes at the end of a section.

[artem.tamazov]

Looks good.

lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp

DecodeStatus AMDGPUDisassembler::getInstruction(MCInst &MI, uint64_t &Size,
 
   if (Res && (MCII->get(MI.getOpcode()).TSFlags & SIInstrFlags::MIMG)) {
     Res = convertMIMGInst(MI);
   }
 
   if (Res && IsSDWA)
     Res = convertSDWAInst(MI);
 
-  Size = Res ? (MaxInstBytesNum - Bytes.size()) : 0;
+  // if the opcode was not recognized we'll assume a Size of 4 bytes
+  Size = Res ? (MaxInstBytesNum - Bytes.size()) : 4;
   return Res;
 }
 
 DecodeStatus AMDGPUDisassembler::convertSDWAInst(MCInst &MI) const {
   if (STI.getFeatureBits()[AMDGPU::FeatureGFX9]) {
     if (AMDGPU::getNamedOperandIdx(MI.getOpcode(), AMDGPU::OpName::sdst) != -1)
       // VOPC - insert clamp
       insertNamedMCOperand(MI, MCOperand::createImm(0), AMDGPU::OpName::clamp);

test/MC/AMDGPU/data.s

+// We check that unrecognized opcodes are disassembled by llvm-objdump as data using the .long directive 
+// RUN: llvm-mc -filetype=obj -triple=amdgcn--amdpal -mcpu=gfx900 -show-encoding %s | llvm-objdump -disassemble -mcpu=gfx900 - | FileCheck %s
+
+.text
+        v_mov_b32     v7, s24
+  v_mov_b32     v8, s25
+  .long         0xabadc0de
+  s_nop         0
+  s_endpgm                  
+  .long         0xbadc0de1, 0xbadc0de2, 0xbadc0de3, 0xbadc0de4
+
+// CHECK: .text
+// CHECK: v_mov_b32
+// CHECK: v_mov_b32
+// CHECK: .long 0xabadc0de
+// CHECK_SAME: : ABADC0DE
+// CHECK: s_endpgm
+// CHECK: .long 0xbadc0de1
+// CHECK: .long 0xbadc0de2
+// CHECK: .long 0xbadc0de3
+// CHECK: .long 0xbadc0de4
+// CHECK-NOT: .long

tools/llvm-objdump/llvm-objdump.cpp

 class AMDGCNPrettyPrinter : public PrettyPrinter {
 public:
   void printInst(MCInstPrinter &IP, const MCInst *MI, ArrayRef<uint8_t> Bytes,
                  uint64_t Address, raw_ostream &OS, StringRef Annot,
                  MCSubtargetInfo const &STI, SourcePrinter *SP) override {
     if (SP && (PrintSource || PrintLines))
       SP->printSourceLine(OS, Address);
 
-    if (!MI) {
-      OS << " <unknown>";
-      return;
-    }
+    typedef support::ulittle32_t U32;
 
+    if (MI) {
       SmallString<40> InstStr;
       raw_svector_ostream IS(InstStr);
 
       IP.printInst(MI, IS, "", STI);
 
-    OS << left_justify(IS.str(), 60) << format("// %012" PRIX64 ": ", Address);
-    typedef support::ulittle32_t U32;
+      OS << left_justify(IS.str(), 60);
+    } else {
+      // an unrecognized encoding - this is probably data so represent it
+      // using the .long directive
+      OS << format("\t.long 0x%08" PRIx32 " ",
+                   static_cast<uint32_t>(*reinterpret_cast<const U32*>(Bytes.data())));
+      OS.indent(42);
+    }
+
+    OS << format("// %012" PRIX64 ": ", Address);
     for (auto D : makeArrayRef(reinterpret_cast<const U32*>(Bytes.data()),
                                Bytes.size() / sizeof(U32)))
       // D should be explicitly casted to uint32_t here as it is passed
       // by format to snprintf as vararg.
       OS << format("%08" PRIX32 " ", static_cast<uint32_t>(D));
 
     if (!Annot.empty())
       OS << "// " << Annot;