Page MenuHomePhabricator

[CUDA][HIP] Fix store of vtbl in ctor
ClosedPublic

Authored by yaxunl on Jun 7 2021, 12:09 PM.

Details

Summary

vtbl itself is in default global address space. When clang emits
ctor, it gets a pointer to the vtbl field based on the this pointer,
then stores vtbl to the pointer.

Since this pointer can point to any address space (e.g. an object
created in stack), this pointer points to default address space, therefore
the pointer to vtbl field in this object should also be in default
address space.

Currently, clang incorrectly casts the pointer to vtbl field in this object
to global address space. This caused assertions in backend.

This patch fixes that by removing the incorrect addr space cast.

Diff Detail

Event Timeline

yaxunl requested review of this revision.Jun 7 2021, 12:09 PM
yaxunl created this revision.
tra accepted this revision.Jun 7 2021, 12:40 PM
This revision is now accepted and ready to land.Jun 7 2021, 12:40 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 8 2021, 7:25 AM

Merging this change broke our out-of-tree CHERI targets (and Arm Morello). We use addrspace(200) for *all* globals (including vtables). It would have been nice if I had been added to this review considering that I added line you are changing here.

If vtables are not in the default globals address space, I think we need another way of expressing this. I think CGM.getContext().getTargetAddressSpace(LangAS::Default)) should also be correct for your use-case?

Merging this change broke our out-of-tree CHERI targets (and Arm Morello). We use addrspace(200) for *all* globals (including vtables). It would have been nice if I had been added to this review considering that I added line you are changing here.

If vtables are not in the default globals address space, I think we need another way of expressing this. I think CGM.getContext().getTargetAddressSpace(LangAS::Default)) should also be correct for your use-case?

vtbl addr space should be the same as this pointer. If I use addr space of this pointer and not assuming it is default addr space, will it work for you? Thanks.

Merging this change broke our out-of-tree CHERI targets (and Arm Morello). We use addrspace(200) for *all* globals (including vtables). It would have been nice if I had been added to this review considering that I added line you are changing here.

If vtables are not in the default globals address space, I think we need another way of expressing this. I think CGM.getContext().getTargetAddressSpace(LangAS::Default)) should also be correct for your use-case?

vtbl addr space should be the same as this pointer. If I use addr space of this pointer and not assuming it is default addr space, will it work for you? Thanks.

Yes that would also work. Thanks!

Hmm. I think "v-tables are in the address space of the object pointer" is not a good assumption. Probably this ought to be determined by the C++ ABI for the target. In principle it could even be class-specific, but I think we can start by assuming it's universal.

It should be decided by the AST-level ABI abstraction so that it properly affects record layout, since different address spaces can have different pointer sizes.

Hmm. I think "v-tables are in the address space of the object pointer" is not a good assumption. Probably this ought to be determined by the C++ ABI for the target. In principle it could even be class-specific, but I think we can start by assuming it's universal.

It should be decided by the AST-level ABI abstraction so that it properly affects record layout, since different address spaces can have different pointer sizes.

Sorry my previous description was not accurate.

Currently vtbl addr space is assumed to be the default global addr space of LLVM IR, which is determined by the data layout of the LLVM IR. This patch did not change that.

The vtbl field of a class is a pointer to default addr space. When the vtbl field gets initialized, it is casted to a pointer to default global addr space so that the vtbl can be stored to it. The addr space of the vtbl field itself should be the same as this pointer. So we were talking about a pointer to the vtbl field of an object, not the addr space of vtbl.

I see. Yes, in that case, that sounds right.