Skip to content

Commit e573366

Browse files
tbkkatbkka
committed
Fix handling of generic MPEs
This exercises a number of cases that the previous logic got wrong, including cases where MPEs are laid out like SPEs, and cases where we use the SPE or MPE layout strategies for enums that have no non-empty payloads. (These cases are superficially similar but differ in how they handle XIs.) These new tests allowed me to correct a number logical flaws about how layouts are selected. In particular, cases with generic payloads are always considered to be non-empty for purposes of selecting a layout strategy. Only cases that are statically known to be empty are considered empty for layout purposes.
1 parent 82ac0a7 commit e573366

12 files changed

+962
-195
lines changed

‎include/swift/RemoteInspection/TypeRefBuilder.h

+6-9
Original file line numberDiff line numberDiff line change
@@ -288,21 +288,18 @@ struct FieldTypeInfo {
288288
int Value;
289289
const TypeRef *TR;
290290
bool Indirect;
291+
bool Generic;
291292

292-
FieldTypeInfo() : Name(""), Value(0), TR(nullptr), Indirect(false) {}
293-
FieldTypeInfo(const std::string &Name, int Value, const TypeRef *TR, bool Indirect)
294-
: Name(Name), Value(Value), TR(TR), Indirect(Indirect) {}
293+
FieldTypeInfo() : Name(""), Value(0), TR(nullptr), Indirect(false), Generic(false) {}
294+
FieldTypeInfo(const std::string &Name, int Value, const TypeRef *TR, bool Indirect, bool Generic)
295+
: Name(Name), Value(Value), TR(TR), Indirect(Indirect), Generic(Generic) {}
295296

296297
static FieldTypeInfo forEmptyCase(std::string Name, int Value) {
297-
return FieldTypeInfo(Name, Value, nullptr, false);
298-
}
299-
300-
static FieldTypeInfo forIndirectCase(std::string Name, int Value, const TypeRef *TR) {
301-
return FieldTypeInfo(Name, Value, TR, true);
298+
return FieldTypeInfo(Name, Value, nullptr, false, false);
302299
}
303300

304301
static FieldTypeInfo forField(std::string Name, int Value, const TypeRef *TR) {
305-
return FieldTypeInfo(Name, Value, TR, false);
302+
return FieldTypeInfo(Name, Value, TR, false, false);
306303
}
307304
};
308305

‎stdlib/public/RemoteInspection/TypeLowering.cpp

+239-180
Large diffs are not rendered by default.

‎stdlib/public/RemoteInspection/TypeRefBuilder.cpp

+7-5
Original file line numberDiff line numberDiff line change
@@ -373,14 +373,16 @@ bool TypeRefBuilder::getFieldTypeRefs(
373373
if (!Unsubstituted)
374374
return false;
375375

376+
// TODO: Consider `struct S<T> { enum E { case a(T); case b(T?) }}`
377+
// This test identifies `a` as a generic case, but not `b`
378+
bool IsGeneric = isa<GenericTypeParameterTypeRef>(Unsubstituted);
379+
376380
auto Substituted = Unsubstituted->subst(*this, *Subs);
377381

378-
if (FD->isEnum() && Field->isIndirectCase()) {
379-
Fields.push_back(FieldTypeInfo::forIndirectCase(FieldName.str(), FieldValue, Substituted));
380-
continue;
381-
}
382+
bool IsIndirect = FD->isEnum() && Field->isIndirectCase();
382383

383-
Fields.push_back(FieldTypeInfo::forField(FieldName.str(), FieldValue, Substituted));
384+
auto FieldTI = FieldTypeInfo(FieldName.str(), FieldValue, Substituted, IsIndirect, IsGeneric);
385+
Fields.push_back(FieldTI);
384386
}
385387
return true;
386388
}

‎validation-test/Reflection/reflect_Enum_MultiPayload_degenerate.swift

+5
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,11 @@ reflect(enum: FooVoid.a([]))
2626

2727
// CHECK: Type info:
2828

