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

[Hans Wennborg via cfe-commits]

On Fri, Jan 27, 2017 at 2:55 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
> On Fri, Jan 27, 2017 at 2:51 PM Mehdi Amini <mehdi.amini at apple.com> wrote:
>>
>> 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> 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?
>
>
> *nod* fair point - though usually there's some amount of risk to any change
> & I tend to err on the "unless there's a known/likely need, choose not to
> take that risk". In this case we've all been using this setup for years
> without incident so while it's possibly a problem it seems somewhat
> abstract.
>
>>
>> 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,
>> 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 :)
>
>
> *nod* sure sure - I'm not fussed either way :)

I'll pass on this one since it's not a regression and not a trivial change.

Thanks,
Hans


>>> > 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>
>>>
>