[MC] Changes to help improve target specific symbol disassembly

[rochauha]

Summary

This commit slightly modifies the MCDisassembler, and llvm-objdump to allow targets to also decode entire symbols.

WebAssembly uses the onSymbolStart hook it to decode preludes. WebAssembly partially
disassembles the symbol in its target specific way; and then falls back to the normal
flow of llvm-objdump.

AMDGPU needs it to decode kernel descriptors entirely, and move to the next symbol.

This commit is to split the above task into 2.

• Changes to llvm-objdump and MC-layer without breaking WebAssembly code [ this commit ]
• AMDGPU's implementation of onSymbolStart that decodes kernel descriptors. [ https://reviews.llvm.org/D80713 ]

PS:
The last 6 bytes of AMDGPU kernel descriptor are reserved and hence 0.
Right now 4 bytes of it are trimmed in llvm-objdump, which makes the kernel descriptor invalid.
I have commented out those AMDGPU specific parts in this commit.
The nop-data.ll test failure in AMDGPU tests is because of this.

[MaskRay]

This commit slightly modifies the MCDisassembler, and llvm-objdump to allow targets to also decode entire symbols.

What does "decode entire symbols" mean?

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

1412

This breaks test/CodeGen/AMDGPU/nop-data.ll

[rochauha]

What does "decode entire symbols" mean?

It means to disassemble all bytes of a particular symbol.

[jhenderson]

(Not got time to look at the patch yet, but one comment I wanted to make)

In D80512#2053760, @rochauha wrote:

What does "decode entire symbols" mean?

It means to disassemble all bytes of a particular symbol.

Are you aware of the --disassemble-symbols switch which disassembles the bytes for a specified symbol already?

[rochauha]

Are you aware of the --disassemble-symbols switch which disassembles the bytes for a specified symbol already?

Yes, but that disassembles the bytes of a specified symbol as instructions.
This commit allows the target to handle certain symbols as part of --disassemble-all; in a way that the target requires.

For example - AMDGPU needs to disassemble kernel descriptors as assembler directives rather than instructions.

[rochauha]

Changes to MCDisassembler and llvm-objdump to allow targets to also decode entire symbols.

Not all symbols need to be disassembled as instructions.
Targets can handle certain symbols as part of --disassemble-all; in a way that they require.

WebAssembly uses the onSymbolStart hook to handle preludes.
This commit tries to generalize onSymbolStart so that other targets can also use it (if they want to)

Now the entire symbol can be disassembled in a target specific way. We then move to the next
symbol or fall back (only if the symbol is not disassembled entirely) to decoding remaining bytes as instructions.

AMDGPU needs it to decode kernel descriptors as assembler directives rather than instructions.

This commit is to split the above task into 2.

• Changes to llvm-objdump and MC-layer without breaking WebAssembly code [ this commit ]
• AMDGPU's implementation of onSymbolStart that decodes kernel descriptors. [ https://reviews.llvm.org/D80713 ]

Edit:

• Uncommented lines in llvm-objdump that broke test/CodeGen/AMDGPU/nop-data.ll (as pointed out by MaskRay)
• Small change to the new helper functions to read bytes
• Changes to this summary

[jhenderson]

Am I right in thinking that this change is now NFC? If so, please add that to the title.

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

76–97

Seems like this whole comment needs updating if you are adding a new enum value.

124–125

Looks like this comment might need reflowing - "particular" at least looks like it belongs on the previous line.

126

';' -> ','

127–128

I don't think this last sentence is a good comment to have, as it implies that WebAssembly and AMDGPU are the only two cases that make use of this, which might be true now, but probably won't be true in the future. I think it's simplest to say "This is used for example by AMDGPU to decode kernel descriptors."

156

Nit: missing trailing full stop.

As it's a TODO, perhaps this whole comment should not use doxygen style comment markers?

167

Nit: missing trailing full stop (feel free to add to the comment below too).

168–175

Variable names should all be UpperCamelCase (index -> Index, doubleWord -> DoubleWord etc). Also applies to code body.

llvm/lib/MC/MCDisassembler/MCDisassembler.cpp

27

There is already a class DataExtractor in Support for reading a series of bytes of various sizes. I think it would make more sense to use that than reinvent the wheel with these methods. It also has error handling, which can be used to detect failures to read and handles endianness.

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

