[WebAssembly] Symbol changes #5: add "symbol index" concept (LLD)
Needs ReviewPublic

Authored by ncw on Thu, Jan 11, 10:45 AM.

Details

Reviewers
sbc100
Summary

Must be committed simultaneously with LLD change, D41954.

Changes

  • See D41954.
  • Changed the stack-pointer.ll to use --relocatable rather than just --emit-relocs. This is because, rather annoyingly, under my new scheme it's not possible to emit the relocs without using --relocatable. You see, the relocs really depend on exporting the symbols, so that the relocs can reference the symbols, but __stack_pointer is the one and only thing we can't export, since it's a mutable global. For the time being (until mutable globals are exportable) I think this is an acceptable limitation on --emit-relocs. I hope that's OK!

Diff Detail

Repository
rLLD LLVM Linker
ncw created this revision.Thu, Jan 11, 10:45 AM
ncw updated this revision to Diff 129658.Fri, Jan 12, 10:32 AM
ncw edited the summary of this revision. (Show Details)
ncw added a reviewer: sbc100.

Updated:

  • Some fixes after rebase
  • Most tests now passing (a couple of bugs still to iron out)

Thankfully there's not too much to change in LLD!

ncw updated this revision to Diff 129732.Fri, Jan 12, 4:45 PM

More updates, fixed all but two tests.

The remaining two tests are rather hard to fix; my changes unfortunately don't play well with "relocatable" output.

Problem #1

The stack-pointer.ll test uses --emit-relocs but not --relocatable. This means that the __stack_pointer symbol is _not_ imported; it's instead defined, but we can't export it (because it's mutable). Thus, the _global_ has an output index, but there is no _symbol_ index for it (so far, behaviour hasn't changed...). What this breaks is that I've made it so that relocs reference the symbol index - so relocs against the stack pointer won't work with --emit-relocs but not --relocatable.

Hmm. Maybe we can just fix the test to use --relocatable :( at least until we can export mutable globals.

Problem #2

The other failing test is init-fini.ll. What causes the problem is that we take the address of a BINDING_LOCAL symbol. When linking with --relocatable, we try and create a relocation against the function and fail. This is because the function isn't exported as a Symbol in the relocatable output.

This can probably be fixed by just exporting Symbols for the locals as well when doing relocatable. We'd need to budge the names to keep them unique, which they won't be if we export the locals from all object files. That would at least make LLD's relocatable output match what Clang outputs.

Conclusion

Linking (relocating) against Symbols rather than functions/globals does make aliases nice and elegant... but breaks some other things.

Another (less good?) solution might be to split the relocations into "FUNCTION_SYMBOL_INDEX_LEB" and "FUNCTION_WASM_INDEX_LEB", where the SYMBOL_INDEX version in outputted by WasmObjectWriter (Clang) and the WASM_INDEX version is outputted by LLD when it's creating relocatable output. The we can cope with both the cases above, at the expense of even more code handling different types of relocations.

ncw updated this revision to Diff 129858.Mon, Jan 15, 7:25 AM

Simple rebase to fix conflicts. The two problems above remain.

ncw updated this revision to Diff 129980.Tue, Jan 16, 9:49 AM
ncw retitled this revision from [WebAssembly] Refactor symbol index handling (LLD) to [WebAssembly] Symbol changes #5: add "symbol index" concept (LLD).
ncw edited the summary of this revision. (Show Details)

Updated: split up the big review into six smaller ones.

The two big "problems" described above are now fixed. Problem #1 (stack pointer incompatible with --emit-relocs is fixed by using --relocatable for that test instead. Problem #2 (taking the address of a local function) is/was fixed in D42105.

ncw added inline comments.Tue, Jan 16, 9:56 AM
test/wasm/archive.ll
32

Updated test expectations: Snuck in an test update here that's not strictly required...

test/wasm/data-layout.ll
54

Updated test expectations: Because the import has been removed (not part of the test assertions, sadly) this index has now gone down by one.

ncw updated this revision to Diff 130187.Wed, Jan 17, 7:50 AM

Updated:

  • Simple rebase to fix conflict with __heap_base change