[PATCH] D41390: Wasm LLD: Don't write out discarded function symbols

[Nicholas Wilson via Phabricator via llvm-commits]

ncw added a comment.

Sorry also for the lengthy responses!

It would be nice to merge as much as possible before the holidays, or there'll be lots of rebasing pain when we come back from holidays and find patches that are weeks out of date.

Having said it's not important, I //might// have time today to add some detection for discarding the segment containing a weak global.



================
Comment at: wasm/InputFunction.cpp:18
+
+InputFunction InputFunction::Discarded(nullptr, nullptr, nullptr);
----------------
sbc100 wrote:
> ncw wrote:
> > sbc100 wrote:
> > > Seems a shame to add a new source file just for this.
> > > 
> > > I've not looked at how the other linker do this, but could we not just have a "Discarded" flag on these objects instead of the these magic statics?
> > The file is doing no harm. We might need the file anyway eventually. Let's not let the length of the file affect any other decisions.
> > 
> > Good question about Discarded, I was initially surprised too.
> > 
> > The "Discarded" implementation I'm using comes directly from the ELF linker, to keep Rui happy. It's actually quite sensible: if you have a flag, there are two problems. a) you have to check the flag everywhere, and b) you have to keep on checking it forever whenever someone adds new code, ie it's a pain for ever.
> > 
> > Redirecting to a dummy Function like "Discarded" is much safer. Since all the fields in Discarded are null, there's no chance at all that you'll use it by mistake. You only need to check against Discarded in a couple of places, everywhere else can reference the InputFuction/InputSegment without worrying - you'll pass on its nullness if you reference the members.
> > 
> > Keeping it similar to ELF-LLD is probably a good enough reason to use the idiom for now, in any case.
> I don't think we should aim for complete parity with ELF.  For example there is no `::Discarded` thing in the COFF version.  Not to say that we shouldn't go this route but I think we need more of a reason.
> 
> ELF seems to use both `Discarded`.. the the `Live` flag.   I don't see why we could just mark this section as `Live = false`, it would need to be checked in exactly the same number of places.   I also  think `Sec->isLive()` is much more readable than `Sec != &InputSection::Discarded)`.  But obviously ELF chose to have both these method so maybe we will run into the same reason they did.
I think COFF and ELF use `isLive` for GC - so we shouldn't use that here for something else. In LLD I think "discarded" means "we've thrown it away, we'll never use it", and sections are initially not discarded, but can be discarded, and that's a definitive decision for the section. "Live" means the section is reachable, and it's initially false and set to true as reachability is calculated during the GC phase.

I could change it to `setDiscarded/isDiscarded`, if you'd prefer that, I don't particularly mind. Rui did request we keep variable naming and idioms consistent between the different backends.


================
Comment at: wasm/InputSegment.h:66
 
+  static InputSegment Discarded;
+
----------------
sbc100 wrote:
> We don't need this if we are not going to do the same for globals, right?
We will be doing the same for globals in the Comdat review D40845 (which stacks on top of this one). I haven't implemented segment discarding based on weak globals (as I have for functions), but I have implemented segment discarding based on Comdats.

This will be used in future.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D41390