29+
// Note: MemoryLayout<FooVoid> says that this really is size=8, alignment=8, stride=8
30+
// Explanation: [Int] is a pointer, so this enum can use NULL to represent the other case
31+
// Aside: In TypeLowering.cpp, enum FooVoid does not have a FixedDescriptor even though
32+
// its not generic. This is why we check for having only a single payload first.
33+
2934
// CHECK-64: (multi_payload_enum size=8 alignment=8 stride=8 num_extra_inhabitants=2147483646 bitwise_takable=1
3035
// CHECK-64: (case name=a index=0 offset=0
3136
// CHECK-64: (struct size=8 alignment=8 stride=8 num_extra_inhabitants=2147483647 bitwise_takable=1

‎validation-test/Reflection/reflect_Enum_MultiPayload_generic.swift

+10-1
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,15 @@ class ClassWithEnumDepth0<T> {
2525
var e: E?
2626
}
2727

28+
/*
29+
print("ClassWithEnumDepth0: ",
30+
MemoryLayout<ClassWithEnumDepth0<S>.E?>.size,
31+
" ",
32+
MemoryLayout<ClassWithEnumDepth0<S>.E?>.alignment,
33+
" ",
34+
MemoryLayout<ClassWithEnumDepth0<S>.E?>.stride)
35+
*/
36+
2837
reflect(object: ClassWithEnumDepth0<S>())
2938

3039
// CHECK: Reflecting an object.
@@ -85,7 +94,6 @@ reflect(object: ClassWithEnumDepth0<S>())
8594
// X32-NEXT: (builtin size=4 alignment=4 stride=4 num_extra_inhabitants=0 bitwise_takable=1))))))
8695
// X32-NEXT: (case name=none index=1))))
8796

