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

[David Blaikie via cfe-commits]

Reid:

Richard and I played around with this with MSVC on godbolt
<http://gcc.godbolt.org/#g:!((g:!((g:!((h:codeEditor,i:(j:1,options:(colouriseAsm:'0',compileOnChange:'0'),source:'int+g(const+int+%26v)%0A%23if+DEF%0A%7B+return+v%3B+%7D%0A%23else%0A%3B%0A%23endif%0A%0Atemplate%3Ctypename+T%3E%0Ainline+int+f()+%7B%0A++static+const+int+n+%3D+0%3B%0A++return+g(n)%3B%0A%7D%0A%0A%23if+DEF%0Atemplate+int+f%3Cint%3E()%3B%0A%23else%0Aextern+template+int+f%3Cint%3E()%3B%0Aint+k()+%7B+return+f%3Cint%3E()%3B+%7D%0A%23endif'),l:'5',n:'1',o:'C%2B%2B+source+%231',t:'0')),k:43.27735263873981,l:'4',n:'0',o:'',s:0,t:'0'),(g:!((g:!((h:compiler,i:(compiler:cl19_64,filters:(),options:/Ox),l:'5',n:'0',o:'%231+with+x86-64+CL+19+RC',t:'0'),(h:output,i:(compiler:1,editor:1),l:'5',n:'0',o:'%231+with+x86-64+CL+19+RC',t:'0')),k:56.722647361260194,l:'4',m:50,n:'0',o:'',s:0,t:'0'),(g:!((h:compiler,i:(compiler:cl19_64,filters:(),options:'/Ox+/DDEF'),l:'5',n:'0',o:'%231+with+x86-64+CL+19+RC',t:'0')),l:'4',m:50,n:'0',o:'',s:0,t:'0')),k:56.722647361260194,l:'3',n:'0',o:'',t:'0')),l:'2',n:'0',o:'',t:'0')),version:4>
but
weren't able to get MSVC to inline an template under an explicit
instantiation declaration, so while we can see that the definition side
doesn't provide a definition for the static local if it optimizes it away
(so that seems consistent with the Itanium ABI and GCC's behavior here - no
weak_odr definition on the definition side) we couldn't check the
declaration/available_externally side to see if it would produce its own
linkonce_odr-type definition.

If you knew of any ways to check, or ways to verify Clang's behavior here
is consistent (it looks like it will be with this patch), that might be
handy.

- Dave

On Fri, Jan 27, 2017 at 2:04 PM David Blaikie via Phabricator via
cfe-commits <cfe-commits at lists.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.
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20170127/d6ccfbea/attachment-0001.html>