1399

If you're going to modify this line, please also add the missing full stop and "skip" -> "Skip". Seems like it's unrelated to this patch.

1408–1409

This also appears to be unrelated to this patch.

[rochauha]

Changes as per review by jhenderson.

[aardappel]

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

127

Maybe retain the original comment that it is also used for WebAssembly?

[scott.linder]

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

76–97

It seems like beyond the need to just update the comment all the uses of DeocdeStatus need to be audited to possibly consider the new variant. The big one is the CHECK macro which relies on the bitwise-AND reduction property of the current encoding, which Ignore seems to break (and maybe it just doesn't really fit in to begin with; what should Ignore & Success mean? It currently means Fail, which doesn't seem particularly helpful).

It seems like what we want instead is to have two orthogonal statuses: a boolean one to indicate whether any decoding is possible and was attempted, and then the ternary status for the decode in the case that it happens. Maybe we just want to leave DecodeStatus alone and have onSymbolStart return an Optional<onSymbolStart> where None indicates no decoding was attempted?

86

This wording seems ambiguous; when I first read it I took it to mean that Ignore and SoftFail are equivalent. As the above describes SoftFail already can this just describe Ignore here? I also don't know what you mean when you say SoftFail can be used for partial disassembly; I thought from the description it just meant the decoded instruction is architecturally invalid, but is a decodeable form?

The comment also seems to use Unpredictable and SoftFail interchangeably, can this all just be changed to refer to SoftFail?

llvm/lib/Target/WebAssembly/Disassembler/WebAssemblyDisassembler.cpp

130

I'm a little confused here on what the intended semantics for this impl are, but it doesn't seem like the intention would be to Ignore here, right? Since llvm-objdump.cpp ignores the return value completely it isn't clear what was really intended (i.e. this could have returned anything, yet chose to return different values for different cases).

Maybe @aardappel can describe the intent a bit better?

155

This seems to be overloading what SoftFail means. It seems like the original intent of SoftFail was to capture a Success which is "poisoned" by architectural meaningless-ness. Thus Success & SoftFail == SoftFail but SoftFail & Fail == Fail.

It seems like Success is the right thing to return here still. Can't the callee know whether or not the "whole" symbol was consumed by just considering the Size value that is returned along with the status?

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

1434

Going along with the comments I left above, I don't think we need to subvert Success and SoftFail here to support "partial" decoding. I would vote to just use Size to detect when the "entire" symbol was decoded.

1437

I would prefer this just fall out of the code below, rather than need to be explicit here. Things like onSymbolEnd should still be considered even when the whole symbol is decoded, right?

[jhenderson]

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

16

Unrelated formatting change. Please revert.

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

1399

Actually, why was this line modified at all?

1408–1409

If it wasn't clear what I meant before, since this change is unrelated to this patch (it's just a pure formatting change and you aren't touching this bit of code), I think it needs reverting.

[rochauha]

Changes as per review by @aardappel and @jhenderson

[jhenderson]

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

1399

My question hasn't been answered. Please revert the changes to this line, or explain why you wish to modify it.

[rochauha]

@scott.linder, the DecodeStatus enum seems to mainly be concerned with instructions.
But I felt thought that we could probably use it with symbols too. Just that a symbol SoftFail would not mean the same thing as an instruction SoftFail.

SoftFail and Ignore are different. When target returns SoftFail or Success, the Size variable must be updated by the target. When returning Ignore, value of Size must be 0.

The idea of using Optional<> and Size value with the status sounds good. Comments from others regarding this would also be helpful.

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

76–97

I believe Ignore should not be used with the other 3 decode statuses.

[rochauha]

@jhenderson, following was your previous comment on line 1399
> If you're going to modify this line, please also add the missing full stop and "skip" -> "Skip". Seems like it's unrelated to this patch.

So I modified the line when making revisions to the commit.

[jhenderson]

In D80512#2062908, @rochauha wrote:

@jhenderson, following was your previous comment on line 1399
> If you're going to modify this line, please also add the missing full stop and "skip" -> "Skip". Seems like it's unrelated to this patch.

So I modified the line when making revisions to the commit.

Right, but the modification prior to my comment there was just to remove a space in the comment. I said "if you are going to modify this line", but on closer look, I'm not sure why you wanted to modify the line in the first place, hence my question.

