testcase
reduced it a bit more

looks like the problem is that ~_Rb_tree() is {Ptr = 0} when CodeGenModule::GetOrCreateLLVMFunction is deciding if it should be emitted. As a consequence FD->isThisDeclarationADefinition() returns false and the destructor is not emitted.

This is true even if there is some real code in ~_Rb_tree().

Looking a bit more it looks like the problem is with the template instantiation. Sema::ActOnFinishFunctionBody is being called on the template of the destructor, but never on the instantiated one. That is why setBody is never called.

Going one level up, the issue is that Sema::MarkDeclarationReferenced is never called on ~_Rb_tree

OK, looks like this needs bigger changes to clang than I can get done tonight. If anyone else is interested in fixing this bug, go for it. I might try again next weekend.

This is what is happening:

We have special code for handing emitting implicit destructors in CodeGenModule::GetOrCreateLLVMFunction. The problem happens when a implicit destructor references an explicit one that is defined in a template. In the testcase the implicit destructor is ~BlockFunction() and the explicit one is ~_Rb_tree().

The problem is that since the special case is on codegen, that is too late to call Sema::MarkDeclarationReferenced. Normally this method is the one that would add the destructor to the PendingImplicitInstantiations list. Being on that list eventually causes InstantiateFunctionDefinition to be called on it and that finally calls setBody.

This is not a problem for non-templates since the body is set early on (no instantiation is required).

Looks like what we need is to replicated the existing logic from CodeGen into Sema. Template instantiation is a form of code gen anyway :-)

Using the attached test as an example, the calls that have to happen are:

• ActOnFinishFunctionBody on the implicit ~BlockFunction
• MarkBaseAndMemberDestructorsReferenced on ~BlockFunction
• MarkDeclarationReferenced on ~_Rb_tree()

Another option is to add a ActOnImplicitConstructorOrDestructor. I don't have enough experience on clang to know which one is better.

OK, looks like this needs bigger changes to clang than I can get done tonight.
If anyone else is interested in fixing this bug, go for it. I might try again
next weekend.

This is what is happening:

We have special code for handing emitting implicit destructors in
CodeGenModule::GetOrCreateLLVMFunction. The problem happens when a implicit
destructor references an explicit one that is defined in a template. In the
testcase the implicit destructor is ~BlockFunction() and the explicit one is
~_Rb_tree().

This seems like a good place to assert() that Sema did its job and marked the implicit destructor as being used.

The problem is that since the special case is on codegen, that is too late to
call Sema::MarkDeclarationReferenced. Normally this method is the one that
would add the destructor to the PendingImplicitInstantiations list. Being on
that list eventually causes InstantiateFunctionDefinition to be called on it
and that finally calls setBody.

Right.

This is not a problem for non-templates since the body is set early on (no
instantiation is required).

Looks like what we need is to replicated the existing logic from CodeGen into
Sema. Template instantiation is a form of code gen anyway :-)

Much of this logic already exists in Sema; we're probably just missing a MarkDeclarationReferenced call where we're implicitly using a destructor.

• Doug

add asserts to the codegen

testcase. With the asserts in place the testcase can be a bit smaller.

patch
The attached patch fixes the reduced tested but is incomplete and introduces new failures in the testsuite.

One testcase that fails the new assert

struct box {
virtual ~box();
};

struct pile_box : public box {
pile_box(box *);
};

pile_box::pile_box(box *pp)
{
}

The issue is that when processing pile_box we just add it to ClassesWithUnmarkedVirtualMembers. We then continue to codegen it, produce the vtable for it and try to codegen ~pile_box() that has not been marked as needed (yet).

Without the assert the code continues to run and eventually ProcessPendingClassesWithUnmarkedVirtualMembers is called and that calls MarkVirtualMembersReferenced which will mark ~pile_box() as used.

Looks like the correct fix is to call MarkVirtualMembersReferenced earlier. What is the reason for delaying it as we do now?

patch
Fixes the issue and the "make test" is OK. Will try to bootstrap clang with it.

Overall looks good; those are great asserts in codegen, thank you.

Please name "needsVtable" "ShouldEmitVtableForCall" or something like that, and have it take a Sema& instead of an ASTContext&, just in case. Also, conventionally the Sema/Context argument comes first, although we're not terribly consistent about that.

The following code still fails the new asserts:

class Option {
virtual ~Option();
};
template class opt : public Option {
virtual bool handleOccurrence() {
}
};
template class opt;

patch
Fixes all the issues mentioned above. Trying a clang bootstrap again.

New reduced testcase that fails with the new asserts:

struct allocator {
allocator(const allocator&);
};
struct TGError {
allocator foo;
TGError(const allocator &message) : foo(message) {
}
};
allocator zed();
TGError foobar() {
throw TGError(zed());
}

patch
Same as before. All know issues fixed, doing a new bootstrap.

And the new reduced testcase is:

struct TypedInit {
virtual ~TypedInit();
};
struct OpInit : public TypedInit {
virtual void resolveBitReference();
};
struct BinOpInit : public OpInit {
virtual void getAsString() const;
};
void foobar() {
new BinOpInit();
}

patch

patch
This one avoids marking a destructor used every time we mask a constructor. Instead it walks the base initializer and marks only those.

The problem with Sema::BuildBaseInitializer is that is is never called in code like

struct TypedInit {
virtual ~TypedInit();
};
struct OpInit : public TypedInit {
virtual void resolveBitReference();
};
struct BinOpInit : public OpInit {
virtual void getAsString() const;
};
void foobar() {
new BinOpInit();
}

the last patch breaks libstdc++ build with:

/data/home/rdivacky/clangbsd/gnu/lib/libstdc++/../../../contrib/libstdc++/include/ext/stdio_sync_filebuf.h:258:34: error:
explicit specialization of 'xsputn' after instantiation
stdio_sync_filebuf<wchar_t>::xsputn(const wchar_t* __s,

I can provide testcase in the evening

patch
This one includes bits from the last review.
"make test" is OK, but I am reducing a clang bootstrap issue with it :-(

Reduced testcase:

class Option {
virtual ~Option() {
}
};
template class opt : public Option {
virtual bool handleOccurrence() {
}
};
extension extern template class opt;
template class opt;

Super-important for 2.7

patch
This is probably a hack, but it fixes the previous failure. Looks like needsVtable is being called only once for

extension extern template class opt;
template class opt;

This testcase was fixed by revision 97589:

$ clang++ -c test.cpp
$ nm test.o | grep _ZN8_Rb_treeIPK4Decl4pairIKS2_9CharUnitsE10_Select1stIS6_E4lessIS2_E9allocatorIS6_EED1Ev
0000000000000000 W _ZN8_Rb_treeIPK4Decl4pairIKS2_9CharUnitsE10_Select1stIS6_E4lessIS2_E9allocatorIS6_EED1Ev

Trying to mark as fixed ....

Trying again...