[PATCH] D43684: [WebAssembly] Add validation to reloc section

[Sam Clegg via Phabricator via llvm-commits]

sbc100 added inline comments.


================
Comment at: include/llvm/Object/Wasm.h:208
+  bool isValidGlobalSymbolIndex(uint32_t Index) const;
+  bool isValidDataSymbolIndex(uint32_t Index) const;
   wasm::WasmFunction &getDefinedFunction(uint32_t Index);
----------------
I find these method names a little confusing.     Perhaps the code will be clearer just to inline them?   (or perhaps drop the final `Index` from the name)?


================
Comment at: lib/Object/WasmObjectFile.cpp:571
   uint32_t RelocCount = readVaruint32(Ptr);
+  uint64_t StartOffset = 0, EndOffset = Section->Content.size();
   while (RelocCount--) {
----------------
I think one variable per line makes this more readable (although I don't see anything in the llvm style it certainly seems to be the defacto style in the code I've been looking at).

Maybe call the first one `LastRelocOffset`?


================
Comment at: lib/Object/WasmObjectFile.cpp:615
 
+Error WasmObjectFile::validateRelocSection() {
+  for (const WasmSection &Section : Sections) {
----------------
Hmm, you can't do this at parse time because the symbol table doesn't exist yet?

In that case i think we need to specify that the reloc sections need to come after the linking metadata section, since they depend on the symbol table.


================
Comment at: test/ObjectYAML/wasm/code_section.yaml:53
+        Flags:           [  ]
+        Function:        1
 ...
----------------
What is this adding the to the test?  Presumably you might want to add new tests to check the failure cases?   


================
Comment at: test/ObjectYAML/wasm/data_section.yaml:35
+        Offset:          0
+        Size:            4
 ...
----------------
Ditto


Repository:
  rL LLVM

https://reviews.llvm.org/D43684