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

[Sam Clegg via Phabricator via llvm-commits]

sbc100 added inline comments.


================
Comment at: test/wasm/locals-duplicate.test:1
+; RUN: llc -filetype=obj -mtriple=wasm32-unknown-unknown-wasm %p/Inputs/locals-duplicate1.ll -o %t1.o
+; RUN: llc -filetype=obj -mtriple=wasm32-unknown-unknown-wasm %p/Inputs/locals-duplicate2.ll -o %t2.o
----------------
ncw wrote:
> sbc100 wrote:
> > Can we give these new files better names?   Perhaps `duplicate-symbols-internal.test`.  Locals makes me think of wasm locals, and llvm doesn't use the term local for these symbols either (I think?).
> Well in LLD we have `Symbol::isLocal`. The name you suggested is better though.
Hmm, you are right.  Confusing different nomenclature.    I think its because in ELF and Wasm binary format we use `local` and `global`.    Given that I'm OK with the `local` and `global` here..  but maybe just replace `public` with `global`?


================
Comment at: wasm/Writer.cpp:602
+  bool ExportHidden = Config->EmitRelocs;
+  std::set<std::string> UsedNames;
+  auto BudgeName = [&](const Symbol *Sym) {
----------------
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.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D42105