Re: [PATCH] D22034: [MSVC][DLL] use local vftable mangling only exported classes with virtual destructor
rnk added inline comments. Comment at: test/CodeGenCXX/dllimport-rtti.cpp:7 @@ -6,3 +6,1 @@ } s; -// MSVC: [[VF_S:.*]] = private unnamed_addr constant [2 x i8*] -// MSVC-DAG: @"\01??_SS@@6B@" = unnamed_addr alias i8*, getelementptr inbounds ([2 x i8*], [2 x i8*]* [[VF_S]], i32 0, i32 1) DmitryPolukhin wrote: > rnk wrote: > > I would've expected this to remain the same, since the implicit default > > ctor of 'S' is constexpr by default in C++14. It seems a lot better to emit > > a local vftable here and get static initialization for 's' than dynamic > > initialization. > The context of evaluation of the whole expression is not constexpr so this > case can be done both ways. But implemented approach is how MSVC behaves in > this case. MSVC has very predictable behavior when local vftable is used - > only when class has virtual d-tor. Current Clang behavior is also very > consistent - always use local vftable. But this patch makes it hard to > predict which table will be used - it becomes use context sensitive instead > of depending only on class declaration. Therefore different translation units > could use different tables and it could cause strange artifacts. Using > dllimported classes in pure constexpr case in my experience is very rare case > so it was kind of fine but implicit constructors much more common case. > > Also thinking more about my patch I realized that fix in MayBeEmittedEagerly > doesn't work if dllimported class is a member of non-imported class so actual > fix would require traversing for all base classes and members for the type. > > So my proposal is to keep things as is for now and abandon this patch if you > have no objection. Sounds good, we also thought this was a reasonable compromise position last time we touched this. https://reviews.llvm.org/D22034 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D22034: [MSVC][DLL] use local vftable mangling only exported classes with virtual destructor
DmitryPolukhin added inline comments. Comment at: test/CodeGenCXX/dllimport-rtti.cpp:7 @@ -6,3 +6,1 @@ } s; -// MSVC: [[VF_S:.*]] = private unnamed_addr constant [2 x i8*] -// MSVC-DAG: @"\01??_SS@@6B@" = unnamed_addr alias i8*, getelementptr inbounds ([2 x i8*], [2 x i8*]* [[VF_S]], i32 0, i32 1) rnk wrote: > I would've expected this to remain the same, since the implicit default ctor > of 'S' is constexpr by default in C++14. It seems a lot better to emit a > local vftable here and get static initialization for 's' than dynamic > initialization. The context of evaluation of the whole expression is not constexpr so this case can be done both ways. But implemented approach is how MSVC behaves in this case. MSVC has very predictable behavior when local vftable is used - only when class has virtual d-tor. Current Clang behavior is also very consistent - always use local vftable. But this patch makes it hard to predict which table will be used - it becomes use context sensitive instead of depending only on class declaration. Therefore different translation units could use different tables and it could cause strange artifacts. Using dllimported classes in pure constexpr case in my experience is very rare case so it was kind of fine but implicit constructors much more common case. Also thinking more about my patch I realized that fix in MayBeEmittedEagerly doesn't work if dllimported class is a member of non-imported class so actual fix would require traversing for all base classes and members for the type. So my proposal is to keep things as is for now and abandon this patch if you have no objection. https://reviews.llvm.org/D22034 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D22034: [MSVC][DLL] use local vftable mangling only exported classes with virtual destructor
rnk added a comment. The approach makes sense to me, but the tests suggest it isn't doing what I'd expect. Comment at: test/CodeGenCXX/dllimport-rtti.cpp:7 @@ -6,3 +6,1 @@ } s; -// MSVC: [[VF_S:.*]] = private unnamed_addr constant [2 x i8*] -// MSVC-DAG: @"\01??_SS@@6B@" = unnamed_addr alias i8*, getelementptr inbounds ([2 x i8*], [2 x i8*]* [[VF_S]], i32 0, i32 1) I would've expected this to remain the same, since the implicit default ctor of 'S' is constexpr by default in C++14. It seems a lot better to emit a local vftable here and get static initialization for 's' than dynamic initialization. https://reviews.llvm.org/D22034 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D22034: [MSVC][DLL] use local vftable mangling only exported classes with virtual destructor
DmitryPolukhin updated this revision to Diff 63950.
DmitryPolukhin marked 2 inline comments as done.
DmitryPolukhin added a comment.
- defer var generation if their type is class with dllimport.
http://reviews.llvm.org/D22034
Files:
include/clang/AST/DeclCXX.h
lib/AST/DeclCXX.cpp
lib/AST/ExprConstant.cpp
lib/AST/MicrosoftMangle.cpp
lib/CodeGen/CGVTables.cpp
lib/CodeGen/CodeGenModule.cpp
lib/CodeGen/MicrosoftCXXABI.cpp
lib/Sema/SemaDeclCXX.cpp
test/CodeGenCXX/dllimport-rtti.cpp
test/CodeGenCXX/dllimport.cpp
Index: test/CodeGenCXX/dllimport.cpp
===
--- test/CodeGenCXX/dllimport.cpp
+++ test/CodeGenCXX/dllimport.cpp
@@ -620,9 +620,31 @@
struct __declspec(dllimport) W { virtual void foo() {} };
USECLASS(W)
// vftable:
-// MO1-DAG: @"\01??_SW@@6B@" = linkonce_odr unnamed_addr constant [1 x i8*] [i8* bitcast (void (%struct.W*)* @"\01?foo@W@@UAEXXZ" to i8*)]
+// MO1-DAG: @"\01??_7W@@6B@" = external dllimport unnamed_addr constant [1 x i8*]
// GO1-DAG: @_ZTV1W = available_externally dllimport unnamed_addr constant [3 x i8*] [i8* null, i8* null, i8* bitcast (void (%struct.W*)* @_ZN1W3fooEv to i8*)]
+struct __declspec(dllimport) W2 {
+ virtual ~W2();
+ virtual void foo() {}
+};
+USECLASS(W2)
+// vftable:
+// MO1-DAG: @"\01??_SW2@@6B@" = linkonce_odr unnamed_addr constant [2 x i8*]
+// GO1-DAG: @_ZTV2W2 = external dllimport unnamed_addr constant [5 x i8*]
+
+struct __declspec(dllimport) W3 {
+ virtual void fn() const {
+ }
+ constexpr W3() = default;
+};
+
+W3 w3;
+W3& w3r = w3;
+
+constexpr W3 cw3;
+const W3 = cw3;
+// MO1-DAG: @"\01??_SW3@@6B@" = linkonce_odr unnamed_addr constant [1 x i8*]
+
struct __declspec(dllimport) KeyFuncClass {
constexpr KeyFuncClass() {}
virtual void foo();
Index: test/CodeGenCXX/dllimport-rtti.cpp
===
--- test/CodeGenCXX/dllimport-rtti.cpp
+++ test/CodeGenCXX/dllimport-rtti.cpp
@@ -4,16 +4,25 @@
struct __declspec(dllimport) S {
virtual void f() {}
} s;
-// MSVC: [[VF_S:.*]] = private unnamed_addr constant [2 x i8*]
-// MSVC-DAG: @"\01??_SS@@6B@" = unnamed_addr alias i8*, getelementptr inbounds ([2 x i8*], [2 x i8*]* [[VF_S]], i32 0, i32 1)
+// MSVC-DAG: @"\01??_7S@@6B@" = external dllimport unnamed_addr constant [2 x i8*]
// MSVC-DAG: @"\01??_R0?AUS@@@8" = linkonce_odr
// MSVC-DAG: @"\01??_R1A@?0A@EA@S@@8" = linkonce_odr
// MSVC-DAG: @"\01??_R2S@@8" = linkonce_odr
// MSVC-DAG: @"\01??_R3S@@8" = linkonce_odr
// GNU-DAG: @_ZTV1S = available_externally dllimport
// GNU-DAG: @_ZTI1S = external dllimport
+struct __declspec(dllimport) S2 {
+ virtual void f() {}
+ virtual ~S2() {}
+} s2;
+// MSVC-DAG: [[VF_S2:.*]] = private unnamed_addr constant [3 x i8*] [i8* bitcast (%rtti.CompleteObjectLocator* @"\01??_R4S2@@6B@" to i8*)
+// MSVC-DAG: @"\01??_SS2@@6B@" = unnamed_addr alias i8*, getelementptr inbounds ([3 x i8*], [3 x i8*]* [[VF_S2]], i32 0, i32 1)
+// MSVC-DAG: @"\01??_R0?AUS2@@@8" = linkonce_odr
+// MSVC-DAG: @"\01??_R1A@?0A@EA@S2@@8" = linkonce_odr
+// MSVC-DAG: @"\01??_R2S2@@8" = linkonce_odr
+
struct U : S {
} u;
Index: lib/Sema/SemaDeclCXX.cpp
===
--- lib/Sema/SemaDeclCXX.cpp
+++ lib/Sema/SemaDeclCXX.cpp
@@ -4809,6 +4809,17 @@
}
}
+ if (!ClassExported) {
+if (Class->getTemplateSpecializationKind() != TSK_Undeclared)
+ Class->setNeedLocalVFTable();
+if (const CXXDestructorDecl *Destructor = Class->getDestructor()) {
+ // Classes with dllimport attribute needs local vftable if they have
+ // virtual d-tor.
+ if (Destructor->isVirtual())
+Class->setNeedLocalVFTable();
+}
+ }
+
if (ClassExported)
DelayedDllExportClasses.push_back(Class);
}
Index: lib/CodeGen/MicrosoftCXXABI.cpp
===
--- lib/CodeGen/MicrosoftCXXABI.cpp
+++ lib/CodeGen/MicrosoftCXXABI.cpp
@@ -1670,9 +1670,13 @@
//
// Because of this unique behavior, we maintain this logic here instead of
// getVTableLinkage.
- llvm::GlobalValue::LinkageTypes VFTableLinkage =
- RD->hasAttr() ? llvm::GlobalValue::LinkOnceODRLinkage
- : CGM.getVTableLinkage(RD);
+ llvm::GlobalValue::LinkageTypes VFTableLinkage = CGM.getVTableLinkage(RD);
+ if (RD->hasAttr()) {
+if (RD->getNeedLocalVFTable())
+ VFTableLinkage = llvm::GlobalValue::LinkOnceODRLinkage;
+else
+ VFTableLinkage = llvm::GlobalValue::ExternalLinkage;
+ }
bool VFTableComesFromAnotherTU =
llvm::GlobalValue::isAvailableExternallyLinkage(VFTableLinkage) ||
llvm::GlobalValue::isExternalLinkage(VFTableLinkage);
@@ -1745,7 +1749,9 @@
if (C)
VTable->setComdat(C);
- if (RD->hasAttr())
+ if (RD->hasAttr() && !RD->getNeedLocalVFTable())
+VFTable->setDLLStorageClass(llvm::GlobalValue::DLLImportStorageClass);
+ else if
Re: [PATCH] D22034: [MSVC][DLL] use local vftable mangling only exported classes with virtual destructor
DmitryPolukhin added a comment. In http://reviews.llvm.org/D22034#482925, @majnemer wrote: > A flag on CXXRecordDecl which is sensitive to the most recent expression > evaluation might not be the best way to go. > Perhaps we should be able to use the VFTableBuilder to build imported and > local vftables for the same vftable? Not entirely sure though... The flag can be only set to true, there is no way to reset it false. So by the end of translation unit it should have proper value and many things are deferred until the end of translation unit (like CodeGenModule::EmitDeferredVTables). But your example shows that normal var generation is not deferred, I missed it. Deferring var generation in CodeGenModule::MayBeEmittedEagerly seems to work fine. http://reviews.llvm.org/D22034 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D22034: [MSVC][DLL] use local vftable mangling only exported classes with virtual destructor
rnk added inline comments. Comment at: lib/AST/ExprConstant.cpp:5766 @@ +5765,3 @@ +if (CXXRecordDecl *CD = dyn_cast( +E->getType()->castAs()->getDecl())) + CD->setNeedLocalVFTable(); Even better: if (auto *CD = E->getType()->getAsCXXRecordDecl()) http://reviews.llvm.org/D22034 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D22034: [MSVC][DLL] use local vftable mangling only exported classes with virtual destructor
majnemer added a comment.
Running your patch against the following:
struct __declspec(dllimport) S {
virtual void f() {}
};
S x;
constexpr S y;
results in:
> Global is marked as dllimport, but not external
> [2 x i8*]* @"\01??_7S@@6B@"
> fatal error: error in backend: Broken module found, compilation aborted!
> clang-3.9: error: clang frontend command failed with exit code 70 (use -v to
> see invocation)
I don't think your approach will correctly handle instances where we need to
emit a local vftable and reference an imported vftable simultaneously.
A flag on CXXRecordDecl which is sensitive to the most recent expression
evaluation might not be the best way to go.
Perhaps we should be able to use the VFTableBuilder to build imported and local
vftables for the same vftable? Not entirely sure though...
Comment at: lib/AST/ExprConstant.cpp:5765
@@ +5764,3 @@
+ if (Info.EvalMode == EvalInfo::EM_ConstantExpression) {
+if (CXXRecordDecl *CD = dyn_cast(
+E->getType()->castAs()->getDecl()))
I'd use `auto *` here.
http://reviews.llvm.org/D22034
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D22034: [MSVC][DLL] use local vftable mangling only exported classes with virtual destructor
DmitryPolukhin updated this revision to Diff 63793.
DmitryPolukhin added a comment.
Sorry for delay with patch rework, PTAL!
- added local vftable for constexpr
- added use of imported vftable
http://reviews.llvm.org/D22034
Files:
include/clang/AST/DeclCXX.h
lib/AST/DeclCXX.cpp
lib/AST/ExprConstant.cpp
lib/AST/MicrosoftMangle.cpp
lib/CodeGen/CGVTables.cpp
lib/CodeGen/MicrosoftCXXABI.cpp
lib/Sema/SemaDeclCXX.cpp
test/CodeGenCXX/dllimport-rtti.cpp
test/CodeGenCXX/dllimport.cpp
Index: test/CodeGenCXX/dllimport.cpp
===
--- test/CodeGenCXX/dllimport.cpp
+++ test/CodeGenCXX/dllimport.cpp
@@ -620,9 +620,28 @@
struct __declspec(dllimport) W { virtual void foo() {} };
USECLASS(W)
// vftable:
-// MO1-DAG: @"\01??_SW@@6B@" = linkonce_odr unnamed_addr constant [1 x i8*] [i8* bitcast (void (%struct.W*)* @"\01?foo@W@@UAEXXZ" to i8*)]
+// MO1-DAG: @"\01??_7W@@6B@" = external dllimport unnamed_addr constant [1 x i8*]
// GO1-DAG: @_ZTV1W = available_externally dllimport unnamed_addr constant [3 x i8*] [i8* null, i8* null, i8* bitcast (void (%struct.W*)* @_ZN1W3fooEv to i8*)]
+struct __declspec(dllimport) W2 {
+ virtual ~W2();
+ virtual void foo() {}
+};
+USECLASS(W2)
+// vftable:
+// MO1-DAG: @"\01??_SW2@@6B@" = linkonce_odr unnamed_addr constant [2 x i8*]
+// GO1-DAG: @_ZTV2W2 = external dllimport unnamed_addr constant [5 x i8*]
+
+struct __declspec(dllimport) W3 {
+ virtual void fn() const {
+ }
+ constexpr W3() = default;
+};
+
+constexpr W3 w3;
+const W3 = w3;
+// MO1-DAG: @"\01??_SW3@@6B@" = linkonce_odr unnamed_addr constant [1 x i8*]
+
struct __declspec(dllimport) KeyFuncClass {
constexpr KeyFuncClass() {}
virtual void foo();
Index: test/CodeGenCXX/dllimport-rtti.cpp
===
--- test/CodeGenCXX/dllimport-rtti.cpp
+++ test/CodeGenCXX/dllimport-rtti.cpp
@@ -4,16 +4,25 @@
struct __declspec(dllimport) S {
virtual void f() {}
} s;
-// MSVC: [[VF_S:.*]] = private unnamed_addr constant [2 x i8*]
-// MSVC-DAG: @"\01??_SS@@6B@" = unnamed_addr alias i8*, getelementptr inbounds ([2 x i8*], [2 x i8*]* [[VF_S]], i32 0, i32 1)
+// MSVC-DAG: @"\01??_7S@@6B@" = external dllimport unnamed_addr constant [2 x i8*]
// MSVC-DAG: @"\01??_R0?AUS@@@8" = linkonce_odr
// MSVC-DAG: @"\01??_R1A@?0A@EA@S@@8" = linkonce_odr
// MSVC-DAG: @"\01??_R2S@@8" = linkonce_odr
// MSVC-DAG: @"\01??_R3S@@8" = linkonce_odr
// GNU-DAG: @_ZTV1S = available_externally dllimport
// GNU-DAG: @_ZTI1S = external dllimport
+struct __declspec(dllimport) S2 {
+ virtual void f() {}
+ virtual ~S2() {}
+} s2;
+// MSVC-DAG: [[VF_S2:.*]] = private unnamed_addr constant [3 x i8*]
+// MSVC-DAG: @"\01??_SS2@@6B@" = unnamed_addr alias i8*, getelementptr inbounds ([3 x i8*], [3 x i8*]* [[VF_S2]], i32 0, i32 1)
+// MSVC-DAG: @"\01??_R0?AUS2@@@8" = linkonce_odr
+// MSVC-DAG: @"\01??_R1A@?0A@EA@S2@@8" = linkonce_odr
+// MSVC-DAG: @"\01??_R2S2@@8" = linkonce_odr
+
struct U : S {
} u;
Index: lib/Sema/SemaDeclCXX.cpp
===
--- lib/Sema/SemaDeclCXX.cpp
+++ lib/Sema/SemaDeclCXX.cpp
@@ -4809,6 +4809,17 @@
}
}
+ if (!ClassExported) {
+if (Class->getTemplateSpecializationKind() != TSK_Undeclared)
+ Class->setNeedLocalVFTable();
+if (const CXXDestructorDecl *Destructor = Class->getDestructor()) {
+ // Classes with dllimport attribute needs local vftable if they have
+ // virtual d-tor.
+ if (Destructor->isVirtual())
+Class->setNeedLocalVFTable();
+}
+ }
+
if (ClassExported)
DelayedDllExportClasses.push_back(Class);
}
Index: lib/CodeGen/MicrosoftCXXABI.cpp
===
--- lib/CodeGen/MicrosoftCXXABI.cpp
+++ lib/CodeGen/MicrosoftCXXABI.cpp
@@ -1670,9 +1670,13 @@
//
// Because of this unique behavior, we maintain this logic here instead of
// getVTableLinkage.
- llvm::GlobalValue::LinkageTypes VFTableLinkage =
- RD->hasAttr() ? llvm::GlobalValue::LinkOnceODRLinkage
- : CGM.getVTableLinkage(RD);
+ llvm::GlobalValue::LinkageTypes VFTableLinkage = CGM.getVTableLinkage(RD);
+ if (RD->hasAttr()) {
+if (RD->getNeedLocalVFTable())
+ VFTableLinkage = llvm::GlobalValue::LinkOnceODRLinkage;
+else
+ VFTableLinkage = llvm::GlobalValue::ExternalLinkage;
+ }
bool VFTableComesFromAnotherTU =
llvm::GlobalValue::isAvailableExternallyLinkage(VFTableLinkage) ||
llvm::GlobalValue::isExternalLinkage(VFTableLinkage);
@@ -1745,7 +1749,9 @@
if (C)
VTable->setComdat(C);
- if (RD->hasAttr())
+ if (RD->hasAttr() && !RD->getNeedLocalVFTable())
+VFTable->setDLLStorageClass(llvm::GlobalValue::DLLImportStorageClass);
+ else if (RD->hasAttr())
VFTable->setDLLStorageClass(llvm::GlobalValue::DLLExportStorageClass);
VFTablesMap[ID] = VFTable;
Index:
Re: [PATCH] D22034: [MSVC][DLL] use local vftable mangling only exported classes with virtual destructor
DmitryPolukhin added a comment. In http://reviews.llvm.org/D22034#475603, @majnemer wrote: > Thinking about this some more, it is possible for clang to emit code that > will make everybody happy: > > If a class is being constructed in a constexpr context and all the vftable > entries it references are marked import, emit local vftables and reference > them in the object. If a vftable entry it references is not marked import, > report an error. > > If a class is constructed via operator new and all the vftable entries it > references are marked import, emit local vftables and store it in the object > after the constructors run. If a vftable entry it references is not marked > import, report an error. > > If a class is constructed via a local or global variable and all the vftable > entries it references are marked import, create a `dllimport > available_externally` vftable. Otherwise create a normal `dllimport > external` vftable. > > I believe this behavior captures the best behavior across the spectrum of > functionality we all care about: it supports devirtualization, constexpr and > importing classes which don't have all their vftable methods exported. I'll try to implement this approach except for special handling for constriction via new that seems to be out of scope for this issue and can be implemented independent later. Small addition, I think there is no sense to check if vftable references to entry that is not marked as dllimport. Linker will do this work for us and in some cases function may come from libraries (static or dynamic) but in the source may not be properly marked as dllimport (it is the behavior that MSVC does check it in compiler and relays on linker). http://reviews.llvm.org/D22034 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D22034: [MSVC][DLL] use local vftable mangling only exported classes with virtual destructor
DmitryPolukhin added a comment. In http://reviews.llvm.org/D22034#475551, @majnemer wrote: > In http://reviews.llvm.org/D22034#475540, @DmitryPolukhin wrote: > > > Here is B::foo is not exported but required to build vftable for D. > > > What happens if D also had a virtual destructor? It seems that in this case MSVC never inline x-tors so there is no problem with using local vftable. I don't propose to do the same inline decisions like MSVC does so Clang may need/use different set of exported members and MSVC depending on optimization levels and other things may change these sets. But it seems that never use exported vftable is not aligned with ABI. MSVC seems to be very consistent about uses exported vftable when it is possible. http://reviews.llvm.org/D22034 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D22034: [MSVC][DLL] use local vftable mangling only exported classes with virtual destructor
majnemer added a comment. Thinking about this some more, it is possible for clang to emit code that will make everybody happy: If a class is being constructed in a constexpr context and all the vftable entries it references are marked import, emit local vftables and reference them in the object. If a vftable entry it references is not marked import, report an error. If a class is constructed via operator new and all the vftable entries it references are marked import, emit local vftables and store it in the object after the constructors run. If a vftable entry it references is not marked import, report an error. If a class is constructed via a local or global variable and all the vftable entries it references are marked import, create a `dllimport available_externally` vftable. Otherwise create a normal `dllimport external` vftable. I believe this behavior captures the best behavior across the spectrum of functionality we all care about: it supports devirtualization, constexpr and importing classes which don't have all their vftable methods exported. http://reviews.llvm.org/D22034 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D22034: [MSVC][DLL] use local vftable mangling only exported classes with virtual destructor
rnk added a comment. In http://reviews.llvm.org/D22034#475540, @DmitryPolukhin wrote: > Here is B::foo is not exported but required to build vftable for D. Also user > may want to explicitly control what should be exported from his library and > may decide to remove some functions from exported interface. Sure, but there are other ways for clients of D to end up referencing the unexported symbol for B::foo, such as devirtualization. Surely you aren't proposing that we power that down just so that we can get the same import/export list as MSVC? http://reviews.llvm.org/D22034 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D22034: [MSVC][DLL] use local vftable mangling only exported classes with virtual destructor
majnemer added a comment.
In http://reviews.llvm.org/D22034#475540, @DmitryPolukhin wrote:
> In http://reviews.llvm.org/D22034#475331, @majnemer wrote:
>
> > Wait, can you give an example of MSVC exporting a vftable but not all the
> > virtual methods (other than the deleting destructor)? I don't believe I've
> > ever come across an example of this.
>
>
> It is possible in code like this:
> class B {
>
> virtual void foo();
>
>
> public:
>
> virtual void bar();
>
> };
>
> class __declspec(dllexport) D : public B {
> public:
>
> virtual void bar();
>
> };
>
> void B::foo() {}
> void B::bar() {}
> void D::bar() {}
>
> Here is B::foo is not exported but required to build vftable for D.
What happens if D also had a virtual destructor?
http://reviews.llvm.org/D22034
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D22034: [MSVC][DLL] use local vftable mangling only exported classes with virtual destructor
DmitryPolukhin added a comment.
In http://reviews.llvm.org/D22034#475331, @majnemer wrote:
> Wait, can you give an example of MSVC exporting a vftable but not all the
> virtual methods (other than the deleting destructor)? I don't believe I've
> ever come across an example of this.
It is possible in code like this:
class B {
virtual void foo();
public:
virtual void bar();
};
class __declspec(dllexport) D : public B {
public:
virtual void bar();
};
void B::foo() {}
void B::bar() {}
void D::bar() {}
Here is B::foo is not exported but required to build vftable for D. Also user
may want to explicitly control what should be exported from his library and may
decide to remove some functions from exported interface.
http://reviews.llvm.org/D22034
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D22034: [MSVC][DLL] use local vftable mangling only exported classes with virtual destructor
majnemer added a comment. In http://reviews.llvm.org/D22034#475002, @DmitryPolukhin wrote: > In http://reviews.llvm.org/D22034#474985, @majnemer wrote: > > > In http://reviews.llvm.org/D22034#474937, @DmitryPolukhin wrote: > > > > > David, do you know real programs that relay on constexpr and dllexport > > > semantic that doesn't work on MSVC? > > > > > > Yes, Chrome relied on these semantics. > > > Hm, does it mean that Chrome has some workaround to bypass that MSVC doesn't > support it? Not an explicit workaround, other MSVC bugs prevent them from miscompiling the exact sequence used in Chrome. > > > > > Anyway current implementation is not compatible with MSVC in much more > > > common case without constexp. > > > > > > > > > I don't see how it is not compatible. The address of the vftable is not > > observable as far as I know... > > > You can easily observe the difference if vftable is exported from DLL but > some virtual functions are not (for example, private). MSVC will uses > imported vftable and Clang will try to rebuild fvtable that will fail on link > time. Wait, can you give an example of MSVC exporting a vftable but not all the virtual methods (other than the deleting destructor)? I don't believe I've ever come across an example of this. http://reviews.llvm.org/D22034 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D22034: [MSVC][DLL] use local vftable mangling only exported classes with virtual destructor
rnk added a comment. In http://reviews.llvm.org/D22034#474889, @majnemer wrote: > Hmm, I'm not so sure this will work with constexpr: > ... Hah, my version of MSVC fails to install the vtable into 's', and crashes on the call to x.fn(). http://reviews.llvm.org/D22034 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D22034: [MSVC][DLL] use local vftable mangling only exported classes with virtual destructor
DmitryPolukhin added a comment. In http://reviews.llvm.org/D22034#474985, @majnemer wrote: > In http://reviews.llvm.org/D22034#474937, @DmitryPolukhin wrote: > > > David, do you know real programs that relay on constexpr and dllexport > > semantic that doesn't work on MSVC? > > > Yes, Chrome relied on these semantics. Hm, does it mean that Chrome has some workaround to bypass that MSVC doesn't support it? > > Anyway current implementation is not compatible with MSVC in much more > > common case without constexp. > > > I don't see how it is not compatible. The address of the vftable is not > observable as far as I know... You can easily observe the difference if vftable is exported from DLL but some virtual functions are not (for example, private). MSVC will uses imported vftable and Clang will try to rebuild fvtable that will fail on link time. > > At the moment my patch works with your example just because it only changes > > mangling but don't use imported vftable that seems to be wrong. > > > Why is it //wrong//? It should obey the ABI in all practical matters but it > might be less efficient space-wise. I think it is not ABI compatible use local vftable when MSVC uses imported. The easy way to observe the difference I already described but it might be other side effect if user start unloading DLL from address space. So user may expect that it is safe to unload library #1 that created object from library #2 because it doesn't have references to library #1 addresses but due to local fvtable it might result in crash. http://reviews.llvm.org/D22034 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D22034: [MSVC][DLL] use local vftable mangling only exported classes with virtual destructor
majnemer added a comment. In http://reviews.llvm.org/D22034#474937, @DmitryPolukhin wrote: > David, do you know real programs that relay on constexpr and dllexport > semantic that doesn't work on MSVC? Yes, Chrome relied on these semantics. > Anyway current implementation is not compatible with MSVC in much more common > case without constexp. I don't see how it is not compatible. The address of the vftable is not observable as far as I know... > At the moment my patch works with your example just because it only changes > mangling but don't use imported vftable that seems to be wrong. Why is it //wrong//? It should obey the ABI in all practical matters but it might be less efficient space-wise. http://reviews.llvm.org/D22034 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D22034: [MSVC][DLL] use local vftable mangling only exported classes with virtual destructor
DmitryPolukhin added a comment. I found the patch that prevents using imported vftable, http://llvm.org/viewvc/llvm-project?view=revision=260548 but the patch has no examples of constexpr + dllimport. http://reviews.llvm.org/D22034 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D22034: [MSVC][DLL] use local vftable mangling only exported classes with virtual destructor
DmitryPolukhin added a comment. David, do you know real programs that relay on constexpr and dllexport semantic that doesn't work on MSVC? If not, I think we might want to report error message instead of miss-compile it as MSVC does. Anyway current implementation is not compatible with MSVC in much more common case without constexp. At the moment my patch works with your example just because it only changes mangling but don't use imported vftable that seems to be wrong. Is it you changes somewhere that prevent using imported vftable? http://reviews.llvm.org/D22034 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D22034: [MSVC][DLL] use local vftable mangling only exported classes with virtual destructor
majnemer added a comment. FWIW, I think that we would be OK to import vftables (and thus use the _7 mangling) in contexts where their construction doesn't need to be constant. It seemed pretty complex to implement but I'd be open to such an implementation. http://reviews.llvm.org/D22034 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D22034: [MSVC][DLL] use local vftable mangling only exported classes with virtual destructor
majnemer added a comment.
Hmm, I'm not so sure this will work with constexpr:
#include
struct __declspec(dllimport) S {
virtual void fn() const {printf("%s\n", "hi");}
constexpr S() = default;
};
constexpr S s;
auto = s;
int main() {
x.fn();
}
Before my change we would reference the imported vftable symbol from the .rdata
section which will fail at link time.
IIRC, MSVC will miscompile this example.
Your change, if I understand correctly, will cause us to import the vftable
symbol which would cause this example to break again.
http://reviews.llvm.org/D22034
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D22034: [MSVC][DLL] use local vftable mangling only exported classes with virtual destructor
DmitryPolukhin created this revision.
DmitryPolukhin added a reviewer: rnk.
DmitryPolukhin added a subscriber: cfe-commits.
MSVC uses non-local mangling for vftable if class with dllexport attribute has
no virtual destructor. I checked all examples that I fixed with MSVC and it
uses '_7' in all cases. For more info see old thread in
microsoft.public.vc.language:
https://groups.google.com/d/msg/microsoft.public.vc.language/atSh_2VSc2w/EgJ3r_7OzVUJ
http://reviews.llvm.org/D22034
Files:
lib/AST/MicrosoftMangle.cpp
test/CodeGenCXX/PR26569.cpp
test/CodeGenCXX/dllimport-rtti.cpp
test/CodeGenCXX/dllimport.cpp
Index: test/CodeGenCXX/dllimport.cpp
===
--- test/CodeGenCXX/dllimport.cpp
+++ test/CodeGenCXX/dllimport.cpp
@@ -620,9 +620,18 @@
struct __declspec(dllimport) W { virtual void foo() {} };
USECLASS(W)
// vftable:
-// MO1-DAG: @"\01??_SW@@6B@" = linkonce_odr unnamed_addr constant [1 x i8*]
[i8* bitcast (void (%struct.W*)* @"\01?foo@W@@UAEXXZ" to i8*)]
+// MO1-DAG: @"\01??_7W@@6B@" = linkonce_odr unnamed_addr constant [1 x i8*]
[i8* bitcast (void (%struct.W*)* @"\01?foo@W@@UAEXXZ" to i8*)]
// GO1-DAG: @_ZTV1W = available_externally dllimport unnamed_addr constant [3
x i8*] [i8* null, i8* null, i8* bitcast (void (%struct.W*)* @_ZN1W3fooEv to
i8*)]
+struct __declspec(dllimport) W2 {
+ virtual ~W2();
+ virtual void foo() {}
+};
+USECLASS(W2)
+// vftable:
+// MO1-DAG: @"\01??_SW2@@6B@" = linkonce_odr unnamed_addr constant [2 x i8*]
+// GO1-DAG: @_ZTV2W2 = external dllimport unnamed_addr constant [5 x i8*]
+
struct __declspec(dllimport) KeyFuncClass {
constexpr KeyFuncClass() {}
virtual void foo();
Index: test/CodeGenCXX/dllimport-rtti.cpp
===
--- test/CodeGenCXX/dllimport-rtti.cpp
+++ test/CodeGenCXX/dllimport-rtti.cpp
@@ -4,8 +4,8 @@
struct __declspec(dllimport) S {
virtual void f() {}
} s;
-// MSVC: [[VF_S:.*]] = private unnamed_addr constant [2 x i8*]
-// MSVC-DAG: @"\01??_SS@@6B@" = unnamed_addr alias i8*, getelementptr inbounds
([2 x i8*], [2 x i8*]* [[VF_S]], i32 0, i32 1)
+// MSVC: [[VF_7:.*]] = private unnamed_addr constant [2 x i8*]
+// MSVC-DAG: @"\01??_7S@@6B@" = unnamed_addr alias i8*, getelementptr inbounds
([2 x i8*], [2 x i8*]* [[VF_7]], i32 0, i32 1)
// MSVC-DAG: @"\01??_R0?AUS@@@8" = linkonce_odr
// MSVC-DAG: @"\01??_R1A@?0A@EA@S@@8" = linkonce_odr
// MSVC-DAG: @"\01??_R2S@@8" = linkonce_odr
Index: test/CodeGenCXX/PR26569.cpp
===
--- test/CodeGenCXX/PR26569.cpp
+++ test/CodeGenCXX/PR26569.cpp
@@ -4,17 +4,17 @@
virtual void m_fn1();
};
template
-class B : virtual A {};
+class B : public virtual A {};
extern template class __declspec(dllimport) B;
class __declspec(dllexport) C : B {};
// CHECK-DAG: @[[VTABLE_C:.*]] = private unnamed_addr constant [2 x i8*] [i8*
bitcast (%rtti.CompleteObjectLocator* @"\01??_R4C@@6B@" to i8*), i8* bitcast
(void (%class.A*)* @"\01?m_fn1@A@@EAEXXZ" to i8*)]
-// CHECK-DAG: @[[VTABLE_B:.*]] = private unnamed_addr constant [2 x i8*] [i8*
bitcast (%rtti.CompleteObjectLocator* @"\01??_R4?$B@H@@6B@" to i8*), i8*
bitcast (void (%class.A*)* @"\01?m_fn1@A@@EAEXXZ" to i8*)],
comdat($"\01??_S?$B@H@@6B@")
+// CHECK-DAG: @[[VTABLE_B:.*]] = private unnamed_addr constant [2 x i8*] [i8*
bitcast (%rtti.CompleteObjectLocator* @"\01??_R4?$B@H@@6B@" to i8*), i8*
bitcast (void (%class.A*)* @"\01?m_fn1@A@@EAEXXZ" to i8*)],
comdat($"\01??_7?$B@H@@6B@")
// CHECK-DAG: @[[VTABLE_A:.*]] = private unnamed_addr constant [2 x i8*] [i8*
bitcast (%rtti.CompleteObjectLocator* @"\01??_R4A@@6B@" to i8*), i8* bitcast
(void (%class.A*)* @"\01?m_fn1@A@@EAEXXZ" to i8*)], comdat($"\01??_7A@@6B@")
// CHECK-DAG: @"\01??_7C@@6B@" = dllexport unnamed_addr alias i8*,
getelementptr inbounds ([2 x i8*], [2 x i8*]* @[[VTABLE_C]], i32 0, i32 1)
-// CHECK-DAG: @"\01??_S?$B@H@@6B@" = unnamed_addr alias i8*, getelementptr
inbounds ([2 x i8*], [2 x i8*]* @[[VTABLE_B]], i32 0, i32 1)
+// CHECK-DAG: @"\01??_7?$B@H@@6B@" = unnamed_addr alias i8*, getelementptr
inbounds ([2 x i8*], [2 x i8*]* @[[VTABLE_B]], i32 0, i32 1)
// CHECK-DAG: @"\01??_7A@@6B@" = unnamed_addr alias i8*, getelementptr
inbounds ([2 x i8*], [2 x i8*]* @[[VTABLE_A]], i32 0, i32 1)
// CHECK-DAG: @"\01??_8?$B@H@@7B@" = available_externally dllimport
unnamed_addr constant [2 x i32] [i32 0, i32 4]
Index: lib/AST/MicrosoftMangle.cpp
===
--- lib/AST/MicrosoftMangle.cpp
+++ lib/AST/MicrosoftMangle.cpp
@@ -2601,7 +2601,15 @@
// is always '6' for vftables.
msvc_hashing_ostream MHO(Out);
MicrosoftCXXNameMangler Mangler(*this, MHO);
- if (Derived->hasAttr())
+ bool NeedLocalVFT = false;
+ if (Derived->hasAttr()) {
+// Only dllimported classes with virtual d-tor may need local vtbl
+// for proper memory deallocation
