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

[Charles Davis]

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