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.
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
