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

[Nicholas Wilson via Phabricator via llvm-commits]

ncw planned changes to this revision.
ncw 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);
----------------
sbc100 wrote:
> 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)?
Will do.


================
Comment at: lib/Object/WasmObjectFile.cpp:615
 
+Error WasmObjectFile::validateRelocSection() {
+  for (const WasmSection &Section : Sections) {
----------------
sbc100 wrote:
> 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.
Hmm, that only works if the reloc sections come together at the end of the file.

* CODE must come before DATA
* Symbol table must come after DATA (since otherwise the Data symbol indexes can't be validated as it's read in)
* Relocs have to come at the end of the file therefore

I can make that change - it'll be a big tedious reordering of all the test output, but should be fairly straightforward.

This PR can go on hold then until it's that's done.


================
Comment at: test/ObjectYAML/wasm/code_section.yaml:53
+        Flags:           [  ]
+        Function:        1
 ...
----------------
sbc100 wrote:
> What is this adding the to the test?  Presumably you might want to add new tests to check the failure cases?   
It's adding to the test since the test contains a file with relocations but no symbol table (so it's an invalid file).

You're right, I could add tests for exercising our handling of bad Wasm files... but we currently have very very few tests for bad Wasm files. What's the policy - is it our intention to have tests for all the various cases of illegal Wasm files?

I guess we would ideally, I'll add a test for the new error messages I've put in.


Repository:
  rL LLVM

https://reviews.llvm.org/D43684