Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Class resilience part 10 #13687

Merged
merged 5 commits into from Jan 3, 2018

Conversation

Projects
None yet
3 participants
@slavapestov
Copy link
Member

slavapestov commented Jan 3, 2018

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-resilience staging flag to be removed, finally!

slavapestov added some commits Jan 3, 2018

IRGen: Correctly set class metadata base offset variable
We need to subtract the address point from the metadata size,
since the metadata size includes prefix matter.
Runtime: Add support for resilient superclasses
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.
@slavapestov

This comment has been minimized.

Copy link
Member Author

slavapestov commented Jan 3, 2018

@swift-ci Please test

@slavapestov

This comment has been minimized.

Copy link
Member Author

slavapestov commented Jan 3, 2018

@swift-ci Please test source compatibility

@slavapestov slavapestov requested a review from rjmccall Jan 3, 2018

@slavapestov

This comment has been minimized.

Copy link
Member Author

slavapestov commented Jan 3, 2018

@rjmccall In case you haven't seen it, do you also mind taking a quick look at #13618, which I merged over the break?

@slavapestov

This comment has been minimized.

Copy link
Member Author

slavapestov commented Jan 3, 2018

@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.

@swift-ci

This comment has been minimized.

Copy link
Contributor

swift-ci commented Jan 3, 2018

Build failed
Swift Test Linux Platform
Git Sha - ecff19611c0a3eb654320dd1016302aa494a9af4

@slavapestov slavapestov force-pushed the slavapestov:class-resilience-part-10 branch to 6af8d18 Jan 3, 2018

@slavapestov

This comment has been minimized.

Copy link
Member Author

slavapestov commented Jan 3, 2018

@swift-ci Please test

1 similar comment
@slavapestov

This comment has been minimized.

Copy link
Member Author

slavapestov commented Jan 3, 2018

@swift-ci Please test

@slavapestov

This comment has been minimized.

Copy link
Member Author

slavapestov commented Jan 3, 2018

@swift-ci Please test source compatibility

3 similar comments
@slavapestov

This comment has been minimized.

Copy link
Member Author

slavapestov commented Jan 3, 2018

@swift-ci Please test source compatibility

@slavapestov

This comment has been minimized.

Copy link
Member Author

slavapestov commented Jan 3, 2018

@swift-ci Please test source compatibility

@slavapestov

This comment has been minimized.

Copy link
Member Author

slavapestov commented Jan 3, 2018

@swift-ci Please test source compatibility

@slavapestov

This comment has been minimized.

Copy link
Member Author

slavapestov commented Jan 3, 2018

@rjmccall I'm merging this, but post-commit review is always welcome ;)

@slavapestov slavapestov merged commit 079efad into apple:master Jan 3, 2018

5 checks passed

Swift Source Compatibility Suite on macOS Platform Build finished.
Details
Swift Test Linux Platform 10665 tests run, 0 skipped, 0 failed.
Details
Swift Test Linux Platform (smoke test)
Details
Swift Test OS X Platform 53445 tests run, 0 skipped, 0 failed.
Details
Swift Test OS X Platform (smoke test)
Details
readGenericArgsOffset(MetadataRef metadata,
NominalTypeDescriptorRef descriptor) {
if (metadata->getKind() == MetadataKind::Class) {
auto *classMetadata = cast<TargetClassMetadata<Runtime>>(metadata);

This comment has been minimized.

@rjmccall

rjmccall Jan 3, 2018

Member

If we support cast<>, I'm sure we also support dyn_cast<>.

This comment has been minimized.

@slavapestov

slavapestov Jan 4, 2018

Author Member

Thanks.

auto result =
descriptor->GenericParams.getOffset(
classMetadata,
cast<TargetClassMetadata<Runtime>>(superMetadata));

This comment has been minimized.

@rjmccall

rjmccall Jan 3, 2018

Member

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>

This comment has been minimized.

@rjmccall

rjmccall Jan 3, 2018

Member

Can we not use Optional<> in this file? I guess we seem not to be.

return Offset;
}

uint32_t getOffset() const {

This comment has been minimized.

@rjmccall

rjmccall Jan 3, 2018

Member

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.

This comment has been minimized.

@slavapestov

slavapestov Jan 4, 2018

Author Member

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))

This comment has been minimized.

@rjmccall

rjmccall Jan 3, 2018

Member

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.

This comment has been minimized.

@slavapestov

slavapestov Jan 4, 2018

Author Member

Done.

@@ -1520,6 +1586,13 @@ struct TargetClassMetadata : public TargetHeapMetadata<Runtime> {
return getter(this);
}

uint32_t getSizeInWords() const {

This comment has been minimized.

@rjmccall

rjmccall Jan 3, 2018

Member

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))

This comment has been minimized.

@rjmccall

rjmccall Jan 3, 2018

Member

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)?

This comment has been minimized.

@slavapestov

slavapestov Jan 4, 2018

Author Member

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.

This comment has been minimized.

@rjmccall

rjmccall Jan 4, 2018

Member

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.

This comment has been minimized.

@rjmccall

rjmccall Jan 3, 2018

Member

Another place to use the term of art for the relative base.

This comment has been minimized.

@slavapestov

slavapestov Jan 4, 2018

Author Member

Done.

///
/// 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.

This comment has been minimized.

@rjmccall

rjmccall Jan 3, 2018

Member

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.

This comment has been minimized.

@rjmccall

rjmccall Jan 3, 2018

Member

Probably best to say that this is from the address point of the metadata record.

This comment has been minimized.

@slavapestov

slavapestov Jan 4, 2018

Author Member

Fixed.

@slavapestov

This comment has been minimized.

Copy link
Member Author

slavapestov commented Jan 4, 2018

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.