[PATCH] D44149: [WebAssembly] Use StringSaver to retain ownership of ctor function body. NFC

[Nicholas Wilson via Phabricator via llvm-commits]

ncw added inline comments.


================
Comment at: wasm/InputChunks.h:159
   StringRef getName() const override { return Name; }
+  StringRef getComdat() const override { return StringRef(); }
 
----------------
sbc100 wrote:
> Why add getComdat() here?  If we really want this to be NFC maybe this part should be separate.
Oh ah - that slipped in. It is NFC because getComdat() currently isn't called (it actually crashes currently), so it can't be changing any existing behaviour that doesn't crash. That chunk just got pulled in "under the radar" because it's next the lines I picked into this PR, sorry. It's necessary for the synthetic undefined stubs.

I could put it in another commit, but it kind of fits in with the "fixes to SyntheticFunction" theme.


================
Comment at: wasm/Writer.cpp:877
   SyntheticFunction *F = make<SyntheticFunction>(
-      *Sig, std::move(FunctionBody), WasmSym::CallCtors->getName());
+      *Sig, toArrayRef(Saver.save(FunctionBody)),
+      WasmSym::CallCtors->getName());
----------------
ruiu wrote:
> I don't think the amount of data we need to copy is large, so it is presumably OK, and I'm not obsessed with micro-optimization, but here we make a copy of std::string here and then immediately discard the original string at the end of the scope. So this makes a redundant copy of a string.
> 
> There is a way to avoid unnecessary copying. If you allocate std::string by `make<std::string>()`, then its body will have the same lifetime as the entire linking process.
True, we could save on that copy. But if we were micro-optimising, `make<std::string>` would be a bit wasteful too, since  `make<>` would have to create a whole new slab/arena for the std::string (and register the arena so the string's dtor is called).

Best of all we'd maybe do `BAlloc.Allocate<char>(5 + 5 + InitFunctions.size()*6 + 1)` and then write directly into the interned buffer right away, with no copies at all.

But I agree it's not worth trying to optimise; this commit is just trying to make the SyntheticFunction ctor more generally useful.

Thanks for the review!


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D44149