[cxx-interop] Fix a crash with [[no_unique_address]] #80786
Conversation
Xazax-hun
requested review from
zoecarver,
egorzhdan,
j-hui,
fahadnayyar,
susmonteiro,
hnrklssn,
rjmccall,
beccadax and
ian-twilightcoder
as code owners
Xazax-hun
force-pushed
the
gaborh/unique-address-padding
branch
from
cf5df84 to
75838d6
Compare
|
@swift-ci please smoke test |
| @@ -24,6 +27,14 @@ struct HasZeroSizedField { | |||
| void set_c(short c) { this->c = c; } | |||
| }; | |||
|
|
|||
| struct ReuseFieldPadding { | |||
| [[no_unique_address]] std::optional<int> a; | |||
There was a problem hiding this comment.
Can you add some getter/setter methods to ensure this field is still accessible via those?
There was a problem hiding this comment.
Also, can you add another test that doesn't really on the layout of std::optional, i.e., using your own padded type?
There was a problem hiding this comment.
Can you add some getter/setter methods to ensure this field is still accessible via those?
Added a getter.
Also, can you add another test that doesn't really on the layout of std::optional, i.e., using your own padded type?
I tried and failed. Not sure what is unique about std::optional but my couple of attempts to make the compiler do the same optimisation with my own type did not work. Will need to look deeper to figure out what is the difference because I tried to recreate the same layout.
| decl->getASTContext().getASTRecordLayout(rd); | ||
| if (!decl->isZeroSize(decl->getASTContext()) && | ||
| fieldLayout.getDataSize() != fieldLayout.getSize()) | ||
| return nullptr; |
There was a problem hiding this comment.
What happens to derived conformances that rely on be presence of this field, eg Sendable or Equatable?
There was a problem hiding this comment.
Good point. Unfortunately, we would not consider these fields for Sendable. Maybe we could conservatively always say types with unimported fields should always be not Sendable? We already have many fields that we do not import for one reason or another.
There was a problem hiding this comment.
I think Equatable might already be really problematic for C++ types. We do not generate metadata for private fields, so I think even if we can auto conform a type to that the generated implementation is probably faulty :/
There was a problem hiding this comment.
It is not a problem for Equatable. Trying to conform a C++ the type to it gives this error message: "error: extension outside of file declaring struct 'ReuseFieldPadding' prevents automatic synthesis of '==' for protocol 'Equatable'". Unfortunately, I don't think we can do anything about the Sendable problem for now.
Absolutely, I would love to support this and that would be the right approach long term. We use |
Swift does not support storing fields in the padding of the previous fields just yet, so let's not import fields like that from C++. Represent them as opaque blobs instead. Fixes #80764
Xazax-hun
force-pushed
the
gaborh/unique-address-padding
branch
from
75838d6 to
664efc6
Compare
|
@swift-ci please smoke test |
Swift does not support storing fields in the padding of the previous fields just yet, so let's not import fields like that from C++. Represent them as opaque blobs instead.
Fixes #80764