[cfe-commits] patch for llvm.org/pr8207

[Douglas Gregor]

On Sep 27, 2010, at 12:49 PM, Nico Weber wrote:

> And now with a more portable test, courtesy of Benjamin Kramer. Only
> the CHECK lines changed.

@@ -1549,6 +1549,13 @@
         InstantiateClass(PointOfInstantiation, Record, Pattern,
                          TemplateArgs,
                          TSK);
+      } else {
+        //if (TSK == TSK_ExplicitInstantiationDefinition &&
+            //Record->getTemplateSpecializationKind() ==
+                //TSK_ExplicitInstantiationDeclaration) {
+          //Record->setTemplateSpecializationKind(TSK);
+          //MarkVTableUsed(PointOfInstantiation, Record, true);
+        //}
       }
       
       Pattern = cast_or_null<CXXRecordDecl>(Record->getDefinition());

I'm guessing that you didn't mean to comment these out :)

Property changes on: test/CodeGenCXX/template-inner-struct-visibility-hidden.cpp
___________________________________________________________________
Added: svn:eol-style
   + LF

That's a little strange... I would have expected "native", or no properties at all.

With those tweak, looks good; please go ahead and commit!

	- Doug

> On Sun, Sep 26, 2010 at 3:14 PM, Nico Weber <thakis at chromium.org> wrote:
>> Updated the patch with a slightly beefier test case. The actual code
>> change is the same as in the previous patch.
>> 
>> Please review.
>> 
>> Nico
>> 
>> On Sat, Sep 25, 2010 at 11:29 PM, Nico Weber <thakis at chromium.org> wrote:
>>> Hi,
>>> 
>>> the attached patch fixes llvm.org/pr8207 (and potentially more). The
>>> reason the bug happens is the following code in CodeGenModule:
>>> 
>>> LangOptions::VisibilityMode
>>> CodeGenModule::getDeclVisibilityMode(const Decl *D) const {
>>>  // ...
>>> 
>>>  if (getLangOptions().CPlusPlus) {
>>>    // Entities subject to an explicit instantiation declaration get default
>>>    // visibility.
>>>    if (const FunctionDecl *Function = dyn_cast<FunctionDecl>(D)) {
>>>      // ...
>>>    } else if (const ClassTemplateSpecializationDecl *ClassSpec
>>>                              = dyn_cast<ClassTemplateSpecializationDecl>(D)) {
>>>      if (ClassSpec->getSpecializationKind()
>>>                                        == TSK_ExplicitInstantiationDeclaration)
>>>        return LangOptions::Default;
>>>    } else if (const CXXRecordDecl *Record = dyn_cast<CXXRecordDecl>(D)) {
>>>      if (Record->getTemplateSpecializationKind()
>>>                                        == TSK_ExplicitInstantiationDeclaration)
>>>        return LangOptions::Default;
>>>    } else if (const VarDecl *Var = dyn_cast<VarDecl>(D)) {
>>>      // ...
>>>    }
>>> 
>>>    // ...
>>>  }
>>> 
>>>  // If this decl is contained in a class, it should have the same visibility
>>>  // as the parent class.
>>>  if (const DeclContext *DC = D->getDeclContext())
>>>    if (DC->isRecord())
>>>      return getDeclVisibilityMode(cast<Decl>(DC));
>>> 
>>>  return getLangOptions().getVisibilityMode();
>>> }
>>> 
>>> 
>>> When a template specialization declaration is overridden with a
>>> template specialization definition, the template specialization kind
>>> of inner structs was not updated. Hence, when the function above
>>> checked the visibility of the inner struct's function, it would look
>>> in the parent class (== the inner struct) because of the code at the
>>> end of the function, and then would return LangOptions::Default,
>>> because the inner struct would still have specialization kind
>>> TSK_ExplicitInstantiationDeclaration.
>>> 
>>> The fix is to correctly re-set the specialization kind of inner
>>> structs on the TSK_ExplicitInstantiationDeclaration ->
>>> TSK_ExplicitInstantiationDefinition transition on the base class.
>>> 
>>> Nico
>>> 
>>> 
>>> ps: I'm not sure if the call to MarkVTableUsed is needed, but it's
>>> called further up in that file when
>>> TSK_ExplicitInstantiationDefinition is set too.
>>> 
>> 
> <clang-visibility.patch>_______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits