Mangle __unaligned in Itanium ABI

[rogfer01]

Summary

__unaligned is not currently mangled in any way in the Itanium ABI. This causes failures when using -fms-extensions and C++ in targets using Itanium ABI.

As suggested by @rsmith the simplest thing to do here is actually mangle the qualifier as a vendor extension.

As an alternative, we could continue not mangling this qualifier. But if I read the code right, in order to get substitutions correctly, we would have to rebuild some types and stripe the __unaligned qualifier prior any mangling and substitution.

This patch also removes the change done in D31976 and updates its test to the new reality.

[rsmith]

lib/AST/ItaniumMangle.cpp

2329–2333

I don't think this is the right place/way to handle this: given

void f(struct X __unaligned *p, struct X *q) {}

it looks like we'll mangle as _Z1fP1XP1X with this patch, which seems wrong: this should presumably instead be _Z1fP1XS0_.

But regardless, I think the right thing to do is to invent a mangling for __unaligned, since we support overloading on it; the most appropriate mangling would be U11__unaligned, per http://itanium-cxx-abi.github.io/cxx-abi/abi.html#mangling-type. (You'll need to extend mangleQualifiers to emit this and hasMangledSubstitutionQualifiers to ignore it.)

[rogfer01]

lib/AST/ItaniumMangle.cpp

2329–2333

Oh. I see. Thanks for the review.

I'll update this patch and then I'll post another one for the mangling of unaligned.

[rogfer01]

Abandon the old approach which cannot possibly work and just opt to mangle __unaligned.

[aaron.ballman]

test/CodeGenCXX/pr33080.cpp

20

Can we also get a test like struct A * __unaligned * __unaligned *? Also, perhaps a test case involving templates would be good.

[rogfer01]

Thanks @aaron.ballman for the review.

I have extended the test with your suggestions.

[aaron.ballman]

Can you run clang-format over both the test files? Aside from that, looks good to me, but you should wait for @rsmith or @majnemer to sign off before committing.

[rogfer01]

Fix formatting of tests.

Thanks for the review @aaron.ballman !

[rnk]

This was filed as https://bugs.llvm.org/show_bug.cgi?id=33178

Isn't there a space in the mangling for vendor extensions? Can we use that here?

[rsmith]

In D33398#769152, @rnk wrote:

Isn't there a space in the mangling for vendor extensions? Can we use that here?

We are using that here: that's what mangleVendorQualifier does.

lib/AST/ItaniumMangle.cpp

2212

Too much indentation here. Also, the ABI requires the "unordered" vendor qualifiers to be emitted in reverse alphabetical order, so this should be emitted after __weak and __strong but before __autoreleasing.

[rogfer01]

Changelog:

• Fix formatting.
• Emit __unaligned in the right order (as defined by the Itanium ABI) when there are Objective-C++ ARC vendor qualifiers.
• New test for __unaligned and ARC's __weak, __strong and __autorelease

[rogfer01]

lib/AST/ItaniumMangle.cpp

2212

I think you meant after __weak but before __strong and __autoreleasing? Maybe I'm misinterpreting something here.

The current patch emits __weak, then __unaligned and then the remaining ARC ones.

[rsmith]

Looks great, thanks!

lib/AST/ItaniumMangle.cpp

2186

first __weak -> __weak first

2212

The alphabet is hard, apparently :) Yes, thanks!

lib/AST/ItaniumMangle.cpp

void CXXNameMangler::mangleQualifiers(Qualifiers Quals) {
   // The ARC ownership qualifiers start with underscores.
   switch (Quals.getObjCLifetime()) {
   // Objective-C ARC Extension:
   //
   //   <type> ::= U "__strong"
   //   <type> ::= U "__weak"
   //   <type> ::= U "__autoreleasing"
   case Qualifiers::OCL_None:
     break;
     
   case Qualifiers::OCL_Weak:
     mangleVendorQualifier("__weak");
     break;
     
   case Qualifiers::OCL_Strong:
     mangleVendorQualifier("__strong");
     break;
case Qualifiers::OCL_ExplicitNone:
     // better ABI compatibility.
     //
     // It's safe to do this because unqualified 'id' won't show up
     // in any type signatures that need to be mangled.
     break;
   }
 
   // <CV-qualifiers> ::= [r] [V] [K]    # restrict (C99), volatile, const
   if (Quals.hasRestrict())
     Out << 'r';
   if (Quals.hasVolatile())
     Out << 'V';
   if (Quals.hasConst())
     Out << 'K';
 }
 
 void CXXNameMangler::mangleVendorQualifier(StringRef name) {
void CXXNameMangler::mangleType(QualType T) {
   if (quals && isa<ArrayType>(T)) {
     ty = Context.getASTContext().getAsArrayType(T);
     quals = Qualifiers();
 
     // Note that we don't update T: we want to add the
     // substitution at the original type.
   }
 
+  // __unaligned is not currently mangled in any way. This implies that it is
+  // not a relevant qualifier for substitutions (while CVR and maybe others
+  // are). This triggers an assertion when this is the only qualifier and the
+  // unqualified type is a class. So let's remove it preventively here.
+  quals.removeUnaligned();
+
   if (quals) {
     mangleQualifiers(quals);
     // Recurse:  even if the qualified type isn't yet substitutable,
     // the unqualified type might be.
     mangleType(QualType(ty, 0));
   } else {
     switch (ty->getTypeClass()) {
 #define ABSTRACT_TYPE(CLASS, PARENT)

test/CodeGenCXX/pr33080.cpp

+// RUN: %clang_cc1 -triple %itanium_abi_triple -fms-extensions -emit-llvm -o- %s | FileCheck %s
+
+void f(__unaligned struct A*) {}
+
+// CHECK: define void @_Z1fP1A(%struct.A*)