r212233 - Fix linkage bug that miscompiled variable templates instantiated from similarly named local types. In essence, this bug ensures that the x<Foo> specialization in function foo() defined as follows differs between two distinct translation units.

[Richard Smith]

On Wed, Jul 2, 2014 at 4:25 PM, Eric Christopher <echristo at gmail.com> wrote:

> It looks like this went out for review but was never reviewed? If so,
> why did you commit?


Agreed, if a patch doesn't meet the "obvious" bar and needs pre-commit
review, it shouldn't be committed until that review happens. In this case,
I think the patch actually does meet the "obvious" bar: it's a trivial
extension of our existing mechanism for other flavors of templates to also
cover variable templates.

One additional post-commit review comment (in addition to the request for a
testcase): perhaps it's time to refactor this code to have common code for
handling any kind of template, rather than having distinct code for each
different flavor of template.

On Wed, Jul 2, 2014 at 4:08 PM, Larisse Voufo <lvoufo at google.com> wrote:
> > Author: lvoufo
> > Date: Wed Jul  2 18:08:34 2014
> > New Revision: 212233
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=212233&view=rev
> > Log:
> > Fix linkage bug that miscompiled variable templates instantiated from
> similarly named local types. In essence, this bug ensures that the x<Foo>
> specialization in function foo() defined as follows differs between two
> distinct translation units.
> >     static int &foo() {
> >        struct Foo { };
> >        return x<Foo>;
> >     }
> >
> > Modified:
> >     cfe/trunk/lib/AST/Decl.cpp
> >
> > Modified: cfe/trunk/lib/AST/Decl.cpp
> > URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/Decl.cpp?rev=212233&r1=212232&r2=212233&view=diff
> >
> ==============================================================================
> > --- cfe/trunk/lib/AST/Decl.cpp (original)
> > +++ cfe/trunk/lib/AST/Decl.cpp Wed Jul  2 18:08:34 2014
> > @@ -471,6 +471,58 @@ static void mergeTemplateLV(LinkageInfo
> >    LV.mergeExternalVisibility(argsLV);
> >  }
> >
> > +/// Should we consider visibility associated with the template
> > +/// arguments and parameters of the given variable template
> > +/// specialization? As usual, follow class template specialization
> > +/// logic up to initialization.
> > +static bool shouldConsiderTemplateVisibility(
> > +                                 const VarTemplateSpecializationDecl
> *spec,
> > +                                 LVComputationKind computation) {
> > +  // Include visibility from the template parameters and arguments
> > +  // only if this is not an explicit instantiation or specialization
> > +  // with direct explicit visibility (and note that implicit
> > +  // instantiations won't have a direct attribute).
> > +  if (!spec->isExplicitInstantiationOrSpecialization())
> > +    return true;
> > +
> > +  // An explicit variable specialization is an independent, top-level
> > +  // declaration.  As such, if it has an explicit visibility attribute,
> > +  // that must directly express the user's intent, and we should honor
> > +  // it.
> > +  if (spec->isExplicitSpecialization() &&
> > +      hasExplicitVisibilityAlready(computation))
> > +    return false;
> > +
> > +  return !hasDirectVisibilityAttribute(spec, computation);
> > +}
> > +
> > +/// Merge in template-related linkage and visibility for the given
> > +/// variable template specialization. As usual, follow class template
> > +/// specialization logic up to initialization.
> > +static void mergeTemplateLV(LinkageInfo &LV,
> > +                            const VarTemplateSpecializationDecl *spec,
> > +                            LVComputationKind computation) {
> > +  bool considerVisibility = shouldConsiderTemplateVisibility(spec,
> computation);
> > +
> > +  // Merge information from the template parameters, but ignore
> > +  // visibility if we're only considering template arguments.
> > +
> > +  VarTemplateDecl *temp = spec->getSpecializedTemplate();
> > +  LinkageInfo tempLV =
> > +    getLVForTemplateParameterList(temp->getTemplateParameters(),
> computation);
> > +  LV.mergeMaybeWithVisibility(tempLV,
> > +           considerVisibility &&
> !hasExplicitVisibilityAlready(computation));
> > +
> > +  // Merge information from the template arguments.  We ignore
> > +  // template-argument visibility if we've got an explicit
> > +  // instantiation with a visibility attribute.
> > +  const TemplateArgumentList &templateArgs = spec->getTemplateArgs();
> > +  LinkageInfo argsLV = getLVForTemplateArgumentList(templateArgs,
> computation);
> > +  if (considerVisibility)
> > +    LV.mergeVisibility(argsLV);
> > +  LV.mergeExternalVisibility(argsLV);
> > +}
> > +
> >  static bool useInlineVisibilityHidden(const NamedDecl *D) {
> >    // FIXME: we should warn if -fvisibility-inlines-hidden is used with
> c.
> >    const LangOptions &Opts = D->getASTContext().getLangOpts();
> > @@ -662,6 +714,14 @@ static LinkageInfo getLVForNamespaceScop
> >      // C99 6.2.2p4 and propagating the visibility attribute, so we
> don't have
> >      // to do it here.
> >
> > +    // As per function and class template specializations (below),
> > +    // consider LV for the template and template arguments.  We're at
> file
> > +    // scope, so we do not need to worry about nested specializations.
> > +    if (const VarTemplateSpecializationDecl *spec
> > +              = dyn_cast<VarTemplateSpecializationDecl>(Var)) {
> > +      mergeTemplateLV(LV, spec, computation);
> > +    }
> > +
> >    //     - a function, unless it has internal linkage; or
> >    } else if (const FunctionDecl *Function = dyn_cast<FunctionDecl>(D)) {
> >      // In theory, we can modify the function's LV by the LV of its
> > @@ -869,6 +929,10 @@ static LinkageInfo getLVForClassMember(c
> >
> >    // Static data members.
> >    } else if (const VarDecl *VD = dyn_cast<VarDecl>(D)) {
> > +    if (const VarTemplateSpecializationDecl *spec
> > +        = dyn_cast<VarTemplateSpecializationDecl>(VD))
> > +      mergeTemplateLV(LV, spec, computation);
> > +
> >      // Modify the variable's linkage by its type, but ignore the
> >      // type's visibility unless it's a definition.
> >      LinkageInfo typeLV = getLVForType(*VD->getType(), computation);
> >
> >
> > _______________________________________________
> > cfe-commits mailing list
> > cfe-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140702/1327a9b0/attachment.html>