[cfe-commits] [PATCH] Constructor and Destructor mangling, along with improved Calling Convention handling for Windows.
Ricky Taylor
rickytaylor26 at gmail.com
Fri Jan 21 03:16:31 PST 2011
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: 4292 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110121/02a8270f/attachment.obj>
More information about the cfe-commits
mailing list