[PATCH] D29233: Fix linkage of static locals in available_externally functions to be DiscardableODR/linkonce_odr

[David Blaikie via cfe-commits]

On Fri, Jan 27, 2017 at 2:13 PM Mehdi Amini <mehdi.amini at apple.com> wrote:

> CC Hans.
>
> This is not a regression (AFAICT), but this is a quality improvement, so
> may be worth considering in the 4.0 branch?
>

Perhaps - I'd generally err on the "if it's not a regression, don't hold
the boat". LLVM's been this way for a while (good question as to how long -
looks like at the latest it was introduced in April 2014 (
https://llvm.org/svn/llvm-project/cfe/trunk@207451,
http://reviews.llvm.org/D3515) & no one's noticed). But no big deal either
way if other's feel like it ought to go in.


>
> —
> Mehdi
>
> > On Jan 27, 2017, at 2:04 PM, David Blaikie via Phabricator <
> reviews at reviews.llvm.org> wrote:
> >
> > dblaikie created this revision.
> >
> > As Mehdi put it, entities should either be
> > available_externally+weak_odr, or linkonce_odr+linkonce_odr. While some
> > functions are emitted a_e/weak, their local variables were emitted
> > a_e/linkonce_odr.
> >
> > While it might be nice to emit them a_e/weak, the Itanium ABI (& best
> > guess at MSVC's behavior as well) requires the local to be
> > linkonce/linkonce.
> >
> >
> > https://reviews.llvm.org/D29233
> >
> > Files:
> >  lib/AST/ASTContext.cpp
> >  test/CodeGenCXX/explicit-instantiation.cpp
> >
> >
> > Index: test/CodeGenCXX/explicit-instantiation.cpp
> > ===================================================================
> > --- test/CodeGenCXX/explicit-instantiation.cpp
> > +++ test/CodeGenCXX/explicit-instantiation.cpp
> > @@ -5,6 +5,9 @@
> > // This check logically is attached to 'template int S<int>::i;' below.
> > // CHECK: @_ZN1SIiE1iE = weak_odr global i32
> >
> > +// This check is logically attached to 'template int
> ExportedStaticLocal::f<int>()' below.
> > +// CHECK-OPT: @_ZZN19ExportedStaticLocal1fIiEEvvE1i = linkonce_odr
> global
> > +
> > template<typename T, typename U, typename Result>
> > struct plus {
> >   Result operator()(const T& t, const U& u) const;
> > @@ -153,3 +156,17 @@
> > template <typename T> void S<T>::g() {}
> > template <typename T> int S<T>::i;
> > template <typename T> void S<T>::S2::h() {}
> > +
> > +namespace ExportedStaticLocal {
> > +void sink(int&);
> > +template <typename T>
> > +inline void f() {
> > +  static int i;
> > +  sink(i);
> > +}
> > +// See the check line at the top of the file.
> > +extern template void f<int>();
> > +void use() {
> > +  f<int>();
> > +}
> > +}
> > Index: lib/AST/ASTContext.cpp
> > ===================================================================
> > --- lib/AST/ASTContext.cpp
> > +++ lib/AST/ASTContext.cpp
> > @@ -8892,22 +8892,27 @@
> >     return GVA_Internal;
> >
> >   if (VD->isStaticLocal()) {
> > -    GVALinkage StaticLocalLinkage = GVA_DiscardableODR;
> >     const DeclContext *LexicalContext = VD->getParentFunctionOrMethod();
> >     while (LexicalContext && !isa<FunctionDecl>(LexicalContext))
> >       LexicalContext = LexicalContext->getLexicalParent();
> >
> > -    // Let the static local variable inherit its linkage from the
> nearest
> > -    // enclosing function.
> > -    if (LexicalContext)
> > -      StaticLocalLinkage =
> > -
> Context.GetGVALinkageForFunction(cast<FunctionDecl>(LexicalContext));
> > -
> > -    // GVA_StrongODR function linkage is stronger than what we need,
> > -    // downgrade to GVA_DiscardableODR.
> > -    // This allows us to discard the variable if we never end up
> needing it.
> > -    return StaticLocalLinkage == GVA_StrongODR ? GVA_DiscardableODR
> > -                                               : StaticLocalLinkage;
> > +    // ObjC Blocks can create local variables that don't have a
> FunctionDecl
> > +    // LexicalContext.
> > +    if (!LexicalContext)
> > +      return GVA_DiscardableODR;
> > +
> > +    // Otherwise, let the static local variable inherit its linkage
> from the
> > +    // nearest enclosing function.
> > +    auto StaticLocalLinkage =
> > +
> Context.GetGVALinkageForFunction(cast<FunctionDecl>(LexicalContext));
> > +
> > +    // Itanium ABI (& MSVC seems to do similarly) requires static
> locals in
> > +    // inline functions to be emitted anywhere they're needed, even if
> the
> > +    // function they are local to is emitted
> StrongODR/AvailableExternally.
> > +    if (StaticLocalLinkage == GVA_StrongODR ||
> > +        StaticLocalLinkage == GVA_AvailableExternally)
> > +      return GVA_DiscardableODR;
> > +    return StaticLocalLinkage;
> >   }
> >
> >   // MSVC treats in-class initialized static data members as definitions.
> >
> >
> > <D29233.86118.patch>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20170127/02a01c82/attachment.html>