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

[Nicholas Wilson via Phabricator via llvm-commits]

ncw added inline comments.


================
Comment at: test/wasm/Inputs/locals-duplicate1.ll:1
+; Will collide: local with public
+ at colliding_global1 = internal default global i32 0, align 4
----------------
sbc100 wrote:
> Are there llvm terms you can use here rather than `local` and `public`?  I think llvm this `internal` and `external` linkage, right?
Yes, you're right, maybe those are better terms. "XXX_LOCAL" is the name of the flag in the MC source code corresponding to the IR keyword "internal". "public" is because I couldn't think of the correct opposite!


================
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
----------------
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.


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


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D42105