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.

[Eric Christopher]

On Wed, Jul 2, 2014 at 4:44 PM, Richard Smith <richard at metafoo.co.uk> wrote:
> 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.
>

Cool. Mostly that if it goes out for review it should stay out for
review until someone acks it.

-eric

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