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

[Mehdi Amini via cfe-commits]

> On Jan 27, 2017, at 2:43 PM, David Blaikie <dblaikie at gmail.com> wrote:
> 
> 
> 
> On Fri, Jan 27, 2017 at 2:13 PM Mehdi Amini <mehdi.amini at apple.com <mailto: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”.

I agree with “don’t hold the boat”, but I usually understand by that “don’t delay the release or change the schedule”, do you mean something else?

It is likely that the bar should get higher as we get closer to the release, I don’t know if the branching point should be considered the point where we stop taking changes that aren’t regression fixes?


> 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 <https://llvm.org/svn/llvm-project/cfe/trunk@207451>, http://reviews.llvm.org/D3515 <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.

I’m (very) biased by the way we (Apple) organize our release: we will cherry-pick any bug fixes (and other “quality improvements”), regression or not, for months after the branching point.

So up to the release manager :)

— 
Mehdi


> > On Jan 27, 2017, at 2:04 PM, David Blaikie via Phabricator <reviews at reviews.llvm.org <mailto: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 <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/3f0ce3d2/attachment-0001.html>