Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

clang emits U symbol while gcc emits W one #6725

Closed
llvmbot opened this issue Feb 19, 2010 · 27 comments
Closed

clang emits U symbol while gcc emits W one #6725

llvmbot opened this issue Feb 19, 2010 · 27 comments
Labels
bugzilla c++

Comments

@llvmbot
Copy link
Collaborator

@llvmbot llvmbot commented Feb 19, 2010

Bugzilla Link 6353
Resolution FIXED
Resolved on Mar 06, 2010 14:07
Version unspecified
OS Linux
Blocks #6253
Attachments test case, test case
Reporter LLVM Bugzilla Contributor
CC @asl,@DougGregor,@rjmccall

Extended Description

when compiling the attached test case I am getting:

pes delta$ clang++ -O0 -c foo.cpp && nm foo.o | grep Rb_tree

             U _ZN8_Rb_treeIPK4Decl4pairIKS2_9CharUnitsE10_Select1stIS6_E4lessIS2_E9allocatorIS6_EED1Ev

pes delta$ g++ -O0 -c foo.cpp && nm foo.o | grep Rb_tree
0000000000000000 W _ZN8_Rb_treeIPK4Decl4pairIKS2_9CharUnitsE10_Select1stIS6_E4lessIS2_E9allocatorIS6_EED1Ev

@llvmbot
Copy link
Collaborator Author

@llvmbot llvmbot commented Feb 22, 2010

testcase
reduced it a bit more

@llvmbot
Copy link
Collaborator Author

@llvmbot llvmbot commented Feb 22, 2010

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

@llvmbot
Copy link
Collaborator Author

@llvmbot llvmbot commented Feb 22, 2010

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.

@llvmbot
Copy link
Collaborator Author

@llvmbot llvmbot commented Feb 22, 2010

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

@llvmbot
Copy link
Collaborator Author

@llvmbot llvmbot commented Feb 22, 2010

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.

@DougGregor
Copy link
Contributor

@DougGregor DougGregor commented Feb 22, 2010

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

@llvmbot
Copy link
Collaborator Author

@llvmbot llvmbot commented Feb 22, 2010

@llvmbot
Copy link
Collaborator Author

@llvmbot llvmbot commented Feb 22, 2010

@llvmbot
Copy link
Collaborator Author

@llvmbot llvmbot commented Feb 24, 2010

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

@llvmbot
Copy link
Collaborator Author

@llvmbot llvmbot commented Feb 25, 2010

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?

@llvmbot
Copy link
Collaborator Author

@llvmbot llvmbot commented Feb 27, 2010

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

@rjmccall
Copy link
Contributor

@rjmccall rjmccall commented Feb 28, 2010

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.

@llvmbot
Copy link
Collaborator Author

@llvmbot llvmbot commented Feb 28, 2010

The following code still fails the new asserts:

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

@llvmbot
Copy link
Collaborator Author

@llvmbot llvmbot commented Feb 28, 2010

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

@llvmbot
Copy link
Collaborator Author

@llvmbot llvmbot commented Feb 28, 2010

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());
}

@llvmbot
Copy link
Collaborator Author

@llvmbot llvmbot commented Feb 28, 2010

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

@llvmbot
Copy link
Collaborator Author

@llvmbot llvmbot commented Feb 28, 2010

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();
}

@llvmbot
Copy link
Collaborator Author

@llvmbot llvmbot commented Feb 28, 2010

patch

@llvmbot
Copy link
Collaborator Author

@llvmbot llvmbot commented Mar 2, 2010

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();
}

@llvmbot
Copy link
Collaborator Author

@llvmbot llvmbot commented Mar 2, 2010

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

@llvmbot
Copy link
Collaborator Author

@llvmbot llvmbot commented Mar 2, 2010

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

@llvmbot
Copy link
Collaborator Author

@llvmbot llvmbot commented Mar 2, 2010

Reduced testcase:

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

@DougGregor
Copy link
Contributor

@DougGregor DougGregor commented Mar 2, 2010

Super-important for 2.7

@llvmbot
Copy link
Collaborator Author

@llvmbot llvmbot commented Mar 2, 2010

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;

@llvmbot
Copy link
Collaborator Author

@llvmbot llvmbot commented Mar 2, 2010

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

@llvmbot
Copy link
Collaborator Author

@llvmbot llvmbot commented Mar 2, 2010

Trying to mark as fixed ....

@llvmbot
Copy link
Collaborator Author

@llvmbot llvmbot commented Mar 2, 2010

Trying again...

@llvmbot llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 3, 2021
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugzilla c++
Projects
None yet
Development

No branches or pull requests

3 participants