Created attachment 4256 [details]
test case

Created attachment 4268 [details]
test case

a little more reduced test case...

Created attachment 4272 [details]
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.

(In reply to comment #7)
> 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

Created attachment 4284 [details]
add asserts to the codegen

Created attachment 4285 [details]
testcase. With the asserts in place the testcase can be a bit smaller.

Created attachment 4299 [details]
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?

Created attachment 4330 [details]
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 DataType> class opt : public Option {
  virtual bool handleOccurrence() {
  }
};
template class opt<unsigned>;

Created attachment 4331 [details]
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());
}

Created attachment 4332 [details]
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();
}

Created attachment 4335 [details]
patch

Created attachment 4353 [details]
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

Created attachment 4360 [details]
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 DataType> class opt : public Option {                                                                                                                                                     
virtual bool handleOccurrence() {                                                                                                                                                                         
}                                                                                                                                                                                                         
};                                                                                                                                                                                                        
__extension__ extern template class opt<unsigned>;                                                                                                                                                        
template class opt<unsigned>;

Super-important for 2.7

Created attachment 4363 [details]
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<unsigned>;                                                                                                                                                        
template class opt<unsigned>;

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