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

[Mehdi Amini via cfe-commits]

CC Hans.

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

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