[Review, -cxx-abi microsoft] Implement proper handling of virtual destructors (single-inheritance case)
Timur Iskhodzhanov
timurrrr at google.com
Tue Jan 29 11:15:50 PST 2013
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?..
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.
>> @@ -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
>> @@ -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.
> Other than these minor points, LGTM overall.
Thanks for the review!
Waiting for John's too.
> 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
More information about the cfe-commits
mailing list