[PATCH] D45848: [WebAssembly] Initial Disassembler.

[Sam Clegg via Phabricator via llvm-commits]

sbc100 added a comment.

Is this ready to be submitted now?



================
Comment at: lib/Target/WebAssembly/Disassembler/WebAssemblyDisassembler.cpp:70
 
-MCDisassembler::DecodeStatus WebAssemblyDisassembler::getInstruction(
-    MCInst &MI, uint64_t &Size, ArrayRef<uint8_t> Bytes, uint64_t /*Address*/,
-    raw_ostream &OS, raw_ostream &CS) const {
+int nextByte(ArrayRef<uint8_t> &Bytes, uint64_t &Size) {
+  if (Size >= Bytes.size()) return -1;
----------------
Mark these internal functions as `static`?


================
Comment at: lib/Target/WebAssembly/Disassembler/WebAssemblyDisassembler.cpp:70
 
-MCDisassembler::DecodeStatus WebAssemblyDisassembler::getInstruction(
-    MCInst &MI, uint64_t &Size, ArrayRef<uint8_t> Bytes, uint64_t /*Address*/,
-    raw_ostream &OS, raw_ostream &CS) const {
+int nextByte(ArrayRef<uint8_t> &Bytes, uint64_t &Size) {
+  if (Size >= Bytes.size()) return -1;
----------------
sbc100 wrote:
> Mark these internal functions as `static`?
Does it make sense to pass an arrayref by ref?


================
Comment at: lib/Target/WebAssembly/Disassembler/WebAssemblyDisassembler.cpp:82
+  for (;;) {
+    auto B = nextByte(Bytes, Size);
+    if (B < 0) return false;
----------------
I general I think llvm style says not to use  `auto` unless is immediately obvious what the type is (i.g. if there is a cast on the right hand side which already names the type).

I also wouldn't use `auto` for start above.   


================
Comment at: lib/Target/WebAssembly/Disassembler/WebAssemblyDisassembler.cpp:89
+             : static_cast<int64_t>(decodeULEB128(Bytes.data() + Start, nullptr,
+                                                  Bytes.data() + Size));
+  MI.addOperand(MCOperand::createImm(Val));
----------------
decodeULEB128 functions already detect malformed LEBs.     I think you can avoid the entire readByte loop and just  pass the `error` argument to decodeULEB128, along with the actual end of the Bytes array.


================
Comment at: lib/Target/WebAssembly/InstPrinter/WebAssemblyInstPrinter.cpp:202
+                                                       const MCSubtargetInfo &
+                                                         STI,
                                                        raw_ostream &O) {
----------------
Formatting looks strange.  `git clang-format origin/master`?


================
Comment at: lib/Target/WebAssembly/InstPrinter/WebAssemblyInstPrinter.h:46
   void printWebAssemblySignatureOperand(const MCInst *MI, unsigned OpNo,
+                                        const MCSubtargetInfo &STI,
                                         raw_ostream &O);
----------------
It looks like none of these functions use this new arg.  Is it needed for some other reason?


================
Comment at: lib/Target/WebAssembly/WebAssembly.td:87
+  string AsmWriterClassName  = "InstPrinter";
+  int PassSubtarget = 1;
+  int Variant = 0;
----------------
Can this be 0 and avoid the new STI argument?


================
Comment at: unittests/MC/Disassembler.cpp:66
+
+TEST(Disassembler, Test2) {
+  llvm::InitializeAllTargetInfos();
----------------
Probably want a better if you want to add this new test :)


================
Comment at: utils/TableGen/DisassemblerEmitter.cpp:133
+  if (Target.getName() == "WebAssembly") {
+    emitWebAssemblyDisassemblerTables(OS, Target.getInstructionsByEnumValue());
+    return;
----------------
This is a standalone function so it should start with capital letter I think


Repository:
  rL LLVM

https://reviews.llvm.org/D45848