PR24595: clang-cl fails to compile vswriter.h header from Windows SDK 8.1 in 32 bit mode

[andreybokhanko]

Summary

As described in PR24595, clang-cl fails to compile vswriter.h header from Windows SDK 8.1 in 32 bit mode. This patch fixes this.

More on vswriter.h is here: https://msdn.microsoft.com/en-us/library/aa384627(v=vs.85).aspx

I'm not sure adding a new warning is the right approach -- maybe we should downgrade "err_conflicting_overriding_cc_attributes" to a Warning with DefaultError attribute?

[rnk]

MSVC appears to ignore __stdcall on virtual destructors, and I think the correct fix is for us to do the same.

See this test case:

$ cat t.cpp
struct A { virtual __stdcall ~A(); };
A::~A() {}
$ cl -c t.cpp
Microsoft (R) C/C++ Optimizing Compiler Version 18.00.31101 for x86
Copyright (C) Microsoft Corporation.  All rights reserved.

t.cpp
$ dumpbin /symbols t.obj | grep ~A
00B 00000000 SECT3  notype ()    External     | ??1A@@UAE@XZ (public: virtual __thiscall A::~A(void))

Notice the "__thiscall" part of the symbol name. The assembly also shows that it pulls 'this' from ecx.

[andreybokhanko]

Reid, thanks for looking into this!

As I discovered, MS compiler transforms stdcall to thiscall for all structors (not only virtual ones) in 32 bit mode. In 64 bit mode, it transforms stdcall to cdecl for everything.

Clang already does the same in 64 bit mode; my updated patch makes us transform stdcall to thiscall for structors in 32 bit mode in MSVC environment.

Please re-review.

Yours,
Andrey

[rnk]

lib/Sema/SemaType.cpp

2272 ↗

