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

[Nicholas Wilson via Phabricator via llvm-commits]

ncw added inline comments.


================
Comment at: include/llvm/Object/Wasm.h:264
-  uint32_t DataSection = 0;
-  uint32_t GlobalSection = 0;
 };
----------------
sbc100 wrote:
> 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.
It came from a previous version of the diff - where I was using the CodeSection member to test whether the code section had been seen. I left it in because I thought that using an invalid (rather than valid) value as the default was better/safer. And I didn't split it out into another revision because I felt that would be overkill... :(

Happy to change to UINT32_MAX, I just used `-1` because that's what the line above already does for StartFunction.


================
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.
----------------
sbc100 wrote:
> 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. 
Quite a few of the sections have ordering constraints... I started with a couple of hardcoded ones, then when it got to around 4/5 I thought it was worth just encoding the rules from the spec. There's some boilerplate to give each section an "order" number, but the actual logic to check is pretty simple, just a check that the order numbers increase from section to section.

It's cheap to implement and understand, and means you don't need to worry about what might go wrong with home-grown checks. You just get a nice guaranteed section order, and that's safe.


================
Comment at: lib/Object/WasmObjectFile.cpp:257
+
+static Error checkCodeSection(WasmObjectFile *Object) {
+  if (Object->functionTypes().size() != Object->functions().size())
----------------
sbc100 wrote:
> Wouldn't these make more sense as private methods?  Aren't you basically passing `this` there?
OK, I can make these methods, it's probably a bit cleaner.


================
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") {
----------------
sbc100 wrote:
> This seems rather nasty.   What can't the check for Name length go here?
Because we use the Name in checkSectionOrder, which has to happen before we actually parse the section (since the order check is precisely there to make sure that sections have their prerequisites before being parsed).

You're right it's not lovely to jump to the string end. I did try changing it so that the "Content" of the custom section doesn't include its name, which works fine actually but it seemed not quite "right" in terms of what aligning the section's Contents with the Wasm contents.


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


Repository:
  rL LLVM

https://reviews.llvm.org/D43940