[cfe-commits] [PATCH] Constructor and Destructor mangling, along with improved Calling Convention handling for Windows.

[Ricky Taylor]

Not sure how, but that last message had an old version of the patch.
I've attached the correct version to this message.

-- Ricky Taylor

On Fri, Jan 21, 2011 at 11:16 AM, Ricky Taylor <rickytaylor26 at gmail.com> wrote:
> On Fri, Jan 21, 2011 at 6:05 AM, Charles Davis <cdavis at mymail.mines.edu> wrote:
>>
>> On 1/20/11 10:12 PM, Ricky Taylor wrote:
>> > Hello,
>> >
>> > I've created a patch for clang, that addresses some issues with the
>> > Microsoft ABI:
>> >
>> > - Constructors and Destructors are now mangled correctly
>> > - 'Repeated Namespaces' are mangled correctly
>> > - The correct default calling convention is used for class/struct methods
>> > - __declspec syntax is now used, (and presumably the GCC equivalent).
>> > - Attributes are passed down the chain correctly, (they were not being
>> > copied into the Declarator).
>> >
>> > The patch is attached below,
>> Hi,
>>
>> I wrote the Microsoft C++ Mangler initially. Comments below:
>>
>> Index: include/clang/Sema/DeclSpec.h
>> ===================================================================
>> --- include/clang/Sema/DeclSpec.h       (revision 123612)
>> +++ include/clang/Sema/DeclSpec.h       (working copy)
>> @@ -1206,7 +1206,7 @@
>>   Declarator(const DeclSpec &ds, TheContext C)
>>     : DS(ds), Range(ds.getSourceRange()), Context(C),
>>       InvalidType(DS.getTypeSpecType() == DeclSpec::TST_error),
>> -      GroupingParens(false), AttrList(0), AsmLabel(0),
>> +         GroupingParens(false), AttrList(ds.getAttributes().getList()),
>> AsmLabel(0),
>>       InlineParamsUsed(false), Extension(false) {
>>   }
>>
>> I think there's a reason they weren't being passed down. I'm not sure
>> about Microsoft attributes, but IIRC GNU attributes on the decl-spec
>> belong to the type, not the declarator. Maybe we should pass down only
>> Microsoft (i.e. __declspec) attributes.
>>
>> Also, you used tab characters instead of spaces. Please don't do that.
>> Lots of people use a tab width of 8, so the indentation looks off.
>>
>> Index: lib/AST/ASTContext.cpp
>> ===================================================================
>> --- lib/AST/ASTContext.cpp      (revision 123612)
>> +++ lib/AST/ASTContext.cpp      (working copy)
>> @@ -885,6 +885,14 @@
>>   return toCharUnitsFromBits(getTypeAlign(T));
>>  }
>>
>> +/// getDefaultCallingConvForDeclarator - Determines the default calling
>> convention
>> +/// for a Declarator before attributes are applied.
>> +CallingConv ASTContext::getDefaultCallingConvForDeclarator(Declarator
>> &D, Scope *S) const
>> +{
>> +       // Pass through to ABI.
>> +       return ABI->getDefaultCallingConvForDeclarator(D, S);
>> +}
>> +
>>
>> You could avoid having to add another method to the C++ ABI object by
>> calling the getDefaultMethodCallingConv() method on the ABI object. Then
>> this becomes:
>>
>>  CallingConv getDefaultCallingConvForDeclarator(Declarator &D, Scope
>> *S) const {
>>          if(<the declarator refers to an instance method>)
>>                  return ABI->getDefaultMethodCallingConv();
>>
>>          return CC_C;
>>  }
>>
>>
>> Index: lib/AST/CXXABI.h
>> ===================================================================
>> --- lib/AST/CXXABI.h    (revision 123612)
>> +++ lib/AST/CXXABI.h    (working copy)
>> @@ -21,6 +21,8 @@
>>
>>  class ASTContext;
>>  class MemberPointerType;
>> +class Declarator;
>> +class Scope;
>>
>>  /// Implements C++ ABI-specific semantic analysis functions.
>>  class CXXABI {
>> @@ -30,10 +32,13 @@
>>   /// Returns the size of a member pointer in multiples of the target
>>   /// pointer size.
>>   virtual unsigned getMemberPointerSize(const MemberPointerType *MPT)
>> const = 0;
>> -
>> +
>>   /// Returns the default calling convention for C++ methods.
>>   virtual CallingConv getDefaultMethodCallConv() const = 0;
>>
>> +  /// Returns the default calling convention for a particular Declarator.
>> +  virtual CallingConv getDefaultCallingConvForDeclarator(Declarator &D,
>> Scope *S) const = 0;
>> +
>>
>> You shouldn't have to do this (see above).
>>
>> Index: lib/AST/MicrosoftCXXABI.cpp
>> ===================================================================
>> --- lib/AST/MicrosoftCXXABI.cpp (revision 123612)
>> +++ lib/AST/MicrosoftCXXABI.cpp (working copy)
>> @@ -17,6 +17,8 @@
>>  #include "clang/AST/DeclCXX.h"
>>  #include "clang/AST/RecordLayout.h"
>>  #include "clang/AST/Type.h"
>> +#include "clang/Sema/DeclSpec.h"
>> +#include "clang/Sema/Scope.h"
>>  #include "clang/Basic/TargetInfo.h"
>>
>>  using namespace clang;
>> @@ -36,6 +38,13 @@
>>       return CC_C;
>>   }
>>
>> +  CallingConv getDefaultCallingConvForDeclarator(Declarator &D, Scope
>> *S) const {
>> +         if(S && (S->getFlags() & Scope::ClassScope))
>> +                 return CC_X86ThisCall;
>> +
>> +         return CC_C;
>> +  }
>> +
>>
>> You should probably make sure that the declarator doesn't have the
>> static storage class. For static methods, the default calling convention
>> is still __cdecl. (Of course, this is after moving the implementation
>> over to ASTContext.)
>>
>> Index: lib/AST/MicrosoftMangle.cpp
>> ===================================================================
>> --- lib/AST/MicrosoftMangle.cpp (revision 123612)
>> +++ lib/AST/MicrosoftMangle.cpp (working copy)
>> @@ -30,6 +30,7 @@
>>  class MicrosoftCXXNameMangler {
>>   MangleContext &Context;
>>   llvm::raw_svector_ostream Out;
>> +  std::vector<llvm::StringRef> Shortcuts;
>>
>> You might consider using a SmallVector here (there can only be 10
>> replacements anyway).
>>
>>   ASTContext &getASTContext() const { return Context.getASTContext(); }
>>
>> @@ -336,11 +337,13 @@
>>       break;
>>
>>     case DeclarationName::CXXConstructorName:
>> -      assert(false && "Can't mangle constructors yet!");
>> +         // <constructor> ::= ?0 class-name
>> +         Out << "?0";
>>       break;
>>
>> Tabs.
>>
>>     case DeclarationName::CXXDestructorName:
>> -      assert(false && "Can't mangle destructors yet!");
>> +         // <destructor> ::= ?1
>> +      Out << "?1";
>>       break;
>>
>> Tabs again. (The second line is okay, though.)
>>
>>     case DeclarationName::CXXConversionFunctionName:
>> @@ -382,7 +385,8 @@
>>   if (const BlockDecl *BD = dyn_cast<BlockDecl>(DC)) {
>>     llvm::SmallString<64> Name;
>>     Context.mangleBlock(BD, Name);
>> -    Out << Name << '@';
>> +
>> +       Out << Name << '@';
>>     return manglePostfix(DC->getParent(), NoFunction);
>>   }
>>
>> Please avoid gratuitous whitespace changes.
>>
>> @@ -525,8 +529,34 @@
>>  }
>>
>>  void MicrosoftCXXNameMangler::mangleSourceName(const IdentifierInfo *II) {
>> -  // <source name> ::= <identifier> @
>> -  Out << II->getName() << '@';
>> +       // <source name> ::= <identifier> @
>> +
>> +       llvm::StringRef Name = II->getName();
>> +
>> +       // Microsoft assign the first 10 names to ascii shortcuts
>> +       // '0'-'9'. They are then used instead of the name@ syntax.
>> +       bool newName = true;
>> +       for(size_t i = 0; i < Shortcuts.size(); i++)
>> +       {
>> +               if(Shortcuts[i].compare(Name) == 0)
>> +               {
>> +                       Out << std::string(1, '0' + i);
>> +                       newName = false;
>> +                       break;
>> +               }
>> +       }
>> +
>> +       // If we weren't in the shortcut list, then we will have to
>> +       // write the whole name out.
>> +       if(newName)
>> +       {
>> +               Out << Name << '@';
>> +
>> +               if(Shortcuts.size() < 10)
>> +               {
>> +                       Shortcuts.push_back(Name);
>> +               }
>> +       }
>>  }
>>
>> You should probably add a test for this to
>> <clang>/test/CodeGenCXX/mangle-ms.cpp.
>>
>> Oh, and tabs again.
>>
>> The whole patch file had mixed line endings (CR-LF and LF). Try to avoid
>> that.
>>
>> Did you make sure to run Clang's tests with your patch, in case you
>> introduced a regression?
>>
>> Other than those, LGTM! Doug, John, your thoughts?
>>
>> Chip
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
> Hello again,
>
> Thanks for your help. I had totally mistaken how the AttributeLists
> were handled, and have corrected that.
>
> I've checked this new patch against clang's tests, and there appear to
> be no regressions.
>
> One thing to note is that I've added a bit definition to ExtInfo, this
> is so that the compiler doesn't print the calling convention of every
> function, only ones you've explicitly declared in your code. This
> could be changed, but it would require that a few of the tests are
> re-written too. (As they depend on the 'plain' function signature
> being printed out.)
>
> I've also gone through and sorted out all the white-space, sorry about that.
>
> Please find attached the improved patch.
>
> -- Ricky Taylor
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: MSVCCallingConvCTorsDTors2.patch
Type: application/octet-stream
Size: 7716 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110121/7d1f79b7/attachment.obj>