[Review, -cxx-abi microsoft] Implement proper handling of virtual destructors (single-inheritance case)
Charles Davis
cdavis5x at gmail.com
Thu Jan 31 10:27:51 PST 2013
On Jan 31, 2013, at 9:10 AM, Timur Iskhodzhanov wrote:
> ping
>
> 2013/1/29 Timur Iskhodzhanov <timurrrr at google.com>:
>> 2013/1/29 Charles Davis <cdavis5x at gmail.com>:
>>>
>>> On Jan 29, 2013, at 9:58 AM, Timur Iskhodzhanov wrote:
>>>
>>>> 2013/1/28 Timur Iskhodzhanov <timurrrr at google.com>:
>>>>> Two problems:
>>>>> 1) I was optimistically running only the new tests and didn't notice
>>>>> I've broken one of my own earlier tests in the Clang sources. I'll fix
>>>>> that soon
>>>>>
>>>>> 2) CGM code has changed over the weekend, I'll have to rebase.
>>>>>
>>>>> I'll ping this thread with a new patch when I resolve these.
>>>> Here it is attached.
>>>> Sorry for the delay, the Windows build was broken a few times in the
>>>> interim and also my VS checkout was borked for some reason.
>>>>
>>>> Please note that I had to change the test/CodeGenCXX/arm.cpp test
>>>> because I believe it is wrong :)
>>>> See the other e-mail thread for the details.
>>>
>>>> diff --git lib/AST/MicrosoftMangle.cpp lib/AST/MicrosoftMangle.cpp
>>>> index 0b77ac8..5a68704 100644
>>>> --- lib/AST/MicrosoftMangle.cpp
>>>> +++ lib/AST/MicrosoftMangle.cpp
>>>> @@ -47,10 +47,11 @@ class MicrosoftCXXNameMangler {
>>>>
>>>> public:
>>>> MicrosoftCXXNameMangler(MangleContext &C, raw_ostream &Out_)
>>>> - : Context(C), Out(Out_), UseNameBackReferences(true) { }
>>>> + : Context(C), Out(Out_), UseNameBackReferences(true), DtorType(Dtor_Complete) { }
>>>>
>>>> raw_ostream &getStream() const { return Out; }
>>>>
>>>> + CXXDtorType DtorType; // FIXME: pass as arg?
>>> Yes, it should be an argument. It seems like a simple enough change.
>> Can you please suggest me a simple way to do this?
>> Currently, MicrosoftMangleContext::mangleCXXDtor calls the
>> MicrosoftCXXNameMangler::mangle(const NamedDecl *D).
>> I don't think adding a CXXDtorType parameter to a generic
>> mangle(NamedDecl) is appropriate.
>> I can probably add ::mangle(const CXXDestructorDecl*,CXXDtorType)
>> though but this might make things harder, e.g. what happens when
>> declaring a static variable inside a dtor?..
I meant when you're actually constructing the name mangler.
The local case reenters the mangler instance to mangle the local scopes, so that won't help here. We'll have to set the destructor type before mangling the name if the local scope is a destructor. But let's not worry about that for now. For one thing, I don't know where we're going to get the destructor kind from...
>>
>> If there's no simple way I'd rather postpone with a FIXME.
>>
>>>> void mangle(const NamedDecl *D, StringRef Prefix = "\01?");
>>>> void mangleName(const NamedDecl *ND);
>>>> void mangleFunctionEncoding(const FunctionDecl *FD);
>>>> @@ -483,11 +484,25 @@ MicrosoftCXXNameMangler::mangleUnqualifiedName(const NamedDecl *ND,
>>>> case DeclarationName::CXXConstructorName:
>>>> Out << "?0";
>>>> break;
>>>> -
>>>> +
>>>> case DeclarationName::CXXDestructorName:
>>>> - Out << "?1";
>>>> + switch (DtorType) {
>>>> + case Dtor_Deleting:
>>>> + Out << "?_G";
>>>> + break;
>>>> + case Dtor_Base:
>>>> + // FIXME: base dtors are not supported yet. Even though the current
>>>> + // codegen won't emit base dtors, we may still need to mangle a base
>>>> + // dtor for a base class destruction in case of inheritance.
>>>> + // For now, just mangle a base dtor the same way as a complete dtor.
>>> IIRC the only real difference between a "base" and "complete" destructor in the MS ABI is a boolean flag that gets passed indicating if it's a base or a complete call (cf. 'for base' TODOs in MicrosoftCXXABI.cpp). They really are one and the same otherwise.
>> I hope to investigate the base dtors and multiple inheritance later.
OK.
>>
>>>> @@ -1109,9 +1124,17 @@ void MicrosoftCXXNameMangler::mangleType(const FunctionType *T,
>>>>
>>>> // <return-type> ::= <type>
>>>> // ::= @ # structors (they have no declared return type)
>>>> - if (IsStructor)
>>>> + if (IsStructor) {
>>>> + if (DtorType == Dtor_Deleting) {
>>>> + // The scalar deleting destructor takes an extra int argument.
>>>> + // However, the FunctionType generated has 0 arguments.
>>>> + // FIXME: This is a temporary hack.
>>>> + // Maybe should fix the FunctionType creation instead?
>>> "Maybe"? ;) Seriously, this is a gross hack. I'd prefer to avoid it, but I guess it's OK (with me anyway) for now if you don't want to fix it yet.
>>>> + Out << "PAXI at Z";
>>> Are you sure that's right? That would seem to imply that the return type is void *--which it appears to be in fact, based on the code we're generating, but my impression was that VS mangled all 'structors' return types as "@" because they have no declared return type. The deleting destructors might be different, though. In any case, do VS-generated destructors get mangled this way?
>> See
>> http://llvm.org/bugs/show_bug.cgi?id=15058#c4
Ah, so (scalar) deleting destructors are mangled as void * (int). Fair enough. We should still fix the FunctionType for them, though. Some time... :)
>>
>>>> @@ -1711,6 +1734,7 @@ void MicrosoftMangleContext::mangleCXXDtor(const CXXDestructorDecl *D,
>>>> CXXDtorType Type,
>>>> raw_ostream & Out) {
>>>> MicrosoftCXXNameMangler mangler(*this, Out);
>>>> + mangler.DtorType = Type;
>>> Yeah, it should definitely be an argument.
>>>
>>>> diff --git lib/CodeGen/CGCXX.cpp lib/CodeGen/CGCXX.cpp
>>>> index 43813fe..bcd3f2c 100644
>>>> --- lib/CodeGen/CGCXX.cpp
>>>> +++ lib/CodeGen/CGCXX.cpp
>>>> @@ -240,7 +240,10 @@ void CodeGenModule::EmitCXXDestructors(const CXXDestructorDecl *D) {
>>>>
>>>> // The destructor used for destructing this as a base class; ignores
>>>> // virtual bases.
>>>> - EmitGlobal(GlobalDecl(D, Dtor_Base));
>>>> + if (!IsMicrosoftABI) {
>>>> + // FIXME: base dtors are not supported in Microsoft ABI yet, skip for now.
>>>> + EmitGlobal(GlobalDecl(D, Dtor_Base));
>>>> + }
>>> Like I said earlier, there's no real distinction in the MS ABI.
>>>
>>>> diff --git lib/CodeGen/CGVTables.cpp lib/CodeGen/CGVTables.cpp
>>>> index 6d40219..d28a4e2 100644
>>>> --- lib/CodeGen/CGVTables.cpp
>>>> +++ lib/CodeGen/CGVTables.cpp
>>>> @@ -736,7 +736,10 @@ CodeGenModule::getVTableLinkage(const CXXRecordDecl *RD) {
>>>> return !Context.getLangOpts().AppleKext ?
>>>> llvm::GlobalVariable::LinkOnceODRLinkage :
>>>> llvm::Function::InternalLinkage;
>>>> -
>>>> +
>>>> + if (IsMicrosoftABI) {
>>>> + return llvm::GlobalVariable::WeakODRLinkage;
>>>> + }
>>> Style nit: we prefer no braces if there's only a single statement.
>> OK, will fix before the next round of review or before committing if John LGTMs.
>>
>>>> diff --git lib/CodeGen/CodeGenModule.cpp lib/CodeGen/CodeGenModule.cpp
>>>> index da419dd..5b3dfd7 100644
>>>> --- lib/CodeGen/CodeGenModule.cpp
>>>> +++ lib/CodeGen/CodeGenModule.cpp
>>>> @@ -83,7 +83,8 @@ CodeGenModule::CodeGenModule(ASTContext &C, const CodeGenOptions &CGO,
>>>> BlockDescriptorType(0), GenericBlockLiteralType(0),
>>>> SanitizerBlacklist(CGO.SanitizerBlacklistFile),
>>>> SanOpts(SanitizerBlacklist.isIn(M) ?
>>>> - SanitizerOptions::Disabled : LangOpts.Sanitize) {
>>>> + SanitizerOptions::Disabled : LangOpts.Sanitize),
>>>> + IsMicrosoftABI(C.getTargetInfo().getCXXABI().isMicrosoft()) {
>>>>
>>>> // Initialize the type cache.
>>>> llvm::LLVMContext &LLVMContext = M.getContext();
>>>> diff --git lib/CodeGen/CodeGenModule.h lib/CodeGen/CodeGenModule.h
>>>> index 75c5e93..a511640 100644
>>>> --- lib/CodeGen/CodeGenModule.h
>>>> +++ lib/CodeGen/CodeGenModule.h
>>>> @@ -375,6 +375,8 @@ class CodeGenModule : public CodeGenTypeCache {
>>>>
>>>> const SanitizerOptions &SanOpts;
>>>>
>>>> + bool IsMicrosoftABI;
>>>> +
>>>> /// @}
>>>> public:
>>>> CodeGenModule(ASTContext &C, const CodeGenOptions &CodeGenOpts,
>>> I thought I added the CGCXXABI class to avoid special cases like this. :) Don't get me wrong; for a quick'n'dirty first pass, I guess it's OK (pending John's approval), but I'd really like to see this gone later.
>> True :)
>> I think we should come up with more tests and understanding of what
>> should be done differently between ABIs before moving the code between
>> classes.
That would definitely be helpful. Now that I've gotten VC10's command-line compilers installed and running, I might be able to help you with that.
>>
>>> Other than these minor points, LGTM overall.
>> Thanks for the review!
>> Waiting for John's too.
Good luck!
Chip
>>
>>> Chip
>>>
>>>>
>>>>> 2013/1/25 Timur Iskhodzhanov <timurrrr at google.com>:
>>>>>> Hi John,
>>>>>>
>>>>>> Attached is a patch that allows me to pass all the single-inheritance
>>>>>> vdtor tests I've extracted while building and running Chromium.
>>>>>>
>>>>>> It includes changing the deleting destructor to take an implicit bool
>>>>>> parameter when compiling in the Microsoft ABI.
>>>>>> Can you please take a look?
>>>>>> See also http://llvm.org/bugs/show_bug.cgi?id=15058 for the details.
>>>>>>
>>>>>> The patch contains a couple of FIXMEs, I'd like to defer them for
>>>>>> later analysis if possible.
>>>>>>
>>>>>> Note that I had to adjust the EmitCXXMemberCall interface slightly; I
>>>>>> think generalizing "VTT" to "implicit parameter in general" should be
>>>>>> OK.
>>>>>>
>>>>>> Please tell me if I need to change anything.
>>>>>>
>>>>>> --
>>>>>> Timur Iskhodzhanov,
>>>>>> Google Russia
>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> Timur Iskhodzhanov,
>>>>> Google Russia
>>>> <bug_15058_2.patch>_______________________________________________
>>>> cfe-commits mailing list
>>>> cfe-commits at cs.uiuc.edu
>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>>
>>
>>
>>
>> --
>> Timur Iskhodzhanov,
>> Google Russia
>
>
>
> --
> Timur Iskhodzhanov,
> Google Russia
More information about the cfe-commits
mailing list