[PATCH] D46929: Fix a mangling failure on clang-cl C++17

[Reid Kleckner via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC332639: Fix a mangling failure on clang-cl C++17 (authored 
by rnk, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D46929?vs=147244&id=147359#toc

Repository:
  rC Clang

https://reviews.llvm.org/D46929

Files:
  lib/AST/VTableBuilder.cpp
  lib/CodeGen/CGCXX.cpp
  lib/CodeGen/ItaniumCXXABI.cpp
  lib/CodeGen/MicrosoftCXXABI.cpp

Index: lib/CodeGen/ItaniumCXXABI.cpp
===
--- lib/CodeGen/ItaniumCXXABI.cpp
+++ lib/CodeGen/ItaniumCXXABI.cpp
@@ -825,7 +825,6 @@
 llvm::Constant *ItaniumCXXABI::BuildMemberPointer(const CXXMethodDecl *MD,
   CharUnits ThisAdjustment) {
   assert(MD->isInstance() && "Member function must not be static!");
-  MD = MD->getCanonicalDecl();
 
   CodeGenTypes &Types = CGM.getTypes();
 
@@ -1640,7 +1639,6 @@
   Address This,
   llvm::Type *Ty,
   SourceLocation Loc) {
-  GD = GD.getCanonicalDecl();
   Ty = Ty->getPointerTo()->getPointerTo();
   auto *MethodDecl = cast(GD.getDecl());
   llvm::Value *VTable = CGF.GetVTablePtr(This, Ty, MethodDecl->getParent());
@@ -1674,7 +1672,7 @@
 VFunc = VFuncLoad;
   }
 
-  CGCallee Callee(MethodDecl, VFunc);
+  CGCallee Callee(MethodDecl->getCanonicalDecl(), VFunc);
   return Callee;
 }
 
Index: lib/CodeGen/MicrosoftCXXABI.cpp
===
--- lib/CodeGen/MicrosoftCXXABI.cpp
+++ lib/CodeGen/MicrosoftCXXABI.cpp
@@ -228,7 +228,6 @@
 
   const CXXRecordDecl *
   getThisArgumentTypeForMethod(const CXXMethodDecl *MD) override {
-MD = MD->getCanonicalDecl();
 if (MD->isVirtual() && !isa(MD)) {
   MethodVFTableLocation ML =
   CGM.getMicrosoftVTableContext().getMethodVFTableLocation(MD);
@@ -1320,23 +1319,21 @@
 
 CharUnits
 MicrosoftCXXABI::getVirtualFunctionPrologueThisAdjustment(GlobalDecl GD) {
-  GD = GD.getCanonicalDecl();
   const CXXMethodDecl *MD = cast(GD.getDecl());
 
-  GlobalDecl LookupGD = GD;
   if (const CXXDestructorDecl *DD = dyn_cast(MD)) {
 // Complete destructors take a pointer to the complete object as a
 // parameter, thus don't need this adjustment.
 if (GD.getDtorType() == Dtor_Complete)
   return CharUnits();
 
 // There's no Dtor_Base in vftable but it shares the this adjustment with
 // the deleting one, so look it up instead.
-LookupGD = GlobalDecl(DD, Dtor_Deleting);
+GD = GlobalDecl(DD, Dtor_Deleting);
   }
 
   MethodVFTableLocation ML =
-  CGM.getMicrosoftVTableContext().getMethodVFTableLocation(LookupGD);
+  CGM.getMicrosoftVTableContext().getMethodVFTableLocation(GD);
   CharUnits Adjustment = ML.VFPtrOffset;
 
   // Normal virtual instance methods need to adjust from the vfptr that first
@@ -1370,7 +1367,6 @@
 return CGF.Builder.CreateConstByteGEP(This, Adjustment);
   }
 
-  GD = GD.getCanonicalDecl();
   const CXXMethodDecl *MD = cast(GD.getDecl());
 
   GlobalDecl LookupGD = GD;
@@ -1839,7 +1835,6 @@
 Address This,
 llvm::Type *Ty,
 SourceLocation Loc) {
-  GD = GD.getCanonicalDecl();
   CGBuilderTy &Builder = CGF.Builder;
 
   Ty = Ty->getPointerTo()->getPointerTo();
@@ -1878,7 +1873,7 @@
 VFunc = Builder.CreateAlignedLoad(VFuncPtr, CGF.getPointerAlign());
   }
 
-  CGCallee Callee(MethodDecl, VFunc);
+  CGCallee Callee(MethodDecl->getCanonicalDecl(), VFunc);
   return Callee;
 }
 
@@ -2737,7 +2732,6 @@
 MicrosoftCXXABI::EmitMemberFunctionPointer(const CXXMethodDecl *MD) {
   assert(MD->isInstance() && "Member function must not be static!");
 
-  MD = MD->getCanonicalDecl();
   CharUnits NonVirtualBaseAdjustment = CharUnits::Zero();
   const CXXRecordDecl *RD = MD->getParent()->getMostRecentDecl();
   CodeGenTypes &Types = CGM.getTypes();
Index: lib/CodeGen/CGCXX.cpp
===
--- lib/CodeGen/CGCXX.cpp
+++ lib/CodeGen/CGCXX.cpp
@@ -267,7 +267,6 @@
   const CXXRecordDecl *RD) {
   assert(!CGF.CGM.getTarget().getCXXABI().isMicrosoft() &&
  "No kext in Microsoft ABI");
-  GD = GD.getCanonicalDecl();
   CodeGenModule &CGM = CGF.CGM;
   llvm::Value *VTable = CGM.getCXXABI().getAddrOfVTable(RD, CharUnits());
   Ty = Ty->getPointerTo()->getPointerTo();
@@ -283,7 +282,7 @@
 CGF.Builder.CreateConstInBoundsGEP1_64(VTable, VTableIndex, "vfnkxt");
   llvm::Value *VFunc =
 CGF.Builder.CreateAlignedLoad(VFuncPtr, CGF.PointerAlignInBytes);
-  CGCallee Callee(GD.getDecl(), VFunc);
+  CGCallee Callee(GD.getDecl()->getCanonicalDecl(), VFunc);
   return Callee;
 }
 
Index: lib/AST/VTab

[PATCH] D46929: Fix a mangling failure on clang-cl C++17

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

Please do, tzik doesn't have commit access yet.


Repository:
  rC Clang

https://reviews.llvm.org/D46929



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46929: Fix a mangling failure on clang-cl C++17

[Reid Kleckner via Phabricator via cfe-commits]

rnk accepted this revision.
rnk added a comment.
This revision is now accepted and ready to land.

lgtm

Shall I go ahead and commit this for you?




Comment at: lib/AST/VTableBuilder.cpp:2507
   const MethodInfo &MI = I.second;
+  assert(MD == MD->getCanonicalDecl());
+

Thanks for that!


Repository:
  rC Clang

https://reviews.llvm.org/D46929



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46929: Fix a mangling failure on clang-cl C++17

[Taiju Tsuiki via Phabricator via cfe-commits]

tzik added a comment.

In https://reviews.llvm.org/D46929#1101780, @rnk wrote:

> I searched around, and I noticed that `VTableContext::getMethodVTableIndex` 
> has the same implied contract that the caller must always provide a canonical 
> declaration or things will break. It looks like it has three callers, and 
> they all do this. We should probably sink the canonicalization into this 
> helper as well and clean up the now-superfluous canonicalizations at the call 
> site.


Updated.
As I'm not sure if it's safe to use non-canonicalized CXXMethodDecl for 
EmitVirtualMemPtrThunk and CGCall, I left using the canonical decl.
For other part, non-canonical decl looks working fine to me.


Repository:
  rC Clang

https://reviews.llvm.org/D46929



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46929: Fix a mangling failure on clang-cl C++17

[Taiju Tsuiki via Phabricator via cfe-commits]

tzik updated this revision to Diff 147244.

Repository:
  rC Clang

https://reviews.llvm.org/D46929

Files:
  lib/AST/VTableBuilder.cpp
  lib/CodeGen/CGCXX.cpp
  lib/CodeGen/ItaniumCXXABI.cpp
  lib/CodeGen/MicrosoftCXXABI.cpp
  test/CodeGenCXX/PR37481.cpp

Index: test/CodeGenCXX/PR37481.cpp
===
--- /dev/null
+++ test/CodeGenCXX/PR37481.cpp
@@ -0,0 +1,17 @@
+// RUN: %clang_cc1 -o /dev/null -emit-llvm -std=c++17 -triple x86_64-pc-windows-msvc %s
+
+struct Foo {
+  virtual void f();
+  virtual void g();
+};
+
+void Foo::f() {}
+void Foo::g() {}
+
+template 
+void h() {}
+
+void x() {
+  h<&Foo::f>();
+  h<&Foo::g>();
+}
Index: lib/CodeGen/MicrosoftCXXABI.cpp
===
--- lib/CodeGen/MicrosoftCXXABI.cpp
+++ lib/CodeGen/MicrosoftCXXABI.cpp
@@ -228,7 +228,6 @@
 
   const CXXRecordDecl *
   getThisArgumentTypeForMethod(const CXXMethodDecl *MD) override {
-MD = MD->getCanonicalDecl();
 if (MD->isVirtual() && !isa(MD)) {
   MethodVFTableLocation ML =
   CGM.getMicrosoftVTableContext().getMethodVFTableLocation(MD);
@@ -1320,23 +1319,21 @@
 
 CharUnits
 MicrosoftCXXABI::getVirtualFunctionPrologueThisAdjustment(GlobalDecl GD) {
-  GD = GD.getCanonicalDecl();
   const CXXMethodDecl *MD = cast(GD.getDecl());
 
-  GlobalDecl LookupGD = GD;
   if (const CXXDestructorDecl *DD = dyn_cast(MD)) {
 // Complete destructors take a pointer to the complete object as a
 // parameter, thus don't need this adjustment.
 if (GD.getDtorType() == Dtor_Complete)
   return CharUnits();
 
 // There's no Dtor_Base in vftable but it shares the this adjustment with
 // the deleting one, so look it up instead.
-LookupGD = GlobalDecl(DD, Dtor_Deleting);
+GD = GlobalDecl(DD, Dtor_Deleting);
   }
 
   MethodVFTableLocation ML =
-  CGM.getMicrosoftVTableContext().getMethodVFTableLocation(LookupGD);
+  CGM.getMicrosoftVTableContext().getMethodVFTableLocation(GD);
   CharUnits Adjustment = ML.VFPtrOffset;
 
   // Normal virtual instance methods need to adjust from the vfptr that first
@@ -1370,7 +1367,6 @@
 return CGF.Builder.CreateConstByteGEP(This, Adjustment);
   }
 
-  GD = GD.getCanonicalDecl();
   const CXXMethodDecl *MD = cast(GD.getDecl());
 
   GlobalDecl LookupGD = GD;
@@ -1839,7 +1835,6 @@
 Address This,
 llvm::Type *Ty,
 SourceLocation Loc) {
-  GD = GD.getCanonicalDecl();
   CGBuilderTy &Builder = CGF.Builder;
 
   Ty = Ty->getPointerTo()->getPointerTo();
@@ -1878,7 +1873,7 @@
 VFunc = Builder.CreateAlignedLoad(VFuncPtr, CGF.getPointerAlign());
   }
 
-  CGCallee Callee(MethodDecl, VFunc);
+  CGCallee Callee(MethodDecl->getCanonicalDecl(), VFunc);
   return Callee;
 }
 
@@ -2737,7 +2732,6 @@
 MicrosoftCXXABI::EmitMemberFunctionPointer(const CXXMethodDecl *MD) {
   assert(MD->isInstance() && "Member function must not be static!");
 
-  MD = MD->getCanonicalDecl();
   CharUnits NonVirtualBaseAdjustment = CharUnits::Zero();
   const CXXRecordDecl *RD = MD->getParent()->getMostRecentDecl();
   CodeGenTypes &Types = CGM.getTypes();
@@ -2760,7 +2754,7 @@
   } else {
 auto &VTableContext = CGM.getMicrosoftVTableContext();
 MethodVFTableLocation ML = VTableContext.getMethodVFTableLocation(MD);
-FirstField = EmitVirtualMemPtrThunk(MD, ML);
+FirstField = EmitVirtualMemPtrThunk(MD->getCanonicalDecl(), ML);
 // Include the vfptr adjustment if the method is in a non-primary vftable.
 NonVirtualBaseAdjustment += ML.VFPtrOffset;
 if (ML.VBase)
Index: lib/CodeGen/ItaniumCXXABI.cpp
===
--- lib/CodeGen/ItaniumCXXABI.cpp
+++ lib/CodeGen/ItaniumCXXABI.cpp
@@ -825,7 +825,6 @@
 llvm::Constant *ItaniumCXXABI::BuildMemberPointer(const CXXMethodDecl *MD,
   CharUnits ThisAdjustment) {
   assert(MD->isInstance() && "Member function must not be static!");
-  MD = MD->getCanonicalDecl();
 
   CodeGenTypes &Types = CGM.getTypes();
 
@@ -1640,7 +1639,6 @@
   Address This,
   llvm::Type *Ty,
   SourceLocation Loc) {
-  GD = GD.getCanonicalDecl();
   Ty = Ty->getPointerTo()->getPointerTo();
   auto *MethodDecl = cast(GD.getDecl());
   llvm::Value *VTable = CGF.GetVTablePtr(This, Ty, MethodDecl->getParent());
@@ -1674,7 +1672,7 @@
 VFunc = VFuncLoad;
   }
 
-  CGCallee Callee(MethodDecl, VFunc);
+  CGCallee Callee(MethodDecl->getCanonicalDecl(), VFunc);
   return Callee;
 }
 
Index: lib/CodeGen/CGCXX.cpp
===
--- lib/CodeGen/CGCXX.cpp
+++ lib/CodeGen/CGCXX.cpp
@@ -267,7 +267,6 @@
 

[PATCH] D46929: Fix a mangling failure on clang-cl C++17

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

I searched around, and I noticed that `VTableContext::getMethodVTableIndex` has 
the same implied contract that the caller must always provide a canonical 
declaration or things will break. It looks like it has three callers, and they 
all do this. We should probably sink the canonicalization into this helper as 
well and clean up the now-superfluous canonicalizations at the call site.


Repository:
  rC Clang

https://reviews.llvm.org/D46929



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46929: Fix a mangling failure on clang-cl C++17

[Taiju Tsuiki via Phabricator via cfe-commits]

tzik created this revision.
tzik added reviewers: majnemer, rnk.
Herald added a subscriber: cfe-commits.

MethodVFTableLocations in MigrosoftVTableContext contains canonicalized
decl. But, it's sometimes asked to lookup for non-canonicalized decl,
and that causes assertion failure, and compilation failure.
Fixes PR37481.


Repository:
  rC Clang

https://reviews.llvm.org/D46929

Files:
  lib/AST/VTableBuilder.cpp
  test/CodeGenCXX/PR37481.cpp


Index: test/CodeGenCXX/PR37481.cpp
===
--- /dev/null
+++ test/CodeGenCXX/PR37481.cpp
@@ -0,0 +1,17 @@
+// RUN: %clang_cc1 -o /dev/null -emit-llvm -std=c++17 -triple 
x86_64-pc-windows-msvc %s
+
+struct Foo {
+  virtual void f();
+  virtual void g();
+};
+
+void Foo::f() {}
+void Foo::g() {}
+
+template 
+void h() {}
+
+void x() {
+  h<&Foo::f>();
+  h<&Foo::g>();
+}
Index: lib/AST/VTableBuilder.cpp
===
--- lib/AST/VTableBuilder.cpp
+++ lib/AST/VTableBuilder.cpp
@@ -3737,6 +3737,8 @@
   if (isa(GD.getDecl()))
 assert(GD.getDtorType() == Dtor_Deleting);
 
+  GD = GD.getCanonicalDecl();
+
   MethodVFTableLocationsTy::iterator I = MethodVFTableLocations.find(GD);
   if (I != MethodVFTableLocations.end())
 return I->second;


Index: test/CodeGenCXX/PR37481.cpp
===
--- /dev/null
+++ test/CodeGenCXX/PR37481.cpp
@@ -0,0 +1,17 @@
+// RUN: %clang_cc1 -o /dev/null -emit-llvm -std=c++17 -triple x86_64-pc-windows-msvc %s
+
+struct Foo {
+  virtual void f();
+  virtual void g();
+};
+
+void Foo::f() {}
+void Foo::g() {}
+
+template 
+void h() {}
+
+void x() {
+  h<&Foo::f>();
+  h<&Foo::g>();
+}
Index: lib/AST/VTableBuilder.cpp
===
--- lib/AST/VTableBuilder.cpp
+++ lib/AST/VTableBuilder.cpp
@@ -3737,6 +3737,8 @@
   if (isa(GD.getDecl()))
 assert(GD.getDtorType() == Dtor_Deleting);
 
+  GD = GD.getCanonicalDecl();
+
   MethodVFTableLocationsTy::iterator I = MethodVFTableLocations.find(GD);
   if (I != MethodVFTableLocations.end())
 return I->second;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits