On Fri, Jan 27, 2017 at 2:55 PM, David Blaikie <dblai...@gmail.com> wrote: > > > On Fri, Jan 27, 2017 at 2:51 PM Mehdi Amini <mehdi.am...@apple.com> wrote: >> >> On Jan 27, 2017, at 2:43 PM, David Blaikie <dblai...@gmail.com> wrote: >> >> >> >> On Fri, Jan 27, 2017 at 2:13 PM Mehdi Amini <mehdi.am...@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 >>> > <revi...@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> >>> > _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits