Skip to content

Class resilience part 10 #13687

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

Merged
merged 5 commits into from
Jan 3, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 0 additions & 5 deletions include/swift/Basic/LangOptions.h
Original file line number Diff line number Diff line change
Expand Up @@ -199,11 +199,6 @@ namespace swift {
/// accesses.
bool DisableTsanInoutInstrumentation = false;

/// \brief Staging flag for class resilience, which we do not want to enable
/// fully until more code is in place, to allow the standard library to be
/// tested with value type resilience only.
bool EnableClassResilience = false;

/// Should we check the target OSs of serialized modules to see that they're
/// new enough?
bool EnableTargetOSChecking = true;
Expand Down
4 changes: 0 additions & 4 deletions include/swift/Option/FrontendOptions.td
Original file line number Diff line number Diff line change
Expand Up @@ -442,10 +442,6 @@ def enable_resilience : Flag<["-"], "enable-resilience">,
HelpText<"Compile the module to export resilient interfaces for all "
"public declarations by default">;

def enable_class_resilience : Flag<["-"], "enable-class-resilience">,
HelpText<"Compile the module to export resilient interfaces for all "
"public classes by default (doesn't work yet)">;

def group_info_path : Separate<["-"], "group-info-path">,
HelpText<"The path to collect the group information of the compiled module">;

Expand Down
49 changes: 42 additions & 7 deletions include/swift/Remote/MetadataReader.h
Original file line number Diff line number Diff line change
Expand Up @@ -992,6 +992,35 @@ class MetadataReader {
swift_runtime_unreachable("Unhandled IsaEncodingKind in switch.");
}

/// Read the offset of the generic parameters of a class from the nominal
/// type descriptor. If the class has a resilient superclass, we also
/// have to read the superclass size and add that to the offset.
///
/// The offset is in units of words, from the start of the class's
/// metadata.
std::pair<bool, uint32_t>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

readGenericArgsOffset(MetadataRef metadata,
NominalTypeDescriptorRef descriptor) {
if (metadata->getKind() == MetadataKind::Class) {
auto *classMetadata = cast<TargetClassMetadata<Runtime>>(metadata);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

if (classMetadata->SuperClass) {
auto superMetadata = readMetadata(classMetadata->SuperClass);
if (!superMetadata)
return std::make_pair(false, 0);

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


return std::make_pair(true, result);
}
}

auto result = descriptor->GenericParams.getOffset();
return std::make_pair(true, result);
}

/// Read a single generic type argument from a bound generic type
/// metadata.
std::pair<bool, StoredPointer>
Expand All @@ -1010,11 +1039,14 @@ class MetadataReader {
return std::make_pair(false, 0);

auto numGenericParams = descriptor->GenericParams.NumPrimaryParams;
auto offsetToGenericArgs =
sizeof(StoredPointer) * (descriptor->GenericParams.Offset);
auto offsetToGenericArgs = readGenericArgsOffset(Meta, descriptor);
if (!offsetToGenericArgs.first)
return std::make_pair(false, 0);

auto addressOfGenericArgAddress =
Meta.getAddress() + offsetToGenericArgs +
index * sizeof(StoredPointer);
(Meta.getAddress() +
offsetToGenericArgs.second * sizeof(StoredPointer) +
index * sizeof(StoredPointer));

if (index >= numGenericParams)
return std::make_pair(false, 0);
Expand Down Expand Up @@ -1357,10 +1389,13 @@ class MetadataReader {
std::vector<BuiltType> substitutions;

auto numGenericParams = descriptor->GenericParams.NumPrimaryParams;
auto offsetToGenericArgs =
sizeof(StoredPointer) * (descriptor->GenericParams.Offset);
auto offsetToGenericArgs = readGenericArgsOffset(metadata, descriptor);
if (!offsetToGenericArgs.first)
return {};

auto addressOfGenericArgAddress =
metadata.getAddress() + offsetToGenericArgs;
(metadata.getAddress() + sizeof(StoredPointer) *
offsetToGenericArgs.second);

using ArgIndex = decltype(descriptor->GenericParams.NumPrimaryParams);
for (ArgIndex i = 0; i < numGenericParams;
Expand Down
101 changes: 87 additions & 14 deletions include/swift/Runtime/Metadata.h
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
#if SWIFT_OBJC_INTEROP
#include <objc/runtime.h>
#endif
#include "llvm/Support/Casting.h"

namespace swift {

Expand Down Expand Up @@ -1029,13 +1030,22 @@ struct GenericContextDescriptor {
};

/// Header for a generic parameter descriptor.
struct GenericParameterDescriptorHeader {
template<typename Runtime>
struct TargetGenericParameterDescriptorHeader {
using StoredPointer = typename Runtime::StoredPointer;
using ClassMetadata = TargetClassMetadata<Runtime>;

private:
/// The offset to the first generic argument from the start of
/// metadata record.
/// metadata record, in words.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

///
/// 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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

uint32_t Offset;

public:
/// The amount of generic requirement data in the metadata record, in
/// words, excluding the lexical parent type. A value of zero means
/// there is no generic requirement data.
Expand All @@ -1056,6 +1066,30 @@ struct GenericParameterDescriptorHeader {
/// Flags for this generic parameter descriptor.
GenericParameterDescriptorFlags Flags;

/// This is factored in a silly way because remote mirrors cannot directly
/// dereference the SuperClass field of class metadata.
uint32_t getOffset(const ClassMetadata *classMetadata,
const ClassMetadata *superMetadata) const {
if (Flags.hasResilientSuperclass())
return superMetadata->getSizeInWords() + Offset;

return Offset;
}

uint32_t getOffset() const {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added comments, but I prefer to keep the names as-is.

assert(!Flags.hasResilientSuperclass());
return Offset;
}

uint32_t getOffset(const TargetMetadata<Runtime> *metadata) const {
if (auto *classMetadata = llvm::dyn_cast<ClassMetadata>(metadata))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

if (auto *superclass = classMetadata->SuperClass)
if (auto *superMetadata = llvm::dyn_cast<ClassMetadata>(superclass))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return getOffset(classMetadata, superMetadata);

return getOffset();
}

/// True if the nominal type has generic requirements other than its
/// parent metadata.
bool hasGenericRequirements() const { return NumGenericRequirements > 0; }
Expand All @@ -1080,14 +1114,31 @@ struct TargetMethodDescriptor {
/// Header for a class vtable descriptor. This is a variable-sized
/// structure that describes how to find and parse a vtable
/// within the type metadata for a class.
struct VTableDescriptorHeader {
/// The offset of the vtable for this class in its metadata, if any.
template <typename Runtime>
struct TargetVTableDescriptorHeader {
using StoredPointer = typename Runtime::StoredPointer;

private:
/// The offset of the vtable for this class in its metadata, if any,
/// 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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

uint32_t VTableOffset;

public:
/// The number of vtable entries. This is the number of MethodDescriptor
/// records following the vtable header in the class's nominal type
/// descriptor, which is equal to the number of words this subclass's vtable
/// entries occupy in instantiated class metadata.
uint32_t VTableSize;

uint32_t getVTableOffset(const TargetClassMetadata<Runtime> *metadata) const {
const auto *description = metadata->getDescription();
if (description->GenericParams.Flags.hasResilientSuperclass())
return metadata->SuperClass->getSizeInWords() + VTableOffset;
return VTableOffset;
}
};

template<typename Runtime> struct TargetNominalTypeDescriptor;
Expand All @@ -1096,7 +1147,7 @@ template<typename Runtime>
using TargetNominalTypeDescriptorTrailingObjects
= swift::ABI::TrailingObjects<TargetNominalTypeDescriptor<Runtime>,
GenericContextDescriptor,
VTableDescriptorHeader,
TargetVTableDescriptorHeader<Runtime>,
TargetMethodDescriptor<Runtime>>;

/// Common information about all nominal types. For generic types, this
Expand All @@ -1116,6 +1167,9 @@ struct TargetNominalTypeDescriptor final
public:
using StoredPointer = typename Runtime::StoredPointer;
using MethodDescriptor = TargetMethodDescriptor<Runtime>;
using VTableDescriptorHeader = TargetVTableDescriptorHeader<Runtime>;
using GenericParameterDescriptorHeader = TargetGenericParameterDescriptorHeader<Runtime>;
using ClassMetadata = TargetClassMetadata<Runtime>;

/// The mangled name of the nominal type.
TargetRelativeDirectPointer<Runtime, const char> Name;
Expand All @@ -1127,19 +1181,22 @@ struct TargetNominalTypeDescriptor final
/// The number of stored properties in the class, not including its
/// superclasses. If there is a field offset vector, this is its length.
uint32_t NumFields;

private:
/// The offset of the field offset vector for this class's stored
/// properties in its metadata, if any. 0 means there is no field offset
/// properties in its metadata, in words. 0 means there is no field offset
/// vector.
///
/// To deal with resilient superclasses correctly, this will
/// eventually need to be relative to the start of this class's
/// metadata area.
/// If this class has a resilient superclass, this offset is relative to
/// the size of the resilient superclass metadata. Otherwise, it is
/// absolute.
uint32_t FieldOffsetVectorOffset;


public:
/// The field names. A doubly-null-terminated list of strings, whose
/// length and order is consistent with that of the field offset vector.
RelativeDirectPointer<const char, /*nullable*/ true> FieldNames;

/// The field type vector accessor. Returns a pointer to an array of
/// type metadata references whose order is consistent with that of the
/// field offset vector.
Expand All @@ -1148,7 +1205,16 @@ struct TargetNominalTypeDescriptor final

/// True if metadata records for this type have a field offset vector for
/// its stored properties.
bool hasFieldOffsetVector() const { return FieldOffsetVectorOffset != 0; }
bool hasFieldOffsetVector() const { return FieldOffsetVectorOffset != 0; }

unsigned getFieldOffsetVectorOffset(const ClassMetadata *metadata) const {
const auto *description = metadata->getDescription();

if (description->GenericParams.Flags.hasResilientSuperclass())
return metadata->SuperClass->getSizeInWords() + FieldOffsetVectorOffset;

return FieldOffsetVectorOffset;
}
} Class;

/// Information about struct types.
Expand Down Expand Up @@ -1503,7 +1569,7 @@ struct TargetClassMetadata : public TargetHeapMetadata<Runtime> {
/// Get a pointer to the field offset vector, if present, or null.
const StoredPointer *getFieldOffsets() const {
assert(isTypeMetadata());
auto offset = getDescription()->Class.FieldOffsetVectorOffset;
auto offset = getDescription()->Class.getFieldOffsetVectorOffset(this);
if (offset == 0)
return nullptr;
auto asWords = reinterpret_cast<const void * const*>(this);
Expand All @@ -1520,6 +1586,13 @@ struct TargetClassMetadata : public TargetHeapMetadata<Runtime> {
return getter(this);
}

uint32_t getSizeInWords() const {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be signed, and I think the name should use your term of art.

assert(isTypeMetadata());
uint32_t size = getClassSize() - getClassAddressPoint();
assert(size % sizeof(StoredPointer) == 0);
return size / sizeof(StoredPointer);
}

static bool classof(const TargetMetadata<Runtime> *metadata) {
return metadata->getKind() == MetadataKind::Class;
}
Expand Down Expand Up @@ -1768,7 +1841,7 @@ struct TargetValueMetadata : public TargetMetadata<Runtime> {

auto asWords = reinterpret_cast<
ConstTargetMetadataPointer<Runtime, swift::TargetMetadata> const *>(this);
return (asWords + Description->GenericParams.Offset);
return (asWords + Description->GenericParams.getOffset(this));
}

const TargetNominalTypeDescriptor<Runtime> *getDescription() const {
Expand Down
3 changes: 0 additions & 3 deletions lib/Frontend/CompilerInvocation.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1011,9 +1011,6 @@ static bool ParseLangArgs(LangOptions &Opts, ArgList &Args,
Opts.EnableExperimentalPropertyBehaviors |=
Args.hasArg(OPT_enable_experimental_property_behaviors);

Opts.EnableClassResilience |=
Args.hasArg(OPT_enable_class_resilience);

if (auto A = Args.getLastArg(OPT_enable_deserialization_recovery,
OPT_disable_deserialization_recovery)) {
Opts.EnableDeserializationRecovery
Expand Down
3 changes: 1 addition & 2 deletions lib/IRGen/ClassMetadataVisitor.h
Original file line number Diff line number Diff line change
Expand Up @@ -93,8 +93,7 @@ template <class Impl> class ClassMetadataVisitor
// Visit the superclass first.
if (Type superclass = type->getSuperclass()) {
auto *superclassDecl = superclass->getClassOrBoundGenericClass();
if (IGM.Context.LangOpts.EnableClassResilience &&
IGM.isResilient(superclassDecl, ResilienceExpansion::Maximal)) {
if (IGM.isResilient(superclassDecl, ResilienceExpansion::Maximal)) {
// Just note that we have a resilient superclass and move on.
//
// Runtime metadata instantiation needs to slide our entries down
Expand Down
8 changes: 0 additions & 8 deletions lib/IRGen/GenClass.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -295,14 +295,6 @@ namespace {

// If the superclass is resilient to us, we cannot statically
// know the layout of either its instances or its class objects.
//
// FIXME: We need to implement indirect field/vtable entry access
// before we can enable this
if (!IGM.Context.LangOpts.EnableClassResilience) {
addFieldsForClass(superclass, superclassType);
NumInherited = Elements.size();
}

ClassHasFixedSize = false;

// Furthermore, if the superclass is a generic context, we have to
Expand Down
15 changes: 12 additions & 3 deletions lib/IRGen/GenMeta.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3529,11 +3529,20 @@ namespace {
IGF.Builder.CreateBitCast(superMetadata, IGF.IGM.Int8PtrTy),
IGM.getPointerAlignment());

Address slot = IGF.Builder.CreateConstByteArrayGEP(
Address sizeSlot = IGF.Builder.CreateConstByteArrayGEP(
metadataAsBytes,
layout.getMetadataSizeOffset());
slot = IGF.Builder.CreateBitCast(slot, IGM.Int32Ty->getPointerTo());
llvm::Value *size = IGF.Builder.CreateLoad(slot);
sizeSlot = IGF.Builder.CreateBitCast(sizeSlot, IGM.Int32Ty->getPointerTo());
llvm::Value *size = IGF.Builder.CreateLoad(sizeSlot);

Address addressPointSlot = IGF.Builder.CreateConstByteArrayGEP(
metadataAsBytes,
layout.getMetadataAddressPointOffset());
addressPointSlot = IGF.Builder.CreateBitCast(addressPointSlot, IGM.Int32Ty->getPointerTo());
llvm::Value *addressPoint = IGF.Builder.CreateLoad(addressPointSlot);

size = IGF.Builder.CreateSub(size, addressPoint);

if (IGM.SizeTy != IGM.Int32Ty)
size = IGF.Builder.CreateZExt(size, IGM.SizeTy);

Expand Down
13 changes: 11 additions & 2 deletions lib/IRGen/MetadataLayout.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -251,8 +251,7 @@ ClassMetadataLayout::ClassMetadataLayout(IRGenModule &IGM, ClassDecl *decl)
// to us, we will access metadata members relative to a base offset.
if (forClass == Target) {
if (Layout.HasResilientSuperclass ||
(IGM.Context.LangOpts.EnableClassResilience &&
IGM.isResilient(forClass, ResilienceExpansion::Maximal))) {
IGM.isResilient(forClass, ResilienceExpansion::Maximal)) {
assert(!DynamicOffsetBase);
DynamicOffsetBase = NextOffset;
}
Expand All @@ -264,6 +263,11 @@ ClassMetadataLayout::ClassMetadataLayout(IRGenModule &IGM, ClassDecl *decl)
super::addClassSize();
}

void addClassAddressPoint() {
Layout.MetadataAddressPoint = getNextOffset();
super::addClassAddressPoint();
}

void addInstanceSize() {
Layout.InstanceSize = getNextOffset();
super::addInstanceSize();
Expand Down Expand Up @@ -345,6 +349,11 @@ Size ClassMetadataLayout::getMetadataSizeOffset() const {
return MetadataSize.getStaticOffset();
}

Size ClassMetadataLayout::getMetadataAddressPointOffset() const {
assert(MetadataAddressPoint.isStatic());
return MetadataAddressPoint.getStaticOffset();
}

Size ClassMetadataLayout::getInstanceSizeOffset() const {
assert(InstanceSize.isStatic());
return InstanceSize.getStaticOffset();
Expand Down
Loading