Class resilience part 10 #13687
Class resilience part 10 #13687
Conversation
We need to subtract the address point from the metadata size, since the metadata size includes prefix matter.
If the nominal type descriptor's resilient superclass flag is set, the generic parameter offset, vtable start offset and field offset start offset are all relative to the start of the class's immedaite members, and not the start of the class metadata. Support this by loading the size of the superclass and adding it to these offsets if the flag is set.
|
@swift-ci Please test |
|
@swift-ci Please test source compatibility |
|
@rjmccall I'm still working on adding new 'evolution' tests, which might uncover more bugs. But I wanted to get this merged in first, since it removes the silly staging flag. The new code is partially exercised by the existing evolution tests. |
|
Build failed |
slavapestov
force-pushed
the
class-resilience-part-10
branch
from
ecff196 to
6af8d18
Compare
|
@swift-ci Please test |
1 similar comment
|
@swift-ci Please test |
|
@swift-ci Please test source compatibility |
3 similar comments
|
@swift-ci Please test source compatibility |
|
@swift-ci Please test source compatibility |
|
@swift-ci Please test source compatibility |
|
@rjmccall I'm merging this, but post-commit review is always welcome ;) |
| readGenericArgsOffset(MetadataRef metadata, | ||
| NominalTypeDescriptorRef descriptor) { | ||
| if (metadata->getKind() == MetadataKind::Class) { | ||
| auto *classMetadata = cast<TargetClassMetadata<Runtime>>(metadata); |
There was a problem hiding this comment.
If we support cast<>, I'm sure we also support dyn_cast<>.
| auto result = | ||
| descriptor->GenericParams.getOffset( | ||
| classMetadata, | ||
| cast<TargetClassMetadata<Runtime>>(superMetadata)); |
There was a problem hiding this comment.
It's a little unfortunate that we have to read the superclass unconditionally here, even if the superclass is not resilient — but it works, and we can improve it later.
| /// | ||
| /// The offset is in units of words, from the start of the class's | ||
| /// metadata. | ||
| std::pair<bool, uint32_t> |
There was a problem hiding this comment.
Can we not use Optional<> in this file? I guess we seem not to be.
| return Offset; | ||
| } | ||
|
|
||
| uint32_t getOffset() const { |
There was a problem hiding this comment.
I think it's generally better to name overloads differently when their semantics are different like this. Here I think you should call out that this method assumes that the offset isn't resilient.
There was a problem hiding this comment.
I've added comments, but I prefer to keep the names as-is.
| } | ||
|
|
||
| uint32_t getOffset(const TargetMetadata<Runtime> *metadata) const { | ||
| if (auto *classMetadata = llvm::dyn_cast<ClassMetadata>(metadata)) |
There was a problem hiding this comment.
Maybe this could check for a resilient superclass first instead of checking N things involving the metadata? And if it's resilient, hey, you know it's a class.
| @@ -1520,6 +1586,13 @@ struct TargetClassMetadata : public TargetHeapMetadata<Runtime> { | |||
| return getter(this); | |||
| } | |||
|
|
|||
| uint32_t getSizeInWords() const { | |||
There was a problem hiding this comment.
I think this should be signed, and I think the name should use your term of art.
| uint32_t getOffset(const TargetMetadata<Runtime> *metadata) const { | ||
| if (auto *classMetadata = llvm::dyn_cast<ClassMetadata>(metadata)) | ||
| if (auto *superclass = classMetadata->SuperClass) | ||
| if (auto *superMetadata = llvm::dyn_cast<ClassMetadata>(superclass)) |
There was a problem hiding this comment.
The superclass will always be a class; you don't need to check this. ClassMetadata::classof doesn't check whether it's type metadata, just that it's a class.
Is it possible to have a resilient superclass that ends up being an ObjC class (i.e. not type metadata)?
There was a problem hiding this comment.
Thanks, I've simplified it. I don't think an Objective-C class can be "resilient" in this manner because Objective-C class metadata always has a fixed size, no? There are no "members" in the sense of generic arguments, field offsets, vtable entries, etc.
There was a problem hiding this comment.
Right, I think that's reasonable. Since that's true, you don't need to check for isTypeMetadata() in the case that you actually have to pull out the superclass size.
| /// in words. | ||
| /// | ||
| /// If this class has a resilient superclass, this offset is relative to the | ||
| /// size of the resilient superclass metadata. Otherwise, it is absolute. |
There was a problem hiding this comment.
Another place to use the term of art for the relative base.
| /// | ||
| /// This is meaningful if NumGenericRequirements is nonzero. | ||
| /// | ||
| /// If this class has a resilient superclass, this offset is relative to the | ||
| /// size of the resilient superclass metadata. Otherwise, it is absolute. |
There was a problem hiding this comment.
I think you really ought to introduce a term of art for the relative base here. I believe you use "immediate class data" in the compiler, so maybe "the start of the class's immediate class data"?
If this is absolute, I think the runtime really ought to validate its correctness, i.e. that it doesn't overlap the superclass's class members. It would be okay to fatal-error if that check fails.
I think we probably ought to prepare the ABI to allow class data to be placed at negative offsets, even if we're not planning to take advantage of that in Swift 5. I think all you really there is (1) some way to know the total size of the immediate class data, not just the offset of its start, and (2) some way to record that it should be placed at a negative offset. Also, I guess, it means all of these offsets should use signed types rather than unsigned types.
| /// The offset to the first generic argument from the start of | ||
| /// metadata record. | ||
| /// metadata record, in words. |
There was a problem hiding this comment.
Probably best to say that this is from the address point of the metadata record.
|
Thanks for the review feedback, @rjmccall. I've made most of the changes except for those to allow negative offsets. I still need to think a bit more about that. New PR coming up. |
Implement runtime support for initializing the vtable and accessing metadata fields in a class whose superclass is resilient, by respecting the HasResilientSuperclass flag in the nominal type descriptor and interpreting certain offsets as being relative to the start of the class's immediate members.
This allows the
-enable-class-resiliencestaging flag to be removed, finally!