[PATCH] D41315: [WebAssembly] Output functions individually

Mon Dec 18 05:46:16 PST 2017

ncw accepted this revision.
ncw added a comment.
This revision is now accepted and ready to land.

Looks good, this definitely goes in the right direction and will be useful.

================
Comment at: wasm/InputFiles.cpp:147

 static void CopyRelocationsRange(std::vector<WasmRelocation> &To,
                                  ArrayRef<WasmRelocation> From, size_t Start,
----------------
sbc100 wrote:
> ruiu wrote:
> > This is not a review comment to lld, but why don't you emit one relocation section for each function? IIUC, this code scans an entire relocation section each time you instantiate a function, so that's O(n*m) where n and m are the number of relocations and functions.
> You are right we probably can/should do better here.
> 
> We tried to design the relocation system such that relocations would apply to the basic primitive of the object format: the wasm section.
> 
> However, the only two sections that contain relocations so far are the `code` and `data` sections, and we are not trying to split these up into GC'able chunks (a single `segment` for data and a single `function` for code).    Going back to revist the relocations in the object file it would probably make a lot more sense at this point to one relocation section per function and per data segments.  i.e. reloc.CODE.1 .. reloc.CODE.n rather than simply `reloc.CODE`.   I've opened bug in our tool-conventions repo to discuss changing this: https://github.com/WebAssembly/tool-conventions/issues/32
The linking tools convention shouldn't //need// to be changed - there's nothing stopping us just using a map when we read in the relocations in LLD. There's minimal advantage to storing the list of relocations per-section in the object file, which doesn't have to reflect whether LLD wants to use a map/vector for lookups.

Either way, I agree this commit doesn't need to be held up on that.

Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D41315