search

WebAssembly / tool-conventions

Conventions supporting interoperatibility between tools working with WebAssembly.
Artistic License 2.0
302 stars 67 forks source link

Reloc target should probably be required to be the same as reloc entry #115

Open aardappel opened 5 years ago

aardappel commented 5 years ago

We currently don't require the 5-byte patchable ULEB in code and elsewhere to contain anything since all information will be overwritten by the linker based on the reloc entry. By convention we put an index there that matches the current file, but this is not required.

Tools like wasm-validate ignore the linking section however, and will report validation errors if the ULEB is always 0.

I propose that either:

  1. We state that a patchable ULEB that does not correspond to its reloc is invalid, and maybe even enforce so in LLD or other consumers.
  2. If we really don't care what is in here, we should make tools like wasm-validate always take the linking section into account and ignore the ULEB.
  3. Don't store the value twice, i.e. remove it from the reloc entry :)

I'm guessing 1. is most practical.

Also noticed that at least for some paths, LLVM is currently hard-coding a zero for these ULEBs, I may look into fixing that: https://github.com/llvm-mirror/llvm/blob/master/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCCodeEmitter.cpp#L166

@sbc100 @binji

binji commented 5 years ago

I like option 1 because I don't have to do anything :-) More than that though, option 2 would be kind of nasty to implement, since it will require reading the linking section, then going back and updating values based on the binary offsets. But since the data's already been read into memory, it would have to go back and read it again after patching the binary manually. Other tools may not have this limitation, however.

sbc100 commented 5 years ago

Hmm.. my understanding is that we already do (1). lld will check that relocation targets and warn if they don't match: https://github.com/llvm/llvm-project/blob/4ac0b9be230596e24e439109f2d23ea3dd81ebfd/lld/wasm/InputChunks.cpp#L47

When llvm generates the object file it patches the values in here: https://github.com/llvm/llvm-project/blob/4ac0b9be230596e24e439109f2d23ea3dd81ebfd/llvm/lib/MC/WasmObjectWriter.cpp#L667

Maybe I'm missing something?

binji commented 5 years ago

It seems that it already does for object files, but we were seeing issues when generating assembly, then using the assembler to generate an object file.

aardappel commented 5 years ago

I was under the impression that is was mere convention. https://github.com/WebAssembly/tool-conventions/blob/master/Linking.md doesn't seem to specify it?

Rather than a warning it should probably be an error. Warnings easily get ignored, and we're pretty precise on what is legal and what is an error elsewhere in wasm too.

As for applyRelocations.. that is somehow not working for me. I'll investigate.

aardappel commented 5 years ago

Ok, ignore the reloc entry being 0, that was an unrelated problem.

However, my original request that this invariant be enforced more rigorously (and as many tools as possible) still stands.