[cfe-commits] [patch] Fix PR6353 and some related issues

[Douglas Gregor]

On Feb 28, 2010, at 10:15 AM, Rafael Espindola wrote:

> The attached patch adds asserts to check that if we are doing codegen
> for a implicit constructor, destructor or copy assignment, then we
> should have marked it as used in Sema. The patch also fixes all cases
> I found in a linux x86 bootstrap. The bootstrap fails when linking opt
> and clang, but it also fails without the test.

That's a great assert for an important invariant. Thanks for working on this!

> One part of the patch I am not very happy with is the change to
> SemaExpr.  It is there to match the logic in  EmitBaseInitializer. The
> problem is that it is always marking the destructor as used. We should
> do so only if it is actually being used in a base initializer and
> exceptions are enabled. Suggestions on how to do so are welcome.


Sema::BuildBaseInitializer is where we perform type-checking for base initializers; I suggest putting this logic there. I also feel like we should always mark the destructor (even when exceptions are disabled), because -fno-exceptions should not change the behavior of the front end for non-exception constructs.

Your patch is definitely an improvement, although we haven't seen the last of this issue. I have a few comments about the patch specifically, but let's step back and determine precisely the semantics we want w.r.t. when vtables will be emitted. I think this is it:

A vtable for a class type C is *used* when we see:
	- A constructor definition for C [1]
	- A destructor definition for C [1]
	- A use of a virtual function in C
	- A typeid expression on an lvalue expression of type C
	- (Did I miss something?)

When a vtable is used somewhere in the program, it naturally has to be defined somewhere in the program. So...

For a non-template class C:
	- If C has a key function, the vtable is emitted in the translation unit containing the definition of the key function [1]
	- If C does not have a key function, the vtable is emitted (with weak linkage) in every translation unit containing a use of the vtable

Class template specializations are more complicated, because we need to make sure that all of the virtual functions in the class template specialization get instantiated wherever the vtable is emitted. Let C<X> be a class template specialization.

First, the easy case. An explicit class template specialization (e.g., template<> class C<X> { ... };) is treated exactly the same way as a non-template class. We don't need to say any more about those.

Otherwise, C<X> is an instantiation. If C<X> has a key function, then the vtable is emitted (and all of the virtual functions instantiated) when the key function is defined. Note that the definition of the key function may come from either an instantiation or from an explicit specialization, e.g.,

	template<class T> struct X {
		virtual void foo(); // key function
		virtual void bar(); // not key function
	};

	template<> void X<int>::foo(); // specialization of key function

	void f(X<int> *xi, X<float> *xf) { 
		xi->bar();  // use of the vtable; key function is an explicit specialization so we don't emit vtable or instantiate virtuals until we see the definition of that explicit specialization
		xf->bar(); // use of vtable; key function will be instantiated, as will all virtual functions, and vtable emitted
	}

If C<X> does not have a key function, then the vtable is emitted (and all of the virtual functions instantiated) wherever the vtable is used.

When emitting a vtable, we need to instantiate all of the virtual functions in C<X> . The C++ standard says we can do this whenever we want, but, practically speaking, we need to follow GCC and delay instantiation until the end of the translation unit.

With that in mind, some comments on the patch. In Sema::MaybeMarkVirtualMembersReferenced:

+  switch (MD->getParent()->getTemplateSpecializationKind()) {
+  case TSK_Undeclared:
+  case TSK_ExplicitSpecialization:
+    // Classes that aren't instantiations of templates don't need their
+    // virtual methods marked until we see the definition of the key 
+    // function.
+    break;

Good, this treats explicit specializations the same way as non-templates.

+  case TSK_ImplicitInstantiation:
+    // This is a constructor of a class template; mark all of the virtual
+    // members as referenced to ensure that they get instantiatied.
+    if (isa<CXXConstructorDecl>(MD))
+      return true;

I think we also want to check isa<CXXDestructorDecl>(MD) here, since a destructor for a class with virtual bases needs to use the vtable.

This case falls through to the following cases; please add a "// Fall through".

+  case TSK_ExplicitInstantiationDeclaration:
+  case TSK_ExplicitInstantiationDefinition:
+    // This is method of a explicit instantiation; mark all of the virtual
+    // members as referenced to ensure that they get instantiatied.
+    return true;

These don't seem right. If there is an explicit instantiation declaration, e.g.,

	extern template class C<X>;

then that suppresses the instantiation of any non-inline functions in C<X>. It should also suppress the vtable, so I would have expected:

  case TSK_ExplicitInstantiationDeclaration:
	return false;

since an explicit instantiation declaration (which typically occurs in a header) is generally followed by an explicit instantiation definition, e.g.,

	template class C<X>;

where it makes sense to always emit the vtable (with weak linkage).

The rest of this function looks good.

+void Sema::MaybeMarkVirtualMembersReferenced(SourceLocation Loc,
+                                             CXXMethodDecl *MD) {
+  CXXRecordDecl *RD = MD->getParent();
+
   // We will need to mark all of the virtual members as referenced to build the
   // vtable.
-  ClassesWithUnmarkedVirtualMembers.push_back(std::make_pair(RD, Loc));
+  // We actually call MarkVirtualMembersReferenced instead of adding to
+  // ClassesWithUnmarkedVirtualMembers because this marking is needed by
+  // codegen that will happend before we finish parsing the file.
+  if (needsVtable(MD, Context))
+    MarkVirtualMembersReferenced(Loc, RD);
 }

Okay. 

@@ -7221,7 +7221,13 @@ void Sema::MarkDeclarationReferenced(SourceLocation Loc, Decl *D) {
       if (!Constructor->isUsed())
         DefineImplicitCopyConstructor(Loc, Constructor, TypeQuals);
     }
-    
+
+    CXXRecordDecl* Parent = Constructor->getParent();
+    if (true /*Exceptions*/ && !Parent->hasTrivialDestructor()) {
+      CXXDestructorDecl *DD = Parent->getDestructor(Context);
+      MarkDeclarationReferenced (Loc, DD);
+    }
+

This is the bit that probably belongs in BuildBaseInitializer.

Within MarkDeclarationReferenced, we seem to be missing logic that would instantiate all of the virtual functions when we refer to a virtual function, e.g., in the call xf->bar(); from my example above, do we get the key function instantiated?

--- a/lib/Sema/SemaExprCXX.cpp
+++ b/lib/Sema/SemaExprCXX.cpp
@@ -426,6 +426,16 @@ bool Sema::CheckCXXThrowOperand(SourceLocation ThrowLoc, Expr *&E) {
                                             : diag::err_throw_incomplete)
                               << E->getSourceRange()))
       return true;
+
+    const RecordType *RT = Ty->getAs<RecordType>();
+    if (!RT)
+      return false;
+
+    const CXXRecordDecl *RD = cast<CXXRecordDecl>(RT->getDecl());
+    if (RD->hasTrivialCopyConstructor())
+      return false;
+    CXXConstructorDecl *CopyCtor = RD->getCopyConstructor(Context, 0);
+    MarkDeclarationReferenced(ThrowLoc, CopyCtor);
   }

I guess this is a hack because we don't copy-initialize a temporary when throwing? It's okay to keep this with a big FIXME, but the right answer here would be to fix

  // FIXME: Construct a temporary here.

at the bottom of that routine.


	- Doug

[1] We note both usage and definition of the vtable via the calls to MaybeMarkVirtualMembersReferenced in Sema::ActOnFinishFunctionBody and Sema::MarkDeclarationReferenced.