[PATCH] D44543: [AsmPrinter] Emit .weak directive for weak linkage on COFF

Thu Mar 26 05:23:15 PDT 2020

mstorsjo added a comment.

In D44543#1942495 <https://reviews.llvm.org/D44543#1942495>, @rnk wrote:

> In D44543#1922223 <https://reviews.llvm.org/D44543#1922223>, @mstorsjo wrote:
>
> > @rnk - what do you think about the two alternative solutions I commented above?
>
>
> The second change seems simpler, but maybe we should adjust the linkonce predicate name. As I understand it, both solutions will preserve the behavior of existing linkonce* and weak* linkages if a comdat is present, which it pretty much always is unless the user is using the `__attribute__((weak))` extension.
>
> It seems to me like the hasLinkOnceDirective predicate isn't really helping us here. I dug into the history, and I see that back around 2010, it controlled the emission of `.linkonce discard`. It got moved to section emission in rG76a07580a <https://reviews.llvm.org/rG76a07580ad0603c933889b0e5bacd40016cb0f3d>. Then, later, when we made all the object file sections use the standard, non-suffixed names, we folded that into the `.section` directive and stopped using the `.linkonce` directive entirely. So, it seems like maybe the MCAsmInfo feature that we really want here should read something like "avoidWeakIfComdat" or "shouldWeakComdatSymbolsBeExternal" or "isWeakComdatExternal" or something like that. shouldUpgradeWeakComdatToExternal...

Thanks for looking into this! That explains the current situation well, and I agree that sounds like a good way of moving forward, not holding on to old mismatched abstractions needlessly.

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

https://reviews.llvm.org/D44543