[cxx-interop] Fix wrong field offsets with value types using inheritance #74570
Conversation
Xazax-hun changed the title 
| Looks like there are still some test failures that I need to investigate. |
Xazax-hun force-pushed the gaborh/inheritance branch 2 times, most recently from 8794431 to 3c086ab Compare | @swift-ci please smoke test |
The code path responsible for calculating field offsets creates opaque fields for all the base classes. This code path simply summed up the sizes of the base classes to get the offset. Unfortunately, this does not work for base classes with vptrs. C++ does not embedd the vptrs from the base classes into the derived class. The derived class has a single vptr. This PR removes this assumption that we can just add the base class sizes up and relies purely on base and field offsets to calculate the sizes for the opaque fields representing the base classes.
Xazax-hun force-pushed the gaborh/inheritance branch from 3c086ab to 2161a1d Compare | @swift-ci please smoke test |
Xazax-hun changed the title | A high-level comment. I could have tried to correctly calculate the ABI by introducing adjustments, like subtracting the size of the vptr from the base's size. But there are complex ABI rules in some cases where fields can be packed into the base's trailing padding. I decided that only relying on the offsets is more reliable and less likely to break when something changes in Clang. |
rjmccall left a comment
rjmccall left a comment There was a problem hiding this comment.
I think the basic approach here is just wrong. If we want to add opaque storage for a (non-empty) base, we should add storage covering its nv-size rather than trying to compute it by the range to the next base in the layout. But really, I think we should be recursing into the base rather than treating it as totally opaque.
| | ||
| auto baseType = base.getType().getCanonicalType(); | ||
| // The empty bases are all at offset 0, so we need to remove them to | ||
| // reliably calculate base subobject sizes using the next base's offset. |
There was a problem hiding this comment.
It is not generally true that empty bases are all at offset zero.
I will give the non-virtual size a try and see if this works out. By recursing into the base you mean collecting all the fields from the base classes as well, right? |
Yes, I mean collecting fields from base classes (offset as necessary) as an alternative to just adding the entire base subobject as opaque storage. While you're at it, please fix this to not just ignore virtual bases. Maybe we don't import these types yet, but (1) there's no reason for this code to handle them incorrectly and (2) I suspect we can visit them indirectly, e.g. if you have a class with a field that has a virtual base. The correct way to handle virtual bases is to handle them exactly like normal bases, but only when you're visiting the object as a complete object type (i.e. not when you're recursively visiting base subobjects in the walk I'm asking you to add). |
| Thanks! I could not get back to this for a while, but now I opened a new PR that does the recursive walk: #75176 This one does not deal with virtual inheritance yet, because that is not required to fix the crash and we have some higher priority items to address first (and the patch is already getting larger enough, so splitting up might also make reviewing easier). |
The code path responsible for calculating field offsets creates opaque fields for all the base classes. This code path simply summed up the sizes of the base classes to get the offset. Unfortunately, this does not work for base classes with vptrs. C++ does not embedd the vptrs from the base classes into the derived class. The derived class has a single vptr. This PR removes this assumption that we can just add the base class sizes up and relies purely on base and field offsets to calculate the sizes for the opaque fields representing the base classes.
Fixes #73136.
rdar://126754931