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

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo added a reviewer: mstorsjo.
mstorsjo added a comment.
Herald added a project: LLVM.

I keep running into this issue in various places, that `__attribute__((weak))` doesn't work on COFF in clang.

In D44543#1041998 <https://reviews.llvm.org/D44543#1041998>, @espindola wrote:

> In D44543#1039750 <https://reviews.llvm.org/D44543#1039750>, @smeenai wrote:
>
> > @rnk, does that make sense to you?
> >
> > I'm not very familiar with this part of LLVM; I ran into this issue because an internal user needed `__attribute__((weak))` semantics on COFF, and having inline assembly with a manual `.weak` directive for a gigantic mangled C++ name isn't the greatest workaround (though it does work). I'm happy to follow through if there's agreement on how to proceed here though.
> >
> > I do have a question though. `linkonce` doesn't appear to do anything by itself on COFF right now. E.g. if I have
> >
> >   define linkonce void @f() {
> >     ret void
> >   }
> >
> >
> > and run `bin/llc -mtriple=x86_64-windows-msvc` on it, the object file produced is the same both with and without the `linkonce`.
>
>
> This is almost certainly an oversight. I don't think I have ever seen a plain linkonce used in practice, so it was probably never implemented.


If I've understood things correctly, this is the norm, see e.g. D71572 <https://reviews.llvm.org/D71572> for needing to add a comdat to make linkonce behave as intended.

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

> One way that we could make forward progress without rewriting LLVM's linkage is to not apply .weak for `weak_odr` linkages. Then we'd say, `weak_odr` is exactly like `linkonce_odr` except it is non-discardable, pending a cleanup to make discardability a separate bit (probably along with a separate "odr" bit that informs the inliner and constant propagation). `weak` linkage would be reserved for things that truly use `__attribute__((weak))`.


I'm also hesitant to adjusting the generic rules for how this behaves on a whim (even though the current `hasLinkOnceDirective` case essentially is COFF only), but some change like the one above (in one form or another) does seem to be needed. As the current code only checks for `hasLinkOnceDirective`, it does decide that it can use `.globl` as the platform in general should be able to describe linkonce semantics, while it only can describe linkonce semantics if there's a comdat.

One way of handling it, which seems to run fine without regression in a larger set of code, is this:

       if (MAI->hasWeakDefDirective()) {
         // .globl _foo
         OutStreamer->emitSymbolAttribute(GVSym, MCSA_Global);
   
         if (!canBeHidden(GV, *MAI))
           // .weak_definition _foo
           OutStreamer->emitSymbolAttribute(GVSym, MCSA_WeakDefinition); 
         else
           OutStreamer->emitSymbolAttribute(GVSym, MCSA_WeakDefAutoPrivate);
  +    } else if (TT.isOSWindows() && TT.isOSBinFormatCOFF() &&
  +               GlobalValue::isWeakLinkage(Linkage) && !GV->hasComdat()) {
  +      // .weak _foo
  +      OutStreamer->emitSymbolAttribute(GVSym, MCSA_Weak);
       } else if (MAI->hasLinkOnceDirective()) {
         // .globl _foo
         OutStreamer->emitSymbolAttribute(GVSym, MCSA_Global); 
         //NOTE: linkonce is handled by the section the symbol was assigned to.
       } else {
         // .weak _foo
         OutStreamer->emitSymbolAttribute(GVSym, MCSA_Weak);
       }

This retains the distinction between linkonce/linkonce_odr and weak/weak_odr that @smeenai was making so far.

Another way of doing it, with a much smaller diff, would be this:

       if (MAI->hasWeakDefDirective()) {
         // .globl _foo
         OutStreamer->emitSymbolAttribute(GVSym, MCSA_Global);
      
         if (!canBeHidden(GV, *MAI))
           // .weak_definition _foo
           OutStreamer->emitSymbolAttribute(GVSym, MCSA_WeakDefinition);
         else
           OutStreamer->emitSymbolAttribute(GVSym, MCSA_WeakDefAutoPrivate);
  -    } else if (MAI->hasLinkOnceDirective()) {
  +    } else if (MAI->hasLinkOnceDirective() && GV->hasComdat()) {
  +      // hasLinkOnceDirective() is essentially COFF-only, and COFF needs a comdat for expressing the linkonce semantics
         // .globl _foo
         OutStreamer->emitSymbolAttribute(GVSym, MCSA_Global);
         //NOTE: linkonce is handled by the section the symbol was assigned to.
       } else {
         // .weak _foo
         OutStreamer->emitSymbolAttribute(GVSym, MCSA_Weak);
       }

(This one I haven't testrun in the same way yet.)

If @smeenai doesn't have the same need for this one any longer, I could comandeer this revision, to start pestering people to get this finished.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D44543