[PATCH] D25590: Add initial support for WebAssembly binary format

[Derek Schuff via llvm-commits]

dschuff added inline comments.


================
Comment at: include/llvm/Object/Wasm.h:10
+//
+// This file declares the WasmObjectFile class, which implement the ObjectFile
+// interface for Wasm files.
----------------
implement->implements


================
Comment at: include/llvm/Object/Wasm.h:11
+// This file declares the WasmObjectFile class, which implement the ObjectFile
+// interface for Wasm files.
+//
----------------
Probably worth putting a link here to the design doc for the wasm binary format: https://github.com/WebAssembly/design/blob/master/BinaryEncoding.md (and later the text spec/standard once we have it)


================
Comment at: include/llvm/Object/Wasm.h:35
+
+class WasmObjectFile : public ObjectFile {
+public:
----------------
We should implement `classof` so that `isa`/`cast` etc will work.


================
Comment at: include/llvm/Object/Wasm.h:41
+
+protected:
+  WasmObjectHeader Header;
----------------
I guess most of the things in `protected` are the necessary overrides to implement `ObjectFile`. But do we need e.g. these data members there? Should they be private?


================
Comment at: include/llvm/Object/Wasm.h:59
+
+  //std::error_code printSymbolName(raw_ostream &OS,
+                                  //DataRefImpl Symb) const override;
----------------
commented-out code.


================
Comment at: include/llvm/Object/Wasm.h:69
+
+  // Same as above for SectionRef.
+  friend class SectionRef;
----------------
These comments don't make sense without also having the one from `SymbolRef`


================
Comment at: include/llvm/ObjectYAML/WasmYAML.h:41
+
+} // namespace llvm::MachOYAML
+} // namespace llvm
----------------
WasmYAML


================
Comment at: include/llvm/ObjectYAML/WasmYAML.h:57
+  static void mapping(IO &IO, std::unique_ptr<WasmYAML::Section> &Section);
+  //static StringRef validate(IO &io, std::unique_ptr<WasmYAML::Section> &Section);
+};
----------------
Commented.


================
Comment at: include/llvm/Support/FileSystem.h:265
+    windows_resource,         ///< Windows compiled resource file (.rc)
+    wasm_object               ///< WebAssembly Object file
   };
----------------
Do you know what the difference is between the various object vs executable file types is in this context? To the extent that it matters, we should probably start with focusing on executables, since that's well-specified, whereas we haven't got the details of what we want for object files nailed down (e.g. symbols, relocations, etc, including even how we will differentiate an object file from an executable).


================
Comment at: lib/Object/WasmObjectFile.cpp:11
+#include "llvm/Object/Wasm.h"
+#include "llvm/Support/LEB128.h"
+#include "llvm/Support/Host.h"
----------------
alphabetize includes.


================
Comment at: lib/Object/WasmObjectFile.cpp:22
+  Error Err;
+  std::unique_ptr<WasmObjectFile> R;
+
----------------
What does `R` stand for? return value?


================
Comment at: lib/Object/WasmObjectFile.cpp:26
+  if (Err)
+     std::move(Err);
+
----------------
should this be `return std::move(Err)`?


================
Comment at: lib/Object/WasmObjectFile.cpp:31
+
+uint32_t readUint32(const uint8_t *Ptr) {
+  uint32_t Result = 0;
----------------
Can any of these be static or anonymous namespace?


================
Comment at: lib/Object/WasmObjectFile.cpp:46
+
+void readSection(WasmSection &Section, const uint8_t *&Ptr, const uint8_t *Start) {
+  Section.Offset = Ptr - Start;
----------------
Does this function have any way to fail?


================
Comment at: lib/Object/WasmObjectFile.cpp:57
+  Header.Magic = getData().substr(0, 4);
+  Header.Version = readUint32(getPtr(4));
+  if (Header.Magic != StringRef("\0asm", 4)) {
----------------
How do we handle running out of bytes/off the end of the buffer early? Or bogus data in section header? For that matter, how does `decodeULEB128()` fail?


================
Comment at: lib/Object/WasmObjectFile.cpp:80
+void WasmObjectFile::moveSymbolNext(DataRefImpl &Symb) const {
+  assert(0);
+}
----------------
use `llvm_unreachable("message, e.g. not yet implemented")` instead of `assert(0)`


================
Comment at: lib/Object/WasmObjectFile.cpp:262
+  assert(0);
+  return "wasm32";
+}
----------------
I don't know if this string has to match an arch name (e.g. "wasm32" or "wasm64") but it may well end up being the case for wasm that we don't have different file formats for wasm32 vs wasm64. So if this is just arbitrary, for now it could just be "wasm".


================
Comment at: lib/Object/WasmObjectFile.cpp:267
+  assert(0);
+  return Triple::UnknownArch;
+}
----------------
This can be `Triple::wasm32` for now.


https://reviews.llvm.org/D25590