Prazek created this revision.
We need to emit barrier if the union field
is CXXRecordDecl because it might have vptrs. The testcode
was wrongly devirtualized. It also proves that having different
groups for different dynamic types is not sufficient.
https://reviews.llvm.org/D31830
Files:
lib/CodeGen/CGExpr.cpp
test/CodeGenCXX/strict-vtable-pointers.cpp
Index: test/CodeGenCXX/strict-vtable-pointers.cpp
===================================================================
--- test/CodeGenCXX/strict-vtable-pointers.cpp
+++ test/CodeGenCXX/strict-vtable-pointers.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 %s -I%S -triple=x86_64-apple-darwin10
-fstrict-vtable-pointers -disable-llvm-passes -O2 -emit-llvm -o %t.ll
+// RUN: %clang_cc1 %s -I%S -triple=x86_64-apple-darwin10
-fstrict-vtable-pointers -std=c++11 -disable-llvm-passes -O2 -emit-llvm -o %t.ll
// RUN: FileCheck --check-prefix=CHECK-CTORS %s < %t.ll
// RUN: FileCheck --check-prefix=CHECK-NEW %s < %t.ll
// RUN: FileCheck --check-prefix=CHECK-DTORS %s < %t.ll
@@ -180,6 +180,62 @@
// CHECK-CTORS-NOT: @llvm.invariant.group.barrier(
// CHECK-CTORS-LABEL: {{^}}}
+struct A {
+ virtual void foo();
+};
+struct B : A {
+ virtual void foo();
+};
+
+union U {
+ A a;
+ B b;
+};
+
+void changeToB(U *u);
+void changeToA(U *u);
+
+void g2(A *a) {
+ a->foo();
+}
+// We have to guard access to union fields with invariant.group, because
+// it is very easy to skip the barrier with unions. In this example the inlined
+// g2 will produce loads with the same !invariant.group metadata, and
+// u->a and u->b would use the same pointer.
+// CHECK-NEW-LABEL: define void @_Z14UnionsBarriersP1U
+void UnionsBarriers(U *u) {
+ // CHECK-NEW: call void @_Z9changeToBP1U(
+ changeToB(u);
+ // CHECK-NEW: call i8* @llvm.invariant.group.barrier(i8*
+ // CHECK-NEW: call void @_Z2g2P1A(%struct.A*
+ g2(&u->b);
+ // CHECK-NEW: call void @_Z9changeToAP1U(%union.U* %6)
+ changeToA(u);
+ // CHECK-NEW: call i8* @llvm.invariant.group.barrier(i8*
+ // call void @_Z2g2P1A(%struct.A* %a)
+ g2(&u->a);
+ // CHECK-NEW-NOT: call i8* @llvm.invariant.group.barrier(i8*
+}
+
+struct HoldingVirtuals {
+ A a;
+};
+
+union U2 {
+ HoldingVirtuals h;
+ int z;
+};
+void take(HoldingVirtuals &);
+
+// CHECK-NEW-LABEL: define void @_Z15UnionsBarriers2R2U2
+void UnionsBarriers2(U2 &u) {
+ // CHECK-NEW-NOT: call i8* @llvm.invariant.group.barrier(i8*
+ // CHECK-NEW: 42
+ u.z += 42;
+ // CHECK-NEW: call i8* @llvm.invariant.group.barrier(i8*
+ // CHECK-NEW: call void @_Z4takeR15HoldingVirtuals(
+ take(u.h);
+}
/** DTORS **/
// CHECK-DTORS-LABEL: define linkonce_odr void @_ZN10StaticBaseD2Ev(
Index: lib/CodeGen/CGExpr.cpp
===================================================================
--- lib/CodeGen/CGExpr.cpp
+++ lib/CodeGen/CGExpr.cpp
@@ -3508,6 +3508,15 @@
assert(!type->isReferenceType() && "union has reference member");
// TODO: handle path-aware TBAA for union.
TBAAPath = false;
+ // Because unions can easily skip invariant.barriers, we need to add
+ // a barrier every time CXXRecord field is referenced. We don't check if
+ // it is dynamic type because it might have dynamic member, so we would
+ // have to check it recursively.
+ if (field->getType().getTypePtr()->getAsCXXRecordDecl() &&
+ CGM.getCodeGenOpts().StrictVTablePointers &&
+ CGM.getCodeGenOpts().OptimizationLevel > 0)
+ addr = Address(Builder.CreateInvariantGroupBarrier(addr.getPointer()),
+ addr.getAlignment());
} else {
// For structs, we GEP to the field that the record layout suggests.
addr = emitAddrOfFieldStorage(*this, addr, field);
Index: test/CodeGenCXX/strict-vtable-pointers.cpp
===================================================================
--- test/CodeGenCXX/strict-vtable-pointers.cpp
+++ test/CodeGenCXX/strict-vtable-pointers.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 %s -I%S -triple=x86_64-apple-darwin10 -fstrict-vtable-pointers -disable-llvm-passes -O2 -emit-llvm -o %t.ll
+// RUN: %clang_cc1 %s -I%S -triple=x86_64-apple-darwin10 -fstrict-vtable-pointers -std=c++11 -disable-llvm-passes -O2 -emit-llvm -o %t.ll
// RUN: FileCheck --check-prefix=CHECK-CTORS %s < %t.ll
// RUN: FileCheck --check-prefix=CHECK-NEW %s < %t.ll
// RUN: FileCheck --check-prefix=CHECK-DTORS %s < %t.ll
@@ -180,6 +180,62 @@
// CHECK-CTORS-NOT: @llvm.invariant.group.barrier(
// CHECK-CTORS-LABEL: {{^}}}
+struct A {
+ virtual void foo();
+};
+struct B : A {
+ virtual void foo();
+};
+
+union U {
+ A a;
+ B b;
+};
+
+void changeToB(U *u);
+void changeToA(U *u);
+
+void g2(A *a) {
+ a->foo();
+}
+// We have to guard access to union fields with invariant.group, because
+// it is very easy to skip the barrier with unions. In this example the inlined
+// g2 will produce loads with the same !invariant.group metadata, and
+// u->a and u->b would use the same pointer.
+// CHECK-NEW-LABEL: define void @_Z14UnionsBarriersP1U
+void UnionsBarriers(U *u) {
+ // CHECK-NEW: call void @_Z9changeToBP1U(
+ changeToB(u);
+ // CHECK-NEW: call i8* @llvm.invariant.group.barrier(i8*
+ // CHECK-NEW: call void @_Z2g2P1A(%struct.A*
+ g2(&u->b);
+ // CHECK-NEW: call void @_Z9changeToAP1U(%union.U* %6)
+ changeToA(u);
+ // CHECK-NEW: call i8* @llvm.invariant.group.barrier(i8*
+ // call void @_Z2g2P1A(%struct.A* %a)
+ g2(&u->a);
+ // CHECK-NEW-NOT: call i8* @llvm.invariant.group.barrier(i8*
+}
+
+struct HoldingVirtuals {
+ A a;
+};
+
+union U2 {
+ HoldingVirtuals h;
+ int z;
+};
+void take(HoldingVirtuals &);
+
+// CHECK-NEW-LABEL: define void @_Z15UnionsBarriers2R2U2
+void UnionsBarriers2(U2 &u) {
+ // CHECK-NEW-NOT: call i8* @llvm.invariant.group.barrier(i8*
+ // CHECK-NEW: 42
+ u.z += 42;
+ // CHECK-NEW: call i8* @llvm.invariant.group.barrier(i8*
+ // CHECK-NEW: call void @_Z4takeR15HoldingVirtuals(
+ take(u.h);
+}
/** DTORS **/
// CHECK-DTORS-LABEL: define linkonce_odr void @_ZN10StaticBaseD2Ev(
Index: lib/CodeGen/CGExpr.cpp
===================================================================
--- lib/CodeGen/CGExpr.cpp
+++ lib/CodeGen/CGExpr.cpp
@@ -3508,6 +3508,15 @@
assert(!type->isReferenceType() && "union has reference member");
// TODO: handle path-aware TBAA for union.
TBAAPath = false;
+ // Because unions can easily skip invariant.barriers, we need to add
+ // a barrier every time CXXRecord field is referenced. We don't check if
+ // it is dynamic type because it might have dynamic member, so we would
+ // have to check it recursively.
+ if (field->getType().getTypePtr()->getAsCXXRecordDecl() &&
+ CGM.getCodeGenOpts().StrictVTablePointers &&
+ CGM.getCodeGenOpts().OptimizationLevel > 0)
+ addr = Address(Builder.CreateInvariantGroupBarrier(addr.getPointer()),
+ addr.getAlignment());
} else {
// For structs, we GEP to the field that the record layout suggests.
addr = emitAddrOfFieldStorage(*this, addr, field);
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
