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

[Charles Davis]

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

> @@ -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?

> @@ -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.

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

Other than these minor points, LGTM overall.

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