(On Diff #33678)

s/is_ctor_or_dtor/IsCtorOrDtor/, it's the local variable convention.

5857–5858 ↗

(On Diff #33678)

I think a more accurate statement would be that MSVC ignores explicit calling convention attributes on structors in all modes. Compile this example with MSVC for x64:

struct HVA {
  double a, b, c, d;
};
struct A {
  __vectorcall A(HVA x);
  double f;
};
A::A(HVA x) : f(x.a + x.b + x.c + x.d) {}

Note the warning:

warning C4166: illegal calling convention for constructor/destructor

And HVA is passed indirectly instead of in XMM0-3.

You should consider adding a warning like this when we adjust structor conventions from something other than __stdcall. MSVC doesn't appear to warn on __stdcall structors, probably because it is used accidentally in places like vswriter.h.

5860 ↗

(On Diff #33678)

A better test for if we should treat structors specially would be:

Context.getTargetInfo().getCXXABI().isMicrosoft()

It's really the MSVC C++ ABI that requires that structors have special conventions. I think it has to do with exception handling.

5868 ↗

(On Diff #33678)

In hindsight, a better name for this variable would be DefaultCC.

5871–5875 ↗

(On Diff #33678)

I think it would be better to wrap both ifs here in if (!IsCtorOrDtor). We should always adjust ctors and dtors.

[andreybokhanko]

Patch updated after fixing all (but one) of Reid's comments.

[andreybokhanko]

lib/Sema/SemaType.cpp

5873–5877 ↗

(On Diff #33945)

I tried, but this leads to massive stability fails. I'm willing to investigate and fix them (if needed), but in a separate patch, OK?

[rnk]

lib/Sema/SemaType.cpp

5855–5856 ↗

(On Diff #33945)

DefaultCC is only meaningful when we're dealing with non-structor member functions. In the structor case, you're conflating it with ToCC. Let's sink the DefaultCC definition down into the 'else' block, since it's only meaningful there.

5857 ↗

(On Diff #33945)

ToCC is always the same in both cases:

CallingConv ToCC = Context.getDefaultCallingConvention(IsVariadic, !IsStatic);

Let's just keep that here, and return early if CurrCC == ToCC, since the common case is that we don't need to do anything.

5864–5865 ↗

(On Diff #33945)

This is reimplementing ASTContext::getDefaultCallingConv. Let's not duplicate that logic.

5867–5868 ↗

(On Diff #33945)

This can be covered by returning earlier if CurrCC == ToCC.

5870–5872 ↗

(On Diff #33945)

This seems wrong, see this test case:

struct A {
  __cdecl A(int x);
  int f;
};
A::A(int x) : f(x) {}

32-bit MSVC turns it into thiscall.

I think what you're really seeing is that variadic constructors need to stay cdecl. getDefaultCallingConvention already handles this case.

5873–5877 ↗

(On Diff #33945)

I'd really rather get the *structor calling convention logic right on the first try.

[andreybokhanko]

Reid,

Sorry for delayed reply. All your comments are fixed (it turned out that usage of DefaultCC instead of ToCC is the root of all evil; after I fixed this, all other problems went away).

Patch updated; please re-review.

Andrey

[rnk]

lgtm, thanks!

test/CodeGenCXX/ctor-dtor-alias.cpp

170 ↗

(On Diff #34477)

Isn't this still in the test9 namespace? Shouldn't this change be reverted?

test/CodeGenCXX/microsoft-abi-structors.cpp

411 ↗

(On Diff #34477)

Hah, looks like we encountered this long ago and didn't go back for it. :)

[andreybokhanko]

Reid, thank you for the review!

One last confirmation (see below), and I'll go ahead.

Yours,
Andrey

test/CodeGenCXX/ctor-dtor-alias.cpp

170 ↗

(On Diff #34477)

Now, after making the change you requested (wrapping checks in "default" part of adjustMemberFunctionCC function with !IsCtorOrDtor (SemaType.cpp:5876)) we start to call ~foo() here, so the check should be changed to:

// CHECK4: call void bitcast (void (%"struct.test9::foo"*)* @_ZN5test93fooD2Ev

Please confirm you are OK with this change and I'll go ahead with commit.

[rnk]

lib/Sema/SemaType.cpp

5876 ↗

(On Diff #34477)

This looks like the !IsCtorOrDtor check that affects Itanium. Isn't it already handled for MS C++ above?

test/CodeGenCXX/ctor-dtor-alias.cpp

170 ↗

(On Diff #34477)

That seems like a bug, we don't want this change to affect destructors on Linux or mingw.

[andreybokhanko]

lib/Sema/SemaType.cpp

5876 ↗

(On Diff #34477)

Reid, now I'm completely confused... I thought you asked me to insert this check for Itanium ABI as well (in your comment from Sep 1st). Am I mistaken? If yes, I can remove it and restore ctor-dtor-alias.cpp test back to original version.

[rnk]

lib/Sema/SemaType.cpp

5876 ↗

(On Diff #34477)

Sorry, it shouldn't affect Itanium. What you have without this check is fine.

[andreybokhanko]

OK -- I removed the check in Itanium ABI part, restored back the test and committed the patch.

Thanks for reviewing!

cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td

 def warn_cconv_knr : Warning<
   err_cconv_knr.Text>,
   InGroup<DiagGroup<"missing-prototype-for-cc">>;
 def err_cconv_varargs : Error<
   "variadic function cannot use %0 calling convention">;
 def warn_cconv_varargs : Warning<
   "%0 calling convention ignored on variadic function">,
   InGroup<IgnoredAttributes>;
+def warn_cconv_structors : Warning<
+  "%0 calling convention ignored on constructor/destructor">,
+  InGroup<IgnoredAttributes>;
 def err_regparm_mismatch : Error<"function declared with regparm(%0) "
   "attribute was previously declared "
   "%plural{0:without the regparm|:with the regparm(%1)}1 attribute">;
 def err_returns_retained_mismatch : Error<
   "function declared with the ns_returns_retained attribute "
   "was previously declared without the ns_returns_retained attribute">;
 def err_objc_precise_lifetime_bad_type : Error<
   "objc_precise_lifetime only applies to retainable types; type here is %0">;

cfe/trunk/include/clang/Sema/DeclSpec.h

bool isFirstDeclarationOfMember() {
     return getContext() == MemberContext && !getDeclSpec().isFriendSpecified();
   }
 
   /// Returns true if this declares a static member.  This cannot be called on a
   /// declarator outside of a MemberContext because we won't know until
   /// redeclaration time if the decl is static.
   bool isStaticMember();
 
+  /// Returns true if this declares a constructor or a destructor.
+  bool isCtorOrDtor();
+
   void setRedeclaration(bool Val) { Redeclaration = Val; }
   bool isRedeclaration() const { return Redeclaration; }
 };
 
 /// \brief This little struct is used to capture information about
 /// structure field declarators, which is basically just a bitfield size.
 struct FieldDeclarator {
   Declarator D;

cfe/trunk/include/clang/Sema/Sema.h

bool checkMSInheritanceAttrOnDefinition(
       CXXRecordDecl *RD, SourceRange Range, bool BestCase,
       MSInheritanceAttr::Spelling SemanticSpelling);
 
   void CheckAlignasUnderalignment(Decl *D);
 
   /// Adjust the calling convention of a method to be the ABI default if it
   /// wasn't specified explicitly.  This handles method types formed from
   /// function type typedefs and typename template arguments.
-  void adjustMemberFunctionCC(QualType &T, bool IsStatic);
+  void adjustMemberFunctionCC(QualType &T, bool IsStatic, bool IsCtorOrDtor,
+                              SourceLocation Loc);
 
   // Check if there is an explicit attribute, but only look through parens.
   // The intent is to look for an attribute on the current declarator, but not
   // one that came from a typedef.
   bool hasExplicitCallingConv(QualType &T);
 
   /// Get the outermost AttributedType node that sets a calling convention.
   /// Valid types should not have multiple attributes with different CCs.

cfe/trunk/lib/Sema/DeclSpec.cpp

 bool Declarator::isStaticMember() {
   assert(getContext() == MemberContext);
   return getDeclSpec().getStorageClassSpec() == DeclSpec::SCS_static ||
          (getName().Kind == UnqualifiedId::IK_OperatorFunctionId &&
           CXXMethodDecl::isStaticOverloadedOperator(
               getName().OperatorFunctionId.Operator));
 }
 
+bool Declarator::isCtorOrDtor() {
+  return (getName().getKind() == UnqualifiedId::IK_ConstructorName) ||
+         (getName().getKind() == UnqualifiedId::IK_DestructorName);
+}
+
 bool DeclSpec::hasTagDefinition() const {
   if (!TypeSpecOwned)
     return false;
   return cast<TagDecl>(getRepAsDecl())->isCompleteDefinition();
 }
 
 /// getParsedSpecifiers - Return a bitmask of which flavors of specifiers this
 /// declaration specifier includes.

cfe/trunk/lib/Sema/SemaDecl.cpp

Sema::ActOnFunctionDeclarator(Scope *S, Declarator &D, DeclContext *DC,
   StorageClass SC = getFunctionStorageClass(*this, D);
 
   if (DeclSpec::TSCS TSCS = D.getDeclSpec().getThreadStorageClassSpec())
     Diag(D.getDeclSpec().getThreadStorageClassSpecLoc(),
          diag::err_invalid_thread)
       << DeclSpec::getSpecifierName(TSCS);
 
   if (D.isFirstDeclarationOfMember())
-    adjustMemberFunctionCC(R, D.isStaticMember());
+    adjustMemberFunctionCC(R, D.isStaticMember(), D.isCtorOrDtor(),
+                           D.getIdentifierLoc());
 
   bool isFriend = false;
   FunctionTemplateDecl *FunctionTemplate = nullptr;
   bool isExplicitSpecialization = false;
   bool isFunctionTemplateSpecialization = false;
 
   bool isDependentClassScopeExplicitSpecialization = false;
   bool HasExplicitTemplateArgs = false;

cfe/trunk/lib/Sema/SemaType.cpp

QualType Sema::BuildMemberPointerType(QualType T, QualType Class,
 
   if (!Class->isDependentType() && !Class->isRecordType()) {
     Diag(Loc, diag::err_mempointer_in_nonclass_type) << Class;
     return QualType();
   }
 
   // Adjust the default free function calling convention to the default method
   // calling convention.
+  bool IsCtorOrDtor =
+      (Entity.getNameKind() == DeclarationName::CXXConstructorName) ||
+      (Entity.getNameKind() == DeclarationName::CXXDestructorName);
   if (T->isFunctionType())
-    adjustMemberFunctionCC(T, /*IsStatic=*/false);
+    adjustMemberFunctionCC(T, /*IsStatic=*/false, IsCtorOrDtor, Loc);
 
   return Context.getMemberPointerType(T, Class.getTypePtr());
 }
 
 /// \brief Build a block pointer type.
 ///
 /// \param T The type to which we'll be building a block pointer.
 ///
bool Sema::hasExplicitCallingConv(QualType &T) {
   while (const AttributedType *AT = dyn_cast<AttributedType>(R)) {
     if (AT->isCallingConv())
       return true;
     R = AT->getModifiedType().IgnoreParens();
   }
   return false;
 }
 
-void Sema::adjustMemberFunctionCC(QualType &T, bool IsStatic) {
+void Sema::adjustMemberFunctionCC(QualType &T, bool IsStatic, bool IsCtorOrDtor,
+                                  SourceLocation Loc) {
   FunctionTypeUnwrapper Unwrapped(*this, T);
   const FunctionType *FT = Unwrapped.get();
   bool IsVariadic = (isa<FunctionProtoType>(FT) &&
                      cast<FunctionProtoType>(FT)->isVariadic());
+  CallingConv CurCC = FT->getCallConv();
+  CallingConv ToCC = Context.getDefaultCallingConvention(IsVariadic, !IsStatic);
+
+  if (CurCC == ToCC)
+    return;
 
-  // Only adjust types with the default convention.  For example, on Windows we
-  // should adjust a __cdecl type to __thiscall for instance methods, and a
+  // MS compiler ignores explicit calling convention attributes on structors. We
+  // should do the same.
+  if (Context.getTargetInfo().getCXXABI().isMicrosoft() && IsCtorOrDtor) {
+    // Issue a warning on ignored calling convention -- except of __stdcall.
+    // Again, this is what MS compiler does.
+    if (CurCC != CC_X86StdCall)
+      Diag(Loc, diag::warn_cconv_structors)
+          << FunctionType::getNameForCallConv(CurCC);
+  // Default adjustment.
+  } else {
+    // Only adjust types with the default convention.  For example, on Windows
+    // we should adjust a __cdecl type to __thiscall for instance methods, and a
     // __thiscall type to __cdecl for static methods.
-  CallingConv CurCC = FT->getCallConv();
-  CallingConv FromCC =
+    CallingConv DefaultCC =
         Context.getDefaultCallingConvention(IsVariadic, IsStatic);
-  CallingConv ToCC = Context.getDefaultCallingConvention(IsVariadic, !IsStatic);
-  if (CurCC != FromCC || FromCC == ToCC)
+
+    if (CurCC != DefaultCC || DefaultCC == ToCC)
       return;
 
     if (hasExplicitCallingConv(T))
       return;
+  }
 
   FT = Context.adjustFunctionType(FT, FT->getExtInfo().withCallingConv(ToCC));
   QualType Wrapped = Unwrapped.wrap(*this, FT);
   T = Context.getAdjustedType(T, Wrapped);
 }
 
 /// HandleVectorSizeAttribute - this attribute is only applicable to integral
 /// and float scalars, although arrays, pointers, and function return values are

cfe/trunk/test/CodeGenCXX/microsoft-abi-structors.cpp

 // RUN: %clang_cc1 -emit-llvm -fno-rtti %s -std=c++11 -o - -mconstructor-aliases -triple=i386-pc-win32 -fno-rtti > %t
 // RUN: FileCheck %s < %t
 // vftables are emitted very late, so do another pass to try to keep the checks
 // in source order.
 // RUN: FileCheck --check-prefix DTORS %s < %t
 // RUN: FileCheck --check-prefix DTORS2 %s < %t
 // RUN: FileCheck --check-prefix DTORS3 %s < %t
+// RUN: FileCheck --check-prefix DTORS4 %s < %t
 //
 // RUN: %clang_cc1 -emit-llvm %s -o - -mconstructor-aliases -triple=x86_64-pc-win32 -fno-rtti | FileCheck --check-prefix DTORS-X64 %s
 
 namespace basic {
 
 class A {
  public:
   A() { }

 };
 B::B(int *a) {}
 B::B(const char *a, ...) {}
 B::B(short *a) {}
 // CHECK: define x86_thiscallcc %"struct.test1::B"* @"\01??0B@test1@@QAE@PAH@Z"
 // CHECK:               (%"struct.test1::B"* returned %this, i32* %a, i32 %is_most_derived)
 // CHECK: define %"struct.test1::B"* @"\01??0B@test1@@QAA@PBDZZ"
 // CHECK:               (%"struct.test1::B"* returned %this, i32 %is_most_derived, i8* %a, ...)
-
-// FIXME: This should be x86_thiscallcc.  MSVC ignores explicit CCs on structors.
-// CHECK: define %"struct.test1::B"* @"\01??0B@test1@@QAA@PAF@Z"
+// CHECK: define x86_thiscallcc %"struct.test1::B"* @"\01??0B@test1@@QAE@PAF@Z"
 // CHECK:               (%"struct.test1::B"* returned %this, i16* %a, i32 %is_most_derived)
 
 void construct_b() {
   int a;
   B b1(&a);
   B b2("%d %d", 1, 2);
 }
 // CHECK-LABEL: define void @"\01?construct_b@test1@@YAXXZ"()

 }
 void *getA() {
   return (void*)new A();
 }
 // CHECK: define internal x86_thiscallcc i8* @"\01??_GA@?A@@UAEPAXI@Z"
 // CHECK:               (%"struct.(anonymous namespace)::A"* %this, i32 %should_call_delete)
 // CHECK: define internal x86_thiscallcc void @"\01??1A@?A@@UAE@XZ"
 // CHECK:               (%"struct.(anonymous namespace)::A"* %this)
+
+// Check that we correctly transform __stdcall to __thiscall for ctors and
+// dtors.
+class G {
+ public:
+  __stdcall G() {};
+// DTORS4: define linkonce_odr x86_thiscallcc %class.G* @"\01??0G@@QAE@XZ"
+  __stdcall ~G() {};
+// DTORS4: define linkonce_odr x86_thiscallcc void @"\01??1G@@QAE@XZ"
+};
+
+extern void testG() {
+  G g;
+}
+

cfe/trunk/test/SemaCXX/decl-microsoft-call-conv.cpp

-// RUN: %clang_cc1 -std=c++14 -triple i686-pc-win32 -fms-extensions -verify %s
+// RUN: %clang_cc1 -std=c++14 -triple i686-pc-win32 -fms-extensions -DMSABI -verify %s
 // RUN: %clang_cc1 -std=c++14 -triple i686-pc-mingw32 -verify %s
 // RUN: %clang_cc1 -std=c++14 -triple i686-pc-mingw32 -fms-extensions -verify %s
 
 typedef void void_fun_t();
 typedef void __cdecl cdecl_fun_t();
 
 // Pointers to free functions
 void            free_func_default(); // expected-note 2 {{previous declaration is here}}
struct S {
   static void __stdcall  static_member_stdcall2();
 
   // Variadic functions can't be other than default or __cdecl
   void            member_variadic_default(int x, ...);
   void __cdecl    member_variadic_cdecl(int x, ...);
 
   static void            static_member_variadic_default(int x, ...);
   static void __cdecl    static_member_variadic_cdecl(int x, ...);
+
+  // Structors can't be other than default in MS ABI environment
+#ifdef MSABI
+  __vectorcall S(); // expected-warning {{vectorcall calling convention ignored on constructor/destructor}}
+#endif
 };
 
 void __cdecl    S::member_default1() {} // expected-error {{function declared 'cdecl' here was previously declared without calling convention}}
 void __thiscall S::member_default2() {}
 
 void __cdecl   S::member_typedef_default() {} // expected-error {{function declared 'cdecl' here was previously declared without calling convention}}
 void __cdecl   S::member_typedef_cdecl1() {} // expected-error {{function declared 'cdecl' here was previously declared without calling convention}}
 void __cdecl   S::member_typedef_cdecl2() {}