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

[Sam Clegg via Phabricator via llvm-commits]

sbc100 added a comment.

Cool.  Didn't expect that to be so simple :)

I'll submit the cosmetic changes separately.



================
Comment at: wasm/InputFunction.cpp:18
+
+InputFunction InputFunction::Discarded(nullptr, nullptr, nullptr);
----------------
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.


================
Comment at: wasm/InputSegment.h:66
 
+  static InputSegment Discarded;
+
----------------
We don't need this if we are not going to do the same for globals, right?


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D41390