[Review, -cxx-abi microsoft] Implement proper handling of virtual destructors (single-inheritance case)
Timur Iskhodzhanov
timurrrr at google.com
Thu Jan 31 08:10:36 PST 2013
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?..
>
> 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
--
Timur Iskhodzhanov,
Google Russia
More information about the cfe-commits
mailing list