[PATCH] D80512: [MC] Changes to help improve target specific symbol disassembly

[Scott Linder via Phabricator via llvm-commits]

scott.linder accepted this revision.
scott.linder added a comment.

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.



================
Comment at: llvm/lib/MC/MCDisassembler/MCDisassembler.cpp:23
                               raw_ostream &CStream) const {
   Size = 0;
+  return None; // Ignore
----------------
Can this be removed? The contract is this value can't be read anyway.


================
Comment at: llvm/lib/MC/MCDisassembler/MCDisassembler.cpp:24
   Size = 0;
-  return MCDisassembler::Success;
+  return None; // Ignore
 }
----------------
I think the comment here is overkill, the comment for the function itself already describes the meaning.


================
Comment at: llvm/lib/Target/WebAssembly/Disassembler/WebAssemblyDisassembler.cpp:132
     if (!nextLEB(FunctionCount, Bytes, Size, false))
-      return MCDisassembler::Fail;
+      return None; // Ignore
     outs() << "        # " << FunctionCount << " functions in section.";
----------------
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.


================
Comment at: llvm/lib/Target/WebAssembly/Disassembler/WebAssemblyDisassembler.cpp:146
             !nextLEB(Type, Bytes, Size, false))
-          return MCDisassembler::Fail;
+          return None; // Ignore
         for (int64_t J = 0; J < Count; J++) {
----------------
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.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1442
+      //
+      //  -> Otherwise we would have decoded some bytes twice. Once by the
+      //  target (until it failed) and then again as bytes.
----------------
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80512/new/

https://reviews.llvm.org/D80512