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 :) - Dave > > — > Mehdi > > > > 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
