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

Douglas Gregor dgregor at apple.com
Mon Jan 24 10:01:02 PST 2011


The Microsoft mangling changes look good, but please supply a test case that tests the new manglings. 

The calling-convention changes are going to need a bit more discussion. The main change is this:

Index: include/clang/AST/Type.h
===================================================================
--- include/clang/AST/Type.h	(revision 123972)
+++ include/clang/AST/Type.h	(working copy)
@@ -2205,11 +2205,12 @@
     // you'll need to adjust both the Bits field below and
     // Type::FunctionTypeBitfields.
 
-    //   |  CC  |noreturn|regparm
-    //   |0 .. 2|   3    |4 ..  6
+    //   |  CC  |noreturn|regparm|CC-default
+    //   |0 .. 2|   3    |4 ..  6|    7
     enum { CallConvMask = 0x7 };
     enum { NoReturnMask = 0x8 };
-    enum { RegParmMask = ~(CallConvMask | NoReturnMask),
+    enum { DefaultCallConvMask = 0x80 };
+    enum { RegParmMask = ~(CallConvMask | NoReturnMask | DefaultCallConvMask),
            RegParmOffset = 4 };
 
     unsigned char Bits;
@@ -2232,8 +2233,9 @@
     ExtInfo() : Bits(0) {}
 
     bool getNoReturn() const { return Bits & NoReturnMask; }
-    unsigned getRegParm() const { return Bits >> RegParmOffset; }
+    unsigned getRegParm() const { return (Bits & RegParmMask) >> RegParmOffset; }
     CallingConv getCC() const { return CallingConv(Bits & CallConvMask); }
+    bool isCCDefault() const { return (getCC() == CC_Default) || (Bits & DefaultCallConvMask); }
 
     bool operator==(ExtInfo Other) const {
       return Bits == Other.Bits;
@@ -2257,9 +2259,13 @@
     }
 
     ExtInfo withCallingConv(CallingConv cc) const {
-      return ExtInfo((Bits & ~CallConvMask) | (unsigned) cc);
+      return ExtInfo((Bits & ~(CallConvMask|DefaultCallConvMask)) | (unsigned) cc);
     }
-
+    
+    ExtInfo withDefaultCallingConv(CallingConv cc) const {
+      return ExtInfo((Bits & ~CallConvMask) | (unsigned) cc | DefaultCallConvMask);
+    }
+    
     void Profile(llvm::FoldingSetNodeID &ID) const {
       ID.AddInteger(Bits);
     }

which partially separates the notion of "is this the default calling convention"? from the description of the calling convention. However, it's not a complete separation, which I find a bit awkward. I could imagine removing CC_Default entirely, then adding the "the function type was written without an explicit calling convention" bit. When the function type itself is created, we pick the calling convention based on context (?) if it's default.

Also, a change like this really needs testing. Here's a fun case:

  typedef void func_type(int);

  struct X {
    func_type f;
  };

presumably, "f" gets the thiscall calling convention because of its context, but then decltype(X::f) is different from func_type.

	- Doug

On Jan 21, 2011, at 7:06 AM, Ricky Taylor wrote:

> 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
>> 
> <MSVCCallingConvCTorsDTors2.patch>_______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits





More information about the cfe-commits mailing list