[rochauha]

@jhenderson apologies for the misunderstanding from my side. I'll undo that change.
That entire chunk of code is commented out in the child revision anyways.

[rochauha]

I'd like to understand the blockers that need to be worked on for this review to be accepted.

[jhenderson]

No more additional comments from me, but I don't know enough about this code to give any further feedback on the actual behaviour changes. @scott.linder should take the lead from here. I'm approving now, but only to clear my blockage. It is NOT an approval of the patch in general.

[MaskRay]

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

1440

The comments should probably be moved before if.

You can say that most targets return Ignore, and state how AMDGPU and WebAssembly are different and why they do continue.

[rochauha]

Changes as per reviews by @scott.linder, @jhenderson and @MaskRay

[rochauha]

To have round trippable disassembly, we fall back to decoding the
remaining bytes as instructions.

If there is a failure, we disassemble the 'failed' region as bytes
before falling back.

• This means that on failure the target must print whatever it disassembled so far as comments? Requires discussion.
• Otherwise we would have decoded some bytes twice. Once by the target (until it failed) and then again as bytes.

If there is Success or SoftFail i.e no 'real' failure, we skip over
Size bytes before falling back.
So if the entire symbol is 'eaten' by the target:

Start += Size  // Now Start = End and we will never decode as instructions

Right now, most targets return None i.e ignore to treat a symbol
separately. But WebAssembly decodes preludes for some symbols.

[rochauha]

Run git-clang-format.

[rochauha]

Ping.

[scott.linder]

LGTM, sorry for the delay in reviewing. My feeling is that we should just keep the simple rules you have outlined in the docs and require the callback implementation ensure things are not decoded twice by buffering their output (if needed) and setting Size correctly.

llvm/lib/MC/MCDisassembler/MCDisassembler.cpp

23

Can this be removed? The contract is this value can't be read anyway.

24–58

I think the comment here is overkill, the comment for the function itself already describes the meaning.

llvm/lib/Target/WebAssembly/Disassembler/WebAssemblyDisassembler.cpp

131

Same as above, I think the comment on each return is overkill and likely to just become out of date at some point in the worst case.

145