88-
8997
class ClassWithEnumDepth1<T> {
9098
enum E<T> {
9199
case t(T)
@@ -154,6 +162,7 @@ reflect(object: ClassWithEnumDepth1<S>())
154162
// X32-NEXT: (builtin size=4 alignment=4 stride=4 num_extra_inhabitants=0 bitwise_takable=1))))))
155163
// X32-NEXT: (case name=none index=1))))
156164

165+
157166
doneReflecting()
158167

159168
// CHECK: Done.

‎validation-test/Reflection/reflect_Enum_MultiPayload_generic2.swift

+79
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,79 @@
1+
// RUN: %empty-directory(%t)
2+
// RUN: %target-build-swift -lswiftSwiftReflectionTest %s -o %t/reflect_Enum_MultiPayload_generic2
3+
// RUN: %target-codesign %t/reflect_Enum_MultiPayload_generic2
4+
5+
// RUN: %target-run %target-swift-reflection-test %t/reflect_Enum_MultiPayload_generic2 | tee /dev/stderr | %FileCheck %s --check-prefix=CHECK --check-prefix=X%target-ptrsize --dump-input=fail
6+
7+
// REQUIRES: reflection_test_support
8+
// REQUIRES: objc_interop
9+
// REQUIRES: executable_test
10+
// UNSUPPORTED: use_os_stdlib
11+
12+
import SwiftReflectionTest
13+
14+
class ClassTypeA {}
15+
class ClassTypeB {}
16+
enum SimplePayload1<T, U>{
17+
case a(T)
18+
case b(U)
19+
}
20+
21+
reflect(enum: SimplePayload1<ClassTypeA, Void>.a(ClassTypeA()))
22+
23+
// X64: Reflecting an enum.
24+
// X64-NEXT: Instance pointer in child address space: 0x{{[0-9a-fA-F]+}}
25+
// X64-NEXT: Type reference:
26+
// X64-NEXT: (bound_generic_enum reflect_Enum_MultiPayload_generic2.SimplePayload1
27+
// X64-NEXT: (class reflect_Enum_MultiPayload_generic2.ClassTypeA)
28+
// X64-NEXT: (tuple))
29+
30+
// MemoryLayout<SimplePayload1<ClassTypeA, Void>> gives 9,8,16
31+
// SimplePayload1 is a BoundGenericTypeRef
32+
// It's getting laid out as a tagged MPE, not as an SPE
33+
// (an SPE would use the XIs of the ptr) XXXXXX OR WOULD IT??? Hmmmmm....
34+
35+
// X64: Type info:
36+
// X64-NEXT: (multi_payload_enum size=9 alignment=8 stride=16 num_extra_inhabitants=254 bitwise_takable=1
37+
// X64-NEXT: (case name=a index=0 offset=0
38+
// X64-NEXT: (reference kind=strong refcounting=native))
39+
// X64-NEXT: (case name=b index=1 offset=0
40+
// X64-NEXT: (tuple size=0 alignment=1 stride=1 num_extra_inhabitants=0 bitwise_takable=1)))
41+
// X64-NEXT: Mangled name: $s34reflect_Enum_MultiPayload_generic214SimplePayload1OyAA10ClassTypeACytG
42+
// X64-NEXT: Demangled name: reflect_Enum_MultiPayload_generic2.SimplePayload1<reflect_Enum_MultiPayload_generic2.ClassTypeA, ()>
43+
44+
// X64: Enum value:
45+
// X64-NEXT: (enum_value name=a index=0
46+
// X64-NEXT: (class reflect_Enum_MultiPayload_generic2.ClassTypeA)
47+
// X64-NEXT: )
48+
49+
// X32: FAIL
50+
51+
reflect(enum: SimplePayload1<ClassTypeA, Void>.b(()))
52+
53+
// X64: Reflecting an enum.
54+
// X64-NEXT: Instance pointer in child address space: 0x{{[0-9a-fA-F]+}}
55+
// X64-NEXT: Type reference:
56+
// X64-NEXT: (bound_generic_enum reflect_Enum_MultiPayload_generic2.SimplePayload1
57+
// X64-NEXT: (class reflect_Enum_MultiPayload_generic2.ClassTypeA)
58+
// X64-NEXT: (tuple))
59+
60+
// X64: Type info:
61+
// X64-NEXT: (multi_payload_enum size=9 alignment=8 stride=16 num_extra_inhabitants=254 bitwise_takable=1
62+
// X64-NEXT: (case name=a index=0 offset=0
63+
// X64-NEXT: (reference kind=strong refcounting=native))
64+
// X64-NEXT: (case name=b index=1 offset=0
65+
// X64-NEXT: (tuple size=0 alignment=1 stride=1 num_extra_inhabitants=0 bitwise_takable=1)))
66+
// X64-NEXT: Mangled name: $s34reflect_Enum_MultiPayload_generic214SimplePayload1OyAA10ClassTypeACytG
67+
// X64-NEXT: Demangled name: reflect_Enum_MultiPayload_generic2.SimplePayload1<reflect_Enum_MultiPayload_generic2.ClassTypeA, ()>
68+
69+
// X64: Enum value:
70+
// X64-NEXT: (enum_value name=b index=1
71+
// X64-NEXT: (tuple)
72+
// X64-NEXT: )
73+
74+
// X32: FAIL
75+
76+
doneReflecting()
77+
78+
// CHECK: Done.
79+

‎validation-test/Reflection/reflect_Enum_MultiPayload_generic3.swift

+69
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,69 @@
1+
// RUN: %empty-directory(%t)
2+
// RUN: %target-build-swift -lswiftSwiftReflectionTest %s -o %t/reflect_Enum_MultiPayload_generic3
3+
// RUN: %target-codesign %t/reflect_Enum_MultiPayload_generic3
4+
5+
// RUN: %target-run %target-swift-reflection-test %t/reflect_Enum_MultiPayload_generic3 | tee /dev/stderr | %FileCheck %s --check-prefix=CHECK --check-prefix=X%target-ptrsize --dump-input=fail
6+
7+
// REQUIRES: reflection_test_support
8+
// REQUIRES: objc_interop
9+
// REQUIRES: executable_test
10+
// UNSUPPORTED: use_os_stdlib
11+
12+
import SwiftReflectionTest
13+
import Darwin
14+
15+
private func debugLog(_ message: @autoclosure () -> String) {
16+
fputs("Child: \(message())\n", stderr)
17+
fflush(stderr)
18+
}
19+
20+
struct StructWithEnumDepth0<T> {
21+
enum E {
22+
case t(T)
23+
case u(Void)
24+
}
25+
var e: E?
26+
}
27+
28+
reflect(enum: StructWithEnumDepth0<Int>?.none)
29+
30+
// CHECK: Reflecting an enum.
31+
// CHECK-NEXT: Instance pointer in child address space: 0x{{[0-9a-fA-F]+}}
32+
// CHECK-NEXT: Type reference:
33+
// CHECK-NEXT: (bound_generic_enum Swift.Optional
34+
// CHECK-NEXT: (bound_generic_struct reflect_Enum_MultiPayload_generic3.StructWithEnumDepth0
35+
// CHECK-NEXT: (struct Swift.Int)))
36+
37+
// MemoryLayout<> on ARM64 macOS gives
38+
// MemoryLayout<StructWithEnumDepth0<Int>.E>.size => 9
39+
// MemoryLayout<StructWithEnumDepth0<Int>>.size => 10
40+
// MemoryLayout<StructWithEnumDepth0<Int>?>.size => 11
41+
42+
// X64: Type info:
43+
// X64-NEXT: (single_payload_enum size=11 alignment=8 stride=16 num_extra_inhabitants=0 bitwise_takable=1
44+
// X64-NEXT: (case name=some index=0 offset=0
45+
// X64-NEXT: (struct size=10 alignment=8 stride=16 num_extra_inhabitants=0 bitwise_takable=1
46+
// X64-NEXT: (field name=e offset=0
47+
// X64-NEXT: (single_payload_enum size=10 alignment=8 stride=16 num_extra_inhabitants=0 bitwise_takable=1
48+
// X64-NEXT: (case name=some index=0 offset=0
49+
// X64-NEXT: (multi_payload_enum size=9 alignment=8 stride=16 num_extra_inhabitants=0 bitwise_takable=1
50+
// X64-NEXT: (case name=t index=0 offset=0
51+
// X64-NEXT: (struct size=8 alignment=8 stride=8 num_extra_inhabitants=0 bitwise_takable=1
52+
// X64-NEXT: (field name=_value offset=0
53+
// X64-NEXT: (builtin size=8 alignment=8 stride=8 num_extra_inhabitants=0 bitwise_takable=1))))
54+
// X64-NEXT: (case name=u index=1 offset=0
55+
// X64-NEXT: (tuple size=0 alignment=1 stride=1 num_extra_inhabitants=0 bitwise_takable=1))))
56+
// X64-NEXT: (case name=none index=1)))))
57+
// X64-NEXT: (case name=none index=1))
58+
// X64-NEXT: Mangled name: $s34reflect_Enum_MultiPayload_generic3010StructWithB6Depth0VySiGSg
59+
// X64-NEXT: Demangled name: Swift.Optional<reflect_Enum_MultiPayload_generic3.StructWithEnumDepth0<Swift.Int>>
60+
61+
// X32: FAIL
62+
63+
// CHECK: Enum value:
64+
// CHECK-NEXT: (enum_value name=none index=1)
65+
66+
doneReflecting()
67+
68+
// CHECK: Done.
69+

‎validation-test/Reflection/reflect_Enum_MultiPayload_generic4.swift

+69
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,69 @@
1+
// RUN: %empty-directory(%t)
2+
// RUN: %target-build-swift -lswiftSwiftReflectionTest %s -o %t/reflect_Enum_MultiPayload_generic4
3+
// RUN: %target-codesign %t/reflect_Enum_MultiPayload_generic4
4+
5+
// RUN: %target-run %target-swift-reflection-test %t/reflect_Enum_MultiPayload_generic4 | tee /dev/stderr | %FileCheck %s --check-prefix=CHECK --check-prefix=X%target-ptrsize --dump-input=fail
6+
7+
// REQUIRES: reflection_test_support
8+
// REQUIRES: objc_interop
9+
// REQUIRES: executable_test
10+
// UNSUPPORTED: use_os_stdlib
11+
12+
import SwiftReflectionTest
13+
import Darwin
14+
15+
private func debugLog(_ message: @autoclosure () -> String) {
16+
fputs("Child: \(message())\n", stderr)
17+
fflush(stderr)
18+
}
19+
20+
struct StructWithEnumDepth0<T> {
21+
enum E {
22+
case t(T)
23+
case u(Int)
24+
}
25+
var e: E?
26+
}
27+
28+
reflect(enum: StructWithEnumDepth0<Int>?.none)
29+
30+
// CHECK: Reflecting an enum.
31+
// CHECK-NEXT: Instance pointer in child address space: 0x{{[0-9a-fA-F]+}}
32+
// CHECK-NEXT: Type reference:
33+
// CHECK-NEXT: (bound_generic_enum Swift.Optional
34+
// CHECK-NEXT: (bound_generic_struct reflect_Enum_MultiPayload_generic4.StructWithEnumDepth0
35+
// CHECK-NEXT: (struct Swift.Int)))
36+
37+
// MemoryLayout<> on ARM64 macOS gives 9,8,16 as the sizes of both SWED0<Int>? and SWED0<Int>.E
38+
// from that, we can infer that SWED0<Int>.E must have a non-zero number of extra inhabitants
39+
40+
// X64: Type info:
41+
// X64-NEXT: (single_payload_enum size=9 alignment=8 stride=16 num_extra_inhabitants=252 bitwise_takable=1
42+
// X64-NEXT: (case name=some index=0 offset=0
43+
// X64-NEXT: (struct size=9 alignment=8 stride=16 num_extra_inhabitants=253 bitwise_takable=1
44+
// X64-NEXT: (field name=e offset=0
45+
// X64-NEXT: (single_payload_enum size=9 alignment=8 stride=16 num_extra_inhabitants=253 bitwise_takable=1
46+
// X64-NEXT: (case name=some index=0 offset=0
47+
// X64-NEXT: (multi_payload_enum size=9 alignment=8 stride=16 num_extra_inhabitants=254 bitwise_takable=1
48+
// X64-NEXT: (case name=t index=0 offset=0
49+
// X64-NEXT: (struct size=8 alignment=8 stride=8 num_extra_inhabitants=0 bitwise_takable=1
50+
// X64-NEXT: (field name=_value offset=0
51+
// X64-NEXT: (builtin size=8 alignment=8 stride=8 num_extra_inhabitants=0 bitwise_takable=1))))
52+
// X64-NEXT: (case name=u index=1 offset=0
53+
// X64-NEXT: (struct size=8 alignment=8 stride=8 num_extra_inhabitants=0 bitwise_takable=1
54+
// X64-NEXT: (field name=_value offset=0
55+
// X64-NEXT: (builtin size=8 alignment=8 stride=8 num_extra_inhabitants=0 bitwise_takable=1))))))
56+
// X64-NEXT: (case name=none index=1)))))
57+
// X64-NEXT: (case name=none index=1))
58+
// X64-NEXT: Mangled name: $s34reflect_Enum_MultiPayload_generic4010StructWithB6Depth0VySiGSg
59+
// X64-NEXT: Demangled name: Swift.Optional<reflect_Enum_MultiPayload_generic4.StructWithEnumDepth0<Swift.Int>>
60+
61+
// X32: FAIL
62+
63+
// CHECK: Enum value:
64+
// CHECK-NEXT: (enum_value name=none index=1)
65+
66+
doneReflecting()
67+
68+
// CHECK: Done.
69+

0 commit comments

Comments
 (0)