[PATCH] D56516: [SanitizerCoverage] Don't create comdat for weak functions.

[Peter Collingbourne via Phabricator via llvm-commits]

pcc accepted this revision.
pcc added inline comments.


================
Comment at: llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp:580
 
-  if (TargetTriple.supportsCOMDAT())
+  if (TargetTriple.supportsCOMDAT() && !F.hasWeakLinkage())
     if (auto Comdat =
----------------
rnk wrote:
> `hasWeakLinkage` will include `weak_odr` linkages, which are often used for explicit template instantiations and dllexport functions. In those cases, the user has promised that there is only "one definition", so there cannot be some overriding strong definition. I think you really want to say `hasWeakAnyLinkage()` or `isInterposable()` here instead. That will affect these linkages:
>     case WeakAnyLinkage:
>     case LinkOnceAnyLinkage:
>     case CommonLinkage:
>     case ExternalWeakLinkage:
>       return true;
> 
> external_weak can only appear on declarations, common can only be on globals, and a linkonce function would be a discardable-but-weak function. There just aren't many use cases for it. I'm not sure when we'd generate it, but I think you might want to avoid putting such things in comdats. I guess the use case would be to emit an overridable aligned operator new, which could be discarded if unused after optimization. You probably don't want to put such things in a comdat either.
> 
> So, in conclusion, isInterposable seems like the right thing to use here.
Agreed.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56516/new/

https://reviews.llvm.org/D56516