In the original implementation, I'm lost on what was intended here. The ".local" has already made it to the output stream by this point. I don't know how to review the change without knowing what the intention was (if this is actually a bug; maybe I just don't understand at all). The behavior is not changing, though, so I don't think we need to wait on understanding the intention to land this change.

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

1442

I think we either need to require the callback buffer output so it can avoid detecting an error in the middle of output, or we need to factor out that buffering by passing in a buffered stream and only appending it to the disassembly output in the Success case.

[rochauha]

Minor changes as per review by @scott.linder.

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/Optional.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/BinaryFormat/XCOFF.h"
 #include "llvm/MC/MCDisassembler/MCSymbolizer.h"
+#include "llvm/Support/Endian.h"
 #include <cstdint>
 #include <memory>
 #include <vector>
 
 namespace llvm {
 
 struct XCOFFSymbolInfo {
   Optional<XCOFF::StorageMappingClass> StorageMappingClass;

 class MCInst;
 class MCSubtargetInfo;
 class raw_ostream;
 
 /// Superclass for all disassemblers. Consumes a memory region and provides an
 /// array of assembly instructions.
 class MCDisassembler {
 public:
   /// Ternary decode status. Most backends will just use Fail and
   /// Success, however some have a concept of an instruction with
   /// understandable semantics but which is architecturally
   /// incorrect. An example of this is ARM UNPREDICTABLE instructions
   /// which are disassemblable but cause undefined behaviour.
   ///
   /// Because it makes sense to disassemble these instructions, there
   /// is a "soft fail" failure mode that indicates the MCInst& is
   /// valid but architecturally incorrect.
   ///
   /// The enum numbers are deliberately chosen such that reduction
   /// from Success->SoftFail ->Fail can be done with a simple
   /// bitwise-AND:
   ///
   ///   LEFT & TOP =  | Success       Unpredictable   Fail
   ///   --------------+-----------------------------------
   ///   Success       | Success       Unpredictable   Fail
   ///   Unpredictable | Unpredictable Unpredictable   Fail
   ///   Fail          | Fail          Fail            Fail
   ///
   /// An easy way of encoding this is as 0b11, 0b01, 0b00 for
   /// Success, SoftFail, Fail respectively.
-  enum DecodeStatus {
-    Fail = 0,
-    SoftFail = 1,
-    Success = 3
-  };
+  enum DecodeStatus { Fail = 0, SoftFail = 1, Success = 3, Ignore = 4 };
 
   MCDisassembler(const MCSubtargetInfo &STI, MCContext &Ctx)
     : Ctx(Ctx), STI(STI) {}
 
   virtual ~MCDisassembler();
 
   /// Returns the disassembly of a single instruction.
   ///
public:
   /// \return         - MCDisassembler::Success if the instruction is valid,
   ///                   MCDisassembler::SoftFail if the instruction was
   ///                                            disassemblable but invalid,
   ///                   MCDisassembler::Fail if the instruction was invalid.
   virtual DecodeStatus getInstruction(MCInst &Instr, uint64_t &Size,
                                       ArrayRef<uint8_t> Bytes, uint64_t Address,
                                       raw_ostream &CStream) const = 0;
 
-  /// May parse any prelude that precedes instructions after the start of a
-  /// symbol. Needed for some targets, e.g. WebAssembly.
+  /// Used to perform separate target specific disassembly for a
+  /// particular symbol. May parse any prelude that precedes instructions
+  /// after the start of a symbol symbol; or the entire symbol.
+  /// Right now WebAssembly implements this, and AMDGPU needs it to deccode
+  /// kernel descriptors.
+  ///
+  /// Base implementation returns
+  /// MCDisassembler::Ignore
   ///
   /// \param Name     - The name of the symbol.
   /// \param Size     - The number of bytes consumed.
   /// \param Address  - The address, in the memory space of region, of the first
   ///                   byte of the symbol.
   /// \param Bytes    - A reference to the actual bytes at the symbol location.
   /// \param CStream  - The stream to print comments and annotations on.
-  /// \return         - MCDisassembler::Success if the bytes are valid,
-  ///                   MCDisassembler::Fail if the bytes were invalid.
-  virtual DecodeStatus onSymbolStart(StringRef Name, uint64_t &Size,
+  /// \return         - MCDisassembler::Success if entire symbol is decoded
+  /// successfully by the target.
+  ///                   MCDisassembler::Fail if the bytes are invalid
+  ///                   MCDisassembler::SoftFail if the target needs to handle
+  ///                   some bytes i.e not the entire symbol
+  ///                   MCDisassembler::Ignore if the target doesn't want to
+  ///                   handle the symbol separately
+  virtual DecodeStatus onSymbolStart(SymbolInfoTy &Symbol, uint64_t &Size,
                                      ArrayRef<uint8_t> Bytes, uint64_t Address,
                                      raw_ostream &CStream) const;
+  /// TODO:
+  /// Implement similar hooks that can be used at other points during
+  /// disassembly. Something along the following lines:
+  /// - onBeforeInstructionDecode()
+  /// - onAfterInstructionDecode()
+  /// - onSymbolEnd()
+  /// It should help move much of the target specific code from llvm-objdump to
+  /// respective target disassemblers
 
 private:
   MCContext &Ctx;
 
 protected:
   // Subtarget information, for instruction decoding predicates if required.
   const MCSubtargetInfo &STI;
   std::unique_ptr<MCSymbolizer> Symbolizer;
 
 public:
+  // Helpers to read bytes
+  bool getWord(support::endianness E, ArrayRef<uint8_t> Bytes, size_t index,
+               uint16_t &word) const;
+
+  bool getDoubleWord(support::endianness E, ArrayRef<uint8_t> Bytes,
+                     size_t index, uint32_t &doubleWord) const;
+
+  bool getQuadWord(support::endianness E, ArrayRef<uint8_t> Bytes, size_t index,
+                   uint64_t &quadWord) const;
+
   // Helpers around MCSymbolizer
   bool tryAddingSymbolicOperand(MCInst &Inst,
                                 int64_t Value,
                                 uint64_t Address, bool IsBranch,
                                 uint64_t Offset, uint64_t InstSize) const;
 
   void tryAddingPcLoadReferenceComment(int64_t Value, uint64_t Address) const;
 

llvm/lib/MC/MCDisassembler/MCDisassembler.cpp

 #include "llvm/Support/raw_ostream.h"
 #include <algorithm>
 
 using namespace llvm;
 
 MCDisassembler::~MCDisassembler() = default;
 
 MCDisassembler::DecodeStatus
-MCDisassembler::onSymbolStart(StringRef Name, uint64_t &Size,
+MCDisassembler::onSymbolStart(SymbolInfoTy &Symbol, uint64_t &Size,
                               ArrayRef<uint8_t> Bytes, uint64_t Address,
                               raw_ostream &CStream) const {
   Size = 0;
-  return MCDisassembler::Success;
+  return MCDisassembler::Ignore;
+}
+
+bool MCDisassembler::getWord(support::endianness E, ArrayRef<uint8_t> Bytes,
+                             size_t index, uint16_t &word) const {
+  if (index + 2 > Bytes.size())
+    return false;
+  if (E == support::little)
+    word = *reinterpret_cast<const support::ulittle16_t *>(&Bytes[index]);
+  else if (E == support::big)
+    word = *reinterpret_cast<const support::ubig16_t *>(&Bytes[index]);
+  return true;
+}
+
+bool MCDisassembler::getDoubleWord(support::endianness E,
+                                   ArrayRef<uint8_t> Bytes, size_t index,
+                                   uint32_t &doubleWord) const {
+  if (index + 4 > Bytes.size())
+    return false;
+  if (E == support::little)
+    doubleWord = *reinterpret_cast<const support::ulittle32_t *>(&Bytes[index]);
+  else if (E == support::big)
+    doubleWord = *reinterpret_cast<const support::ubig32_t *>(&Bytes[index]);
+  return true;
+}
+
+bool MCDisassembler::getQuadWord(support::endianness E, ArrayRef<uint8_t> Bytes,
+                                 size_t index, uint64_t &quadWord) const {
+  if (index + 8 > Bytes.size())
+    return false;
+  if (E == support::little)
+    quadWord = *reinterpret_cast<const support::ulittle64_t *>(&Bytes[index]);
+  else if (E == support::big)
+    quadWord = *reinterpret_cast<const support::ubig64_t *>(&Bytes[index]);
+  return true;
 }
 
 bool MCDisassembler::tryAddingSymbolicOperand(MCInst &Inst, int64_t Value,
                                               uint64_t Address, bool IsBranch,
                                               uint64_t Offset,
                                               uint64_t InstSize) const {
   if (Symbolizer)
     return Symbolizer->tryAddingSymbolicOperand(

llvm/lib/Target/WebAssembly/Disassembler/WebAssemblyDisassembler.cpp

 static constexpr int WebAssemblyInstructionTableSize = 256;
 
 class WebAssemblyDisassembler final : public MCDisassembler {
   std::unique_ptr<const MCInstrInfo> MCII;
 
   DecodeStatus getInstruction(MCInst &Instr, uint64_t &Size,
                               ArrayRef<uint8_t> Bytes, uint64_t Address,
                               raw_ostream &CStream) const override;
-  DecodeStatus onSymbolStart(StringRef Name, uint64_t &Size,
+  DecodeStatus onSymbolStart(SymbolInfoTy &Symbol, uint64_t &Size,
                              ArrayRef<uint8_t> Bytes, uint64_t Address,
                              raw_ostream &CStream) const override;
 
 public:
   WebAssemblyDisassembler(const MCSubtargetInfo &STI, MCContext &Ctx,
                           std::unique_ptr<const MCInstrInfo> MCII)
       : MCDisassembler(STI, Ctx), MCII(std::move(MCII)) {}
 };
if (std::is_floating_point<T>::value) {
     MI.addOperand(MCOperand::createFPImm(static_cast<double>(Val)));
   } else {
     MI.addOperand(MCOperand::createImm(static_cast<int64_t>(Val)));
   }
   return true;
 }
 
 MCDisassembler::DecodeStatus WebAssemblyDisassembler::onSymbolStart(
-    StringRef Name, uint64_t &Size, ArrayRef<uint8_t> Bytes, uint64_t Address,
-    raw_ostream &CStream) const {
+    SymbolInfoTy &Symbol, uint64_t &Size, ArrayRef<uint8_t> Bytes,
+    uint64_t Address, raw_ostream &CStream) const {
   Size = 0;
   if (Address == 0) {
     // Start of a code section: we're parsing only the function count.
     int64_t FunctionCount;
     if (!nextLEB(FunctionCount, Bytes, Size, false))
-      return MCDisassembler::Fail;
+      return MCDisassembler::Ignore;
     outs() << "        # " << FunctionCount << " functions in section.";
   } else {
     // Parse the start of a single function.
     int64_t BodySize, LocalEntryCount;
     if (!nextLEB(BodySize, Bytes, Size, false) ||
         !nextLEB(LocalEntryCount, Bytes, Size, false))
-      return MCDisassembler::Fail;
+      return MCDisassembler::Ignore;
     if (LocalEntryCount) {
       outs() << "        .local ";
       for (int64_t I = 0; I < LocalEntryCount; I++) {
         int64_t Count, Type;
         if (!nextLEB(Count, Bytes, Size, false) ||
             !nextLEB(Type, Bytes, Size, false))
-          return MCDisassembler::Fail;
+          return MCDisassembler::Ignore;
         for (int64_t J = 0; J < Count; J++) {
           if (I || J)
             outs() << ", ";
           outs() << WebAssembly::anyTypeToString(Type);
         }
       }
     }
   }
   outs() << "\n";
-  return MCDisassembler::Success;
+  return MCDisassembler::SoftFail;
 }
 
 MCDisassembler::DecodeStatus WebAssemblyDisassembler::getInstruction(
     MCInst &MI, uint64_t &Size, ArrayRef<uint8_t> Bytes, uint64_t /*Address*/,
     raw_ostream &CS) const {
   CommentStream = &CS;
   Size = 0;
   int Opc = nextByte(Bytes, Size);

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

for (unsigned SI = 0, SE = Symbols.size(); SI != SE; ++SI) {
         outs() << "\nDisassembly of section ";
         if (!SegmentName.empty())
           outs() << SegmentName << ",";
         outs() << SectionName << ":\n";
       }
 
       if (Obj->isELF() && Obj->getArch() == Triple::amdgcn) {
         if (Symbols[SI].Type == ELF::STT_AMDGPU_HSA_KERNEL) {
-          // skip amd_kernel_code_t at the begining of kernel symbol (256 bytes)
+          // skip amd_kernel_code_t at the begining of kernel symbol (256bytes)
           Start += 256;
         }
         if (SI == SE - 1 ||
             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)
+          while (End > Limit && *reinterpret_cast<const support::ulittle32_t *>(
+                                    &Bytes[End - 4]) == 0)
             End -= 4;
         }
       }
 
       outs() << '\n';
       if (!NoLeadingAddr)
         outs() << format(Is64Bits ? "%016" PRIx64 " " : "%08" PRIx64 " ",
                          SectionAddr + Start + VMAAdjustment);
       if (Obj->isXCOFF() && SymbolDescription) {
         outs() << getXCOFFSymbolDescription(Symbols[SI], SymbolName) << ":\n";
       } else
         outs() << '<' << SymbolName << ">:\n";
 
       // Don't print raw contents of a virtual section. A virtual section
       // doesn't have any contents in the file.
       if (Section.isVirtual()) {
         outs() << "...\n";
         continue;
       }
 
-      // Some targets (like WebAssembly) have a special prelude at the start
-      // of each symbol.
-      DisAsm->onSymbolStart(SymbolName, Size, Bytes.slice(Start, End - Start),
-                            SectionAddr + Start, CommentStream);
+      MCDisassembler::DecodeStatus status = DisAsm->onSymbolStart(
+          Symbols[SI], Size, Bytes.slice(Start, End - Start),
+
+      if (status == MCDisassembler::Success || status == MCDisassembler::Fail) {
+        if (status == MCDisassembler::Fail)
+          outs() << "Error in decoding " << SymbolName << "\n";
+        continue;
+      }
+
+      // Only fall back to normal flow if the symbol is decoded partially
+      // (SoftFail) or when the target ignores the symbol for special treatment.
+      // Right now:
+      // - WebAssembly decodes preludes (return SoftFail / Ignore)
+      // - AMDGPU needs it to decode kernel descriptors
+      //               (return Success / Fail / Ignore)
+
       Start += Size;
 
       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