[PATCH] D45848: [WebAssembly] Initial Disassembler.

[Wouter van Oortmerssen via Phabricator via llvm-commits]

aardappel marked 9 inline comments as done.
aardappel added a comment.

It can be ready to submit if we agree that for the moment having non-sense registers in the output is ok (looks like this will need works elsewhere before that can be properly fixed).
It could also probably need more tests. I haven't run it on a larger binary yet.



================
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:
> sbc100 wrote:
> > Mark these internal functions as `static`?
> Does it make sense to pass an arrayref by ref?
No, it doesn't.. it was originally mutable, forgot to change it.


================
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));
----------------
sbc100 wrote:
> 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.
You're totally right.. I had initially assumed there was a possibility of it reading past the end.


================
Comment at: lib/Target/WebAssembly/InstPrinter/WebAssemblyInstPrinter.cpp:202
+                                                       const MCSubtargetInfo &
+                                                         STI,
                                                        raw_ostream &O) {
----------------
sbc100 wrote:
> Formatting looks strange.  `git clang-format origin/master`?
did `git clang-format HEAD^` since I am maybe behind on master..


================
Comment at: lib/Target/WebAssembly/InstPrinter/WebAssemblyInstPrinter.h:46
   void printWebAssemblySignatureOperand(const MCInst *MI, unsigned OpNo,
+                                        const MCSubtargetInfo &STI,
                                         raw_ostream &O);
----------------
sbc100 wrote:
> It looks like none of these functions use this new arg.  Is it needed for some other reason?
Because of the changes I made in the .td files, the generated code that interfaces with this code now expects these extra arguments. No idea why.


================
Comment at: lib/Target/WebAssembly/WebAssembly.td:87
+  string AsmWriterClassName  = "InstPrinter";
+  int PassSubtarget = 1;
+  int Variant = 0;
----------------
sbc100 wrote:
> Can this be 0 and avoid the new STI argument?
Ahh that's where that came from, thanks :)


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


================
Comment at: utils/TableGen/DisassemblerEmitter.cpp:133
+  if (Target.getName() == "WebAssembly") {
+    emitWebAssemblyDisassemblerTables(OS, Target.getInstructionsByEnumValue());
+    return;
----------------
sbc100 wrote:
> This is a standalone function so it should start with capital letter I think
I don't see any mention of special treatmeant of stand-alone functions in https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly
In fact, it has an example that happens to be a stand-alone function which starts with lowercase.


Repository:
  rL LLVM

https://reviews.llvm.org/D45848