Skip to content

IRGen: Correctly compute instanceStart for a Swift class that starts … #29219

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
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
25 changes: 24 additions & 1 deletion lib/IRGen/ClassLayout.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -29,12 +29,14 @@ ClassLayout::ClassLayout(const StructLayoutBuilder &builder,
llvm::Type *classTy,
ArrayRef<VarDecl *> allStoredProps,
ArrayRef<FieldAccess> allFieldAccesses,
ArrayRef<ElementLayout> allElements)
ArrayRef<ElementLayout> allElements,
Size headerSize)
: MinimumAlign(builder.getAlignment()),
MinimumSize(builder.getSize()),
IsFixedLayout(builder.isFixedLayout()),
Options(options),
Ty(classTy),
HeaderSize(headerSize),
AllStoredProperties(allStoredProps),
AllFieldAccesses(allFieldAccesses),
AllElements(allElements) { }
Expand All @@ -46,12 +48,33 @@ Size ClassLayout::getInstanceStart() const {
elements = elements.drop_front();

// Ignore empty elements.
bool haveSeenEmpty = false;
if (element.isEmpty()) {
continue;
} else if (element.hasByteOffset()) {
// FIXME: assumes layout is always sequential!
return element.getByteOffset();
} else {
// We used to crash for classes that have an empty and a resilient field
// during intialization.
// class CrashInInit {
// var empty = EmptyStruct()
// var resilient = ResilientThing()
// }
// What happened was that for such a class we we would compute a
// instanceStart of 0. The shared cache builder would then slide the value
// of the constant ivar offset for the empty field from 0 to 16. However
// the field offset for empty fields is assume to be zero and the runtime
// does not compute a different value for the empty field and so the field
// offset for the empty field stays 0. The runtime then trys to reconcile
// the field offset and the ivar offset trying to write to the ivar
// offset. However, the ivar offset is marked as constant and so we
// crashed.
// This can be avoided by correctly computing the instanceStart for such a
// class to be 16 such that the shared cache builder does not update the
// value of the empty field.
if (!Options.contains(ClassMetadataFlags::ClassHasObjCAncestry))
return HeaderSize;
return Size(0);
}
}
Expand Down
6 changes: 5 additions & 1 deletion lib/IRGen/ClassLayout.h
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,9 @@ class ClassLayout {
/// The LLVM type for instances of this class.
llvm::Type *Ty;

/// The header size of this class.
Size HeaderSize;

/// Lazily-initialized array of all fragile stored properties directly defined
/// in the class itself.
ArrayRef<VarDecl *> AllStoredProperties;
Expand All @@ -131,7 +134,8 @@ class ClassLayout {
llvm::Type *classTy,
ArrayRef<VarDecl *> allStoredProps,
ArrayRef<FieldAccess> allFieldAccesses,
ArrayRef<ElementLayout> allElements);
ArrayRef<ElementLayout> allElements,
Size headerSize);

Size getInstanceStart() const;

Expand Down
6 changes: 5 additions & 1 deletion lib/IRGen/GenClass.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,8 @@ namespace {

ClassMetadataOptions Options;

Size HeaderSize;

public:
ClassLayoutBuilder(IRGenModule &IGM, SILType classType,
ReferenceCounting refcounting,
Expand All @@ -178,11 +180,13 @@ namespace {
case ReferenceCounting::Native:
// For native classes, place a full object header.
addHeapHeader();
HeaderSize = CurSize;
break;
case ReferenceCounting::ObjC:
// For ObjC-inheriting classes, we don't reliably know the size of the
// base class, but NSObject only has an `isa` pointer at most.
addNSObjectHeader();
HeaderSize = CurSize;
break;
case ReferenceCounting::Block:
case ReferenceCounting::Unknown:
Expand Down Expand Up @@ -222,7 +226,7 @@ namespace {
auto allElements = IGM.Context.AllocateCopy(Elements);

return ClassLayout(*this, Options, classTy,
allStoredProps, allFieldAccesses, allElements);
allStoredProps, allFieldAccesses, allElements, HeaderSize);
}

private:
Expand Down
20 changes: 19 additions & 1 deletion test/IRGen/metadata.swift
Original file line number Diff line number Diff line change
@@ -1,6 +1,24 @@
// RUN: %empty-directory(%t)
// RUN: %target-swift-frontend -emit-module -enable-library-evolution -emit-module-path=%t/resilient_struct.swiftmodule -module-name=resilient_struct %S/../Inputs/resilient_struct.swift
// RUN: %target-swift-frontend -module-name A -I %t %S/Inputs/metadata2.swift -primary-file %s -emit-ir | %FileCheck %s
// RUN: %target-swift-frontend -module-name A -I %t %S/Inputs/metadata2.swift -primary-file %s -emit-ir | %FileCheck %s --check-prefix=CHECK --check-prefix=CHECK-%target-os

import resilient_struct

enum Singleton {
case only
}

// CHECK: @"$s1A1GC14zeroSizedFieldAA9SingletonOvpWvd" = hidden constant i{{(64|32)}} 0
// Check that the instance start is after the header (at 8 or 16).
// CHECK-macosx: _DATA__TtC1A1G = private constant {{.*}} { i32 128, i32 {{(16|8)}}
// CHECK-ios: _DATA__TtC1A1G = private constant {{.*}} { i32 128, i32 {{(16|8)}}
// CHECK-watchos: _DATA__TtC1A1G = private constant {{.*}} { i32 128, i32 {{(16|8)}}
// CHECK-tvos: _DATA__TtC1A1G = private constant {{.*}} { i32 128, i32 {{(16|8)}}

class G {
var zeroSizedField = Singleton.only
var r = ResilientInt(i:1)
}

// CHECK-LABEL: define {{.*}}swiftcc %swift.metadata_response @"$s1A12MyControllerCMr"(%swift.type*, i8*, i8**)
// CHECK-NOT: ret
Expand Down