Skip to content

Commit b80e33b

Browse files
mikeashtbkka
mikeash
authored and
tbkka
committed
Fix reuse of the payload area when the discriminator != 0
1 parent 57f54dd commit b80e33b

File tree

3 files changed

+182
-30
lines changed

3 files changed

+182
-30
lines changed

‎include/swift/Reflection/ReflectionContext.h

Lines changed: 27 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -751,44 +751,51 @@ class ReflectionContext
751751
return false;
752752
unsigned long NonPayloadCaseCount = FieldCount - 1;
753753
unsigned long PayloadExtraInhabitants = PayloadCase.TI.getNumExtraInhabitants();
754-
unsigned discriminator = 0;
754+
unsigned Discriminator = 0;
755755
auto PayloadSize = PayloadCase.TI.getSize();
756756
if (NonPayloadCaseCount >= PayloadExtraInhabitants) {
757757
// There are more cases than inhabitants, we need a separate discriminator.
758758
auto TagInfo = getEnumTagCounts(PayloadSize, NonPayloadCaseCount, 1);
759759
auto TagSize = TagInfo.numTagBytes;
760760
auto TagAddress = RemoteAddress(EnumAddress.getAddressData() + PayloadSize);
761-
if (!getReader().readInteger(TagAddress, TagSize, &discriminator))
761+
if (!getReader().readInteger(TagAddress, TagSize, &Discriminator)) {
762+
printf(">>>> readXI failed to read discriminator\n\n");
762763
return false;
764+
}
763765
}
764766

765-
if (PayloadExtraInhabitants == 0) {
766-
// Payload has no XI, so discriminator fully determines the case
767-
*CaseIndex = discriminator;
767+
if (PayloadSize == 0) {
768+
// Payload carries no information, so discriminator fully determines the case
769+
*CaseIndex = Discriminator;
768770
return true;
769-
} else if (discriminator == 0) {
770-
// The value overlays the payload ... ask the payload to decode it.
771-
int t;
772-
if (!PayloadCase.TI.readExtraInhabitantIndex(getReader(), EnumAddress, &t)) {
771+
} else if (Discriminator == 0) {
772+
// The payload area carries all the information...
773+
if (PayloadExtraInhabitants == 0) {
774+
*CaseIndex = 0;
775+
return true;
776+
}
777+
int XITag = 0;
778+
if (!PayloadCase.TI.readExtraInhabitantIndex(getReader(), EnumAddress, &XITag)) {
773779
return false;
774780
}
775-
if (t < 0) {
776-
*CaseIndex = 0;
781+
if (XITag < 0) { // Valid (not extra) inhabitant
782+
*CaseIndex = 0; // Payload case is always #0
777783
return true;
778-
} else if ((unsigned long)t <= NonPayloadCaseCount) {
779-
*CaseIndex = t + 1;
784+
} else if ((unsigned)XITag <= NonPayloadCaseCount) {
785+
*CaseIndex = XITag + 1;
780786
return true;
781787
}
782788
return false;
783789
} else {
784-
// The entire payload area is available for additional cases:
785-
auto TagSize = std::max(PayloadSize, 4U); // XXX TODO XXX CHECK THIS
786-
auto offset = 1 + PayloadExtraInhabitants; // Cases coded with discriminator = 0
787-
unsigned casesInPayload = 1 << (TagSize * 8U);
788-
unsigned payloadCode;
789-
if (!getReader().readInteger(EnumAddress, TagSize, &payloadCode))
790+
// No payload: Payload area is reused for more cases
791+
unsigned PayloadTag = 0;
792+
auto PayloadTagSize = std::min(PayloadSize, decltype(PayloadSize)(sizeof(PayloadTag)));
793+
if (!getReader().readInteger(EnumAddress, PayloadTagSize, &PayloadTag)) {
790794
return false;
791-
*CaseIndex = offset + (discriminator - 1) * casesInPayload + payloadCode;
795+
}
796+
auto XICases = 1U + PayloadExtraInhabitants; // Cases coded with XIs when discriminator = 0
797+
auto PayloadCases = 1U << (PayloadTagSize * 8U);
798+
*CaseIndex = XICases + (Discriminator - 1) * PayloadCases + PayloadTag;
792799
return true;
793800
}
794801
}

‎stdlib/public/Reflection/TypeLowering.cpp

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -258,11 +258,24 @@ bool RecordTypeInfo::readExtraInhabitantIndex(remote::MemoryReader &reader,
258258
int *extraInhabitantIndex) const {
259259
switch (SubKind) {
260260
case RecordKind::Invalid:
261-
case RecordKind::ThickFunction:
262261
case RecordKind::OpaqueExistential:
263262
case RecordKind::ClosureContext:
264263
return false;
265264

265+
case RecordKind::ThickFunction: {
266+
if (Fields.size() != 2) {
267+
return false;
268+
}
269+
auto function = Fields[0];
270+
auto context = Fields[1];
271+
if (function.Offset != 0) {
272+
return false;
273+
}
274+
auto functionFieldAddress = address;
275+
return function.TI.readExtraInhabitantIndex(
276+
reader, functionFieldAddress, extraInhabitantIndex);
277+
}
278+
266279
case RecordKind::ClassExistential:
267280
case RecordKind::ExistentialMetatype:
268281
case RecordKind::ErrorExistential:

‎validation-test/Reflection/reflect_Enum_value.swift

Lines changed: 141 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -61,8 +61,9 @@ var a: Int
6161

6262
enum OneStructPayload {
6363
case payloadA(StructInt)
64-
case otherA
65-
case otherB
64+
case cowboyAlice
65+
case cowboyBob
66+
case cowboyCharlie
6667
}
6768

6869
reflect(enumValue: OneStructPayload.payloadA(StructInt(a: 0)))
@@ -72,12 +73,12 @@ reflect(enumValue: OneStructPayload.payloadA(StructInt(a: 0)))
7273
// CHECK-NEXT: (enum reflect_Enum_value.OneStructPayload)
7374
// CHECK-NEXT: Value: .payloadA(_)
7475

75-
reflect(enumValue: OneStructPayload.otherA)
76+
reflect(enumValue: OneStructPayload.cowboyCharlie)
7677

7778
// CHECK: Reflecting an enum value.
7879
// CHECK-NEXT: Type reference:
7980
// CHECK-NEXT: (enum reflect_Enum_value.OneStructPayload)
80-
// CHECK-NEXT: Value: .otherA
81+
// CHECK-NEXT: Value: .cowboyCharlie
8182

8283
@objc class ObjCClass : NSObject {
8384
var a: Int = 0
@@ -173,14 +174,14 @@ reflect(enumValue: Optional<Optional<OneSwiftClassPayload>>.some(.none))
173174
// CHECK-NEXT: (enum reflect_Enum_value.OneSwiftClassPayload)))
174175
// CHECK-NEXT: Value: .some(.none)
175176

176-
reflect(enumValue: Optional<Optional<OneSwiftClassPayload>>.some(.some(.otherA)))
177+
reflect(enumValue: Optional<Optional<OneSwiftClassPayload>>.some(.some(.otherC)))
177178

178179
// CHECK: Reflecting an enum value.
179180
// CHECK-NEXT: Type reference:
180181
// CHECK-NEXT: (bound_generic_enum Swift.Optional
181182
// CHECK-NEXT: (bound_generic_enum Swift.Optional
182183
// CHECK-NEXT: (enum reflect_Enum_value.OneSwiftClassPayload)))
183-
// CHECK-NEXT: Value: .some(.some(.otherA))
184+
// CHECK-NEXT: Value: .some(.some(.otherC))
184185

185186
reflect(enumValue: Optional<Optional<OneSwiftClassPayload>>.some(.some(.otherE)))
186187

@@ -211,12 +212,12 @@ case otherB
211212
case payloadA(MixedStruct)
212213
}
213214

214-
reflect(enumValue: OneMixedStructPayload.otherA)
215+
reflect(enumValue: OneMixedStructPayload.otherB)
215216

216217
// CHECK: Reflecting an enum value.
217218
// CHECK-NEXT: Type reference:
218219
// CHECK-NEXT: (enum reflect_Enum_value.OneMixedStructPayload)
219-
// CHECK-NEXT: Value: .otherA
220+
// CHECK-NEXT: Value: .otherB
220221

221222
reflect(enumValue: OneMixedStructPayload.payloadA(MixedStruct()))
222223

@@ -247,7 +248,138 @@ reflect(enumValue: OneNestedPayload.alternateB)
247248
// CHECK-NEXT: (enum reflect_Enum_value.OneNestedPayload)
248249
// CHECK-NEXT: Value: .alternateB
249250

250-
// XXX TODO: enum with tuple payload, enum with optional payload, indirect enum, enum with closure/function payload XXX
251+
252+
enum OneTuplePayload {
253+
case holderA((i: Int, c: SwiftClass))
254+
case emptyA
255+
case emptyB
256+
case emptyC
257+
}
258+
259+
reflect(enumValue: OneTuplePayload.holderA((i: 7, c: SwiftClass())))
260+
261+
// CHECK: Reflecting an enum value.
262+
// CHECK-NEXT: Type reference:
263+
// CHECK-NEXT: (enum reflect_Enum_value.OneTuplePayload)
264+
// CHECK-NEXT: Value: .holderA(_)
265+
266+
reflect(enumValue: OneTuplePayload.emptyB)
267+
268+
// CHECK: Reflecting an enum value.
269+
// CHECK-NEXT: Type reference:
270+
// CHECK-NEXT: (enum reflect_Enum_value.OneTuplePayload)
271+
// CHECK-NEXT: Value: .emptyB
272+
273+
func foo() -> Int { return 7; }
274+
275+
enum OneFunctionPayload {
276+
case cargoA(() -> Int)
277+
case alternateA
278+
case alternateB
279+
case alternateC
280+
}
281+
282+
reflect(enumValue: OneFunctionPayload.cargoA(foo))
283+
284+
// CHECK: Reflecting an enum value.
285+
// CHECK-NEXT: Type reference:
286+
// CHECK-NEXT: (enum reflect_Enum_value.OneFunctionPayload)
287+
// CHECK-NEXT: Value: .cargoA(_)
288+
289+
reflect(enumValue: OneFunctionPayload.alternateC)
290+
291+
// CHECK: Reflecting an enum value.
292+
// CHECK-NEXT: Type reference:
293+
// CHECK-NEXT: (enum reflect_Enum_value.OneFunctionPayload)
294+
// CHECK-NEXT: Value: .alternateC
295+
296+
func tester1() {
297+
let a = 7
298+
299+
func foo() -> Int { return a; }
300+
301+
enum OneClosurePayload {
302+
case cargoA(() -> Int)
303+
case alternateA
304+
case alternateB
305+
case alternateC
306+
}
307+
308+
reflect(enumValue: OneClosurePayload.cargoA(foo))
309+
310+
// CHECK: Reflecting an enum value.
311+
// CHECK-NEXT: Type reference:
312+
313+
// XXX TODO: Figure out why the type reference is dumped differently sometimes:
314+
// XXXX-NEXT: (nominal with unmangled suffix
315+
// XXXX-NEXT: (enum OneClosurePayload #1 in reflect_Enum_value.tester1() -> ())
316+
317+
// CHECK: Value: .cargoA(_)
318+
319+
reflect(enumValue: OneClosurePayload.alternateB)
320+
321+
// CHECK: Reflecting an enum value.
322+
// CHECK-NEXT: Type reference:
323+
324+
// XXX TODO: Figure out why the type reference is dumped differently sometimes:
325+
// XXXX-NEXT: (nominal with unmangled suffix
326+
// XXXX-NEXT: (enum OneClosurePayload #1 in reflect_Enum_value.tester1() -> ())
327+
328+
// CHECK: Value: .alternateB
329+
}
330+
331+
tester1()
332+
333+
334+
enum OneOptionalPayload {
335+
case boxA(Optional<Int>)
336+
case unboxA
337+
case unboxB
338+
case unboxC
339+
case unboxD
340+
case unboxE
341+
}
342+
343+
reflect(enumValue: OneOptionalPayload.boxA(7))
344+
345+
// CHECK: Reflecting an enum value.
346+
// CHECK-NEXT: Type reference:
347+
// CHECK-NEXT: (enum reflect_Enum_value.OneOptionalPayload)
348+
// CHECK-NEXT: Value: .boxA(.some(_))
349+
350+
reflect(enumValue: OneOptionalPayload.unboxE)
351+
352+
// CHECK: Reflecting an enum value.
353+
// CHECK-NEXT: Type reference:
354+
// CHECK-NEXT: (enum reflect_Enum_value.OneOptionalPayload)
355+
// CHECK-NEXT: Value: .unboxE
356+
357+
indirect enum OneIndirectPayload {
358+
case child(OneIndirectPayload)
359+
case leafA
360+
case leafB
361+
case leafC
362+
case leafD
363+
case leafE
364+
case leafF
365+
}
366+
367+
reflect(enumValue: OneIndirectPayload.child(.leafF))
368+
369+
// CHECK: Reflecting an enum value.
370+
// CHECK-NEXT: Type reference:
371+
// CHECK-NEXT: (enum reflect_Enum_value.OneIndirectPayload)
372+
// CHECK-NEXT: Value: .child(_)
373+
374+
reflect(enumValue: OneIndirectPayload.leafF)
375+
376+
// CHECK: Reflecting an enum value.
377+
// CHECK-NEXT: Type reference:
378+
// CHECK-NEXT: (enum reflect_Enum_value.OneIndirectPayload)
379+
// CHECK-NEXT: Value: .leafF
380+
381+
382+
// XXX TODO: test enum with thin function payload XXX
251383

252384
doneReflecting()
253385
// CHECK: Done.

0 commit comments

Comments
 (0)