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

[Reid Kleckner via Phabricator via llvm-commits]

rnk added a subscriber: smeenai.
rnk added a comment.

In D56516#1353365 <https://reviews.llvm.org/D56516#1353365>, @morehouse wrote:

> In D56516#1353359 <https://reviews.llvm.org/D56516#1353359>, @bd1976llvm wrote:
>
> > - For ELF targets, this fix is functionally correct, but it will have the effect of retaining the section content for the weak symbols in the output, potentially slowing the link and making the output larger (unless you use -gc-sections).
>
>
> Yes, but this is definitely preferred over breaking weak/strong semantics for our users.


Yes, I think I agree with your conclusions for ELF. With weak vs. strong linkage, the strong symbol can appear later, and it will win. With comdats, the first symbol will win, regardless of strength. Putting such symbols into a comdat isn't going to work.

However, it looks like this code still creates sancov metadata for such a weak function, even though it might not be linked in. Is that a problem? Will it create relocations to discarded data? Is the !associated metadata enough to make it get discarded when the strong user symbol overrides the weak one?

>> Hopefully, someone who is an expert on the other file formats might comment (Reid?).
> 
> @rnk:  How do weak/strong symbols in COMDATs interact on COFF?

Well, GCC has some extensions for implementing weak symbols in COFF. I forget if we implement them. Maybe @smeenai remembers, I think Facebook wanted weak symbols for mingw. The Visual C++ linker doesn't implement weak symbols. I think this change will be fine with my suggestion to keep applying comdats when the function is weak_odr. We should also test all the relevant linkages, like plain `linkonce` and `weak_odr`.



================
Comment at: llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp:580
 
-  if (TargetTriple.supportsCOMDAT())
+  if (TargetTriple.supportsCOMDAT() && !F.hasWeakLinkage())
     if (auto Comdat =
----------------
`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.


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

https://reviews.llvm.org/D56516