[Review, -cxx-abi microsoft] Implement proper handling of virtual destructors (single-inheritance case)

[Timur Iskhodzhanov]

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