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

[Sam Clegg via llvm-commits]

sbc100 added inline comments.


================
Comment at: include/llvm/Object/Wasm.h:26
+static const char WasmMagic[] = { '\0', 'a', 's', 'm' };
+static uint32_t WasmVersion = 0xc;
+
----------------
sunfish wrote:
> Let's update this to 0xd too.
I will probably wait until 0xd branched get merged then update this patch.    Since this patch has now grown significantly do you think I should split it into smaller chunks?   Perhaps I should land just the YAML<->wasm stuff with tests.. and leave the readobj stuff for a second patch?


================
Comment at: include/llvm/Support/FileSystem.h:265
+    windows_resource,         ///< Windows compiled resource file (.rc)
+    wasm_object               ///< WebAssembly Object file
   };
----------------
dschuff wrote:
> 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).
This is just an enumeration of different magic number types.  I don't think we know at this point if we want more than one magic number.  


================
Comment at: lib/Object/WasmObjectFile.cpp:22
+  Error Err;
+  std::unique_ptr<WasmObjectFile> R;
+
----------------
dschuff wrote:
> What does `R` stand for? return value?
I guess so.   Cargo culted.  I gave it a more meaningful name.


================
Comment at: lib/Object/WasmObjectFile.cpp:46
+
+void readSection(WasmSection &Section, const uint8_t *&Ptr, const uint8_t *Start) {
+  Section.Offset = Ptr - Start;
----------------
dschuff wrote:
> Does this function have any way to fail?
Yes, it would be nice to not read past EOF in the cast of malformed file.  I'm not sure how to do this since we don't konw how many bytes will be consumbed by decodeULEB128 but I'll figure it out.  Added a TODO for now.


https://reviews.llvm.org/D25590