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

Sam Clegg via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Mar 1 09:09:14 PST 2018

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`



More information about the llvm-commits mailing list