[PATCH] D47299: [CodeGenCXX] Emit strip.invariant.group with -fstrict-vtable-pointers
This revision was not accepted when it landed; it landed in state "Needs
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rC336137: [CodeGenCXX] Emit strip.invariant.group with
-fstrict-vtable-pointers (authored by Prazek, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D47299?vs=151575=153767#toc
Repository:
rC Clang
https://reviews.llvm.org/D47299
Files:
include/clang/AST/DeclCXX.h
include/clang/AST/Type.h
lib/AST/Type.cpp
lib/CodeGen/CGExpr.cpp
lib/CodeGen/CGExprScalar.cpp
test/CodeGenCXX/strict-vtable-pointers.cpp
Index: lib/CodeGen/CGExpr.cpp
===
--- lib/CodeGen/CGExpr.cpp
+++ lib/CodeGen/CGExpr.cpp
@@ -3870,6 +3870,18 @@
}
Address addr = base.getAddress();
+ if (auto *ClassDef = dyn_cast(rec)) {
+if (CGM.getCodeGenOpts().StrictVTablePointers &&
+ClassDef->isDynamicClass()) {
+ // Getting to any field of dynamic object requires stripping dynamic
+ // information provided by invariant.group. This is because accessing
+ // fields may leak the real address of dynamic object, which could result
+ // in miscompilation when leaked pointer would be compared.
+ auto *stripped = Builder.CreateStripInvariantGroup(addr.getPointer());
+ addr = Address(stripped, addr.getAlignment());
+}
+ }
+
unsigned RecordCVR = base.getVRQualifiers();
if (rec->isUnion()) {
// For unions, there is no pointer adjustment.
Index: lib/CodeGen/CGExprScalar.cpp
===
--- lib/CodeGen/CGExprScalar.cpp
+++ lib/CodeGen/CGExprScalar.cpp
@@ -1621,6 +1621,24 @@
CE->getLocStart());
}
+if (CGF.CGM.getCodeGenOpts().StrictVTablePointers) {
+ const QualType SrcType = E->getType();
+
+ if (SrcType.mayBeNotDynamicClass() && DestTy.mayBeDynamicClass()) {
+// Casting to pointer that could carry dynamic information (provided by
+// invariant.group) requires launder.
+Src = Builder.CreateLaunderInvariantGroup(Src);
+ } else if (SrcType.mayBeDynamicClass() && DestTy.mayBeNotDynamicClass()) {
+// Casting to pointer that does not carry dynamic information (provided
+// by invariant.group) requires stripping it. Note that we don't do it
+// if the source could not be dynamic type and destination could be
+// dynamic because dynamic information is already laundered. It is
+// because launder(strip(src)) == launder(src), so there is no need to
+// add extra strip before launder.
+Src = Builder.CreateStripInvariantGroup(Src);
+ }
+}
+
return Builder.CreateBitCast(Src, DstTy);
}
case CK_AddressSpaceConversion: {
@@ -1757,12 +1775,31 @@
llvm::Value* IntResult =
Builder.CreateIntCast(Src, MiddleTy, InputSigned, "conv");
-return Builder.CreateIntToPtr(IntResult, DestLLVMTy);
+auto *IntToPtr = Builder.CreateIntToPtr(IntResult, DestLLVMTy);
+
+if (CGF.CGM.getCodeGenOpts().StrictVTablePointers) {
+ // Going from integer to pointer that could be dynamic requires reloading
+ // dynamic information from invariant.group.
+ if (DestTy.mayBeDynamicClass())
+IntToPtr = Builder.CreateLaunderInvariantGroup(IntToPtr);
+}
+return IntToPtr;
}
- case CK_PointerToIntegral:
+ case CK_PointerToIntegral: {
assert(!DestTy->isBooleanType() && "bool should use PointerToBool");
-return Builder.CreatePtrToInt(Visit(E), ConvertType(DestTy));
+auto *PtrExpr = Visit(E);
+
+if (CGF.CGM.getCodeGenOpts().StrictVTablePointers) {
+ const QualType SrcType = E->getType();
+ // Casting to integer requires stripping dynamic information as it does
+ // not carries it.
+ if (SrcType.mayBeDynamicClass())
+PtrExpr = Builder.CreateStripInvariantGroup(PtrExpr);
+}
+
+return Builder.CreatePtrToInt(PtrExpr, ConvertType(DestTy));
+ }
case CK_ToVoid: {
CGF.EmitIgnoredExpr(E);
return nullptr;
@@ -3241,6 +3278,23 @@
Result = Builder.CreateICmp(SICmpOpc, LHS, RHS, "cmp");
} else {
// Unsigned integers and pointers.
+
+ if (CGF.CGM.getCodeGenOpts().StrictVTablePointers &&
+ !isa(LHS) &&
+ !isa(RHS)) {
+
+// Dynamic information is required to be stripped for comparisons,
+// because it could leak the dynamic information. Based on comparisons
+// of pointers to dynamic objects, the optimizer can replace one pointer
+// with another, which might be incorrect in presence of invariant
+// groups. Comparison with null is safe because null does not carry any
+// dynamic information.
+if (LHSTy.mayBeDynamicClass())
+ LHS = Builder.CreateStripInvariantGroup(LHS);
+if (RHSTy.mayBeDynamicClass())
+ RHS =
[PATCH] D47299: [CodeGenCXX] Emit strip.invariant.group with -fstrict-vtable-pointers
rjmccall added inline comments.
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:1647
+ }
+}
+
Prazek wrote:
> rsmith wrote:
> > Prazek wrote:
> > > rjmccall wrote:
> > > > Prazek wrote:
> > > > > rjmccall wrote:
> > > > > > Incidentally, how do you protect against code like this?
> > > > > >
> > > > > > A *ptr;
> > > > > > reinterpret_cast(ptr) = new B();
> > > > > > ptr->foo();
> > > > > >
> > > > > > Presumably there needs to be a launder/strip here, but I guess it
> > > > > > would have to be introduced by the middle-end when forwarding the
> > > > > > store? The way I've written this is an aliasing violation, but (1)
> > > > > > I assume your pass isn't disabled whenever strict-aliasing is
> > > > > > disabled and (2) you can do this with a memcpy and still pretty
> > > > > > reliably expect that LLVM will be able to eventually forward the
> > > > > > store.
> > > > > Can you add more info on what is A and B so I can make sure I
> > > > > understand it correctly?
> > > > > Is the prefix of the layout the same for both, but they are not in
> > > > > the hierarchy?
> > > > >
> > > > > I haven't thought about the strict aliasing. I think the only sane
> > > > > way would be to require strict aliasing for the strict vtable
> > > > > pointers.
> > > > It's whatever case you're worried about such that you have to launder
> > > > member accesses and bitcasts.
> > > >
> > > > And like I mentioned, relying on strict aliasing isn't enough because
> > > > you can do it legally with memcpy. Maybe it's okay to consider it UB?
> > > > I'm not sure about that.
> > > AFAIK reinterpreting one class as another is UB if they are not in
> > > hierarchy (especially calling virtual function on reinterpreted class),
> > > not sure if strict aliasing should allow it anyway (if it would be a hand
> > > written custom vptr then it should be ok with strict aliasing turned off,
> > > but with vptr I don't think it is legal).
> > > @rsmith Can you comment on that?
> > OK, here's how I think about what we're doing here:
> >
> > We view the IR-level pointer value for a pointer to a dynamic class type as
> > being a fat pointer, containing the actual pointer value and also a tag
> > indicating the dynamic type of the object (only notionally, though -- the
> > actual bit representation of the pointer doesn't include the extra
> > information, but we don't ever emit IR that inspects the bit representation
> > of the fat pointer to avoid exposing that fact). In that model, if you try
> > to type pun between a pointer to a dynamic class type and a pointer to a
> > non-dynamic-class type, that can't be expected to work because the
> > (notional) value is different, much as type punning between a derived and
> > base class pointer wouldn't work for a pointer to something other than a
> > base class at offset zero.
> >
> > I think @rjmccall's example is OK, because both `A` and `B` would need to
> > be dynamic class types in a hierarchy to work, and that means we'd be using
> > the same notional pointer representation. A slight variation of that
> > example:
> >
> > ```
> > struct A {};
> > struct B : A { virtual void f(); };
> > struct C : B { void f(); } c;
> > A *p =
> > B *q;
> > memcpy(, , sizeof(B*)); // or q = std::bit_cast(p);
> > q->f();
> > ```
> >
> > ... would be UB, because the representation of an `A*` and a `B*` are
> > different (a `B*` contains a tag and an `A*` does not).
> Does this answer satisfy you John? Can I push it to trunk?
Yeah, Richard's answer makes sense to me.
Repository:
rL LLVM
https://reviews.llvm.org/D47299
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47299: [CodeGenCXX] Emit strip.invariant.group with -fstrict-vtable-pointers
Prazek marked 10 inline comments as done.
Prazek added inline comments.
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:1647
+ }
+}
+
rsmith wrote:
> Prazek wrote:
> > rjmccall wrote:
> > > Prazek wrote:
> > > > rjmccall wrote:
> > > > > Incidentally, how do you protect against code like this?
> > > > >
> > > > > A *ptr;
> > > > > reinterpret_cast(ptr) = new B();
> > > > > ptr->foo();
> > > > >
> > > > > Presumably there needs to be a launder/strip here, but I guess it
> > > > > would have to be introduced by the middle-end when forwarding the
> > > > > store? The way I've written this is an aliasing violation, but (1) I
> > > > > assume your pass isn't disabled whenever strict-aliasing is disabled
> > > > > and (2) you can do this with a memcpy and still pretty reliably
> > > > > expect that LLVM will be able to eventually forward the store.
> > > > Can you add more info on what is A and B so I can make sure I
> > > > understand it correctly?
> > > > Is the prefix of the layout the same for both, but they are not in the
> > > > hierarchy?
> > > >
> > > > I haven't thought about the strict aliasing. I think the only sane way
> > > > would be to require strict aliasing for the strict vtable pointers.
> > > It's whatever case you're worried about such that you have to launder
> > > member accesses and bitcasts.
> > >
> > > And like I mentioned, relying on strict aliasing isn't enough because you
> > > can do it legally with memcpy. Maybe it's okay to consider it UB? I'm
> > > not sure about that.
> > AFAIK reinterpreting one class as another is UB if they are not in
> > hierarchy (especially calling virtual function on reinterpreted class), not
> > sure if strict aliasing should allow it anyway (if it would be a hand
> > written custom vptr then it should be ok with strict aliasing turned off,
> > but with vptr I don't think it is legal).
> > @rsmith Can you comment on that?
> OK, here's how I think about what we're doing here:
>
> We view the IR-level pointer value for a pointer to a dynamic class type as
> being a fat pointer, containing the actual pointer value and also a tag
> indicating the dynamic type of the object (only notionally, though -- the
> actual bit representation of the pointer doesn't include the extra
> information, but we don't ever emit IR that inspects the bit representation
> of the fat pointer to avoid exposing that fact). In that model, if you try to
> type pun between a pointer to a dynamic class type and a pointer to a
> non-dynamic-class type, that can't be expected to work because the (notional)
> value is different, much as type punning between a derived and base class
> pointer wouldn't work for a pointer to something other than a base class at
> offset zero.
>
> I think @rjmccall's example is OK, because both `A` and `B` would need to be
> dynamic class types in a hierarchy to work, and that means we'd be using the
> same notional pointer representation. A slight variation of that example:
>
> ```
> struct A {};
> struct B : A { virtual void f(); };
> struct C : B { void f(); } c;
> A *p =
> B *q;
> memcpy(, , sizeof(B*)); // or q = std::bit_cast(p);
> q->f();
> ```
>
> ... would be UB, because the representation of an `A*` and a `B*` are
> different (a `B*` contains a tag and an `A*` does not).
Does this answer satisfy you John? Can I push it to trunk?
Repository:
rL LLVM
https://reviews.llvm.org/D47299
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47299: [CodeGenCXX] Emit strip.invariant.group with -fstrict-vtable-pointers
rsmith added inline comments.
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:1647
+ }
+}
+
Prazek wrote:
> rjmccall wrote:
> > Prazek wrote:
> > > rjmccall wrote:
> > > > Incidentally, how do you protect against code like this?
> > > >
> > > > A *ptr;
> > > > reinterpret_cast(ptr) = new B();
> > > > ptr->foo();
> > > >
> > > > Presumably there needs to be a launder/strip here, but I guess it would
> > > > have to be introduced by the middle-end when forwarding the store? The
> > > > way I've written this is an aliasing violation, but (1) I assume your
> > > > pass isn't disabled whenever strict-aliasing is disabled and (2) you
> > > > can do this with a memcpy and still pretty reliably expect that LLVM
> > > > will be able to eventually forward the store.
> > > Can you add more info on what is A and B so I can make sure I understand
> > > it correctly?
> > > Is the prefix of the layout the same for both, but they are not in the
> > > hierarchy?
> > >
> > > I haven't thought about the strict aliasing. I think the only sane way
> > > would be to require strict aliasing for the strict vtable pointers.
> > It's whatever case you're worried about such that you have to launder
> > member accesses and bitcasts.
> >
> > And like I mentioned, relying on strict aliasing isn't enough because you
> > can do it legally with memcpy. Maybe it's okay to consider it UB? I'm not
> > sure about that.
> AFAIK reinterpreting one class as another is UB if they are not in hierarchy
> (especially calling virtual function on reinterpreted class), not sure if
> strict aliasing should allow it anyway (if it would be a hand written custom
> vptr then it should be ok with strict aliasing turned off, but with vptr I
> don't think it is legal).
> @rsmith Can you comment on that?
OK, here's how I think about what we're doing here:
We view the IR-level pointer value for a pointer to a dynamic class type as
being a fat pointer, containing the actual pointer value and also a tag
indicating the dynamic type of the object (only notionally, though -- the
actual bit representation of the pointer doesn't include the extra information,
but we don't ever emit IR that inspects the bit representation of the fat
pointer to avoid exposing that fact). In that model, if you try to type pun
between a pointer to a dynamic class type and a pointer to a non-dynamic-class
type, that can't be expected to work because the (notional) value is different,
much as type punning between a derived and base class pointer wouldn't work for
a pointer to something other than a base class at offset zero.
I think @rjmccall's example is OK, because both `A` and `B` would need to be
dynamic class types in a hierarchy to work, and that means we'd be using the
same notional pointer representation. A slight variation of that example:
```
struct A {};
struct B : A { virtual void f(); };
struct C : B { void f(); } c;
A *p =
B *q;
memcpy(, , sizeof(B*)); // or q = std::bit_cast(p);
q->f();
```
... would be UB, because the representation of an `A*` and a `B*` are different
(a `B*` contains a tag and an `A*` does not).
Repository:
rL LLVM
https://reviews.llvm.org/D47299
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47299: [CodeGenCXX] Emit strip.invariant.group with -fstrict-vtable-pointers
Prazek added inline comments. Comment at: clang/lib/CodeGen/CGExprScalar.cpp:1647 + } +} + rjmccall wrote: > Prazek wrote: > > rjmccall wrote: > > > Incidentally, how do you protect against code like this? > > > > > > A *ptr; > > > reinterpret_cast(ptr) = new B(); > > > ptr->foo(); > > > > > > Presumably there needs to be a launder/strip here, but I guess it would > > > have to be introduced by the middle-end when forwarding the store? The > > > way I've written this is an aliasing violation, but (1) I assume your > > > pass isn't disabled whenever strict-aliasing is disabled and (2) you can > > > do this with a memcpy and still pretty reliably expect that LLVM will be > > > able to eventually forward the store. > > Can you add more info on what is A and B so I can make sure I understand it > > correctly? > > Is the prefix of the layout the same for both, but they are not in the > > hierarchy? > > > > I haven't thought about the strict aliasing. I think the only sane way > > would be to require strict aliasing for the strict vtable pointers. > It's whatever case you're worried about such that you have to launder member > accesses and bitcasts. > > And like I mentioned, relying on strict aliasing isn't enough because you can > do it legally with memcpy. Maybe it's okay to consider it UB? I'm not sure > about that. AFAIK reinterpreting one class as another is UB if they are not in hierarchy (especially calling virtual function on reinterpreted class), not sure if strict aliasing should allow it anyway (if it would be a hand written custom vptr then it should be ok with strict aliasing turned off, but with vptr I don't think it is legal). @rsmith Can you comment on that? Repository: rL LLVM https://reviews.llvm.org/D47299 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47299: [CodeGenCXX] Emit strip.invariant.group with -fstrict-vtable-pointers
rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGExprScalar.cpp:1623 + const CXXRecordDecl *SourceClassDecl = + E->getType().getTypePtr()->getPointeeCXXRecordDecl(); + const CXXRecordDecl *DstClassDecl = DestTy->getPointeeCXXRecordDecl(); Prazek wrote: > rjmccall wrote: > > Unnecessary `getTypePtr()`. > getType returns QualType and it does not have getPointeeCXXRecordDecl. Am I > missing something? `operator->` on `QualType` drills to the `Type*`. Comment at: clang/lib/CodeGen/CGExprScalar.cpp:1647 + } +} + Prazek wrote: > rjmccall wrote: > > Incidentally, how do you protect against code like this? > > > > A *ptr; > > reinterpret_cast(ptr) = new B(); > > ptr->foo(); > > > > Presumably there needs to be a launder/strip here, but I guess it would > > have to be introduced by the middle-end when forwarding the store? The way > > I've written this is an aliasing violation, but (1) I assume your pass > > isn't disabled whenever strict-aliasing is disabled and (2) you can do this > > with a memcpy and still pretty reliably expect that LLVM will be able to > > eventually forward the store. > Can you add more info on what is A and B so I can make sure I understand it > correctly? > Is the prefix of the layout the same for both, but they are not in the > hierarchy? > > I haven't thought about the strict aliasing. I think the only sane way would > be to require strict aliasing for the strict vtable pointers. It's whatever case you're worried about such that you have to launder member accesses and bitcasts. And like I mentioned, relying on strict aliasing isn't enough because you can do it legally with memcpy. Maybe it's okay to consider it UB? I'm not sure about that. Repository: rL LLVM https://reviews.llvm.org/D47299 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47299: [CodeGenCXX] Emit strip.invariant.group with -fstrict-vtable-pointers
Prazek marked an inline comment as done. Prazek added inline comments. Comment at: clang/lib/CodeGen/CGExprScalar.cpp:1624 + E->getType().getTypePtr()->getPointeeCXXRecordDecl(); + const CXXRecordDecl *DstClassDecl = DestTy->getPointeeCXXRecordDecl(); + rjmccall wrote: > The opposite of `Dst` is `Src`. Alternatively, the opposite of `Source` is > `Destination` (or `Result`). Please pick. Not sure if it still holds (if the DestTy should be DstTy, but I haven't declared it myself) Repository: rL LLVM https://reviews.llvm.org/D47299 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47299: [CodeGenCXX] Emit strip.invariant.group with -fstrict-vtable-pointers
Prazek added inline comments.
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:1633
+ bool DstMayNotBeDynamic =
+ !DstClassDecl || DstClassDecl->mayBeNotDynamicClass();
+ if (SourceMayBeNotDynamic && DstMayBeDynamic) {
rjmccall wrote:
> If you made a couple of tiny helper functions here that you could invoke on
> either `SourceClassDecl` or `DstClassDecl`, you could avoid some redundant
> logic and also make the calls self-documenting enough to legibly inline into
> the `if`-conditions.
>
> ...in fact, since you start from a QualType in every case, maybe you should
> just define the helper as a method there.
oh yea, it is definitely better!
Repository:
rL LLVM
https://reviews.llvm.org/D47299
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47299: [CodeGenCXX] Emit strip.invariant.group with -fstrict-vtable-pointers
Prazek updated this revision to Diff 151575.
Prazek added a comment.
Refactor
Repository:
rL LLVM
https://reviews.llvm.org/D47299
Files:
clang/include/clang/AST/DeclCXX.h
clang/include/clang/AST/Type.h
clang/lib/AST/Type.cpp
clang/lib/CodeGen/CGExpr.cpp
clang/lib/CodeGen/CGExprScalar.cpp
clang/test/CodeGenCXX/strict-vtable-pointers.cpp
Index: clang/test/CodeGenCXX/strict-vtable-pointers.cpp
===
--- clang/test/CodeGenCXX/strict-vtable-pointers.cpp
+++ clang/test/CodeGenCXX/strict-vtable-pointers.cpp
@@ -5,7 +5,8 @@
// RUN: FileCheck --check-prefix=CHECK-LINK-REQ %s < %t.ll
typedef __typeof__(sizeof(0)) size_t;
-void *operator new(size_t, void*) throw();
+void *operator new(size_t, void *) throw();
+using uintptr_t = unsigned long long;
struct NotTrivialDtor {
~NotTrivialDtor();
@@ -17,7 +18,7 @@
};
struct DynamicDerived : DynamicBase1 {
- void foo();
+ void foo() override;
};
struct DynamicBase2 {
@@ -28,8 +29,8 @@
};
struct DynamicDerivedMultiple : DynamicBase1, DynamicBase2 {
- virtual void foo();
- virtual void bar();
+ void foo() override;
+ void bar() override;
};
struct StaticBase {
@@ -47,9 +48,8 @@
struct DynamicFromVirtualStatic2 : virtual StaticBase {
};
-struct DynamicFrom2Virtuals :
-DynamicFromVirtualStatic1,
-DynamicFromVirtualStatic2 {
+struct DynamicFrom2Virtuals : DynamicFromVirtualStatic1,
+ DynamicFromVirtualStatic2 {
};
// CHECK-NEW-LABEL: define void @_Z12LocalObjectsv()
@@ -89,7 +89,6 @@
// CHECK-CTORS: call i8* @llvm.launder.invariant.group.p0i8(
// CHECK-CTORS-LABEL: {{^}}}
-
// CHECK-NEW-LABEL: define void @_Z9Pointers1v()
// CHECK-NEW-NOT: @llvm.launder.invariant.group.p0i8(
// CHECK-NEW-LABEL: call void @_ZN12DynamicBase1C1Ev(
@@ -134,7 +133,6 @@
// CHECK-CTORS-NOT: call i8* @llvm.launder.invariant.group.p0i8(
// CHECK-CTORS-LABEL: {{^}}}
-
struct DynamicDerived;
// CHECK-CTORS-LABEL: define linkonce_odr void @_ZN14DynamicDerivedC2Ev(
@@ -164,14 +162,12 @@
// CHECK-CTORS: call void @_ZN12DynamicBase2C2Ev(
// CHECK-CTORS-NOT: @llvm.launder.invariant.group.p0i8
-
// CHECK-CTORS: %[[THIS10:.*]] = bitcast %struct.DynamicDerivedMultiple* %[[THIS0]] to i32 (...)***
// CHECK-CTORS: store {{.*}} @_ZTV22DynamicDerivedMultiple, i32 0, inrange i32 0, i32 2) {{.*}} %[[THIS10]]
// CHECK-CTORS: %[[THIS11:.*]] = bitcast %struct.DynamicDerivedMultiple* %[[THIS0]] to i8*
// CHECK-CTORS: %[[THIS_ADD:.*]] = getelementptr inbounds i8, i8* %[[THIS11]], i64 16
// CHECK-CTORS: %[[THIS12:.*]] = bitcast i8* %[[THIS_ADD]] to i32 (...)***
-
// CHECK-CTORS: store {{.*}} @_ZTV22DynamicDerivedMultiple, i32 0, inrange i32 1, i32 2) {{.*}} %[[THIS12]]
// CHECK-CTORS-LABEL: {{^}}}
@@ -182,9 +178,10 @@
struct A {
virtual void foo();
+ int m;
};
struct B : A {
- virtual void foo();
+ void foo() override;
};
union U {
@@ -209,7 +206,7 @@
// CHECK-NEW: call i8* @llvm.launder.invariant.group.p0i8(i8*
// CHECK-NEW: call void @_Z2g2P1A(%struct.A*
g2(>b);
- // CHECK-NEW: call void @_Z9changeToAP1U(%union.U*
+ // CHECK-NEW: call void @_Z9changeToAP1U(%union.U*
changeToA(u);
// CHECK-NEW: call i8* @llvm.launder.invariant.group.p0i8(i8*
// call void @_Z2g2P1A(%struct.A* %a)
@@ -294,21 +291,287 @@
take(u.v3);
}
+// CHECK-NEW-LABEL: define void @_Z7comparev()
+void compare() {
+ A *a = new A;
+ a->foo();
+ // CHECK-NEW: call i8* @llvm.launder.invariant.group.p0i8(i8*
+ A *b = new (a) B;
+
+ // CHECK-NEW: %[[a:.*]] = call i8* @llvm.strip.invariant.group.p0i8(i8*
+ // CHECK-NEW: %[[a2:.*]] = bitcast i8* %[[a]] to %struct.A*
+ // CHECK-NEW: %[[b:.*]] = call i8* @llvm.strip.invariant.group.p0i8(i8*
+ // CHECK-NEW: %[[b2:.*]] = bitcast i8* %[[b]] to %struct.A*
+ // CHECK-NEW: %cmp = icmp eq %struct.A* %[[a2]], %[[b2]]
+ if (a == b)
+b->foo();
+}
+
+// CHECK-NEW-LABEL: compare2
+bool compare2(A *a, A *a2) {
+ // CHECK-NEW: %[[a:.*]] = call i8* @llvm.strip.invariant.group.p0i8(i8*
+ // CHECK-NEW: %[[a2:.*]] = bitcast i8* %[[a]] to %struct.A*
+ // CHECK-NEW: %[[b:.*]] = call i8* @llvm.strip.invariant.group.p0i8(i8*
+ // CHECK-NEW: %[[b2:.*]] = bitcast i8* %[[b]] to %struct.A*
+ // CHECK-NEW: %cmp = icmp ult %struct.A* %[[a2]], %[[b2]]
+ return a < a2;
+}
+// CHECK-NEW-LABEL: compareIntPointers
+bool compareIntPointers(int *a, int *b) {
+ // CHECK-NEW-NOT: call i8* @llvm.strip.invariant.group
+ return a == b;
+}
+
+struct HoldingOtherVirtuals {
+ B b;
+};
+
+// There is no need to add barriers for comparision of pointer to classes
+// that are not dynamic.
+// CHECK-NEW-LABEL: compare5
+bool compare5(HoldingOtherVirtuals *a, HoldingOtherVirtuals *b) {
+ // CHECK-NEW-NOT: call i8* @llvm.strip.invariant.group
+ return a == b;
+}
+// CHECK-NEW-LABEL: compareNull
+bool compareNull(A *a) {
+ // CHECK-NEW-NOT: call i8* @llvm.strip.invariant.group
+
+ if (a != nullptr)
[PATCH] D47299: [CodeGenCXX] Emit strip.invariant.group with -fstrict-vtable-pointers
Prazek marked 5 inline comments as done. Prazek added inline comments. Comment at: clang/lib/CodeGen/CGExprScalar.cpp:1623 + const CXXRecordDecl *SourceClassDecl = + E->getType().getTypePtr()->getPointeeCXXRecordDecl(); + const CXXRecordDecl *DstClassDecl = DestTy->getPointeeCXXRecordDecl(); rjmccall wrote: > Unnecessary `getTypePtr()`. getType returns QualType and it does not have getPointeeCXXRecordDecl. Am I missing something? Comment at: clang/lib/CodeGen/CGExprScalar.cpp:1647 + } +} + rjmccall wrote: > Incidentally, how do you protect against code like this? > > A *ptr; > reinterpret_cast(ptr) = new B(); > ptr->foo(); > > Presumably there needs to be a launder/strip here, but I guess it would have > to be introduced by the middle-end when forwarding the store? The way I've > written this is an aliasing violation, but (1) I assume your pass isn't > disabled whenever strict-aliasing is disabled and (2) you can do this with a > memcpy and still pretty reliably expect that LLVM will be able to eventually > forward the store. Can you add more info on what is A and B so I can make sure I understand it correctly? Is the prefix of the layout the same for both, but they are not in the hierarchy? I haven't thought about the strict aliasing. I think the only sane way would be to require strict aliasing for the strict vtable pointers. Repository: rL LLVM https://reviews.llvm.org/D47299 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47299: [CodeGenCXX] Emit strip.invariant.group with -fstrict-vtable-pointers
rjmccall added inline comments.
Comment at: clang/include/clang/AST/DeclCXX.h:784
+return !hasDefinition() || !isDynamicClass() || hasAnyDependentBases();
+ }
+
Both of these new methods deserve doc comments explaining that they're
conservative checks because the class might be incomplete or dependent.
I think `NonDynamic` would read better than `NotDynamic`.
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:1623
+ const CXXRecordDecl *SourceClassDecl =
+ E->getType().getTypePtr()->getPointeeCXXRecordDecl();
+ const CXXRecordDecl *DstClassDecl = DestTy->getPointeeCXXRecordDecl();
Unnecessary `getTypePtr()`.
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:1624
+ E->getType().getTypePtr()->getPointeeCXXRecordDecl();
+ const CXXRecordDecl *DstClassDecl = DestTy->getPointeeCXXRecordDecl();
+
The opposite of `Dst` is `Src`. Alternatively, the opposite of `Source` is
`Destination` (or `Result`). Please pick.
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:1633
+ bool DstMayNotBeDynamic =
+ !DstClassDecl || DstClassDecl->mayBeNotDynamicClass();
+ if (SourceMayBeNotDynamic && DstMayBeDynamic) {
If you made a couple of tiny helper functions here that you could invoke on
either `SourceClassDecl` or `DstClassDecl`, you could avoid some redundant
logic and also make the calls self-documenting enough to legibly inline into
the `if`-conditions.
...in fact, since you start from a QualType in every case, maybe you should
just define the helper as a method there.
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:1647
+ }
+}
+
Incidentally, how do you protect against code like this?
A *ptr;
reinterpret_cast(ptr) = new B();
ptr->foo();
Presumably there needs to be a launder/strip here, but I guess it would have to
be introduced by the middle-end when forwarding the store? The way I've
written this is an aliasing violation, but (1) I assume your pass isn't
disabled whenever strict-aliasing is disabled and (2) you can do this with a
memcpy and still pretty reliably expect that LLVM will be able to eventually
forward the store.
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:1791
+ // dynamic information from invariant.group.
+ if (DstClassDecl && DstClassDecl->mayBeDynamicClass())
+IntToPtr = Builder.CreateLaunderInvariantGroup(IntToPtr);
Another place you could definitely just use that helper function on QualType.
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:1802
+ const CXXRecordDecl *ClassDecl =
+ E->getType().getTypePtr()->getPointeeCXXRecordDecl();
+ // Casting to integer requires stripping dynamic information as it does
Same.
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3305
+ if (RD->mayBeDynamicClass())
+RHS = Builder.CreateStripInvariantGroup(RHS);
+ }
Yeah, helper function on QualType, please. :)
Repository:
rL LLVM
https://reviews.llvm.org/D47299
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47299: [CodeGenCXX] Emit strip.invariant.group with -fstrict-vtable-pointers
Prazek added a comment. @rjmccall @rsmith friendly ping Repository: rL LLVM https://reviews.llvm.org/D47299 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47299: [CodeGenCXX] Emit strip.invariant.group with -fstrict-vtable-pointers
Prazek updated this revision to Diff 148746.
Prazek marked 3 inline comments as done.
Prazek added a comment.
one more test
Repository:
rL LLVM
https://reviews.llvm.org/D47299
Files:
clang/include/clang/AST/DeclCXX.h
clang/lib/CodeGen/CGExpr.cpp
clang/lib/CodeGen/CGExprScalar.cpp
clang/test/CodeGenCXX/strict-vtable-pointers.cpp
Index: clang/test/CodeGenCXX/strict-vtable-pointers.cpp
===
--- clang/test/CodeGenCXX/strict-vtable-pointers.cpp
+++ clang/test/CodeGenCXX/strict-vtable-pointers.cpp
@@ -5,7 +5,8 @@
// RUN: FileCheck --check-prefix=CHECK-LINK-REQ %s < %t.ll
typedef __typeof__(sizeof(0)) size_t;
-void *operator new(size_t, void*) throw();
+void *operator new(size_t, void *) throw();
+using uintptr_t = unsigned long long;
struct NotTrivialDtor {
~NotTrivialDtor();
@@ -17,7 +18,7 @@
};
struct DynamicDerived : DynamicBase1 {
- void foo();
+ void foo() override;
};
struct DynamicBase2 {
@@ -28,8 +29,8 @@
};
struct DynamicDerivedMultiple : DynamicBase1, DynamicBase2 {
- virtual void foo();
- virtual void bar();
+ void foo() override;
+ void bar() override;
};
struct StaticBase {
@@ -47,9 +48,8 @@
struct DynamicFromVirtualStatic2 : virtual StaticBase {
};
-struct DynamicFrom2Virtuals :
-DynamicFromVirtualStatic1,
-DynamicFromVirtualStatic2 {
+struct DynamicFrom2Virtuals : DynamicFromVirtualStatic1,
+ DynamicFromVirtualStatic2 {
};
// CHECK-NEW-LABEL: define void @_Z12LocalObjectsv()
@@ -89,7 +89,6 @@
// CHECK-CTORS: call i8* @llvm.launder.invariant.group.p0i8(
// CHECK-CTORS-LABEL: {{^}}}
-
// CHECK-NEW-LABEL: define void @_Z9Pointers1v()
// CHECK-NEW-NOT: @llvm.launder.invariant.group.p0i8(
// CHECK-NEW-LABEL: call void @_ZN12DynamicBase1C1Ev(
@@ -134,7 +133,6 @@
// CHECK-CTORS-NOT: call i8* @llvm.launder.invariant.group.p0i8(
// CHECK-CTORS-LABEL: {{^}}}
-
struct DynamicDerived;
// CHECK-CTORS-LABEL: define linkonce_odr void @_ZN14DynamicDerivedC2Ev(
@@ -164,14 +162,12 @@
// CHECK-CTORS: call void @_ZN12DynamicBase2C2Ev(
// CHECK-CTORS-NOT: @llvm.launder.invariant.group.p0i8
-
// CHECK-CTORS: %[[THIS10:.*]] = bitcast %struct.DynamicDerivedMultiple* %[[THIS0]] to i32 (...)***
// CHECK-CTORS: store {{.*}} @_ZTV22DynamicDerivedMultiple, i32 0, inrange i32 0, i32 2) {{.*}} %[[THIS10]]
// CHECK-CTORS: %[[THIS11:.*]] = bitcast %struct.DynamicDerivedMultiple* %[[THIS0]] to i8*
// CHECK-CTORS: %[[THIS_ADD:.*]] = getelementptr inbounds i8, i8* %[[THIS11]], i64 16
// CHECK-CTORS: %[[THIS12:.*]] = bitcast i8* %[[THIS_ADD]] to i32 (...)***
-
// CHECK-CTORS: store {{.*}} @_ZTV22DynamicDerivedMultiple, i32 0, inrange i32 1, i32 2) {{.*}} %[[THIS12]]
// CHECK-CTORS-LABEL: {{^}}}
@@ -182,9 +178,10 @@
struct A {
virtual void foo();
+ int m;
};
struct B : A {
- virtual void foo();
+ void foo() override;
};
union U {
@@ -209,7 +206,7 @@
// CHECK-NEW: call i8* @llvm.launder.invariant.group.p0i8(i8*
// CHECK-NEW: call void @_Z2g2P1A(%struct.A*
g2(>b);
- // CHECK-NEW: call void @_Z9changeToAP1U(%union.U*
+ // CHECK-NEW: call void @_Z9changeToAP1U(%union.U*
changeToA(u);
// CHECK-NEW: call i8* @llvm.launder.invariant.group.p0i8(i8*
// call void @_Z2g2P1A(%struct.A* %a)
@@ -294,21 +291,287 @@
take(u.v3);
}
+// CHECK-NEW-LABEL: define void @_Z7comparev()
+void compare() {
+ A *a = new A;
+ a->foo();
+ // CHECK-NEW: call i8* @llvm.launder.invariant.group.p0i8(i8*
+ A *b = new (a) B;
+
+ // CHECK-NEW: %[[a:.*]] = call i8* @llvm.strip.invariant.group.p0i8(i8*
+ // CHECK-NEW: %[[a2:.*]] = bitcast i8* %[[a]] to %struct.A*
+ // CHECK-NEW: %[[b:.*]] = call i8* @llvm.strip.invariant.group.p0i8(i8*
+ // CHECK-NEW: %[[b2:.*]] = bitcast i8* %[[b]] to %struct.A*
+ // CHECK-NEW: %cmp = icmp eq %struct.A* %[[a2]], %[[b2]]
+ if (a == b)
+b->foo();
+}
+
+// CHECK-NEW-LABEL: compare2
+bool compare2(A *a, A *a2) {
+ // CHECK-NEW: %[[a:.*]] = call i8* @llvm.strip.invariant.group.p0i8(i8*
+ // CHECK-NEW: %[[a2:.*]] = bitcast i8* %[[a]] to %struct.A*
+ // CHECK-NEW: %[[b:.*]] = call i8* @llvm.strip.invariant.group.p0i8(i8*
+ // CHECK-NEW: %[[b2:.*]] = bitcast i8* %[[b]] to %struct.A*
+ // CHECK-NEW: %cmp = icmp ult %struct.A* %[[a2]], %[[b2]]
+ return a < a2;
+}
+// CHECK-NEW-LABEL: compareIntPointers
+bool compareIntPointers(int *a, int *b) {
+ // CHECK-NEW-NOT: call i8* @llvm.strip.invariant.group
+ return a == b;
+}
+
+struct HoldingOtherVirtuals {
+ B b;
+};
+
+// There is no need to add barriers for comparision of pointer to classes
+// that are not dynamic.
+// CHECK-NEW-LABEL: compare5
+bool compare5(HoldingOtherVirtuals *a, HoldingOtherVirtuals *b) {
+ // CHECK-NEW-NOT: call i8* @llvm.strip.invariant.group
+ return a == b;
+}
+// CHECK-NEW-LABEL: compareNull
+bool compareNull(A *a) {
+ // CHECK-NEW-NOT: call i8* @llvm.strip.invariant.group
+
+ if (a != nullptr)
+return
[PATCH] D47299: [CodeGenCXX] Emit strip.invariant.group with -fstrict-vtable-pointers
Prazek updated this revision to Diff 148745.
Prazek added a comment.
Herald added a subscriber: hiraditya.
Added launder when going from possiblyNotDynamic to possiblyDynamic
emitting strip for pointer -> int only if poitner is possiblyDynamic
Repository:
rL LLVM
https://reviews.llvm.org/D47299
Files:
clang/include/clang/AST/DeclCXX.h
clang/lib/CodeGen/CGExpr.cpp
clang/lib/CodeGen/CGExprScalar.cpp
clang/test/CodeGenCXX/strict-vtable-pointers.cpp
llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
Index: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
===
--- llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
+++ llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
@@ -1205,6 +1205,7 @@
if (StrippedInvariantGroupsArg == StrippedArg)
return nullptr;
+ return nullptr;
if (II.getIntrinsicID() == Intrinsic::launder_invariant_group)
return cast(IC.Builder.CreateLaunderInvariantGroup(StrippedInvariantGroupsArg));
if (II.getIntrinsicID() == Intrinsic::strip_invariant_group)
Index: clang/test/CodeGenCXX/strict-vtable-pointers.cpp
===
--- clang/test/CodeGenCXX/strict-vtable-pointers.cpp
+++ clang/test/CodeGenCXX/strict-vtable-pointers.cpp
@@ -5,7 +5,8 @@
// RUN: FileCheck --check-prefix=CHECK-LINK-REQ %s < %t.ll
typedef __typeof__(sizeof(0)) size_t;
-void *operator new(size_t, void*) throw();
+void *operator new(size_t, void *) throw();
+using uintptr_t = unsigned long long;
struct NotTrivialDtor {
~NotTrivialDtor();
@@ -17,7 +18,7 @@
};
struct DynamicDerived : DynamicBase1 {
- void foo();
+ void foo() override;
};
struct DynamicBase2 {
@@ -28,8 +29,8 @@
};
struct DynamicDerivedMultiple : DynamicBase1, DynamicBase2 {
- virtual void foo();
- virtual void bar();
+ void foo() override;
+ void bar() override;
};
struct StaticBase {
@@ -47,9 +48,8 @@
struct DynamicFromVirtualStatic2 : virtual StaticBase {
};
-struct DynamicFrom2Virtuals :
-DynamicFromVirtualStatic1,
-DynamicFromVirtualStatic2 {
+struct DynamicFrom2Virtuals : DynamicFromVirtualStatic1,
+ DynamicFromVirtualStatic2 {
};
// CHECK-NEW-LABEL: define void @_Z12LocalObjectsv()
@@ -89,7 +89,6 @@
// CHECK-CTORS: call i8* @llvm.launder.invariant.group.p0i8(
// CHECK-CTORS-LABEL: {{^}}}
-
// CHECK-NEW-LABEL: define void @_Z9Pointers1v()
// CHECK-NEW-NOT: @llvm.launder.invariant.group.p0i8(
// CHECK-NEW-LABEL: call void @_ZN12DynamicBase1C1Ev(
@@ -134,7 +133,6 @@
// CHECK-CTORS-NOT: call i8* @llvm.launder.invariant.group.p0i8(
// CHECK-CTORS-LABEL: {{^}}}
-
struct DynamicDerived;
// CHECK-CTORS-LABEL: define linkonce_odr void @_ZN14DynamicDerivedC2Ev(
@@ -164,14 +162,12 @@
// CHECK-CTORS: call void @_ZN12DynamicBase2C2Ev(
// CHECK-CTORS-NOT: @llvm.launder.invariant.group.p0i8
-
// CHECK-CTORS: %[[THIS10:.*]] = bitcast %struct.DynamicDerivedMultiple* %[[THIS0]] to i32 (...)***
// CHECK-CTORS: store {{.*}} @_ZTV22DynamicDerivedMultiple, i32 0, inrange i32 0, i32 2) {{.*}} %[[THIS10]]
// CHECK-CTORS: %[[THIS11:.*]] = bitcast %struct.DynamicDerivedMultiple* %[[THIS0]] to i8*
// CHECK-CTORS: %[[THIS_ADD:.*]] = getelementptr inbounds i8, i8* %[[THIS11]], i64 16
// CHECK-CTORS: %[[THIS12:.*]] = bitcast i8* %[[THIS_ADD]] to i32 (...)***
-
// CHECK-CTORS: store {{.*}} @_ZTV22DynamicDerivedMultiple, i32 0, inrange i32 1, i32 2) {{.*}} %[[THIS12]]
// CHECK-CTORS-LABEL: {{^}}}
@@ -182,9 +178,10 @@
struct A {
virtual void foo();
+ int m;
};
struct B : A {
- virtual void foo();
+ void foo() override;
};
union U {
@@ -209,7 +206,7 @@
// CHECK-NEW: call i8* @llvm.launder.invariant.group.p0i8(i8*
// CHECK-NEW: call void @_Z2g2P1A(%struct.A*
g2(>b);
- // CHECK-NEW: call void @_Z9changeToAP1U(%union.U*
+ // CHECK-NEW: call void @_Z9changeToAP1U(%union.U*
changeToA(u);
// CHECK-NEW: call i8* @llvm.launder.invariant.group.p0i8(i8*
// call void @_Z2g2P1A(%struct.A* %a)
@@ -294,21 +291,283 @@
take(u.v3);
}
+// CHECK-NEW-LABEL: define void @_Z7comparev()
+void compare() {
+ A *a = new A;
+ a->foo();
+ // CHECK-NEW: call i8* @llvm.launder.invariant.group.p0i8(i8*
+ A *b = new (a) B;
+
+ // CHECK-NEW: %[[a:.*]] = call i8* @llvm.strip.invariant.group.p0i8(i8*
+ // CHECK-NEW: %[[a2:.*]] = bitcast i8* %[[a]] to %struct.A*
+ // CHECK-NEW: %[[b:.*]] = call i8* @llvm.strip.invariant.group.p0i8(i8*
+ // CHECK-NEW: %[[b2:.*]] = bitcast i8* %[[b]] to %struct.A*
+ // CHECK-NEW: %cmp = icmp eq %struct.A* %[[a2]], %[[b2]]
+ if (a == b)
+b->foo();
+}
+
+// CHECK-NEW-LABEL: compare2
+bool compare2(A *a, A *a2) {
+ // CHECK-NEW: %[[a:.*]] = call i8* @llvm.strip.invariant.group.p0i8(i8*
+ // CHECK-NEW: %[[a2:.*]] = bitcast i8* %[[a]] to %struct.A*
+ // CHECK-NEW: %[[b:.*]] = call i8* @llvm.strip.invariant.group.p0i8(i8*
+ // CHECK-NEW: %[[b2:.*]] =
[PATCH] D47299: [CodeGenCXX] Emit strip.invariant.group with -fstrict-vtable-pointers
Prazek added inline comments. Comment at: clang/lib/CodeGen/CGExprScalar.cpp:1626-1627 + + // Casting to pointer that does not carry dynamic information (provided by + // invariant.group) requires stripping it. + Src = Builder.CreateStripInvariantGroup(Src); rsmith wrote: > Are there any cases where we need a barrier when the destination type is a > dynamic type here? No, I don't think so. I will also add test for that. Repository: rL LLVM https://reviews.llvm.org/D47299 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47299: [CodeGenCXX] Emit strip.invariant.group with -fstrict-vtable-pointers
rsmith added inline comments.
Comment at: clang/include/clang/AST/DeclCXX.h:779
+ bool mayBeDynamicClass() const {
+return !isCompleteDefinition() || isDynamicClass();
+ }
`isCompleteDefinition` checks whether this declaration of the class is a
definition, not whether it has a definition anywhere; the latter is what you
need here. You can use `hasDefinition` to check that.
Please also check `hasAnyDependentBases()` (or add a comment to this function
to indicate that it may return `false` for a templated class whose
instantiations might be dynamic classes) -- if a class has dependent bases, we
might not find out that it's a dynamic class until it's instantiated.
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:1626-1627
+
+ // Casting to pointer that does not carry dynamic information (provided
by
+ // invariant.group) requires stripping it.
+ Src = Builder.CreateStripInvariantGroup(Src);
Are there any cases where we need a barrier when the destination type is a
dynamic type here?
Repository:
rL LLVM
https://reviews.llvm.org/D47299
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47299: [CodeGenCXX] Emit strip.invariant.group with -fstrict-vtable-pointers
Prazek created this revision.
Prazek added reviewers: rjmccall, rsmith, amharc, kuhar.
Herald added a subscriber: llvm-commits.
Emmiting new intrinsic that strips invariant.groups to make
devirtulization sound, as described in RFC: Devirtualization v2.
Repository:
rL LLVM
https://reviews.llvm.org/D47299
Files:
clang/include/clang/AST/DeclCXX.h
clang/lib/CodeGen/CGExpr.cpp
clang/lib/CodeGen/CGExprScalar.cpp
clang/test/CodeGenCXX/strict-vtable-pointers.cpp
Index: clang/test/CodeGenCXX/strict-vtable-pointers.cpp
===
--- clang/test/CodeGenCXX/strict-vtable-pointers.cpp
+++ clang/test/CodeGenCXX/strict-vtable-pointers.cpp
@@ -5,7 +5,8 @@
// RUN: FileCheck --check-prefix=CHECK-LINK-REQ %s < %t.ll
typedef __typeof__(sizeof(0)) size_t;
-void *operator new(size_t, void*) throw();
+void *operator new(size_t, void *) throw();
+using uintptr_t = unsigned long long;
struct NotTrivialDtor {
~NotTrivialDtor();
@@ -17,7 +18,7 @@
};
struct DynamicDerived : DynamicBase1 {
- void foo();
+ void foo() override;
};
struct DynamicBase2 {
@@ -28,8 +29,8 @@
};
struct DynamicDerivedMultiple : DynamicBase1, DynamicBase2 {
- virtual void foo();
- virtual void bar();
+ void foo() override;
+ void bar() override;
};
struct StaticBase {
@@ -47,9 +48,8 @@
struct DynamicFromVirtualStatic2 : virtual StaticBase {
};
-struct DynamicFrom2Virtuals :
-DynamicFromVirtualStatic1,
-DynamicFromVirtualStatic2 {
+struct DynamicFrom2Virtuals : DynamicFromVirtualStatic1,
+ DynamicFromVirtualStatic2 {
};
// CHECK-NEW-LABEL: define void @_Z12LocalObjectsv()
@@ -89,7 +89,6 @@
// CHECK-CTORS: call i8* @llvm.launder.invariant.group.p0i8(
// CHECK-CTORS-LABEL: {{^}}}
-
// CHECK-NEW-LABEL: define void @_Z9Pointers1v()
// CHECK-NEW-NOT: @llvm.launder.invariant.group.p0i8(
// CHECK-NEW-LABEL: call void @_ZN12DynamicBase1C1Ev(
@@ -134,7 +133,6 @@
// CHECK-CTORS-NOT: call i8* @llvm.launder.invariant.group.p0i8(
// CHECK-CTORS-LABEL: {{^}}}
-
struct DynamicDerived;
// CHECK-CTORS-LABEL: define linkonce_odr void @_ZN14DynamicDerivedC2Ev(
@@ -164,14 +162,12 @@
// CHECK-CTORS: call void @_ZN12DynamicBase2C2Ev(
// CHECK-CTORS-NOT: @llvm.launder.invariant.group.p0i8
-
// CHECK-CTORS: %[[THIS10:.*]] = bitcast %struct.DynamicDerivedMultiple* %[[THIS0]] to i32 (...)***
// CHECK-CTORS: store {{.*}} @_ZTV22DynamicDerivedMultiple, i32 0, inrange i32 0, i32 2) {{.*}} %[[THIS10]]
// CHECK-CTORS: %[[THIS11:.*]] = bitcast %struct.DynamicDerivedMultiple* %[[THIS0]] to i8*
// CHECK-CTORS: %[[THIS_ADD:.*]] = getelementptr inbounds i8, i8* %[[THIS11]], i64 16
// CHECK-CTORS: %[[THIS12:.*]] = bitcast i8* %[[THIS_ADD]] to i32 (...)***
-
// CHECK-CTORS: store {{.*}} @_ZTV22DynamicDerivedMultiple, i32 0, inrange i32 1, i32 2) {{.*}} %[[THIS12]]
// CHECK-CTORS-LABEL: {{^}}}
@@ -182,9 +178,10 @@
struct A {
virtual void foo();
+ int m;
};
struct B : A {
- virtual void foo();
+ void foo() override;
};
union U {
@@ -209,7 +206,7 @@
// CHECK-NEW: call i8* @llvm.launder.invariant.group.p0i8(i8*
// CHECK-NEW: call void @_Z2g2P1A(%struct.A*
g2(>b);
- // CHECK-NEW: call void @_Z9changeToAP1U(%union.U*
+ // CHECK-NEW: call void @_Z9changeToAP1U(%union.U*
changeToA(u);
// CHECK-NEW: call i8* @llvm.launder.invariant.group.p0i8(i8*
// call void @_Z2g2P1A(%struct.A* %a)
@@ -294,21 +291,206 @@
take(u.v3);
}
+// CHECK-NEW-LABEL: define void @_Z7comparev()
+void compare() {
+ A *a = new A;
+ a->foo();
+ // CHECK-NEW: call i8* @llvm.launder.invariant.group.p0i8(i8*
+ A *b = new (a) B;
+
+ // CHECK-NEW: %[[a:.*]] = call i8* @llvm.strip.invariant.group.p0i8(i8*
+ // CHECK-NEW: %[[a2:.*]] = bitcast i8* %[[a]] to %struct.A*
+ // CHECK-NEW: %[[b:.*]] = call i8* @llvm.strip.invariant.group.p0i8(i8*
+ // CHECK-NEW: %[[b2:.*]] = bitcast i8* %[[b]] to %struct.A*
+ // CHECK-NEW: %cmp = icmp eq %struct.A* %[[a2]], %[[b2]]
+ if (a == b)
+b->foo();
+}
+
+// CHECK-NEW-LABEL: compare2
+bool compare2(A *a, A *a2) {
+ // CHECK-NEW: %[[a:.*]] = call i8* @llvm.strip.invariant.group.p0i8(i8*
+ // CHECK-NEW: %[[a2:.*]] = bitcast i8* %[[a]] to %struct.A*
+ // CHECK-NEW: %[[b:.*]] = call i8* @llvm.strip.invariant.group.p0i8(i8*
+ // CHECK-NEW: %[[b2:.*]] = bitcast i8* %[[b]] to %struct.A*
+ // CHECK-NEW: %cmp = icmp ult %struct.A* %[[a2]], %[[b2]]
+ return a < a2;
+}
+// CHECK-NEW-LABEL: compareIntPointers
+bool compareIntPointers(int *a, int *b) {
+ // CHECK-NEW-NOT: call i8* @llvm.strip.invariant.group
+ return a == b;
+}
+
+struct HoldingOtherVirtuals {
+ B b;
+};
+
+// There is no need to add barriers for comparision of pointer to classes
+// that are not dynamic.
+// CHECK-NEW-LABEL: compare5
+bool compare5(HoldingOtherVirtuals *a, HoldingOtherVirtuals *b) {
+ // CHECK-NEW-NOT: call i8* @llvm.strip.invariant.group
+ return a == b;
+}
+// CHECK-NEW-LABEL:
