[PATCH] D43940: [WebAssembly] Reorder reloc sections to come after symtab and validate section order

[Sam Clegg via Phabricator via llvm-commits]

sbc100 added inline comments.


================
Comment at: include/llvm/Object/Wasm.h:264
-  uint32_t DataSection = 0;
-  uint32_t GlobalSection = 0;
 };
----------------
How about "#define INVALID_SECTION UINT32_MAX".   Less magical, and less C-style casting seems better.

Actually it looks like these invalid values are not checked anywhere.  This part of the change seems unrelated.


================
Comment at: lib/Object/WasmObjectFile.cpp:208
+// for custom sections whose order we don't know.
+enum : unsigned {
+  // The order of standard sections is precisely given by the spec.
----------------
This seems like it might be overkill to me.  My git feeling is that we are only trying to encode a few rules here, but I could be wrong. 


================
Comment at: lib/Object/WasmObjectFile.cpp:257
+
+static Error checkCodeSection(WasmObjectFile *Object) {
+  if (Object->functionTypes().size() != Object->functions().size())
----------------
Wouldn't these make more sense as private methods?  Aren't you basically passing `this` there?


================
Comment at: lib/Object/WasmObjectFile.cpp:329
+  if ((Err = checkCodeSection(this)))
+    return;
 }
----------------
No point in this conditional.


================
Comment at: lib/Object/WasmObjectFile.cpp:683
                                          const uint8_t *Ptr, const uint8_t *End) {
-  Sec.Name = readString(Ptr);
+  Ptr = Sec.Name.bytes_end();
   if (Sec.Name == "name") {
----------------
This seems rather nasty.   What can't the check for Name length go here?


================
Comment at: lib/Object/WasmObjectFile.cpp:778
+                                            object_error::parse_failed);
+    FunctionTypes.push_back(Type);
   }
----------------
Make this a separate change?  `[WebAssembly] Validate function signatures in object files`


Repository:
  rL LLVM

https://reviews.llvm.org/D43940