[PATCH] D42105: [WebAssembly] Symbol changes #4: export relocatable, LLD only

[Nicholas Wilson via Phabricator via llvm-commits]

ncw added inline comments.


================
Comment at: wasm/Writer.cpp:602
+  bool ExportHidden = Config->EmitRelocs;
+  std::set<std::string> UsedNames;
+  auto BudgeName = [&](const Symbol *Sym) {
----------------
sbc100 wrote:
> ncw wrote:
> > sbc100 wrote:
> > > ncw wrote:
> > > > sbc100 wrote:
> > > > > ncw wrote:
> > > > > > I realise that `std::set<std::string>` isn't very idiomatic in LLVM, but you can why I chose it. I couldn't think of another way of doing it though that didn't require more code doing explicit buffer management.
> > > > > > 
> > > > > > Suggestions welcome - otherwise it's hopefully "good enough" for now, especially as it's short-lived and only used for exported functions (which I'd expect to be a small number compared to the overall output size).
> > > > > I think `llvm::DenseSet<StringRef>` is used elsewhere in lld and probably makes sense here.  Or llvm::StringSet?  I'm not sure what the tradeoffs are between those two.
> > > > `DenseSet<StringRef>` and `StringSet` both assume that the string itself is owned externally. But the "budged" names can't be stored just as a StringRef, something needs to actually own the buffer to the string itself, hence using `std::string`. And you can't use a StringRef to refer to things in a `vector<std::string>` either (can you?) because you have to careful mutating the vector not to reallocate the strings and invalidate any StringRefs (I suppose we can rely on move ctors these days).
> > > > 
> > > > Nuisance. I don't suppose it's good style to use `make<std::string>` or similar magic to allocate strings?
> > > > 
> > > > I could maybe see about doing some more explicit buffer management to allow use of an `llvm::StringSet`.
> > > In that case std::set seems reasonable to me.
> > Aha! With some more searching, I've found the idiomatic LLVM solution.
> > 
> > It's called `llvm::Saver`, a global instance of `llvm::StringSaver` that lets you get a safe/stable StringRef for a fresh string.
> > 
> > Updated now to use that.
> Nice!
> 
> I'm not familiar with StringSaver, but would it make sense to use a saver that has the same lifetime as UsedNames?  i.e. a function scoped saver? 
A function-scoped one wouldn't work. The `StringSet<> UsedNames` is function-local, but the StringRefs that are returned get put into the WasmExportEntrys in `ExportedSymbols`, so are used outside this function.


================
Comment at: wasm/Writer.cpp:390
+  std::vector<std::pair<StringRef, uint32_t>> SymbolInfo;
+  for (const WasmExportEntry &E : ExportedSymbols) {
+    uint32_t Flags =
----------------
sbc100 wrote:
> Don't we want to iterate through all symbols here, not just the exported ones?   This can be a followup change I think.
Oh! For "defined" symbols, we can't iterate through the non-exported ones (any functions not exported don't correspond to a Symbol).

But yes, the imports need to be included too. Oops.


================
Comment at: wasm/Writer.cpp:600
   for (Symbol *Sym : Symtab->getSymbols()) {
-    if (!Sym->isUndefined() || Sym->isWeak())
+    if (!Sym->isUndefined() || (Sym->isWeak() && !Config->EmitRelocs))
       continue;
----------------
sbc100 wrote:
> I think this is unrelated?  At least all the tests still pass without this line.
It is needed eventually; Patch #5 will fail without it. This is similar to patch #4b (D42237). Maybe I split this badly and it should have gone in with that...

I'll pull this out into another patch.


Repository:
  rL LLVM

https://reviews.llvm.org/D42105