[llvm] [AMDGPU] Add disassembler diagnostics for invalid kernel descriptors (PR #87400)

Emma Pilkington via llvm-commits llvm-commits at lists.llvm.org
Wed Apr 10 06:04:43 PDT 2024


https://github.com/epilk updated https://github.com/llvm/llvm-project/pull/87400

>From 9d47bc98ee745689093b95ac050e8d10b0c9bdbe Mon Sep 17 00:00:00 2001
From: Emma Pilkington <emma.pilkington95 at gmail.com>
Date: Tue, 2 Apr 2024 09:00:07 -0400
Subject: [PATCH 1/5] [llvm-objdump] Start reporting errors from
 onSymbolStart()

Previously these were being ignored, but no disassembler actually used
them, so this is really NFC. This does repurpose the stated use of the
CStream parameter, which was for any comment. The AMDGPU and WebAssembly
disassemblers both just emit comments to outs(), so having a dedicated
parameter for this seems unnecessary.
---
 .../llvm/MC/MCDisassembler/MCDisassembler.h   |  5 ++--
 llvm/tools/llvm-objdump/llvm-objdump.cpp      | 25 ++++++++++++++++---
 2 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h b/llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h
index 7dd8b0b7d3778d..122c1e8f48b191 100644
--- a/llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h
+++ b/llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h
@@ -147,7 +147,8 @@ class MCDisassembler {
   /// \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.
+  /// \param ErrS     - A stream to print newline seperated error comments that
+  ///                   will be emitted if Fail is returned.
   /// \return         - MCDisassembler::Success if bytes are decoded
   ///                   successfully. Size must hold the number of bytes that
   ///                   were decoded.
@@ -160,7 +161,7 @@ class MCDisassembler {
   ///                   case.
   virtual std::optional<DecodeStatus>
   onSymbolStart(SymbolInfoTy &Symbol, uint64_t &Size, ArrayRef<uint8_t> Bytes,
-                uint64_t Address, raw_ostream &CStream) const;
+                uint64_t Address, raw_ostream &ErrS) const;
   // TODO:
   // Implement similar hooks that can be used at other points during
   // disassembly. Something along the following lines:
diff --git a/llvm/tools/llvm-objdump/llvm-objdump.cpp b/llvm/tools/llvm-objdump/llvm-objdump.cpp
index 9b65ea5a99e4d8..82269a8e51a9c9 100644
--- a/llvm/tools/llvm-objdump/llvm-objdump.cpp
+++ b/llvm/tools/llvm-objdump/llvm-objdump.cpp
@@ -2051,9 +2051,12 @@ disassembleObject(ObjectFile &Obj, const ObjectFile &DbgObj,
       for (size_t SHI = 0; SHI < SymbolsHere.size(); ++SHI) {
         SymbolInfoTy Symbol = SymbolsHere[SHI];
 
+        SmallString<40> ErrorComments;
+        raw_svector_ostream ErrorCommentsStream(ErrorComments);
+
         auto Status = DT->DisAsm->onSymbolStart(
             Symbol, Size, Bytes.slice(Start, End - Start), SectionAddr + Start,
-            CommentStream);
+            ErrorCommentsStream);
 
         if (!Status) {
           // If onSymbolStart returns std::nullopt, that means it didn't trigger
@@ -2078,9 +2081,23 @@ disassembleObject(ObjectFile &Obj, const ObjectFile &DbgObj,
           // distance to the next symbol, and sometimes it will be just a
           // prologue and we should start disassembling instructions from where
           // it left off.
-          outs() << DT->Context->getAsmInfo()->getCommentString()
-                 << " error in decoding " << SymNamesHere[SHI]
-                 << " : decoding failed region as bytes.\n";
+
+          // Print any error comments onSymbolStart emitted, or a generic error.
+          StringRef CommentStr = DT->Context->getAsmInfo()->getCommentString();
+          if (!ErrorComments.empty()) {
+            StringRef Comments = ErrorComments;
+            do {
+              StringRef Line;
+              std::tie(Line, Comments) = Comments.split('\n');
+              outs() << CommentStr << " error decoding " << SymNamesHere[SHI]
+                     << ": " << Line << '\n';
+            } while (!Comments.empty());
+            outs() << CommentStr << " decoding failed region as bytes.\n";
+          } else {
+            outs() << CommentStr << " error in decoding " << SymNamesHere[SHI]
+                   << " : decoding failed region as bytes.\n";
+          }
+
           for (uint64_t I = 0; I < Size; ++I) {
             outs() << "\t.byte\t " << format_hex(Bytes[I], 1, /*Upper=*/true)
                    << "\n";

>From 352b5e36d94f6f0cbd631b78ab3c05ea04247141 Mon Sep 17 00:00:00 2001
From: Emma Pilkington <emma.pilkington95 at gmail.com>
Date: Tue, 2 Apr 2024 09:00:15 -0400
Subject: [PATCH 2/5] [AMDGPU] Add disassembler diagnostics for invalid kernel
 descriptors

These mostly are checking for various reserved bits being set. The
diagnostics for gpu-dependent reserved bits have a bit more context
since they seem like the most likely ones to be observed in practice.
---
 llvm/docs/AMDGPUUsage.rst                     |   6 +-
 .../Disassembler/AMDGPUDisassembler.cpp       | 180 +++++++++++++-----
 .../AMDGPU/kernel-descriptor-errors.test      |  73 +++++++
 .../AMDGPU/kernel-descriptor-rsrc-errors.test |  92 +++++++++
 4 files changed, 299 insertions(+), 52 deletions(-)
 create mode 100644 llvm/test/MC/Disassembler/AMDGPU/kernel-descriptor-errors.test
 create mode 100644 llvm/test/MC/Disassembler/AMDGPU/kernel-descriptor-rsrc-errors.test

diff --git a/llvm/docs/AMDGPUUsage.rst b/llvm/docs/AMDGPUUsage.rst
index 22c1d1f186ea54..0055b72160bfef 100644
--- a/llvm/docs/AMDGPUUsage.rst
+++ b/llvm/docs/AMDGPUUsage.rst
@@ -4652,7 +4652,7 @@ The fields used by CP for code objects before V3 also match those specified in
                                                      entry point instruction
                                                      which must be 256 byte
                                                      aligned.
-     351:272 20                                      Reserved, must be 0.
+     351:192 20                                      Reserved, must be 0.
              bytes
      383:352 4 bytes COMPUTE_PGM_RSRC3               GFX6-GFX9
                                                        Reserved, must be 0.
@@ -5248,14 +5248,14 @@ The fields used by CP for code objects before V3 also match those specified in
      5:0     6 bits  ACCUM_OFFSET                    Offset of a first AccVGPR in the unified register file. Granularity 4.
                                                      Value 0-63. 0 - accum-offset = 4, 1 - accum-offset = 8, ...,
                                                      63 - accum-offset = 256.
-     6:15    10                                      Reserved, must be 0.
+     15:6    10                                      Reserved, must be 0.
              bits
      16      1 bit   TG_SPLIT                        - If 0 the waves of a work-group are
                                                        launched in the same CU.
                                                      - If 1 the waves of a work-group can be
                                                        launched in different CUs. The waves
                                                        cannot use S_BARRIER or LDS.
-     17:31   15                                      Reserved, must be 0.
+     31:17   15                                      Reserved, must be 0.
              bits
      32      **Total size 4 bytes.**
      ======= ===================================================================================================================
diff --git a/llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp b/llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp
index 8c42304ce0bee5..95278783952d92 100644
--- a/llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp
+++ b/llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp
@@ -33,6 +33,7 @@
 #include "llvm/MC/MCSubtargetInfo.h"
 #include "llvm/MC/TargetRegistry.h"
 #include "llvm/Support/AMDHSAKernelDescriptor.h"
+#include "llvm/Support/SaveAndRestore.h"
 
 using namespace llvm;
 
@@ -1780,6 +1781,29 @@ bool AMDGPUDisassembler::hasKernargPreload() const {
 //===----------------------------------------------------------------------===//
 // AMDGPU specific symbol handling
 //===----------------------------------------------------------------------===//
+
+/// Print a string describing the reserved bit range specified by Mask with
+/// offset BaseBytes for use in error comments. Mask is a single continuous
+/// range of 1s surrounded by zeros. The format here is meant to align with the
+/// tables that describe these bits in llvm.org/docs/AMDGPUUsage.html.
+static SmallString<20> getBitRangeFromMask(uint32_t Mask, unsigned BaseBytes) {
+  SmallString<20> Result;
+  raw_svector_ostream S(Result);
+
+  int TrailingZeros = llvm::countr_zero(Mask);
+  int PopCount = llvm::popcount(Mask);
+
+  if (PopCount == 1) {
+    S << "bit (" << (TrailingZeros + BaseBytes * CHAR_BIT) << ")";
+  } else {
+    S << "bits in range ("
+      << (TrailingZeros + PopCount - 1 + BaseBytes * CHAR_BIT) << ":"
+      << (TrailingZeros + BaseBytes * CHAR_BIT) << ")";
+  }
+
+  return Result;
+}
+
 #define GET_FIELD(MASK) (AMDHSA_BITS_GET(FourByteBuffer, MASK))
 #define PRINT_DIRECTIVE(DIRECTIVE, MASK)                                       \
   do {                                                                         \
@@ -1791,6 +1815,23 @@ bool AMDGPUDisassembler::hasKernargPreload() const {
              << GET_FIELD(MASK) << '\n';                                       \
   } while (0)
 
+#define CHECK_RESERVED_BITS_IMPL(MASK, DESC, MSG)                              \
+  do {                                                                         \
+    if (FourByteBuffer & (MASK)) {                                             \
+      *CommentStream << "kernel descriptor " DESC " reserved "                 \
+                     << getBitRangeFromMask((MASK), 0) << " set" MSG "\n";     \
+      return MCDisassembler::Fail;                                             \
+    }                                                                          \
+  } while (0)
+
+#define CHECK_RESERVED_BITS(MASK) CHECK_RESERVED_BITS_IMPL(MASK, #MASK, "")
+#define CHECK_RESERVED_BITS_MSG(MASK, MSG)                                     \
+  CHECK_RESERVED_BITS_IMPL(MASK, #MASK, ", " MSG)
+#define CHECK_RESERVED_BITS_DESC(MASK, DESC)                                   \
+  CHECK_RESERVED_BITS_IMPL(MASK, DESC, "")
+#define CHECK_RESERVED_BITS_DESC_MSG(MASK, DESC, MSG)                          \
+  CHECK_RESERVED_BITS_IMPL(MASK, DESC, ", " MSG)
+
 // NOLINTNEXTLINE(readability-identifier-naming)
 MCDisassembler::DecodeStatus AMDGPUDisassembler::decodeCOMPUTE_PGM_RSRC1(
     uint32_t FourByteBuffer, raw_string_ostream &KdStream) const {
@@ -1833,8 +1874,9 @@ MCDisassembler::DecodeStatus AMDGPUDisassembler::decodeCOMPUTE_PGM_RSRC1(
   uint32_t GranulatedWavefrontSGPRCount =
       GET_FIELD(COMPUTE_PGM_RSRC1_GRANULATED_WAVEFRONT_SGPR_COUNT);
 
-  if (isGFX10Plus() && GranulatedWavefrontSGPRCount)
-    return MCDisassembler::Fail;
+  if (isGFX10Plus())
+    CHECK_RESERVED_BITS_MSG(COMPUTE_PGM_RSRC1_GRANULATED_WAVEFRONT_SGPR_COUNT,
+                            "must be zero on gfx10+");
 
   uint32_t NextFreeSGPR = (GranulatedWavefrontSGPRCount + 1) *
                           AMDGPU::IsaInfo::getSGPREncodingGranule(&STI);
@@ -1845,8 +1887,7 @@ MCDisassembler::DecodeStatus AMDGPUDisassembler::decodeCOMPUTE_PGM_RSRC1(
   KdStream << Indent << ".amdhsa_reserve_xnack_mask " << 0 << '\n';
   KdStream << Indent << ".amdhsa_next_free_sgpr " << NextFreeSGPR << "\n";
 
-  if (FourByteBuffer & COMPUTE_PGM_RSRC1_PRIORITY)
-    return MCDisassembler::Fail;
+  CHECK_RESERVED_BITS(COMPUTE_PGM_RSRC1_PRIORITY);
 
   PRINT_DIRECTIVE(".amdhsa_float_round_mode_32",
                   COMPUTE_PGM_RSRC1_FLOAT_ROUND_MODE_32);
@@ -1857,37 +1898,33 @@ MCDisassembler::DecodeStatus AMDGPUDisassembler::decodeCOMPUTE_PGM_RSRC1(
   PRINT_DIRECTIVE(".amdhsa_float_denorm_mode_16_64",
                   COMPUTE_PGM_RSRC1_FLOAT_DENORM_MODE_16_64);
 
-  if (FourByteBuffer & COMPUTE_PGM_RSRC1_PRIV)
-    return MCDisassembler::Fail;
+  CHECK_RESERVED_BITS(COMPUTE_PGM_RSRC1_PRIV);
 
   if (!isGFX12Plus())
     PRINT_DIRECTIVE(".amdhsa_dx10_clamp",
                     COMPUTE_PGM_RSRC1_GFX6_GFX11_ENABLE_DX10_CLAMP);
 
-  if (FourByteBuffer & COMPUTE_PGM_RSRC1_DEBUG_MODE)
-    return MCDisassembler::Fail;
+  CHECK_RESERVED_BITS(COMPUTE_PGM_RSRC1_DEBUG_MODE);
 
   if (!isGFX12Plus())
     PRINT_DIRECTIVE(".amdhsa_ieee_mode",
                     COMPUTE_PGM_RSRC1_GFX6_GFX11_ENABLE_IEEE_MODE);
 
-  if (FourByteBuffer & COMPUTE_PGM_RSRC1_BULKY)
-    return MCDisassembler::Fail;
-
-  if (FourByteBuffer & COMPUTE_PGM_RSRC1_CDBG_USER)
-    return MCDisassembler::Fail;
+  CHECK_RESERVED_BITS(COMPUTE_PGM_RSRC1_BULKY);
+  CHECK_RESERVED_BITS(COMPUTE_PGM_RSRC1_CDBG_USER);
 
   if (isGFX9Plus())
     PRINT_DIRECTIVE(".amdhsa_fp16_overflow", COMPUTE_PGM_RSRC1_GFX9_PLUS_FP16_OVFL);
 
   if (!isGFX9Plus())
-    if (FourByteBuffer & COMPUTE_PGM_RSRC1_GFX6_GFX8_RESERVED0)
-      return MCDisassembler::Fail;
-  if (FourByteBuffer & COMPUTE_PGM_RSRC1_RESERVED1)
-    return MCDisassembler::Fail;
+    CHECK_RESERVED_BITS_DESC_MSG(COMPUTE_PGM_RSRC1_GFX6_GFX8_RESERVED0,
+                                 "COMPUTE_PGM_RSRC1", "must be zero pre-gfx9");
+
+  CHECK_RESERVED_BITS_DESC(COMPUTE_PGM_RSRC1_RESERVED1, "COMPUTE_PGM_RSRC1");
+
   if (!isGFX10Plus())
-    if (FourByteBuffer & COMPUTE_PGM_RSRC1_GFX6_GFX9_RESERVED2)
-      return MCDisassembler::Fail;
+    CHECK_RESERVED_BITS_DESC_MSG(COMPUTE_PGM_RSRC1_GFX6_GFX9_RESERVED2,
+                                 "COMPUTE_PGM_RSRC1", "must be zero pre-gfx10");
 
   if (isGFX10Plus()) {
     PRINT_DIRECTIVE(".amdhsa_workgroup_processor_mode",
@@ -1925,14 +1962,9 @@ MCDisassembler::DecodeStatus AMDGPUDisassembler::decodeCOMPUTE_PGM_RSRC2(
   PRINT_DIRECTIVE(".amdhsa_system_vgpr_workitem_id",
                   COMPUTE_PGM_RSRC2_ENABLE_VGPR_WORKITEM_ID);
 
-  if (FourByteBuffer & COMPUTE_PGM_RSRC2_ENABLE_EXCEPTION_ADDRESS_WATCH)
-    return MCDisassembler::Fail;
-
-  if (FourByteBuffer & COMPUTE_PGM_RSRC2_ENABLE_EXCEPTION_MEMORY)
-    return MCDisassembler::Fail;
-
-  if (FourByteBuffer & COMPUTE_PGM_RSRC2_GRANULATED_LDS_SIZE)
-    return MCDisassembler::Fail;
+  CHECK_RESERVED_BITS(COMPUTE_PGM_RSRC2_ENABLE_EXCEPTION_ADDRESS_WATCH);
+  CHECK_RESERVED_BITS(COMPUTE_PGM_RSRC2_ENABLE_EXCEPTION_MEMORY);
+  CHECK_RESERVED_BITS(COMPUTE_PGM_RSRC2_GRANULATED_LDS_SIZE);
 
   PRINT_DIRECTIVE(
       ".amdhsa_exception_fp_ieee_invalid_op",
@@ -1951,8 +1983,7 @@ MCDisassembler::DecodeStatus AMDGPUDisassembler::decodeCOMPUTE_PGM_RSRC2(
   PRINT_DIRECTIVE(".amdhsa_exception_int_div_zero",
                   COMPUTE_PGM_RSRC2_ENABLE_EXCEPTION_INT_DIVIDE_BY_ZERO);
 
-  if (FourByteBuffer & COMPUTE_PGM_RSRC2_RESERVED0)
-    return MCDisassembler::Fail;
+  CHECK_RESERVED_BITS_DESC(COMPUTE_PGM_RSRC2_RESERVED0, "COMPUTE_PGM_RSRC2");
 
   return MCDisassembler::Success;
 }
@@ -1966,11 +1997,13 @@ MCDisassembler::DecodeStatus AMDGPUDisassembler::decodeCOMPUTE_PGM_RSRC3(
     KdStream << Indent << ".amdhsa_accum_offset "
              << (GET_FIELD(COMPUTE_PGM_RSRC3_GFX90A_ACCUM_OFFSET) + 1) * 4
              << '\n';
-    if (FourByteBuffer & COMPUTE_PGM_RSRC3_GFX90A_RESERVED0)
-      return MCDisassembler::Fail;
+
     PRINT_DIRECTIVE(".amdhsa_tg_split", COMPUTE_PGM_RSRC3_GFX90A_TG_SPLIT);
-    if (FourByteBuffer & COMPUTE_PGM_RSRC3_GFX90A_RESERVED1)
-      return MCDisassembler::Fail;
+
+    CHECK_RESERVED_BITS_DESC_MSG(COMPUTE_PGM_RSRC3_GFX90A_RESERVED0,
+                                 "COMPUTE_PGM_RSRC3", "must be zero on gfx90a");
+    CHECK_RESERVED_BITS_DESC_MSG(COMPUTE_PGM_RSRC3_GFX90A_RESERVED1,
+                                 "COMPUTE_PGM_RSRC3", "must be zero on gfx90a");
   } else if (isGFX10Plus()) {
     // Bits [0-3].
     if (!isGFX12Plus()) {
@@ -1983,8 +2016,9 @@ MCDisassembler::DecodeStatus AMDGPUDisassembler::decodeCOMPUTE_PGM_RSRC3(
             COMPUTE_PGM_RSRC3_GFX10_GFX11_SHARED_VGPR_COUNT);
       }
     } else {
-      if (FourByteBuffer & COMPUTE_PGM_RSRC3_GFX12_PLUS_RESERVED0)
-        return MCDisassembler::Fail;
+      CHECK_RESERVED_BITS_DESC_MSG(COMPUTE_PGM_RSRC3_GFX12_PLUS_RESERVED0,
+                                   "COMPUTE_PGM_RSRC3",
+                                   "must be zero on gfx12+");
     }
 
     // Bits [4-11].
@@ -1999,36 +2033,41 @@ MCDisassembler::DecodeStatus AMDGPUDisassembler::decodeCOMPUTE_PGM_RSRC3(
       PRINT_PSEUDO_DIRECTIVE_COMMENT(
           "INST_PREF_SIZE", COMPUTE_PGM_RSRC3_GFX12_PLUS_INST_PREF_SIZE);
     } else {
-      if (FourByteBuffer & COMPUTE_PGM_RSRC3_GFX10_RESERVED1)
-        return MCDisassembler::Fail;
+      CHECK_RESERVED_BITS_DESC_MSG(COMPUTE_PGM_RSRC3_GFX10_RESERVED1,
+                                   "COMPUTE_PGM_RSRC3",
+                                   "must be zero on gfx10");
     }
 
     // Bits [12].
-    if (FourByteBuffer & COMPUTE_PGM_RSRC3_GFX10_PLUS_RESERVED2)
-      return MCDisassembler::Fail;
+    CHECK_RESERVED_BITS_DESC_MSG(COMPUTE_PGM_RSRC3_GFX10_PLUS_RESERVED2,
+                                 "COMPUTE_PGM_RSRC3", "must be zero on gfx10+");
 
     // Bits [13].
     if (isGFX12Plus()) {
       PRINT_PSEUDO_DIRECTIVE_COMMENT("GLG_EN",
                                      COMPUTE_PGM_RSRC3_GFX12_PLUS_GLG_EN);
     } else {
-      if (FourByteBuffer & COMPUTE_PGM_RSRC3_GFX10_GFX11_RESERVED3)
-        return MCDisassembler::Fail;
+      CHECK_RESERVED_BITS_DESC_MSG(COMPUTE_PGM_RSRC3_GFX10_GFX11_RESERVED3,
+                                   "COMPUTE_PGM_RSRC3",
+                                   "must be zero on gfx10 or gfx11");
     }
 
     // Bits [14-30].
-    if (FourByteBuffer & COMPUTE_PGM_RSRC3_GFX10_PLUS_RESERVED4)
-      return MCDisassembler::Fail;
+    CHECK_RESERVED_BITS_DESC_MSG(COMPUTE_PGM_RSRC3_GFX10_PLUS_RESERVED4,
+                                 "COMPUTE_PGM_RSRC3", "must be zero on gfx10+");
 
     // Bits [31].
     if (isGFX11Plus()) {
       PRINT_PSEUDO_DIRECTIVE_COMMENT("IMAGE_OP",
                                      COMPUTE_PGM_RSRC3_GFX11_PLUS_IMAGE_OP);
     } else {
-      if (FourByteBuffer & COMPUTE_PGM_RSRC3_GFX10_RESERVED5)
-        return MCDisassembler::Fail;
+      CHECK_RESERVED_BITS_DESC_MSG(COMPUTE_PGM_RSRC3_GFX10_RESERVED5,
+                                   "COMPUTE_PGM_RSRC3",
+                                   "must be zero on gfx10");
     }
   } else if (FourByteBuffer) {
+    *CommentStream
+        << "kernel descriptor COMPUTE_PGM_RSRC3 must be all zero before gfx9\n";
     return MCDisassembler::Fail;
   }
   return MCDisassembler::Success;
@@ -2036,6 +2075,30 @@ MCDisassembler::DecodeStatus AMDGPUDisassembler::decodeCOMPUTE_PGM_RSRC3(
 #undef PRINT_PSEUDO_DIRECTIVE_COMMENT
 #undef PRINT_DIRECTIVE
 #undef GET_FIELD
+#undef CHECK_RESERVED_BITS_IMPL
+#undef CHECK_RESERVED_BITS
+#undef CHECK_RESERVED_BITS_MSG
+#undef CHECK_RESERVED_BITS_DESC
+#undef CHECK_RESERVED_BITS_DESC_MSG
+
+static void printReservedKDBitsError(raw_ostream *CommentStream, uint32_t Mask,
+                                     unsigned BaseInBytes, StringRef Msg = "") {
+  *CommentStream << "kernel descriptor reserved "
+                 << getBitRangeFromMask(Mask, BaseInBytes) << " set";
+  if (!Msg.empty())
+    *CommentStream << ", " << Msg;
+  *CommentStream << '\n';
+}
+
+static void printReservedKDBytesError(raw_ostream *CommentStream,
+                                      unsigned BaseInBytes,
+                                      unsigned WidthInBytes) {
+  // Print an error comment in the same format as the "Kernel Descriptor"
+  // table here: https://llvm.org/docs/AMDGPUUsage.html#kernel-descriptor .
+  *CommentStream << "kernel descriptor reserved bits set in range "
+                 << ((BaseInBytes + WidthInBytes) * CHAR_BIT - 1) << ":"
+                 << (BaseInBytes * CHAR_BIT) << '\n';
+}
 
 MCDisassembler::DecodeStatus
 AMDGPUDisassembler::decodeKernelDescriptorDirective(
@@ -2080,6 +2143,7 @@ AMDGPUDisassembler::decodeKernelDescriptorDirective(
     ReservedBytes = DE.getBytes(Cursor, 4);
     for (int I = 0; I < 4; ++I) {
       if (ReservedBytes[I] != 0) {
+        printReservedKDBytesError(CommentStream, amdhsa::RESERVED0_OFFSET, 4);
         return MCDisassembler::Fail;
       }
     }
@@ -2097,6 +2161,7 @@ AMDGPUDisassembler::decodeKernelDescriptorDirective(
     ReservedBytes = DE.getBytes(Cursor, 20);
     for (int I = 0; I < 20; ++I) {
       if (ReservedBytes[I] != 0) {
+        printReservedKDBytesError(CommentStream, amdhsa::RESERVED1_OFFSET, 20);
         return MCDisassembler::Fail;
       }
     }
@@ -2135,12 +2200,18 @@ AMDGPUDisassembler::decodeKernelDescriptorDirective(
     PRINT_DIRECTIVE(".amdhsa_user_sgpr_private_segment_size",
                     KERNEL_CODE_PROPERTY_ENABLE_SGPR_PRIVATE_SEGMENT_SIZE);
 
-    if (TwoByteBuffer & KERNEL_CODE_PROPERTY_RESERVED0)
+    if (TwoByteBuffer & KERNEL_CODE_PROPERTY_RESERVED0) {
+      printReservedKDBitsError(CommentStream, KERNEL_CODE_PROPERTY_RESERVED0,
+                               amdhsa::KERNEL_CODE_PROPERTIES_OFFSET);
       return MCDisassembler::Fail;
+    }
 
     // Reserved for GFX9
     if (isGFX9() &&
         (TwoByteBuffer & KERNEL_CODE_PROPERTY_ENABLE_WAVEFRONT_SIZE32)) {
+      printReservedKDBitsError(
+          CommentStream, KERNEL_CODE_PROPERTY_ENABLE_WAVEFRONT_SIZE32,
+          amdhsa::KERNEL_CODE_PROPERTIES_OFFSET, "must be zero on gfx9");
       return MCDisassembler::Fail;
     } else if (isGFX10Plus()) {
       PRINT_DIRECTIVE(".amdhsa_wavefront_size32",
@@ -2151,8 +2222,11 @@ AMDGPUDisassembler::decodeKernelDescriptorDirective(
       PRINT_DIRECTIVE(".amdhsa_uses_dynamic_stack",
                       KERNEL_CODE_PROPERTY_USES_DYNAMIC_STACK);
 
-    if (TwoByteBuffer & KERNEL_CODE_PROPERTY_RESERVED1)
+    if (TwoByteBuffer & KERNEL_CODE_PROPERTY_RESERVED1) {
+      printReservedKDBitsError(CommentStream, KERNEL_CODE_PROPERTY_RESERVED1,
+                               amdhsa::KERNEL_CODE_PROPERTIES_OFFSET);
       return MCDisassembler::Fail;
+    }
 
     return MCDisassembler::Success;
 
@@ -2174,8 +2248,10 @@ AMDGPUDisassembler::decodeKernelDescriptorDirective(
     // 4 bytes from here are reserved, must be 0.
     ReservedBytes = DE.getBytes(Cursor, 4);
     for (int I = 0; I < 4; ++I) {
-      if (ReservedBytes[I] != 0)
+      if (ReservedBytes[I] != 0) {
+        printReservedKDBytesError(CommentStream, amdhsa::RESERVED3_OFFSET, 4);
         return MCDisassembler::Fail;
+      }
     }
     return MCDisassembler::Success;
 
@@ -2189,8 +2265,10 @@ AMDGPUDisassembler::decodeKernelDescriptorDirective(
 MCDisassembler::DecodeStatus AMDGPUDisassembler::decodeKernelDescriptor(
     StringRef KdName, ArrayRef<uint8_t> Bytes, uint64_t KdAddress) const {
   // CP microcode requires the kernel descriptor to be 64 aligned.
-  if (Bytes.size() != 64 || KdAddress % 64 != 0)
+  if (Bytes.size() != 64 || KdAddress % 64 != 0) {
+    *CommentStream << "kernel descriptor must be 64-byte aligned\n";
     return MCDisassembler::Fail;
+  }
 
   // FIXME: We can't actually decode "in order" as is done below, as e.g. GFX10
   // requires us to know the setting of .amdhsa_wavefront_size32 in order to
@@ -2235,9 +2313,12 @@ AMDGPUDisassembler::onSymbolStart(SymbolInfoTy &Symbol, uint64_t &Size,
   // Fix the spurious symbol issue for AMDGPU kernels. Exists for both Code
   // Object V2 and V3 when symbols are marked protected.
 
+  SaveAndRestore CommentStreamRAII(CommentStream, &CStream);
+
   // amd_kernel_code_t for Code Object V2.
   if (Symbol.Type == ELF::STT_AMDGPU_HSA_KERNEL) {
     Size = 256;
+    *CommentStream << "code object v2 is not supported";
     return MCDisassembler::Fail;
   }
 
@@ -2247,6 +2328,7 @@ AMDGPUDisassembler::onSymbolStart(SymbolInfoTy &Symbol, uint64_t &Size,
     Size = 64; // Size = 64 regardless of success or failure.
     return decodeKernelDescriptor(Name.drop_back(3), Bytes, Address);
   }
+
   return std::nullopt;
 }
 
diff --git a/llvm/test/MC/Disassembler/AMDGPU/kernel-descriptor-errors.test b/llvm/test/MC/Disassembler/AMDGPU/kernel-descriptor-errors.test
new file mode 100644
index 00000000000000..bdfd70f8a022d1
--- /dev/null
+++ b/llvm/test/MC/Disassembler/AMDGPU/kernel-descriptor-errors.test
@@ -0,0 +1,73 @@
+# RUN: yaml2obj %s -DGPU=GFX1100 -DKD=0000000000000000000000001000000000000000000000001000000000000000000000000000000000000000000000000300AC60800000000004000000000000 \
+# RUN:   | llvm-objdump --disassemble-symbols=test.kd - | FileCheck %s --check-prefix=RES_4
+# RES_4: ; error decoding test.kd: kernel descriptor reserved bits set in range 127:96
+# RES_4-NEXT: ; decoding failed region as bytes
+
+# RUN: yaml2obj %s -DGPU=GFX1100 -DKD=0000000000000000000000000000000000000000000000001000000000000000000000000000000000000000000000000300AC60800000000004000000000000 \
+# RUN:   | llvm-objdump --disassemble-symbols=test.kd - | FileCheck %s --check-prefix=RES_20
+# RES_20: ; error decoding test.kd: kernel descriptor reserved bits set in range 351:192
+# RES_20-NEXT: ; decoding failed region as bytes
+
+# RUN: yaml2obj %s -DGPU=GFX1100 -DKD=0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000300AC60800000000004000000000001 \
+# RUN:   | llvm-objdump --disassemble-symbols=test.kd - | FileCheck %s --check-prefix=RES_4_2
+# RES_4_2: ; error decoding test.kd: kernel descriptor reserved bits set in range 511:480
+# RES_4_2-NEXT: ; decoding failed region as bytes
+
+# RUN: yaml2obj %s -DGPU=GFX90A -DKD=00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000006000000000000 \
+# RUN:   | llvm-objdump --disassemble-symbols=test.kd - | FileCheck %s --check-prefix=RES_457
+# RES_457: ; error decoding test.kd: kernel descriptor reserved bits in range (457:455) set
+# RES_457-NEXT: ; decoding failed region as bytes
+
+# RUN: yaml2obj %s -DGPU=GFX90A -DKD=0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000c000000000000 \
+# RUN:   | llvm-objdump --disassemble-symbols=test.kd - | FileCheck %s --check-prefix=WF32
+# WF32: ; error decoding test.kd: kernel descriptor reserved bit (458) set, must be zero on gfx9
+# WF32-NEXT: ; decoding failed region as bytes
+
+# RUN: yaml2obj %s -DGPU=GFX1100 -DKD=0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000300AC60800000000024000000000000 \
+# RUN:   | llvm-objdump --disassemble-symbols=test.kd - | FileCheck %s --check-prefix=RES_463
+# RES_463: ; error decoding test.kd: kernel descriptor reserved bits in range (463:460) set
+# RES_463-NEXT: ; decoding failed region as bytes
+
+--- !ELF
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  OSABI:           ELFOSABI_AMDGPU_HSA
+  ABIVersion:      0x3
+  Type:            ET_REL
+  Machine:         EM_AMDGPU
+  Flags:           [ EF_AMDGPU_MACH_AMDGCN_[[GPU]], EF_AMDGPU_FEATURE_XNACK_UNSUPPORTED_V4, EF_AMDGPU_FEATURE_SRAMECC_UNSUPPORTED_V4 ]
+  SectionHeaderStringTable: .strtab
+Sections:
+  - Name:            .text
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
+    AddressAlign:    0x4
+    Content:         [[KD]]
+  - Name:            .rela.text
+    Type:            SHT_RELA
+    Flags:           [ SHF_INFO_LINK ]
+    Link:            .symtab
+    AddressAlign:    0x8
+    Info:            .text
+    Relocations:
+      - Offset:          0x10
+        Symbol:          test
+        Type:            R_AMDGPU_REL64
+        Addend:          16
+  - Type:            SectionHeaderTable
+    Sections:
+      - Name:            .strtab
+      - Name:            .text
+      - Name:            .rela.text
+      - Name:            .symtab
+Symbols:
+  - Name:            test.kd
+    Type:            STT_OBJECT
+    Section:         .text
+    Binding:         STB_GLOBAL
+    Size:            0x40
+  - Name:            test
+    Binding:         STB_GLOBAL
+    Other:           [ STV_PROTECTED ]
+...
diff --git a/llvm/test/MC/Disassembler/AMDGPU/kernel-descriptor-rsrc-errors.test b/llvm/test/MC/Disassembler/AMDGPU/kernel-descriptor-rsrc-errors.test
new file mode 100644
index 00000000000000..1a54bac5fa40f0
--- /dev/null
+++ b/llvm/test/MC/Disassembler/AMDGPU/kernel-descriptor-rsrc-errors.test
@@ -0,0 +1,92 @@
+# RUN: yaml2obj %s -DGPU=GFX1200 -DSRC1=0300AC60 -DSRC2=80000000 -DSRC3=00000000 \
+# RUN:   | llvm-objdump --disassemble-symbols=test.kd - | FileCheck %s --check-prefix=VALID
+# VALID: .amdhsa_kernel test
+
+# RUN: yaml2obj %s -DGPU=GFX1200 -DSRC1=4300AC60 -DSRC2=80000000 -DSRC3=00000000 \
+# RUN:   | llvm-objdump --disassemble-symbols=test.kd - | FileCheck %s --check-prefix=SRC1_9_6
+# SRC1_9_6: ; error decoding test.kd: kernel descriptor COMPUTE_PGM_RSRC1_GRANULATED_WAVEFRONT_SGPR_COUNT reserved bits in range (9:6) set, must be zero on gfx10+
+# SRC1_9_6-NEXT: ; decoding failed region as bytes
+
+# RUN: yaml2obj %s -DGPU=GFX1200 -DSRC1=0308AC60 -DSRC2=80000000 -DSRC3=00000000 \
+# RUN:   | llvm-objdump --disassemble-symbols=test.kd - | FileCheck %s --check-prefix=SRC1_PRIORITY
+# SRC1_PRIORITY: ; error decoding test.kd: kernel descriptor COMPUTE_PGM_RSRC1_PRIORITY reserved bits in range (11:10) set
+# SRC1_PRIORITY-NEXT: ; decoding failed region as bytes
+
+# RUN: yaml2obj %s -DGPU=GFX1200 -DSRC1=0300BC60 -DSRC2=80000000 -DSRC3=00000000 \
+# RUN:   | llvm-objdump --disassemble-symbols=test.kd - | FileCheck %s --check-prefix=SRC1_PRIV
+# SRC1_PRIV: ; error decoding test.kd: kernel descriptor COMPUTE_PGM_RSRC1_PRIV reserved bit (20) set
+# SRC1_PRIV-NEXT: ; decoding failed region as bytes
+
+# RUN: yaml2obj %s -DGPU=GFX801 -DSRC1=0300AC64 -DSRC2=80000000 -DSRC3=00000000 \
+# RUN:   | llvm-objdump --disassemble-symbols=test.kd - | FileCheck %s --check-prefix=SRC1_6_8
+# SRC1_6_8: ; error decoding test.kd: kernel descriptor COMPUTE_PGM_RSRC1 reserved bit (26) set, must be zero pre-gfx9
+# SRC1_6_8-NEXT: ; decoding failed region as bytes
+
+# RUN: yaml2obj %s -DGPU=GFX1200 -DSRC1=0300AC60 -DSRC2=80200000 -DSRC3=00000000 \
+# RUN:   | llvm-objdump --disassemble-symbols=test.kd - | FileCheck %s --check-prefix=RSRC2
+# RSRC2: ; error decoding test.kd: kernel descriptor COMPUTE_PGM_RSRC2_ENABLE_EXCEPTION_ADDRESS_WATCH reserved bit (13) set
+# RSRC2-NEXT: ; decoding failed region as bytes
+
+# RUN: yaml2obj %s -DGPU=GFX90A -DSRC1=0300AC60 -DSRC2=80000000 -DSRC3=40000000 \
+# RUN:   | llvm-objdump --disassemble-symbols=test.kd - | FileCheck %s --check-prefix=RSRC3_90A
+# RSRC3_90A: ; error decoding test.kd: kernel descriptor COMPUTE_PGM_RSRC3 reserved bits in range (15:6) set, must be zero on gfx90a
+# RSRC3_90A-NEXT: ; decoding failed region as bytes
+
+# RUN: yaml2obj %s -DGPU=GFX1200 -DSRC1=0300AC60 -DSRC2=80000000 -DSRC3=01000000 \
+# RUN:   | llvm-objdump --disassemble-symbols=test.kd - | FileCheck %s --check-prefix=RSRC3_RES
+# RSRC3_RES: ; error decoding test.kd: kernel descriptor COMPUTE_PGM_RSRC3 reserved bits in range (3:0) set, must be zero on gfx12+
+# RSRC3_RES-NEXT: ; decoding failed region as bytes
+
+# RUN: yaml2obj %s -DGPU=GFX1100 -DSRC1=0300AC60 -DSRC2=80000000 -DSRC3=00000100 \
+# RUN:   | llvm-objdump --disassemble-symbols=test.kd - | FileCheck %s --check-prefix=RSRC3_10
+# RSRC3_10: ; error decoding test.kd: kernel descriptor COMPUTE_PGM_RSRC3 reserved bits in range (30:14) set, must be zero on gfx10+
+# RSRC3_10-NEXT: ; decoding failed region as bytes
+
+# RUN: yaml2obj %s -DGPU=GFX801 -DSRC1=0300AC60 -DSRC2=80000000 -DSRC3=00000001 \
+# RUN:   | llvm-objdump --disassemble-symbols=test.kd - | FileCheck %s --check-prefix=RSRC3_PRE_9
+# RSRC3_PRE_9: ; error decoding test.kd: kernel descriptor COMPUTE_PGM_RSRC3 must be all zero before gfx9
+# RSRC3_PRE_9-NEXT: ; decoding failed region as bytes
+
+--- !ELF
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  OSABI:           ELFOSABI_AMDGPU_HSA
+  ABIVersion:      0x3
+  Type:            ET_REL
+  Machine:         EM_AMDGPU
+  Flags:           [ EF_AMDGPU_MACH_AMDGCN_[[GPU]], EF_AMDGPU_FEATURE_XNACK_UNSUPPORTED_V4, EF_AMDGPU_FEATURE_SRAMECC_UNSUPPORTED_V4 ]
+  SectionHeaderStringTable: .strtab
+Sections:
+  - Name:            .text
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
+    AddressAlign:    0x4
+    Content:         0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000[[SRC3]][[SRC1]][[SRC2]]0004000000000000
+  - Name:            .rela.text
+    Type:            SHT_RELA
+    Flags:           [ SHF_INFO_LINK ]
+    Link:            .symtab
+    AddressAlign:    0x8
+    Info:            .text
+    Relocations:
+      - Offset:          0x10
+        Symbol:          test
+        Type:            R_AMDGPU_REL64
+        Addend:          16
+  - Type:            SectionHeaderTable
+    Sections:
+      - Name:            .strtab
+      - Name:            .text
+      - Name:            .rela.text
+      - Name:            .symtab
+Symbols:
+  - Name:            test.kd
+    Type:            STT_OBJECT
+    Section:         .text
+    Binding:         STB_GLOBAL
+    Size:            0x40
+  - Name:            test
+    Binding:         STB_GLOBAL
+    Other:           [ STV_PROTECTED ]
+...

>From a8524c0c3dae6c2cb624629542f48cadf90e01af Mon Sep 17 00:00:00 2001
From: Emma Pilkington <emma.pilkington95 at gmail.com>
Date: Wed, 3 Apr 2024 09:10:50 -0400
Subject: [PATCH 3/5] Fix a typo

---
 llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h b/llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h
index 122c1e8f48b191..21a6e92a2ab7c3 100644
--- a/llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h
+++ b/llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h
@@ -147,7 +147,7 @@ class MCDisassembler {
   /// \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 ErrS     - A stream to print newline seperated error comments that
+  /// \param ErrS     - A stream to print newline separated error comments that
   ///                   will be emitted if Fail is returned.
   /// \return         - MCDisassembler::Success if bytes are decoded
   ///                   successfully. Size must hold the number of bytes that

>From 408d35dd8830fa15d7a528a48943568ed57f8549 Mon Sep 17 00:00:00 2001
From: Emma Pilkington <emma.pilkington95 at gmail.com>
Date: Mon, 8 Apr 2024 09:37:08 -0400
Subject: [PATCH 4/5] Refactor onSymbolStart to report errors using Error,
 change return value to bool.

---
 .../llvm/MC/MCDisassembler/MCDisassembler.h   |  32 ++--
 llvm/lib/MC/MCDisassembler/MCDisassembler.cpp |   9 +-
 .../Disassembler/AMDGPUDisassembler.cpp       | 166 +++++++++---------
 .../AMDGPU/Disassembler/AMDGPUDisassembler.h  |  29 ++-
 .../Disassembler/WebAssemblyDisassembler.cpp  |  26 +--
 .../AMDGPU/kernel-descriptor-errors.test      |   6 +-
 llvm/tools/llvm-objdump/llvm-objdump.cpp      |  83 ++++-----
 7 files changed, 167 insertions(+), 184 deletions(-)

diff --git a/llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h b/llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h
index 21a6e92a2ab7c3..d5cbeeb82e6d4f 100644
--- a/llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h
+++ b/llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h
@@ -12,6 +12,7 @@
 #include "llvm/ADT/StringRef.h"
 #include "llvm/BinaryFormat/XCOFF.h"
 #include "llvm/MC/MCDisassembler/MCSymbolizer.h"
+#include "llvm/Support/Error.h"
 #include <cstdint>
 #include <memory>
 #include <vector>
@@ -139,29 +140,26 @@ class MCDisassembler {
   /// start of a symbol, or the entire symbol.
   /// This is used for example by WebAssembly to decode preludes.
   ///
-  /// Base implementation returns std::nullopt. So all targets by default ignore
-  /// to treat symbols separately.
+  /// Base implementation returns false. So all targets by default decline to
+  /// treat symbols separately.
   ///
   /// \param Symbol   - 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 ErrS     - A stream to print newline separated error comments that
-  ///                   will be emitted if Fail is returned.
-  /// \return         - MCDisassembler::Success if bytes are decoded
-  ///                   successfully. Size must hold the number of bytes that
-  ///                   were decoded.
-  ///                 - MCDisassembler::Fail if the bytes are invalid. Size
-  ///                   must hold the number of bytes that were decoded before
-  ///                   failing. The target must print nothing. This can be
-  ///                   done by buffering the output if needed.
-  ///                 - std::nullopt if the target doesn't want to handle the
-  ///                   symbol separately. Value of Size is ignored in this
-  ///                   case.
-  virtual std::optional<DecodeStatus>
-  onSymbolStart(SymbolInfoTy &Symbol, uint64_t &Size, ArrayRef<uint8_t> Bytes,
-                uint64_t Address, raw_ostream &ErrS) const;
+  /// \param Err      - An out parameter that indicates whether an error was
+  ///                   found.
+  /// \return         - True if this symbol triggered some target specific
+  ///                   disassembly for this symbol. Size must be set with the
+  ///                   number of bytes consumed regardless of whether an error
+  ///                   was found.
+  ///                 - False if the target doesn't want to handle the symbol
+  ///                   separately. The value of Size is ignored in this case,
+  ///                   and Err must not be set.
+  virtual bool onSymbolStart(SymbolInfoTy &Symbol, uint64_t &Size,
+                             ArrayRef<uint8_t> Bytes, uint64_t Address,
+                             Error &Err) const;
   // TODO:
   // Implement similar hooks that can be used at other points during
   // disassembly. Something along the following lines:
diff --git a/llvm/lib/MC/MCDisassembler/MCDisassembler.cpp b/llvm/lib/MC/MCDisassembler/MCDisassembler.cpp
index 80c32ac5608220..e1393e81966ce5 100644
--- a/llvm/lib/MC/MCDisassembler/MCDisassembler.cpp
+++ b/llvm/lib/MC/MCDisassembler/MCDisassembler.cpp
@@ -13,11 +13,10 @@ using namespace llvm;
 
 MCDisassembler::~MCDisassembler() = default;
 
-std::optional<MCDisassembler::DecodeStatus>
-MCDisassembler::onSymbolStart(SymbolInfoTy &Symbol, uint64_t &Size,
-                              ArrayRef<uint8_t> Bytes, uint64_t Address,
-                              raw_ostream &CStream) const {
-  return std::nullopt;
+bool MCDisassembler::onSymbolStart(SymbolInfoTy &Symbol, uint64_t &Size,
+                                   ArrayRef<uint8_t> Bytes, uint64_t Address,
+                                   Error &Err) const {
+  return false;
 }
 
 uint64_t MCDisassembler::suggestBytesToSkip(ArrayRef<uint8_t> Bytes,
diff --git a/llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp b/llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp
index 95278783952d92..68ecf5bc3f9c7c 100644
--- a/llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp
+++ b/llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp
@@ -33,7 +33,6 @@
 #include "llvm/MC/MCSubtargetInfo.h"
 #include "llvm/MC/TargetRegistry.h"
 #include "llvm/Support/AMDHSAKernelDescriptor.h"
-#include "llvm/Support/SaveAndRestore.h"
 
 using namespace llvm;
 
@@ -1786,19 +1785,19 @@ bool AMDGPUDisassembler::hasKernargPreload() const {
 /// offset BaseBytes for use in error comments. Mask is a single continuous
 /// range of 1s surrounded by zeros. The format here is meant to align with the
 /// tables that describe these bits in llvm.org/docs/AMDGPUUsage.html.
-static SmallString<20> getBitRangeFromMask(uint32_t Mask, unsigned BaseBytes) {
-  SmallString<20> Result;
+static SmallString<32> getBitRangeFromMask(uint32_t Mask, unsigned BaseBytes) {
+  SmallString<32> Result;
   raw_svector_ostream S(Result);
 
   int TrailingZeros = llvm::countr_zero(Mask);
   int PopCount = llvm::popcount(Mask);
 
   if (PopCount == 1) {
-    S << "bit (" << (TrailingZeros + BaseBytes * CHAR_BIT) << ")";
+    S << "bit (" << (TrailingZeros + BaseBytes * CHAR_BIT) << ')';
   } else {
     S << "bits in range ("
-      << (TrailingZeros + PopCount - 1 + BaseBytes * CHAR_BIT) << ":"
-      << (TrailingZeros + BaseBytes * CHAR_BIT) << ")";
+      << (TrailingZeros + PopCount - 1 + BaseBytes * CHAR_BIT) << ':'
+      << (TrailingZeros + BaseBytes * CHAR_BIT) << ')';
   }
 
   return Result;
@@ -1818,9 +1817,10 @@ static SmallString<20> getBitRangeFromMask(uint32_t Mask, unsigned BaseBytes) {
 #define CHECK_RESERVED_BITS_IMPL(MASK, DESC, MSG)                              \
   do {                                                                         \
     if (FourByteBuffer & (MASK)) {                                             \
-      *CommentStream << "kernel descriptor " DESC " reserved "                 \
-                     << getBitRangeFromMask((MASK), 0) << " set" MSG "\n";     \
-      return MCDisassembler::Fail;                                             \
+      return createStringError(std::errc::invalid_argument,                    \
+                               "kernel descriptor " DESC                       \
+                               " reserved %s set" MSG,                         \
+                               getBitRangeFromMask((MASK), 0).c_str());        \
     }                                                                          \
   } while (0)
 
@@ -1833,7 +1833,7 @@ static SmallString<20> getBitRangeFromMask(uint32_t Mask, unsigned BaseBytes) {
   CHECK_RESERVED_BITS_IMPL(MASK, DESC, ", " MSG)
 
 // NOLINTNEXTLINE(readability-identifier-naming)
-MCDisassembler::DecodeStatus AMDGPUDisassembler::decodeCOMPUTE_PGM_RSRC1(
+Error AMDGPUDisassembler::decodeCOMPUTE_PGM_RSRC1(
     uint32_t FourByteBuffer, raw_string_ostream &KdStream) const {
   using namespace amdhsa;
   StringRef Indent = "\t";
@@ -1937,11 +1937,11 @@ MCDisassembler::DecodeStatus AMDGPUDisassembler::decodeCOMPUTE_PGM_RSRC1(
     PRINT_DIRECTIVE(".amdhsa_round_robin_scheduling",
                     COMPUTE_PGM_RSRC1_GFX12_PLUS_ENABLE_WG_RR_EN);
 
-  return MCDisassembler::Success;
+  return Error::success();
 }
 
 // NOLINTNEXTLINE(readability-identifier-naming)
-MCDisassembler::DecodeStatus AMDGPUDisassembler::decodeCOMPUTE_PGM_RSRC2(
+Error AMDGPUDisassembler::decodeCOMPUTE_PGM_RSRC2(
     uint32_t FourByteBuffer, raw_string_ostream &KdStream) const {
   using namespace amdhsa;
   StringRef Indent = "\t";
@@ -1985,11 +1985,11 @@ MCDisassembler::DecodeStatus AMDGPUDisassembler::decodeCOMPUTE_PGM_RSRC2(
 
   CHECK_RESERVED_BITS_DESC(COMPUTE_PGM_RSRC2_RESERVED0, "COMPUTE_PGM_RSRC2");
 
-  return MCDisassembler::Success;
+  return Error::success();
 }
 
 // NOLINTNEXTLINE(readability-identifier-naming)
-MCDisassembler::DecodeStatus AMDGPUDisassembler::decodeCOMPUTE_PGM_RSRC3(
+Error AMDGPUDisassembler::decodeCOMPUTE_PGM_RSRC3(
     uint32_t FourByteBuffer, raw_string_ostream &KdStream) const {
   using namespace amdhsa;
   StringRef Indent = "\t";
@@ -2066,11 +2066,11 @@ MCDisassembler::DecodeStatus AMDGPUDisassembler::decodeCOMPUTE_PGM_RSRC3(
                                    "must be zero on gfx10");
     }
   } else if (FourByteBuffer) {
-    *CommentStream
-        << "kernel descriptor COMPUTE_PGM_RSRC3 must be all zero before gfx9\n";
-    return MCDisassembler::Fail;
+    return createStringError(
+        std::errc::invalid_argument,
+        "kernel descriptor COMPUTE_PGM_RSRC3 must be all zero before gfx9");
   }
-  return MCDisassembler::Success;
+  return Error::success();
 }
 #undef PRINT_PSEUDO_DIRECTIVE_COMMENT
 #undef PRINT_DIRECTIVE
@@ -2081,27 +2081,28 @@ MCDisassembler::DecodeStatus AMDGPUDisassembler::decodeCOMPUTE_PGM_RSRC3(
 #undef CHECK_RESERVED_BITS_DESC
 #undef CHECK_RESERVED_BITS_DESC_MSG
 
-static void printReservedKDBitsError(raw_ostream *CommentStream, uint32_t Mask,
-                                     unsigned BaseInBytes, StringRef Msg = "") {
-  *CommentStream << "kernel descriptor reserved "
-                 << getBitRangeFromMask(Mask, BaseInBytes) << " set";
-  if (!Msg.empty())
-    *CommentStream << ", " << Msg;
-  *CommentStream << '\n';
+/// Create an error object to return from onSymbolStart for reserved kernel
+/// descriptor bits being set.
+static Error createReservedKDBitsError(uint32_t Mask, unsigned BaseBytes,
+                                       const char *Msg = "") {
+  return createStringError(
+      std::errc::invalid_argument, "kernel descriptor reserved %s set%s%s",
+      getBitRangeFromMask(Mask, BaseBytes).c_str(), *Msg ? ", " : "", Msg);
 }
 
-static void printReservedKDBytesError(raw_ostream *CommentStream,
-                                      unsigned BaseInBytes,
-                                      unsigned WidthInBytes) {
-  // Print an error comment in the same format as the "Kernel Descriptor"
+/// Create an error object to return from onSymbolStart for reserved kernel
+/// descriptor bytes being set.
+static Error createReservedKDBytesError(unsigned BaseInBytes,
+                                        unsigned WidthInBytes) {
+  // Create an error comment in the same format as the "Kernel Descriptor"
   // table here: https://llvm.org/docs/AMDGPUUsage.html#kernel-descriptor .
-  *CommentStream << "kernel descriptor reserved bits set in range "
-                 << ((BaseInBytes + WidthInBytes) * CHAR_BIT - 1) << ":"
-                 << (BaseInBytes * CHAR_BIT) << '\n';
+  return createStringError(
+      std::errc::invalid_argument,
+      "kernel descriptor reserved bits in range (%u:%u) set",
+      (BaseInBytes + WidthInBytes) * CHAR_BIT - 1, BaseInBytes * CHAR_BIT);
 }
 
-MCDisassembler::DecodeStatus
-AMDGPUDisassembler::decodeKernelDescriptorDirective(
+Error AMDGPUDisassembler::decodeKernelDescriptorDirective(
     DataExtractor::Cursor &Cursor, ArrayRef<uint8_t> Bytes,
     raw_string_ostream &KdStream) const {
 #define PRINT_DIRECTIVE(DIRECTIVE, MASK)                                       \
@@ -2124,48 +2125,44 @@ AMDGPUDisassembler::decodeKernelDescriptorDirective(
     FourByteBuffer = DE.getU32(Cursor);
     KdStream << Indent << ".amdhsa_group_segment_fixed_size " << FourByteBuffer
              << '\n';
-    return MCDisassembler::Success;
+    return Error::success();
 
   case amdhsa::PRIVATE_SEGMENT_FIXED_SIZE_OFFSET:
     FourByteBuffer = DE.getU32(Cursor);
     KdStream << Indent << ".amdhsa_private_segment_fixed_size "
              << FourByteBuffer << '\n';
-    return MCDisassembler::Success;
+    return Error::success();
 
   case amdhsa::KERNARG_SIZE_OFFSET:
     FourByteBuffer = DE.getU32(Cursor);
     KdStream << Indent << ".amdhsa_kernarg_size "
              << FourByteBuffer << '\n';
-    return MCDisassembler::Success;
+    return Error::success();
 
   case amdhsa::RESERVED0_OFFSET:
     // 4 reserved bytes, must be 0.
     ReservedBytes = DE.getBytes(Cursor, 4);
     for (int I = 0; I < 4; ++I) {
-      if (ReservedBytes[I] != 0) {
-        printReservedKDBytesError(CommentStream, amdhsa::RESERVED0_OFFSET, 4);
-        return MCDisassembler::Fail;
-      }
+      if (ReservedBytes[I] != 0)
+        return createReservedKDBytesError(amdhsa::RESERVED0_OFFSET, 4);
     }
-    return MCDisassembler::Success;
+    return Error::success();
 
   case amdhsa::KERNEL_CODE_ENTRY_BYTE_OFFSET_OFFSET:
     // KERNEL_CODE_ENTRY_BYTE_OFFSET
     // So far no directive controls this for Code Object V3, so simply skip for
     // disassembly.
     DE.skip(Cursor, 8);
-    return MCDisassembler::Success;
+    return Error::success();
 
   case amdhsa::RESERVED1_OFFSET:
     // 20 reserved bytes, must be 0.
     ReservedBytes = DE.getBytes(Cursor, 20);
     for (int I = 0; I < 20; ++I) {
-      if (ReservedBytes[I] != 0) {
-        printReservedKDBytesError(CommentStream, amdhsa::RESERVED1_OFFSET, 20);
-        return MCDisassembler::Fail;
-      }
+      if (ReservedBytes[I] != 0)
+        return createReservedKDBytesError(amdhsa::RESERVED1_OFFSET, 20);
     }
-    return MCDisassembler::Success;
+    return Error::success();
 
   case amdhsa::COMPUTE_PGM_RSRC3_OFFSET:
     FourByteBuffer = DE.getU32(Cursor);
@@ -2200,19 +2197,16 @@ AMDGPUDisassembler::decodeKernelDescriptorDirective(
     PRINT_DIRECTIVE(".amdhsa_user_sgpr_private_segment_size",
                     KERNEL_CODE_PROPERTY_ENABLE_SGPR_PRIVATE_SEGMENT_SIZE);
 
-    if (TwoByteBuffer & KERNEL_CODE_PROPERTY_RESERVED0) {
-      printReservedKDBitsError(CommentStream, KERNEL_CODE_PROPERTY_RESERVED0,
-                               amdhsa::KERNEL_CODE_PROPERTIES_OFFSET);
-      return MCDisassembler::Fail;
-    }
+    if (TwoByteBuffer & KERNEL_CODE_PROPERTY_RESERVED0)
+      return createReservedKDBitsError(KERNEL_CODE_PROPERTY_RESERVED0,
+                                       amdhsa::KERNEL_CODE_PROPERTIES_OFFSET);
 
     // Reserved for GFX9
     if (isGFX9() &&
         (TwoByteBuffer & KERNEL_CODE_PROPERTY_ENABLE_WAVEFRONT_SIZE32)) {
-      printReservedKDBitsError(
-          CommentStream, KERNEL_CODE_PROPERTY_ENABLE_WAVEFRONT_SIZE32,
+      return createReservedKDBitsError(
+          KERNEL_CODE_PROPERTY_ENABLE_WAVEFRONT_SIZE32,
           amdhsa::KERNEL_CODE_PROPERTIES_OFFSET, "must be zero on gfx9");
-      return MCDisassembler::Fail;
     } else if (isGFX10Plus()) {
       PRINT_DIRECTIVE(".amdhsa_wavefront_size32",
                       KERNEL_CODE_PROPERTY_ENABLE_WAVEFRONT_SIZE32);
@@ -2223,12 +2217,11 @@ AMDGPUDisassembler::decodeKernelDescriptorDirective(
                       KERNEL_CODE_PROPERTY_USES_DYNAMIC_STACK);
 
     if (TwoByteBuffer & KERNEL_CODE_PROPERTY_RESERVED1) {
-      printReservedKDBitsError(CommentStream, KERNEL_CODE_PROPERTY_RESERVED1,
-                               amdhsa::KERNEL_CODE_PROPERTIES_OFFSET);
-      return MCDisassembler::Fail;
+      return createReservedKDBitsError(KERNEL_CODE_PROPERTY_RESERVED1,
+                                       amdhsa::KERNEL_CODE_PROPERTIES_OFFSET);
     }
 
-    return MCDisassembler::Success;
+    return Error::success();
 
   case amdhsa::KERNARG_PRELOAD_OFFSET:
     using namespace amdhsa;
@@ -2242,33 +2235,32 @@ AMDGPUDisassembler::decodeKernelDescriptorDirective(
       PRINT_DIRECTIVE(".amdhsa_user_sgpr_kernarg_preload_offset",
                       KERNARG_PRELOAD_SPEC_OFFSET);
     }
-    return MCDisassembler::Success;
+    return Error::success();
 
   case amdhsa::RESERVED3_OFFSET:
     // 4 bytes from here are reserved, must be 0.
     ReservedBytes = DE.getBytes(Cursor, 4);
     for (int I = 0; I < 4; ++I) {
-      if (ReservedBytes[I] != 0) {
-        printReservedKDBytesError(CommentStream, amdhsa::RESERVED3_OFFSET, 4);
-        return MCDisassembler::Fail;
-      }
+      if (ReservedBytes[I] != 0)
+        return createReservedKDBytesError(amdhsa::RESERVED3_OFFSET, 4);
     }
-    return MCDisassembler::Success;
+    return Error::success();
 
   default:
     llvm_unreachable("Unhandled index. Case statements cover everything.");
-    return MCDisassembler::Fail;
+    return Error::success();
   }
 #undef PRINT_DIRECTIVE
 }
 
-MCDisassembler::DecodeStatus AMDGPUDisassembler::decodeKernelDescriptor(
-    StringRef KdName, ArrayRef<uint8_t> Bytes, uint64_t KdAddress) const {
+Error AMDGPUDisassembler::decodeKernelDescriptor(StringRef KdName,
+                                                 ArrayRef<uint8_t> Bytes,
+                                                 uint64_t KdAddress) const {
+
   // CP microcode requires the kernel descriptor to be 64 aligned.
-  if (Bytes.size() != 64 || KdAddress % 64 != 0) {
-    *CommentStream << "kernel descriptor must be 64-byte aligned\n";
-    return MCDisassembler::Fail;
-  }
+  if (Bytes.size() != 64 || KdAddress % 64 != 0)
+    return createStringError(std::errc::invalid_argument,
+                             "kernel descriptor must be 64-byte aligned");
 
   // FIXME: We can't actually decode "in order" as is done below, as e.g. GFX10
   // requires us to know the setting of .amdhsa_wavefront_size32 in order to
@@ -2290,46 +2282,46 @@ MCDisassembler::DecodeStatus AMDGPUDisassembler::decodeKernelDescriptor(
 
   DataExtractor::Cursor C(0);
   while (C && C.tell() < Bytes.size()) {
-    MCDisassembler::DecodeStatus Status =
-        decodeKernelDescriptorDirective(C, Bytes, KdStream);
+    Error Err = decodeKernelDescriptorDirective(C, Bytes, KdStream);
 
     cantFail(C.takeError());
 
-    if (Status == MCDisassembler::Fail)
-      return MCDisassembler::Fail;
+    if (Err)
+      return Err;
   }
   KdStream << ".end_amdhsa_kernel\n";
   outs() << KdStream.str();
-  return MCDisassembler::Success;
+  return Error::success();
 }
 
-std::optional<MCDisassembler::DecodeStatus>
-AMDGPUDisassembler::onSymbolStart(SymbolInfoTy &Symbol, uint64_t &Size,
-                                  ArrayRef<uint8_t> Bytes, uint64_t Address,
-                                  raw_ostream &CStream) const {
+bool AMDGPUDisassembler::onSymbolStart(SymbolInfoTy &Symbol, uint64_t &Size,
+                                       ArrayRef<uint8_t> Bytes,
+                                       uint64_t Address, Error &Err) const {
   // Right now only kernel descriptor needs to be handled.
   // We ignore all other symbols for target specific handling.
   // TODO:
   // Fix the spurious symbol issue for AMDGPU kernels. Exists for both Code
   // Object V2 and V3 when symbols are marked protected.
 
-  SaveAndRestore CommentStreamRAII(CommentStream, &CStream);
+  ErrorAsOutParameter ErrOutParam(&Err);
 
   // amd_kernel_code_t for Code Object V2.
   if (Symbol.Type == ELF::STT_AMDGPU_HSA_KERNEL) {
     Size = 256;
-    *CommentStream << "code object v2 is not supported";
-    return MCDisassembler::Fail;
+    Err = createStringError(std::errc::invalid_argument,
+                            "code object v2 is not supported");
+    return true;
   }
 
   // Code Object V3 kernel descriptors.
   StringRef Name = Symbol.Name;
   if (Symbol.Type == ELF::STT_OBJECT && Name.ends_with(StringRef(".kd"))) {
     Size = 64; // Size = 64 regardless of success or failure.
-    return decodeKernelDescriptor(Name.drop_back(3), Bytes, Address);
+    Err = decodeKernelDescriptor(Name.drop_back(3), Bytes, Address);
+    return true;
   }
 
-  return std::nullopt;
+  return false;
 }
 
 //===----------------------------------------------------------------------===//
diff --git a/llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.h b/llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.h
index 6a4cb120872089..282844918d39a0 100644
--- a/llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.h
+++ b/llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.h
@@ -161,38 +161,37 @@ class AMDGPUDisassembler : public MCDisassembler {
     return MCDisassembler::Fail;
   }
 
-  std::optional<DecodeStatus>
-  onSymbolStart(SymbolInfoTy &Symbol, uint64_t &Size, ArrayRef<uint8_t> Bytes,
-                uint64_t Address, raw_ostream &CStream) const override;
+  bool onSymbolStart(SymbolInfoTy &Symbol, uint64_t &Size,
+                     ArrayRef<uint8_t> Bytes, uint64_t Address,
+                     Error &Err) const override;
 
-  DecodeStatus decodeKernelDescriptor(StringRef KdName, ArrayRef<uint8_t> Bytes,
-                                      uint64_t KdAddress) const;
+  Error decodeKernelDescriptor(StringRef KdName, ArrayRef<uint8_t> Bytes,
+                               uint64_t KdAddress) const;
 
-  DecodeStatus
-  decodeKernelDescriptorDirective(DataExtractor::Cursor &Cursor,
-                                  ArrayRef<uint8_t> Bytes,
-                                  raw_string_ostream &KdStream) const;
+  Error decodeKernelDescriptorDirective(DataExtractor::Cursor &Cursor,
+                                        ArrayRef<uint8_t> Bytes,
+                                        raw_string_ostream &KdStream) const;
 
   /// Decode as directives that handle COMPUTE_PGM_RSRC1.
   /// \param FourByteBuffer - Bytes holding contents of COMPUTE_PGM_RSRC1.
   /// \param KdStream       - Stream to write the disassembled directives to.
   // NOLINTNEXTLINE(readability-identifier-naming)
-  DecodeStatus decodeCOMPUTE_PGM_RSRC1(uint32_t FourByteBuffer,
-                                       raw_string_ostream &KdStream) const;
+  Error decodeCOMPUTE_PGM_RSRC1(uint32_t FourByteBuffer,
+                                raw_string_ostream &KdStream) const;
 
   /// Decode as directives that handle COMPUTE_PGM_RSRC2.
   /// \param FourByteBuffer - Bytes holding contents of COMPUTE_PGM_RSRC2.
   /// \param KdStream       - Stream to write the disassembled directives to.
   // NOLINTNEXTLINE(readability-identifier-naming)
-  DecodeStatus decodeCOMPUTE_PGM_RSRC2(uint32_t FourByteBuffer,
-                                       raw_string_ostream &KdStream) const;
+  Error decodeCOMPUTE_PGM_RSRC2(uint32_t FourByteBuffer,
+                                raw_string_ostream &KdStream) const;
 
   /// Decode as directives that handle COMPUTE_PGM_RSRC3.
   /// \param FourByteBuffer - Bytes holding contents of COMPUTE_PGM_RSRC3.
   /// \param KdStream       - Stream to write the disassembled directives to.
   // NOLINTNEXTLINE(readability-identifier-naming)
-  DecodeStatus decodeCOMPUTE_PGM_RSRC3(uint32_t FourByteBuffer,
-                                       raw_string_ostream &KdStream) const;
+  Error decodeCOMPUTE_PGM_RSRC3(uint32_t FourByteBuffer,
+                                raw_string_ostream &KdStream) const;
 
   void convertEXPInst(MCInst &MI) const;
   void convertVINTERPInst(MCInst &MI) const;
diff --git a/llvm/lib/Target/WebAssembly/Disassembler/WebAssemblyDisassembler.cpp b/llvm/lib/Target/WebAssembly/Disassembler/WebAssemblyDisassembler.cpp
index 861f9f8b5b1ba9..3382349fa83086 100644
--- a/llvm/lib/Target/WebAssembly/Disassembler/WebAssemblyDisassembler.cpp
+++ b/llvm/lib/Target/WebAssembly/Disassembler/WebAssemblyDisassembler.cpp
@@ -47,9 +47,10 @@ class WebAssemblyDisassembler final : public MCDisassembler {
   DecodeStatus getInstruction(MCInst &Instr, uint64_t &Size,
                               ArrayRef<uint8_t> Bytes, uint64_t Address,
                               raw_ostream &CStream) const override;
-  std::optional<DecodeStatus>
-  onSymbolStart(SymbolInfoTy &Symbol, uint64_t &Size, ArrayRef<uint8_t> Bytes,
-                uint64_t Address, raw_ostream &CStream) const override;
+
+  bool onSymbolStart(SymbolInfoTy &Symbol, uint64_t &Size,
+                     ArrayRef<uint8_t> Bytes, uint64_t Address,
+                     Error &Err) const override;
 
 public:
   WebAssemblyDisassembler(const MCSubtargetInfo &STI, MCContext &Ctx,
@@ -122,31 +123,32 @@ bool parseImmediate(MCInst &MI, uint64_t &Size, ArrayRef<uint8_t> Bytes) {
   return true;
 }
 
-std::optional<MCDisassembler::DecodeStatus>
-WebAssemblyDisassembler::onSymbolStart(SymbolInfoTy &Symbol, uint64_t &Size,
-                                       ArrayRef<uint8_t> Bytes,
-                                       uint64_t Address,
-                                       raw_ostream &CStream) const {
+bool WebAssemblyDisassembler::onSymbolStart(SymbolInfoTy &Symbol,
+                                            uint64_t &Size,
+                                            ArrayRef<uint8_t> Bytes,
+                                            uint64_t Address,
+                                            Error &Err) const {
+  ErrorAsOutParameter ErrOutParam(&Err);
   Size = 0;
   if (Symbol.Type == wasm::WASM_SYMBOL_TYPE_SECTION) {
     // Start of a code section: we're parsing only the function count.
     int64_t FunctionCount;
     if (!nextLEB(FunctionCount, Bytes, Size, false))
-      return std::nullopt;
+      return false;
     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 std::nullopt;
+      return false;
     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 std::nullopt;
+          return false;
         for (int64_t J = 0; J < Count; J++) {
           if (I || J)
             outs() << ", ";
@@ -156,7 +158,7 @@ WebAssemblyDisassembler::onSymbolStart(SymbolInfoTy &Symbol, uint64_t &Size,
     }
   }
   outs() << "\n";
-  return MCDisassembler::Success;
+  return true;
 }
 
 MCDisassembler::DecodeStatus WebAssemblyDisassembler::getInstruction(
diff --git a/llvm/test/MC/Disassembler/AMDGPU/kernel-descriptor-errors.test b/llvm/test/MC/Disassembler/AMDGPU/kernel-descriptor-errors.test
index bdfd70f8a022d1..fdca11b95caa63 100644
--- a/llvm/test/MC/Disassembler/AMDGPU/kernel-descriptor-errors.test
+++ b/llvm/test/MC/Disassembler/AMDGPU/kernel-descriptor-errors.test
@@ -1,16 +1,16 @@
 # RUN: yaml2obj %s -DGPU=GFX1100 -DKD=0000000000000000000000001000000000000000000000001000000000000000000000000000000000000000000000000300AC60800000000004000000000000 \
 # RUN:   | llvm-objdump --disassemble-symbols=test.kd - | FileCheck %s --check-prefix=RES_4
-# RES_4: ; error decoding test.kd: kernel descriptor reserved bits set in range 127:96
+# RES_4: ; error decoding test.kd: kernel descriptor reserved bits in range (127:96) set
 # RES_4-NEXT: ; decoding failed region as bytes
 
 # RUN: yaml2obj %s -DGPU=GFX1100 -DKD=0000000000000000000000000000000000000000000000001000000000000000000000000000000000000000000000000300AC60800000000004000000000000 \
 # RUN:   | llvm-objdump --disassemble-symbols=test.kd - | FileCheck %s --check-prefix=RES_20
-# RES_20: ; error decoding test.kd: kernel descriptor reserved bits set in range 351:192
+# RES_20: ; error decoding test.kd: kernel descriptor reserved bits in range (351:192) set
 # RES_20-NEXT: ; decoding failed region as bytes
 
 # RUN: yaml2obj %s -DGPU=GFX1100 -DKD=0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000300AC60800000000004000000000001 \
 # RUN:   | llvm-objdump --disassemble-symbols=test.kd - | FileCheck %s --check-prefix=RES_4_2
-# RES_4_2: ; error decoding test.kd: kernel descriptor reserved bits set in range 511:480
+# RES_4_2: ; error decoding test.kd: kernel descriptor reserved bits in range (511:480) set
 # RES_4_2-NEXT: ; decoding failed region as bytes
 
 # RUN: yaml2obj %s -DGPU=GFX90A -DKD=00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000006000000000000 \
diff --git a/llvm/tools/llvm-objdump/llvm-objdump.cpp b/llvm/tools/llvm-objdump/llvm-objdump.cpp
index 82269a8e51a9c9..31b47f8affca5c 100644
--- a/llvm/tools/llvm-objdump/llvm-objdump.cpp
+++ b/llvm/tools/llvm-objdump/llvm-objdump.cpp
@@ -2051,58 +2051,51 @@ disassembleObject(ObjectFile &Obj, const ObjectFile &DbgObj,
       for (size_t SHI = 0; SHI < SymbolsHere.size(); ++SHI) {
         SymbolInfoTy Symbol = SymbolsHere[SHI];
 
-        SmallString<40> ErrorComments;
-        raw_svector_ostream ErrorCommentsStream(ErrorComments);
-
-        auto Status = DT->DisAsm->onSymbolStart(
+        Error Err = Error::success();
+        bool Responded = DT->DisAsm->onSymbolStart(
             Symbol, Size, Bytes.slice(Start, End - Start), SectionAddr + Start,
-            ErrorCommentsStream);
+            Err);
 
-        if (!Status) {
-          // If onSymbolStart returns std::nullopt, that means it didn't trigger
-          // any interesting handling for this symbol. Try the other symbols
-          // defined at this address.
+        if (!Responded) {
+          // This symbol didn't trigger any interesting handling. Try the other
+          // symbols defined at this address. Errors can only be returned if the
+          // disassembler responds.
+          cantFail(std::move(Err));
           continue;
         }
 
-        if (*Status == MCDisassembler::Fail) {
-          // If onSymbolStart returns Fail, that means it identified some kind
-          // of special data at this address, but wasn't able to disassemble it
-          // meaningfully. So we fall back to disassembling the failed region
-          // as bytes, assuming that the target detected the failure before
-          // printing anything.
-          //
-          // Return values Success or SoftFail (i.e no 'real' failure) are
-          // expected to mean that the target has emitted its own output.
-          //
-          // Either way, 'Size' will have been set to the amount of data
-          // covered by whatever prologue the target identified. So we advance
-          // our own position to beyond that. Sometimes that will be the entire
-          // distance to the next symbol, and sometimes it will be just a
-          // prologue and we should start disassembling instructions from where
-          // it left off.
-
-          // Print any error comments onSymbolStart emitted, or a generic error.
-          StringRef CommentStr = DT->Context->getAsmInfo()->getCommentString();
-          if (!ErrorComments.empty()) {
-            StringRef Comments = ErrorComments;
-            do {
-              StringRef Line;
-              std::tie(Line, Comments) = Comments.split('\n');
-              outs() << CommentStr << " error decoding " << SymNamesHere[SHI]
-                     << ": " << Line << '\n';
-            } while (!Comments.empty());
-            outs() << CommentStr << " decoding failed region as bytes.\n";
-          } else {
-            outs() << CommentStr << " error in decoding " << SymNamesHere[SHI]
-                   << " : decoding failed region as bytes.\n";
-          }
-
-          for (uint64_t I = 0; I < Size; ++I) {
-            outs() << "\t.byte\t " << format_hex(Bytes[I], 1, /*Upper=*/true)
-                   << "\n";
+        // If onSymbolStart responded and set Err, that means it identified some
+        // kind of special data at this address, but wasn't able to disassemble
+        // it meaningfully. So we fall back to printing the error out and
+        // disassembling the failed region as bytes, assuming that the target
+        // detected the failure before printing anything.
+        //
+        // Either way, 'Size' will have been set to the amount of data covered
+        // by whatever prologue the target identified. So we advance our own
+        // position to beyond that. Sometimes that will be the entire distance
+        // to the next symbol, and sometimes it will be just a prologue and we
+        // should start disassembling instructions from where it left off.
+
+        if (Err) {
+          std::string ErrMsgStr = toString(std::move(Err));
+          StringRef ErrMsg = ErrMsgStr;
+          do {
+            StringRef Line;
+            std::tie(Line, ErrMsg) = ErrMsg.split('\n');
+            outs() << DT->Context->getAsmInfo()->getCommentString()
+                   << " error decoding " << SymNamesHere[SHI] << ": " << Line
+                   << '\n';
+          } while (!ErrMsg.empty());
+
+          if (Size) {
+            outs() << DT->Context->getAsmInfo()->getCommentString()
+                   << " decoding failed region as bytes.\n";
+            for (uint64_t I = 0; I < Size; ++I)
+              outs() << "\t.byte\t " << format_hex(Bytes[I], 1, /*Upper=*/true)
+                     << '\n';
           }
         }
+
         Start += Size;
         break;
       }

>From 6114654fb5807bcf67744178958a2ea03ea03a20 Mon Sep 17 00:00:00 2001
From: Emma Pilkington <emma.pilkington95 at gmail.com>
Date: Wed, 10 Apr 2024 07:28:53 -0400
Subject: [PATCH 5/5] Use Expected<bool>, remove period in error comment

---
 .../llvm/MC/MCDisassembler/MCDisassembler.h   | 14 ++--
 llvm/lib/MC/MCDisassembler/MCDisassembler.cpp |  7 +-
 .../Disassembler/AMDGPUDisassembler.cpp       | 64 +++++++++----------
 .../AMDGPU/Disassembler/AMDGPUDisassembler.h  | 30 +++++----
 .../Disassembler/WebAssemblyDisassembler.cpp  | 16 ++---
 llvm/tools/llvm-objdump/llvm-objdump.cpp      | 33 +++++-----
 6 files changed, 79 insertions(+), 85 deletions(-)

diff --git a/llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h b/llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h
index d5cbeeb82e6d4f..901bfcf5fa54f4 100644
--- a/llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h
+++ b/llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h
@@ -148,18 +148,18 @@ class MCDisassembler {
   /// \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 Err      - An out parameter that indicates whether an error was
-  ///                   found.
   /// \return         - True if this symbol triggered some target specific
   ///                   disassembly for this symbol. Size must be set with the
-  ///                   number of bytes consumed regardless of whether an error
-  ///                   was found.
+  ///                   number of bytes consumed.
+  ///                 - Error if this symbol triggered some target specific
+  ///                   disassembly for this symbol, but an error was found with
+  ///                   it. Size must be set with the number of bytes consumed.
   ///                 - False if the target doesn't want to handle the symbol
   ///                   separately. The value of Size is ignored in this case,
   ///                   and Err must not be set.
-  virtual bool onSymbolStart(SymbolInfoTy &Symbol, uint64_t &Size,
-                             ArrayRef<uint8_t> Bytes, uint64_t Address,
-                             Error &Err) const;
+  virtual Expected<bool> onSymbolStart(SymbolInfoTy &Symbol, uint64_t &Size,
+                                       ArrayRef<uint8_t> Bytes,
+                                       uint64_t Address) const;
   // TODO:
   // Implement similar hooks that can be used at other points during
   // disassembly. Something along the following lines:
diff --git a/llvm/lib/MC/MCDisassembler/MCDisassembler.cpp b/llvm/lib/MC/MCDisassembler/MCDisassembler.cpp
index e1393e81966ce5..6aa4b0e4fcb99d 100644
--- a/llvm/lib/MC/MCDisassembler/MCDisassembler.cpp
+++ b/llvm/lib/MC/MCDisassembler/MCDisassembler.cpp
@@ -13,9 +13,10 @@ using namespace llvm;
 
 MCDisassembler::~MCDisassembler() = default;
 
-bool MCDisassembler::onSymbolStart(SymbolInfoTy &Symbol, uint64_t &Size,
-                                   ArrayRef<uint8_t> Bytes, uint64_t Address,
-                                   Error &Err) const {
+Expected<bool> MCDisassembler::onSymbolStart(SymbolInfoTy &Symbol,
+                                             uint64_t &Size,
+                                             ArrayRef<uint8_t> Bytes,
+                                             uint64_t Address) const {
   return false;
 }
 
diff --git a/llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp b/llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp
index 68ecf5bc3f9c7c..dc1bf92771b443 100644
--- a/llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp
+++ b/llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp
@@ -1833,7 +1833,7 @@ static SmallString<32> getBitRangeFromMask(uint32_t Mask, unsigned BaseBytes) {
   CHECK_RESERVED_BITS_IMPL(MASK, DESC, ", " MSG)
 
 // NOLINTNEXTLINE(readability-identifier-naming)
-Error AMDGPUDisassembler::decodeCOMPUTE_PGM_RSRC1(
+Expected<bool> AMDGPUDisassembler::decodeCOMPUTE_PGM_RSRC1(
     uint32_t FourByteBuffer, raw_string_ostream &KdStream) const {
   using namespace amdhsa;
   StringRef Indent = "\t";
@@ -1937,11 +1937,11 @@ Error AMDGPUDisassembler::decodeCOMPUTE_PGM_RSRC1(
     PRINT_DIRECTIVE(".amdhsa_round_robin_scheduling",
                     COMPUTE_PGM_RSRC1_GFX12_PLUS_ENABLE_WG_RR_EN);
 
-  return Error::success();
+  return true;
 }
 
 // NOLINTNEXTLINE(readability-identifier-naming)
-Error AMDGPUDisassembler::decodeCOMPUTE_PGM_RSRC2(
+Expected<bool> AMDGPUDisassembler::decodeCOMPUTE_PGM_RSRC2(
     uint32_t FourByteBuffer, raw_string_ostream &KdStream) const {
   using namespace amdhsa;
   StringRef Indent = "\t";
@@ -1985,11 +1985,11 @@ Error AMDGPUDisassembler::decodeCOMPUTE_PGM_RSRC2(
 
   CHECK_RESERVED_BITS_DESC(COMPUTE_PGM_RSRC2_RESERVED0, "COMPUTE_PGM_RSRC2");
 
-  return Error::success();
+  return true;
 }
 
 // NOLINTNEXTLINE(readability-identifier-naming)
-Error AMDGPUDisassembler::decodeCOMPUTE_PGM_RSRC3(
+Expected<bool> AMDGPUDisassembler::decodeCOMPUTE_PGM_RSRC3(
     uint32_t FourByteBuffer, raw_string_ostream &KdStream) const {
   using namespace amdhsa;
   StringRef Indent = "\t";
@@ -2070,7 +2070,7 @@ Error AMDGPUDisassembler::decodeCOMPUTE_PGM_RSRC3(
         std::errc::invalid_argument,
         "kernel descriptor COMPUTE_PGM_RSRC3 must be all zero before gfx9");
   }
-  return Error::success();
+  return true;
 }
 #undef PRINT_PSEUDO_DIRECTIVE_COMMENT
 #undef PRINT_DIRECTIVE
@@ -2102,7 +2102,7 @@ static Error createReservedKDBytesError(unsigned BaseInBytes,
       (BaseInBytes + WidthInBytes) * CHAR_BIT - 1, BaseInBytes * CHAR_BIT);
 }
 
-Error AMDGPUDisassembler::decodeKernelDescriptorDirective(
+Expected<bool> AMDGPUDisassembler::decodeKernelDescriptorDirective(
     DataExtractor::Cursor &Cursor, ArrayRef<uint8_t> Bytes,
     raw_string_ostream &KdStream) const {
 #define PRINT_DIRECTIVE(DIRECTIVE, MASK)                                       \
@@ -2125,19 +2125,19 @@ Error AMDGPUDisassembler::decodeKernelDescriptorDirective(
     FourByteBuffer = DE.getU32(Cursor);
     KdStream << Indent << ".amdhsa_group_segment_fixed_size " << FourByteBuffer
              << '\n';
-    return Error::success();
+    return true;
 
   case amdhsa::PRIVATE_SEGMENT_FIXED_SIZE_OFFSET:
     FourByteBuffer = DE.getU32(Cursor);
     KdStream << Indent << ".amdhsa_private_segment_fixed_size "
              << FourByteBuffer << '\n';
-    return Error::success();
+    return true;
 
   case amdhsa::KERNARG_SIZE_OFFSET:
     FourByteBuffer = DE.getU32(Cursor);
     KdStream << Indent << ".amdhsa_kernarg_size "
              << FourByteBuffer << '\n';
-    return Error::success();
+    return true;
 
   case amdhsa::RESERVED0_OFFSET:
     // 4 reserved bytes, must be 0.
@@ -2146,14 +2146,14 @@ Error AMDGPUDisassembler::decodeKernelDescriptorDirective(
       if (ReservedBytes[I] != 0)
         return createReservedKDBytesError(amdhsa::RESERVED0_OFFSET, 4);
     }
-    return Error::success();
+    return true;
 
   case amdhsa::KERNEL_CODE_ENTRY_BYTE_OFFSET_OFFSET:
     // KERNEL_CODE_ENTRY_BYTE_OFFSET
     // So far no directive controls this for Code Object V3, so simply skip for
     // disassembly.
     DE.skip(Cursor, 8);
-    return Error::success();
+    return true;
 
   case amdhsa::RESERVED1_OFFSET:
     // 20 reserved bytes, must be 0.
@@ -2162,7 +2162,7 @@ Error AMDGPUDisassembler::decodeKernelDescriptorDirective(
       if (ReservedBytes[I] != 0)
         return createReservedKDBytesError(amdhsa::RESERVED1_OFFSET, 20);
     }
-    return Error::success();
+    return true;
 
   case amdhsa::COMPUTE_PGM_RSRC3_OFFSET:
     FourByteBuffer = DE.getU32(Cursor);
@@ -2221,7 +2221,7 @@ Error AMDGPUDisassembler::decodeKernelDescriptorDirective(
                                        amdhsa::KERNEL_CODE_PROPERTIES_OFFSET);
     }
 
-    return Error::success();
+    return true;
 
   case amdhsa::KERNARG_PRELOAD_OFFSET:
     using namespace amdhsa;
@@ -2235,7 +2235,7 @@ Error AMDGPUDisassembler::decodeKernelDescriptorDirective(
       PRINT_DIRECTIVE(".amdhsa_user_sgpr_kernarg_preload_offset",
                       KERNARG_PRELOAD_SPEC_OFFSET);
     }
-    return Error::success();
+    return true;
 
   case amdhsa::RESERVED3_OFFSET:
     // 4 bytes from here are reserved, must be 0.
@@ -2244,18 +2244,17 @@ Error AMDGPUDisassembler::decodeKernelDescriptorDirective(
       if (ReservedBytes[I] != 0)
         return createReservedKDBytesError(amdhsa::RESERVED3_OFFSET, 4);
     }
-    return Error::success();
+    return true;
 
   default:
     llvm_unreachable("Unhandled index. Case statements cover everything.");
-    return Error::success();
+    return true;
   }
 #undef PRINT_DIRECTIVE
 }
 
-Error AMDGPUDisassembler::decodeKernelDescriptor(StringRef KdName,
-                                                 ArrayRef<uint8_t> Bytes,
-                                                 uint64_t KdAddress) const {
+Expected<bool> AMDGPUDisassembler::decodeKernelDescriptor(
+    StringRef KdName, ArrayRef<uint8_t> Bytes, uint64_t KdAddress) const {
 
   // CP microcode requires the kernel descriptor to be 64 aligned.
   if (Bytes.size() != 64 || KdAddress % 64 != 0)
@@ -2282,43 +2281,40 @@ Error AMDGPUDisassembler::decodeKernelDescriptor(StringRef KdName,
 
   DataExtractor::Cursor C(0);
   while (C && C.tell() < Bytes.size()) {
-    Error Err = decodeKernelDescriptorDirective(C, Bytes, KdStream);
+    Expected<bool> Res = decodeKernelDescriptorDirective(C, Bytes, KdStream);
 
     cantFail(C.takeError());
 
-    if (Err)
-      return Err;
+    if (!Res)
+      return Res;
   }
   KdStream << ".end_amdhsa_kernel\n";
   outs() << KdStream.str();
-  return Error::success();
+  return true;
 }
 
-bool AMDGPUDisassembler::onSymbolStart(SymbolInfoTy &Symbol, uint64_t &Size,
-                                       ArrayRef<uint8_t> Bytes,
-                                       uint64_t Address, Error &Err) const {
+Expected<bool> AMDGPUDisassembler::onSymbolStart(SymbolInfoTy &Symbol,
+                                                 uint64_t &Size,
+                                                 ArrayRef<uint8_t> Bytes,
+                                                 uint64_t Address) const {
   // Right now only kernel descriptor needs to be handled.
   // We ignore all other symbols for target specific handling.
   // TODO:
   // Fix the spurious symbol issue for AMDGPU kernels. Exists for both Code
   // Object V2 and V3 when symbols are marked protected.
 
-  ErrorAsOutParameter ErrOutParam(&Err);
-
   // amd_kernel_code_t for Code Object V2.
   if (Symbol.Type == ELF::STT_AMDGPU_HSA_KERNEL) {
     Size = 256;
-    Err = createStringError(std::errc::invalid_argument,
-                            "code object v2 is not supported");
-    return true;
+    return createStringError(std::errc::invalid_argument,
+                             "code object v2 is not supported");
   }
 
   // Code Object V3 kernel descriptors.
   StringRef Name = Symbol.Name;
   if (Symbol.Type == ELF::STT_OBJECT && Name.ends_with(StringRef(".kd"))) {
     Size = 64; // Size = 64 regardless of success or failure.
-    Err = decodeKernelDescriptor(Name.drop_back(3), Bytes, Address);
-    return true;
+    return decodeKernelDescriptor(Name.drop_back(3), Bytes, Address);
   }
 
   return false;
diff --git a/llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.h b/llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.h
index 282844918d39a0..2061d83af3da0f 100644
--- a/llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.h
+++ b/llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.h
@@ -161,37 +161,39 @@ class AMDGPUDisassembler : public MCDisassembler {
     return MCDisassembler::Fail;
   }
 
-  bool onSymbolStart(SymbolInfoTy &Symbol, uint64_t &Size,
-                     ArrayRef<uint8_t> Bytes, uint64_t Address,
-                     Error &Err) const override;
+  Expected<bool> onSymbolStart(SymbolInfoTy &Symbol, uint64_t &Size,
+                               ArrayRef<uint8_t> Bytes,
+                               uint64_t Address) const override;
 
-  Error decodeKernelDescriptor(StringRef KdName, ArrayRef<uint8_t> Bytes,
-                               uint64_t KdAddress) const;
-
-  Error decodeKernelDescriptorDirective(DataExtractor::Cursor &Cursor,
+  Expected<bool> decodeKernelDescriptor(StringRef KdName,
                                         ArrayRef<uint8_t> Bytes,
-                                        raw_string_ostream &KdStream) const;
+                                        uint64_t KdAddress) const;
+
+  Expected<bool>
+  decodeKernelDescriptorDirective(DataExtractor::Cursor &Cursor,
+                                  ArrayRef<uint8_t> Bytes,
+                                  raw_string_ostream &KdStream) const;
 
   /// Decode as directives that handle COMPUTE_PGM_RSRC1.
   /// \param FourByteBuffer - Bytes holding contents of COMPUTE_PGM_RSRC1.
   /// \param KdStream       - Stream to write the disassembled directives to.
   // NOLINTNEXTLINE(readability-identifier-naming)
-  Error decodeCOMPUTE_PGM_RSRC1(uint32_t FourByteBuffer,
-                                raw_string_ostream &KdStream) const;
+  Expected<bool> decodeCOMPUTE_PGM_RSRC1(uint32_t FourByteBuffer,
+                                         raw_string_ostream &KdStream) const;
 
   /// Decode as directives that handle COMPUTE_PGM_RSRC2.
   /// \param FourByteBuffer - Bytes holding contents of COMPUTE_PGM_RSRC2.
   /// \param KdStream       - Stream to write the disassembled directives to.
   // NOLINTNEXTLINE(readability-identifier-naming)
-  Error decodeCOMPUTE_PGM_RSRC2(uint32_t FourByteBuffer,
-                                raw_string_ostream &KdStream) const;
+  Expected<bool> decodeCOMPUTE_PGM_RSRC2(uint32_t FourByteBuffer,
+                                         raw_string_ostream &KdStream) const;
 
   /// Decode as directives that handle COMPUTE_PGM_RSRC3.
   /// \param FourByteBuffer - Bytes holding contents of COMPUTE_PGM_RSRC3.
   /// \param KdStream       - Stream to write the disassembled directives to.
   // NOLINTNEXTLINE(readability-identifier-naming)
-  Error decodeCOMPUTE_PGM_RSRC3(uint32_t FourByteBuffer,
-                                raw_string_ostream &KdStream) const;
+  Expected<bool> decodeCOMPUTE_PGM_RSRC3(uint32_t FourByteBuffer,
+                                         raw_string_ostream &KdStream) const;
 
   void convertEXPInst(MCInst &MI) const;
   void convertVINTERPInst(MCInst &MI) const;
diff --git a/llvm/lib/Target/WebAssembly/Disassembler/WebAssemblyDisassembler.cpp b/llvm/lib/Target/WebAssembly/Disassembler/WebAssemblyDisassembler.cpp
index 3382349fa83086..3585b5f4a5c9ad 100644
--- a/llvm/lib/Target/WebAssembly/Disassembler/WebAssemblyDisassembler.cpp
+++ b/llvm/lib/Target/WebAssembly/Disassembler/WebAssemblyDisassembler.cpp
@@ -48,9 +48,9 @@ class WebAssemblyDisassembler final : public MCDisassembler {
                               ArrayRef<uint8_t> Bytes, uint64_t Address,
                               raw_ostream &CStream) const override;
 
-  bool onSymbolStart(SymbolInfoTy &Symbol, uint64_t &Size,
-                     ArrayRef<uint8_t> Bytes, uint64_t Address,
-                     Error &Err) const override;
+  Expected<bool> onSymbolStart(SymbolInfoTy &Symbol, uint64_t &Size,
+                               ArrayRef<uint8_t> Bytes,
+                               uint64_t Address) const override;
 
 public:
   WebAssemblyDisassembler(const MCSubtargetInfo &STI, MCContext &Ctx,
@@ -123,12 +123,10 @@ bool parseImmediate(MCInst &MI, uint64_t &Size, ArrayRef<uint8_t> Bytes) {
   return true;
 }
 
-bool WebAssemblyDisassembler::onSymbolStart(SymbolInfoTy &Symbol,
-                                            uint64_t &Size,
-                                            ArrayRef<uint8_t> Bytes,
-                                            uint64_t Address,
-                                            Error &Err) const {
-  ErrorAsOutParameter ErrOutParam(&Err);
+Expected<bool> WebAssemblyDisassembler::onSymbolStart(SymbolInfoTy &Symbol,
+                                                      uint64_t &Size,
+                                                      ArrayRef<uint8_t> Bytes,
+                                                      uint64_t Address) const {
   Size = 0;
   if (Symbol.Type == wasm::WASM_SYMBOL_TYPE_SECTION) {
     // Start of a code section: we're parsing only the function count.
diff --git a/llvm/tools/llvm-objdump/llvm-objdump.cpp b/llvm/tools/llvm-objdump/llvm-objdump.cpp
index 31b47f8affca5c..01b7e0ffc26b39 100644
--- a/llvm/tools/llvm-objdump/llvm-objdump.cpp
+++ b/llvm/tools/llvm-objdump/llvm-objdump.cpp
@@ -2051,33 +2051,30 @@ disassembleObject(ObjectFile &Obj, const ObjectFile &DbgObj,
       for (size_t SHI = 0; SHI < SymbolsHere.size(); ++SHI) {
         SymbolInfoTy Symbol = SymbolsHere[SHI];
 
-        Error Err = Error::success();
-        bool Responded = DT->DisAsm->onSymbolStart(
-            Symbol, Size, Bytes.slice(Start, End - Start), SectionAddr + Start,
-            Err);
+        Expected<bool> RespondedOrErr = DT->DisAsm->onSymbolStart(
+            Symbol, Size, Bytes.slice(Start, End - Start), SectionAddr + Start);
 
-        if (!Responded) {
+        if (RespondedOrErr && !*RespondedOrErr) {
           // This symbol didn't trigger any interesting handling. Try the other
-          // symbols defined at this address. Errors can only be returned if the
-          // disassembler responds.
-          cantFail(std::move(Err));
+          // symbols defined at this address.
           continue;
         }
 
-        // If onSymbolStart responded and set Err, that means it identified some
+        // If onSymbolStart returned an Error, that means it identified some
         // kind of special data at this address, but wasn't able to disassemble
         // it meaningfully. So we fall back to printing the error out and
         // disassembling the failed region as bytes, assuming that the target
         // detected the failure before printing anything.
         //
-        // Either way, 'Size' will have been set to the amount of data covered
-        // by whatever prologue the target identified. So we advance our own
-        // position to beyond that. Sometimes that will be the entire distance
-        // to the next symbol, and sometimes it will be just a prologue and we
-        // should start disassembling instructions from where it left off.
-
-        if (Err) {
-          std::string ErrMsgStr = toString(std::move(Err));
+        // Regardless of whether onSymbolStart returned an Error or true, 'Size'
+        // will have been set to the amount of data covered by whatever prologue
+        // the target identified. So we advance our own position to beyond that.
+        // Sometimes that will be the entire distance to the next symbol, and
+        // sometimes it will be just a prologue and we should start
+        // disassembling instructions from where it left off.
+
+        if (!RespondedOrErr) {
+          std::string ErrMsgStr = toString(RespondedOrErr.takeError());
           StringRef ErrMsg = ErrMsgStr;
           do {
             StringRef Line;
@@ -2089,7 +2086,7 @@ disassembleObject(ObjectFile &Obj, const ObjectFile &DbgObj,
 
           if (Size) {
             outs() << DT->Context->getAsmInfo()->getCommentString()
-                   << " decoding failed region as bytes.\n";
+                   << " decoding failed region as bytes\n";
             for (uint64_t I = 0; I < Size; ++I)
               outs() << "\t.byte\t " << format_hex(Bytes[I], 1, /*Upper=*/true)
                      << '\n';



More information about the llvm-commits mailing list