[PATCH] D109841: Fix vtbl field addr space

[Yaxun Liu via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGabe8b354e37d: Fix vtbl field addr space (authored by yaxunl).
Herald added a project: clang.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109841/new/

https://reviews.llvm.org/D109841

Files:
  clang/lib/CodeGen/CGClass.cpp


Index: clang/lib/CodeGen/CGClass.cpp
===
--- clang/lib/CodeGen/CGClass.cpp
+++ clang/lib/CodeGen/CGClass.cpp
@@ -2502,6 +2502,8 @@
 
   // Apply the offsets.
   Address VTableField = LoadCXXThisAddress();
+  unsigned ThisAddrSpace =
+  VTableField.getPointer()->getType()->getPointerAddressSpace();
 
   if (!NonVirtualOffset.isZero() || VirtualOffset)
 VTableField = ApplyNonVirtualAndVirtualOffset(
@@ -2516,12 +2518,11 @@
   llvm::FunctionType::get(CGM.Int32Ty, /*isVarArg=*/true)
   ->getPointerTo(ProgAS)
   ->getPointerTo(GlobalsAS);
-  // vtable field is is derived from `this` pointer, therefore it should be in
-  // default address space.
-  VTableField = Builder.CreatePointerBitCastOrAddrSpaceCast(
-  VTableField, VTablePtrTy->getPointerTo());
-  VTableAddressPoint = Builder.CreatePointerBitCastOrAddrSpaceCast(
-  VTableAddressPoint, VTablePtrTy);
+  // vtable field is is derived from `this` pointer, therefore they should be 
in
+  // the same addr space. Note that this might not be LLVM address space 0.
+  VTableField = Builder.CreateBitCast(VTableField,
+  
VTablePtrTy->getPointerTo(ThisAddrSpace));
+  VTableAddressPoint = Builder.CreateBitCast(VTableAddressPoint, VTablePtrTy);
 
   llvm::StoreInst *Store = Builder.CreateStore(VTableAddressPoint, 
VTableField);
   TBAAAccessInfo TBAAInfo = CGM.getTBAAVTablePtrAccessInfo(VTablePtrTy);


Index: clang/lib/CodeGen/CGClass.cpp
===
--- clang/lib/CodeGen/CGClass.cpp
+++ clang/lib/CodeGen/CGClass.cpp
@@ -2502,6 +2502,8 @@
 
   // Apply the offsets.
   Address VTableField = LoadCXXThisAddress();
+  unsigned ThisAddrSpace =
+  VTableField.getPointer()->getType()->getPointerAddressSpace();
 
   if (!NonVirtualOffset.isZero() || VirtualOffset)
 VTableField = ApplyNonVirtualAndVirtualOffset(
@@ -2516,12 +2518,11 @@
   llvm::FunctionType::get(CGM.Int32Ty, /*isVarArg=*/true)
   ->getPointerTo(ProgAS)
   ->getPointerTo(GlobalsAS);
-  // vtable field is is derived from `this` pointer, therefore it should be in
-  // default address space.
-  VTableField = Builder.CreatePointerBitCastOrAddrSpaceCast(
-  VTableField, VTablePtrTy->getPointerTo());
-  VTableAddressPoint = Builder.CreatePointerBitCastOrAddrSpaceCast(
-  VTableAddressPoint, VTablePtrTy);
+  // vtable field is is derived from `this` pointer, therefore they should be in
+  // the same addr space. Note that this might not be LLVM address space 0.
+  VTableField = Builder.CreateBitCast(VTableField,
+  VTablePtrTy->getPointerTo(ThisAddrSpace));
+  VTableAddressPoint = Builder.CreateBitCast(VTableAddressPoint, VTablePtrTy);
 
   llvm::StoreInst *Store = Builder.CreateStore(VTableAddressPoint, VTableField);
   TBAAAccessInfo TBAAInfo = CGM.getTBAAVTablePtrAccessInfo(VTablePtrTy);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109841: Fix vtbl field addr space

[John McCall via Phabricator via cfe-commits]

rjmccall accepted this revision.
rjmccall added a comment.

LGTM


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109841/new/

https://reviews.llvm.org/D109841

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

[PATCH] D109841: Fix vtbl field addr space

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl updated this revision to Diff 372792.
yaxunl edited the summary of this revision.
yaxunl added a comment.
Herald added a subscriber: jrtc27.

fix comments and casts


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109841/new/

https://reviews.llvm.org/D109841

Files:
  clang/lib/CodeGen/CGClass.cpp


Index: clang/lib/CodeGen/CGClass.cpp
===
--- clang/lib/CodeGen/CGClass.cpp
+++ clang/lib/CodeGen/CGClass.cpp
@@ -2502,6 +2502,8 @@
 
   // Apply the offsets.
   Address VTableField = LoadCXXThisAddress();
+  unsigned ThisAddrSpace =
+  VTableField.getPointer()->getType()->getPointerAddressSpace();
 
   if (!NonVirtualOffset.isZero() || VirtualOffset)
 VTableField = ApplyNonVirtualAndVirtualOffset(
@@ -2516,12 +2518,11 @@
   llvm::FunctionType::get(CGM.Int32Ty, /*isVarArg=*/true)
   ->getPointerTo(ProgAS)
   ->getPointerTo(GlobalsAS);
-  // vtable field is is derived from `this` pointer, therefore it should be in
-  // default address space.
-  VTableField = Builder.CreatePointerBitCastOrAddrSpaceCast(
-  VTableField, VTablePtrTy->getPointerTo());
-  VTableAddressPoint = Builder.CreatePointerBitCastOrAddrSpaceCast(
-  VTableAddressPoint, VTablePtrTy);
+  // vtable field is is derived from `this` pointer, therefore they should be 
in
+  // the same addr space. Note that this might not be LLVM address space 0.
+  VTableField = Builder.CreateBitCast(VTableField,
+  
VTablePtrTy->getPointerTo(ThisAddrSpace));
+  VTableAddressPoint = Builder.CreateBitCast(VTableAddressPoint, VTablePtrTy);
 
   llvm::StoreInst *Store = Builder.CreateStore(VTableAddressPoint, 
VTableField);
   TBAAAccessInfo TBAAInfo = CGM.getTBAAVTablePtrAccessInfo(VTablePtrTy);


Index: clang/lib/CodeGen/CGClass.cpp
===
--- clang/lib/CodeGen/CGClass.cpp
+++ clang/lib/CodeGen/CGClass.cpp
@@ -2502,6 +2502,8 @@
 
   // Apply the offsets.
   Address VTableField = LoadCXXThisAddress();
+  unsigned ThisAddrSpace =
+  VTableField.getPointer()->getType()->getPointerAddressSpace();
 
   if (!NonVirtualOffset.isZero() || VirtualOffset)
 VTableField = ApplyNonVirtualAndVirtualOffset(
@@ -2516,12 +2518,11 @@
   llvm::FunctionType::get(CGM.Int32Ty, /*isVarArg=*/true)
   ->getPointerTo(ProgAS)
   ->getPointerTo(GlobalsAS);
-  // vtable field is is derived from `this` pointer, therefore it should be in
-  // default address space.
-  VTableField = Builder.CreatePointerBitCastOrAddrSpaceCast(
-  VTableField, VTablePtrTy->getPointerTo());
-  VTableAddressPoint = Builder.CreatePointerBitCastOrAddrSpaceCast(
-  VTableAddressPoint, VTablePtrTy);
+  // vtable field is is derived from `this` pointer, therefore they should be in
+  // the same addr space. Note that this might not be LLVM address space 0.
+  VTableField = Builder.CreateBitCast(VTableField,
+  VTablePtrTy->getPointerTo(ThisAddrSpace));
+  VTableAddressPoint = Builder.CreateBitCast(VTableAddressPoint, VTablePtrTy);
 
   llvm::StoreInst *Store = Builder.CreateStore(VTableAddressPoint, VTableField);
   TBAAAccessInfo TBAAInfo = CGM.getTBAAVTablePtrAccessInfo(VTablePtrTy);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109841: Fix vtbl field addr space

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl marked 2 inline comments as done.
yaxunl added a comment.

In D109841#3002404 , @arichardson 
wrote:

> Thanks, I can confirm that this fixes the assertions we were seeing after 
> merging D103835 . A few minor suggestions 
> below:
>
> There's a typo in the commit message, I'd maybe change it to:
> `Storing the vtable field of an object should use the same address space as 
> the this pointer.`
> And maybe change `This caused issue for some out of tree project.` to 
> something like `This assumption (added in 
> 054cc3b1b469de4b0cb25d1dc3af43c679c5dc44) caused issues for the out-of-tree 
> CHERI targets`.

will do




Comment at: clang/lib/CodeGen/CGClass.cpp:2522-2523
+  // vtable field is is derived from `this` pointer, therefore they should be 
in
+  // the same addr space. Note it may not be the default address space of LLVM
+  // IR.
   VTableField = Builder.CreatePointerBitCastOrAddrSpaceCast(

arichardson wrote:
> Maybe this is clearer?
will do



Comment at: clang/lib/CodeGen/CGClass.cpp:2526-2527
+  VTableField, VTablePtrTy->getPointerTo(ThisAddrSpace));
   VTableAddressPoint = Builder.CreatePointerBitCastOrAddrSpaceCast(
   VTableAddressPoint, VTablePtrTy);
 

arichardson wrote:
> As noted by @rjmccall, changing this line still allows all tests to pass 
> (including our newly added out-of-tree CHERI ones).
will do. I think we can also make changes so that

 
```
VTableField = Builder.CreateBitCast(
  VTableField, VTablePtrTy->getPointerTo(ThisAddrSpace));
```


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109841/new/

https://reviews.llvm.org/D109841

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

[PATCH] D109841: Fix vtbl field addr space

[Alexander Richardson via Phabricator via cfe-commits]

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

Thanks, I can confirm that this fixes the assertions we were seeing after 
merging D103835 . A few minor suggestions 
below:

There's a typo in the commit message, I'd maybe change it to:
`Storing the vtable field of an object should use the same address space as the 
this pointer.`
And maybe change `This caused issue for some out of tree project.` to something 
like `This assumption (added in 054cc3b1b469de4b0cb25d1dc3af43c679c5dc44) 
caused issues for the out-of-tree CHERI targets`.




Comment at: clang/lib/CodeGen/CGClass.cpp:2522-2523
+  // vtable field is is derived from `this` pointer, therefore they should be 
in
+  // the same addr space. Note it may not be the default address space of LLVM
+  // IR.
   VTableField = Builder.CreatePointerBitCastOrAddrSpaceCast(

Maybe this is clearer?



Comment at: clang/lib/CodeGen/CGClass.cpp:2526-2527
+  VTableField, VTablePtrTy->getPointerTo(ThisAddrSpace));
   VTableAddressPoint = Builder.CreatePointerBitCastOrAddrSpaceCast(
   VTableAddressPoint, VTablePtrTy);
 

As noted by @rjmccall, changing this line still allows all tests to pass 
(including our newly added out-of-tree CHERI ones).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109841/new/

https://reviews.llvm.org/D109841

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

[PATCH] D109841: Fix vtbl field addr space

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

The following line is always just making a bitcast, then.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109841/new/

https://reviews.llvm.org/D109841

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

[PATCH] D109841: Fix vtbl field addr space

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl created this revision.
yaxunl added reviewers: tra, rjmccall, arichardson.
yaxunl requested review of this revision.

vtbl filed of an object should have same addr space of the object.
Currently it is assumed to be addr space 0 but this may not
be true. This caused issue for some out of tree project.


https://reviews.llvm.org/D109841

Files:
  clang/lib/CodeGen/CGClass.cpp


Index: clang/lib/CodeGen/CGClass.cpp
===
--- clang/lib/CodeGen/CGClass.cpp
+++ clang/lib/CodeGen/CGClass.cpp
@@ -2502,6 +2502,8 @@
 
   // Apply the offsets.
   Address VTableField = LoadCXXThisAddress();
+  unsigned ThisAddrSpace =
+  VTableField.getPointer()->getType()->getPointerAddressSpace();
 
   if (!NonVirtualOffset.isZero() || VirtualOffset)
 VTableField = ApplyNonVirtualAndVirtualOffset(
@@ -2516,10 +2518,11 @@
   llvm::FunctionType::get(CGM.Int32Ty, /*isVarArg=*/true)
   ->getPointerTo(ProgAS)
   ->getPointerTo(GlobalsAS);
-  // vtable field is is derived from `this` pointer, therefore it should be in
-  // default address space.
+  // vtable field is is derived from `this` pointer, therefore they should be 
in
+  // the same addr space. Note it may not be the default address space of LLVM
+  // IR.
   VTableField = Builder.CreatePointerBitCastOrAddrSpaceCast(
-  VTableField, VTablePtrTy->getPointerTo());
+  VTableField, VTablePtrTy->getPointerTo(ThisAddrSpace));
   VTableAddressPoint = Builder.CreatePointerBitCastOrAddrSpaceCast(
   VTableAddressPoint, VTablePtrTy);
 


Index: clang/lib/CodeGen/CGClass.cpp
===
--- clang/lib/CodeGen/CGClass.cpp
+++ clang/lib/CodeGen/CGClass.cpp
@@ -2502,6 +2502,8 @@
 
   // Apply the offsets.
   Address VTableField = LoadCXXThisAddress();
+  unsigned ThisAddrSpace =
+  VTableField.getPointer()->getType()->getPointerAddressSpace();
 
   if (!NonVirtualOffset.isZero() || VirtualOffset)
 VTableField = ApplyNonVirtualAndVirtualOffset(
@@ -2516,10 +2518,11 @@
   llvm::FunctionType::get(CGM.Int32Ty, /*isVarArg=*/true)
   ->getPointerTo(ProgAS)
   ->getPointerTo(GlobalsAS);
-  // vtable field is is derived from `this` pointer, therefore it should be in
-  // default address space.
+  // vtable field is is derived from `this` pointer, therefore they should be in
+  // the same addr space. Note it may not be the default address space of LLVM
+  // IR.
   VTableField = Builder.CreatePointerBitCastOrAddrSpaceCast(
-  VTableField, VTablePtrTy->getPointerTo());
+  VTableField, VTablePtrTy->getPointerTo(ThisAddrSpace));
   VTableAddressPoint = Builder.CreatePointerBitCastOrAddrSpaceCast(
   VTableAddressPoint, VTablePtrTy);
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits