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.

Sign up for GitHub

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

Fix multi-payload enums with Class Existential payloads on 32-bit targets #77027

Merged
merged 3 commits into from
Oct 17, 2024
Merged

Fix multi-payload enums with Class Existential payloads on 32-bit targets #77027

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a105cf5
Fix multi-payload enums with Class Existential payloads on 32-bit tar…
tbkka Oct 15, 2024
502941c
More arcana about MPEs carrying class existentials
tbkka Oct 16, 2024
cfc7b2e
Use SWIFT_FALLTHROUGH
tbkka Oct 17, 2024
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
24 changes: 18 additions & 6 deletions stdlib/public/RemoteInspection/TypeLowering.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -437,14 +437,26 @@ BitMask RecordTypeInfo::getSpareBits(TypeConverter &TC, bool &hasAddrOnly) const
// Mask the rest of the fields as usual...
break;
}
case RecordKind::ClassExistential:
// Class existential is a data pointer that does expose spare bits
// ... so we can fall through ...
case RecordKind::ClassExistential: {
// First pointer in a Class Existential is the class pointer
// itself, which can be tagged or have other mysteries on 64-bit, so
// it exposes no spare bits from the first word there...
auto pointerBytes = TC.targetPointerSize();
if (pointerBytes == 8) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If anyone is trying to get RemoteMirror to work on non-Darwin, they may need to tweak this some more.

auto zeroPointerSizedMask = BitMask::zeroMask(pointerBytes);
mask.andMask(zeroPointerSizedMask, 0);
}
// Otherwise, it's the same as an Existential Metatype
DISPATCH_FALLTHROUGH;
}
case RecordKind::ExistentialMetatype: {
// Initial metadata pointer has spare bits
// All the pointers in an Existential Metatype expose spare bits...
auto pointerBytes = TC.targetPointerSize();
auto mpePointerSpareBits = TC.getBuilder().getMultiPayloadEnumPointerMask();
mask.andMask(mpePointerSpareBits, 0);
mask.keepOnlyLeastSignificantBytes(TC.targetPointerSize());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The truncation in the previous code here inadvertently marked everything beyond the first pointer as spare bits. This accidentally worked for 64-bit because MPEs pick the most-significant spare bit.

auto mpePointerSpareBitMask = BitMask(pointerBytes, mpePointerSpareBits);
for (int offset = 0; offset < (int)getSize(); offset += pointerBytes) {
mask.andMask(mpePointerSpareBitMask, offset);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the new code, we loop to apply the standard pointer spare bit mask to every pointer-sized field.

}
return mask;
}
case RecordKind::ErrorExistential:
Expand Down
118 changes: 112 additions & 6 deletions validation-test/Reflection/reflect_Enum_values10.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,6 @@
// UNSUPPORTED: use_os_stdlib
// UNSUPPORTED: asan

// This is broken on ARM64_32, disable it temporarily until we can fix it. rdar://137351613
// UNSUPPORTED: CPU=arm64_32
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yay! It works for 32-bit watchOS now.


import SwiftReflectionTest

protocol P : AnyObject {
Expand All @@ -24,7 +21,8 @@ class C : P {
init() { a = 0; b = 0; }
}

// MemoryLayout<B>.size == 8
// On 64-bit: MemoryLayout<B>.size == 8
// On 32-bit: MemoryLayout<B>.size == 4
enum B {
case a(C)
case b(C)
Expand All @@ -44,8 +42,8 @@ reflect(enumValue: B.b(C()))
// CHECKALL-NEXT: (enum reflect_Enum_values10.B)
// CHECKALL-NEXT: Value: .b(_)

// MemoryLayout<Q>.size == 16
// MemoryLayout<P>.size == 16
// On 64-bit: MemoryLayout<Q>.size == MemoryLayout<P>.size == 16
// On 32-bit: MemoryLayout<Q>.size == MemoryLayout<P>.size == 8
enum Q {
case a(P)
case b(P)
Expand All @@ -65,6 +63,114 @@ reflect(enumValue: Q.b(C()))
// CHECKALL-NEXT: (enum reflect_Enum_values10.Q)
// CHECKALL-NEXT: Value: .b(_)

enum B1 {
case a(C)
case b
}

reflect(enumValue: B1.a(C()))

// CHECKALL: Reflecting an enum value.
// CHECKALL-NEXT: Type reference:
// CHECKALL-NEXT: (enum reflect_Enum_values10.B1)
// CHECKALL-NEXT: Value: .a(_)

reflect(enumValue: B1.b)

// CHECKALL: Reflecting an enum value.
// CHECKALL-NEXT: Type reference:
// CHECKALL-NEXT: (enum reflect_Enum_values10.B1)
// CHECKALL-NEXT: Value: .b

enum Q1 {
case a(P)
case b
}

reflect(enumValue: Q1.a(C()))

// CHECKALL: Reflecting an enum value.
// CHECKALL-NEXT: Type reference:
// CHECKALL-NEXT: (enum reflect_Enum_values10.Q1)
// CHECKALL-NEXT: Value: .a(_)

reflect(enumValue: Q1.b)

// CHECKALL: Reflecting an enum value.
// CHECKALL-NEXT: Type reference:
// CHECKALL-NEXT: (enum reflect_Enum_values10.Q1)
// CHECKALL-NEXT: Value: .b

enum B2 {
case a(C)
case b(C)
case c(C)
case d(C)
case e(C)
}

reflect(enumValue: B2.a(C()))

// CHECKALL: Reflecting an enum value.
// CHECKALL-NEXT: Type reference:
// CHECKALL-NEXT: (enum reflect_Enum_values10.B2)
// CHECKALL-NEXT: Value: .a(_)

reflect(enumValue: B2.e(C()))

// CHECKALL: Reflecting an enum value.
// CHECKALL-NEXT: Type reference:
// CHECKALL-NEXT: (enum reflect_Enum_values10.B2)
// CHECKALL-NEXT: Value: .e(_)

// On 64-bit: MemoryLayout<Q>.size == MemoryLayout<P>.size == 16
// On 32-bit: MemoryLayout<Q>.size == MemoryLayout<P>.size == 8
enum Q2 {
case a(P)
case b(P)
case c(P)
case d(P)
case e(P)
}

reflect(enumValue: Q2.a(C()))

// CHECKALL: Reflecting an enum value.
// CHECKALL-NEXT: Type reference:
// CHECKALL-NEXT: (enum reflect_Enum_values10.Q2)
// CHECKALL-NEXT: Value: .a(_)

reflect(enumValue: Q2.e(C()))

// CHECKALL: Reflecting an enum value.
// CHECKALL-NEXT: Type reference:
// CHECKALL-NEXT: (enum reflect_Enum_values10.Q2)
// CHECKALL-NEXT: Value: .e(_)

reflect(enumValue: Optional<Q2>.some(.a(C())))

// CHECKALL: Reflecting an enum value.
// CHECKALL-NEXT: Type reference:
// CHECKALL-NEXT: (bound_generic_enum Swift.Optional
// CHECKALL-NEXT: (enum reflect_Enum_values10.Q2))
// CHECKALL-NEXT: Value: .some(.a(_))

reflect(enumValue: Optional<Q2>.some(.e(C())))

// CHECKALL: Reflecting an enum value.
// CHECKALL-NEXT: Type reference:
// CHECKALL-NEXT: (bound_generic_enum Swift.Optional
// CHECKALL-NEXT: (enum reflect_Enum_values10.Q2))
// CHECKALL-NEXT: Value: .some(.e(_))

reflect(enumValue: Optional<Q2>.none)

// CHECKALL: Reflecting an enum value.
// CHECKALL-NEXT: Type reference:
// CHECKALL-NEXT: (bound_generic_enum Swift.Optional
// CHECKALL-NEXT: (enum reflect_Enum_values10.Q2))
// CHECKALL-NEXT: Value: .none

doneReflecting()

// CHECKALL: Done.
Expand Down