RemoteMirror: Project some multi-payload enums (#30357)

tbkka · web-flow · commit ee36c1cc3eea · 2020-03-11T18:48:39.000-07:00
* First part of multi-payload enum support

This handles multi-payload enums with fixed
layouts that don't use spare payload bits.
It includes XI calculations that allow us to
handle single-payload enums where the payload
ultimately includes a multi-payload enum
(For example, on 32-bit platforms, String uses
a multi-payload enum, so this now supports single-payload
enums carrying Strings.)
diff --git a/include/swift/Reflection/ReflectionContext.h b/include/swift/Reflection/ReflectionContext.h
@@ -788,7 +788,7 @@ class ReflectionContext
         return false;
       } else {
         // No payload: Payload area is reused for more cases
-        unsigned PayloadTag = 0;
+        uint32_t PayloadTag = 0;
         auto PayloadTagSize = std::min(PayloadSize, decltype(PayloadSize)(sizeof(PayloadTag)));
         if (!getReader().readInteger(EnumAddress, PayloadTagSize, &PayloadTag)) {
           return false;
@@ -801,7 +801,50 @@ class ReflectionContext
     }
 
     case RecordKind::MultiPayloadEnum: {
-      // TODO: Support multipayload enums
+      // Collect basic statistics about the enum
+      unsigned long PayloadCaseCount = 0;
+      unsigned long NonPayloadCaseCount = 0;
+      unsigned long PayloadSize = 0;
+      for (auto Field : Fields) {
+        if (Field.TR != 0) {
+          PayloadCaseCount += 1;
+          if (Field.TI.getSize() > PayloadSize) {
+            PayloadSize = Field.TI.getSize();
+          }
+        } else {
+          NonPayloadCaseCount += 1;
+        }
+      }
+      if (EnumSize > PayloadSize) {
+        // If the compiler laid this out with a separate tag, use that.
+        unsigned tag = 0;
+        auto TagSize = EnumSize - PayloadSize;
+        auto TagAddress = remote::RemoteAddress(EnumAddress.getAddressData() + PayloadSize);
+        if (!getReader().readInteger(TagAddress, TagSize, &tag)
+           || tag >= Fields.size()) {
+          return false;
+        }
+        if (tag < PayloadCaseCount) {
+          *CaseIndex = tag;
+          return true;
+        }
+        auto PayloadTagSize = std::min(PayloadSize, 4UL);
+        // Treat the tag as a page selector; payload carries the offset within the page
+        auto Page = tag - PayloadCaseCount;
+        // Zero for 32-bit because we'll never have more than one page
+        auto PageSize = PayloadTagSize >= 4 ? 0 : 1 << (PayloadSize * 8U);
+        auto PageStart = Page * PageSize;
+        unsigned PayloadTag;
+        if (!getReader().readInteger(EnumAddress, PayloadTagSize, &PayloadTag)) {
+          return false;
+        }
+        *CaseIndex = PageStart + PayloadTag + PayloadCaseCount;
+        return true;
+      } else {
+        // XXX TODO: If the payloads have common spare bits (e.g., all pointers)
+        // then use those to decode the case.
+        return false;
+      }
       break;
     }
 
diff --git a/include/swift/Remote/MemoryReader.h b/include/swift/Remote/MemoryReader.h
@@ -64,44 +64,20 @@ class MemoryReader {
   /// Attempts to read an integer of the specified size from the given
   /// address in the remote process.
   ///
-  /// Returns false if the operation failed, the request size is not
-  /// 1, 2, 4, or 8, or the request size is larger than the provided destination.
+  /// Returns false if the operation failed, or the request size is
+  /// larger than the provided destination.
   template <typename IntegerType>
   bool readInteger(RemoteAddress address, size_t bytes, IntegerType *dest) {
-    if (bytes > sizeof(IntegerType))
-      return false;
-    switch (bytes) {
-    case 1: {
-      uint8_t n;
-      if (!readInteger(address, &n))
-        return false;
-      *dest = n;
-      break;
-    }
-    case 2: {
-      uint16_t n;
-      if (!readInteger(address, &n))
-        return false;
-      *dest = n;
-      break;
-    }
-    case 4: {
-      uint32_t n;
-      if (!readInteger(address, &n))
-        return false;
-      *dest = n;
-      break;
-    }
-    case 8: {
-      uint64_t n;
-      if (!readInteger(address, &n))
-        return false;
-      *dest = n;
-      break;
-    }
-    default:
+    *dest = 0;
+    if ((bytes > sizeof(IntegerType))
+        || !readBytes(address, (uint8_t *)dest, bytes)) {
       return false;
     }
+    // FIXME: Assumes host and target have the same endianness.
+    // TODO: Query DLQ for endianness of target, compare to endianness of host.
+#if defined(__BIG_ENDIAN__)
+    *dest >>= (sizeof(IntegerType) - bytes);
+#endif
     return true;
   }
 
diff --git a/stdlib/public/Reflection/TypeLowering.cpp b/stdlib/public/Reflection/TypeLowering.cpp
@@ -363,9 +363,53 @@ bool RecordTypeInfo::readExtraInhabitantIndex(remote::MemoryReader &reader,
     return false;
   }
 
-  case RecordKind::MultiPayloadEnum:// XXX TODO
-    return false;
-    
+  case RecordKind::MultiPayloadEnum: {
+    // Multi payload enums can export XIs from the tag, if any.
+    // They never export XIs from their payloads.
+    auto Fields = getFields();
+    unsigned long PayloadCaseCount = 0;
+    unsigned long NonPayloadCaseCount = 0;
+    unsigned long PayloadSize = 0;
+    for (auto Field : Fields) {
+      if (Field.TR != 0) {
+        PayloadCaseCount += 1;
+        if (Field.TI.getSize() > PayloadSize) {
+          PayloadSize = Field.TI.getSize();
+        }
+      } else {
+        NonPayloadCaseCount += 1;
+      }
+    }
+    if (getSize() > PayloadSize) {
+      // Multipayload enums that do use a separate tag
+      // export XIs from that tag.
+      unsigned tag = 0;
+      auto TagSize = getSize() - PayloadSize;
+      auto TagAddress = remote::RemoteAddress(address.getAddressData() + PayloadSize);
+      if (!reader.readInteger(TagAddress, TagSize, &tag))
+        return false;
+      if (tag < Fields.size()) {
+        *extraInhabitantIndex = -1; // Valid payload, not an XI
+      } else if (TagSize >= 4) {
+        // This is really just the 32-bit 2s-complement negation of `tag`, but
+        // coded so as to ensure we cannot overflow or underflow.
+        *extraInhabitantIndex = static_cast<int>(
+          std::numeric_limits<uint32_t>::max()
+          - (tag & std::numeric_limits<uint32_t>::max())
+          + 1);
+      } else {
+        // XIs are coded starting from the highest value that fits
+        // E.g., for 1-byte tag, tag 255 == XI #0, tag 254 == XI #1, etc.
+        unsigned maxTag = (1U << (TagSize * 8U)) - 1;
+        *extraInhabitantIndex = maxTag - tag;
+      }
+      return true;
+    } else {
+      // Multipayload enums that don't use a separate tag never
+      // export XIs.
+      return false;
+    }
+  }
   }
   return false;
 }
@@ -1260,7 +1304,7 @@ class EnumTypeInfoBuilder {
         Size += tagCounts.numTagBytes;
         // Dynamic multi-payload enums use the tag representations not assigned
         // to cases for extra inhabitants.
-        if (tagCounts.numTagBytes >= 32) {
+        if (tagCounts.numTagBytes >= 4) {
           NumExtraInhabitants = ValueWitnessFlags::MaxNumExtraInhabitants;
         } else {
           NumExtraInhabitants =
diff --git a/validation-test/Reflection/reflect_Enum_TwoCaseTwoPayloads.swift b/validation-test/Reflection/reflect_Enum_TwoCaseTwoPayloads.swift
@@ -269,6 +269,30 @@ reflect(object: ClassWithTwoCaseTwoPayloadsEnum())
 // CHECK-32:           (case name=none index=1)))
 // CHECK-32:       (case name=none index=1))))
 
+reflect(enumValue: TwoCaseTwoPayloadsEnum.valid(Marker()))
+
+// CHECK-64: Reflecting an enum value.
+// CHECK-64-NEXT: Type reference:
+// CHECK-64-NEXT: (enum reflect_Enum_TwoCaseTwoPayloads.TwoCaseTwoPayloadsEnum)
+// CHECK-64-NEXT: Value: .valid(_)
+
+// CHECK-32: Reflecting an enum value.
+// CHECK-32-NEXT: Type reference:
+// CHECK-32-NEXT: (enum reflect_Enum_TwoCaseTwoPayloads.TwoCaseTwoPayloadsEnum)
+// CHECK-32-NEXT: Value: .valid(_)
+
+reflect(enumValue: TwoCaseTwoPayloadsEnum.invalid(7))
+
+// CHECK-64: Reflecting an enum value.
+// CHECK-64-NEXT: Type reference:
+// CHECK-64-NEXT: (enum reflect_Enum_TwoCaseTwoPayloads.TwoCaseTwoPayloadsEnum)
+// CHECK-64-NEXT: Value: .invalid(_)
+
+// CHECK-32: Reflecting an enum value.
+// CHECK-32-NEXT: Type reference:
+// CHECK-32-NEXT: (enum reflect_Enum_TwoCaseTwoPayloads.TwoCaseTwoPayloadsEnum)
+// CHECK-32-NEXT: Value: .invalid(_)
+
 doneReflecting()
 
 // CHECK-64: Done.
diff --git a/validation-test/Reflection/reflect_Enum_value.swift b/validation-test/Reflection/reflect_Enum_value.swift
@@ -378,6 +378,107 @@ reflect(enumValue: OneIndirectPayload.leafF)
 // CHECK-NEXT: (enum reflect_Enum_value.OneIndirectPayload)
 // CHECK-NEXT: Value: .leafF
 
+enum MultiPayload {
+case stampA
+case envelopeA(Int64)
+case stampB
+case envelopeB(Double)
+case stampC
+case envelopeC((Int32, Int32))
+case stampD
+case stampE
+}
+
+enum SinglePayloadEnumWithMultiPayloadEnumPayload {
+case payloadA(MultiPayload)
+case alsoA
+case alsoB
+case alsoC
+case alsoD
+}
+
+reflect(enumValue: SinglePayloadEnumWithMultiPayloadEnumPayload.payloadA(.stampB))
+
+// CHECK: Reflecting an enum value.
+// CHECK-NEXT: Type reference:
+// CHECK-NEXT: (enum reflect_Enum_value.SinglePayloadEnumWithMultiPayloadEnumPayload)
+// CHECK-NEXT: Value: .payloadA(.stampB)
+
+reflect(enumValue: SinglePayloadEnumWithMultiPayloadEnumPayload.payloadA(.envelopeC((1,2))))
+
+// CHECK: Reflecting an enum value.
+// CHECK-NEXT: Type reference:
+// CHECK-NEXT: (enum reflect_Enum_value.SinglePayloadEnumWithMultiPayloadEnumPayload)
+// CHECK-NEXT: Value: .payloadA(.envelopeC(_))
+
+reflect(enumValue: SinglePayloadEnumWithMultiPayloadEnumPayload.alsoC)
+
+// CHECK: Reflecting an enum value.
+// CHECK-NEXT: Type reference:
+// CHECK-NEXT: (enum reflect_Enum_value.SinglePayloadEnumWithMultiPayloadEnumPayload)
+// CHECK-NEXT: Value: .alsoC
+
+reflect(enumValue: Optional<Optional<SinglePayloadEnumWithMultiPayloadEnumPayload>>.some(.none))
+
+// CHECK: Reflecting an enum value.
+// CHECK-NEXT: Type reference:
+// CHECK-NEXT: (bound_generic_enum Swift.Optional
+// CHECK-NEXT:  (bound_generic_enum Swift.Optional
+// CHECK-NEXT:   (enum reflect_Enum_value.SinglePayloadEnumWithMultiPayloadEnumPayload)))
+// CHECK-NEXT: Value: .some(.none)
+
+enum SmallMultiPayload {
+case stampA
+case envelopeA(Int8)
+case stampB
+case envelopeB(Int16)
+case stampC
+case envelopeC((UInt8, Int8))
+case stampD
+case stampE
+}
+
+enum SinglePayloadEnumWithSmallMultiPayloadEnumPayload {
+case payloadA(SmallMultiPayload)
+case alsoA
+case alsoB
+case alsoC
+case alsoD
+}
+
+reflect(enumValue: SinglePayloadEnumWithSmallMultiPayloadEnumPayload.payloadA(.stampB))
+
+// CHECK: Reflecting an enum value.
+// CHECK-NEXT: Type reference:
+// CHECK-NEXT: (enum reflect_Enum_value.SinglePayloadEnumWithSmallMultiPayloadEnumPayload)
+// CHECK-NEXT: Value: .payloadA(.stampB)
+
+reflect(enumValue: SinglePayloadEnumWithSmallMultiPayloadEnumPayload.payloadA(.envelopeC((1,2))))
+
+// CHECK: Reflecting an enum value.
+// CHECK-NEXT: Type reference:
+// CHECK-NEXT: (enum reflect_Enum_value.SinglePayloadEnumWithSmallMultiPayloadEnumPayload)
+// CHECK-NEXT: Value: .payloadA(.envelopeC(_))
+
+reflect(enumValue: SinglePayloadEnumWithSmallMultiPayloadEnumPayload.alsoC)
+
+// CHECK: Reflecting an enum value.
+// CHECK-NEXT: Type reference:
+// CHECK-NEXT: (enum reflect_Enum_value.SinglePayloadEnumWithSmallMultiPayloadEnumPayload)
+// CHECK-NEXT: Value: .alsoC
+
+reflect(enumValue: Optional<Optional<SinglePayloadEnumWithSmallMultiPayloadEnumPayload>>.some(.none))
+
+// CHECK: Reflecting an enum value.
+// CHECK-NEXT: Type reference:
+// CHECK-NEXT: (bound_generic_enum Swift.Optional
+// CHECK-NEXT:  (bound_generic_enum Swift.Optional
+// CHECK-NEXT:   (enum reflect_Enum_value.SinglePayloadEnumWithSmallMultiPayloadEnumPayload)))
+// CHECK-NEXT: Value: .some(.none)
+
+
+// XXX TODO: Multipayload enums with pointer payloads
+
 
 // XXX TODO: test enum with thin function payload XXX
 
diff --git a/validation-test/Reflection/reflect_Enum_values.swift b/validation-test/Reflection/reflect_Enum_values.swift
@@ -1575,12 +1575,9 @@ reflect(enum: ManyCasesOneStringPayload.payload("hello, world"))
 // CHECK32-NEXT:   (case name=otherC index=3))
 
 // CHECKALL: Enum value:
-// CHECK64-NEXT: (enum_value name=payload index=0
-// CHECK64-NEXT: (struct Swift.String)
-// CHECK64-NEXT: )
-
-// XXX Note: 32-bit String contains a multi_payload_enum which projectEnumValue cannot yet handle.
-// CHECK32-NEXT: swift_reflection_projectEnumValue failed.
+// CHECKALL-NEXT: (enum_value name=payload index=0
+// CHECKALL-NEXT: (struct Swift.String)
+// CHECKALL-NEXT: )
 
 reflect(enum: ManyCasesOneStringPayload.otherB)
 
@@ -1646,12 +1643,7 @@ reflect(enum: ManyCasesOneStringPayload.otherB)
 
 
 // CHECKALL: Enum value:
-// CHECK64-NEXT: (enum_value name=otherB index=2)
-
-// XXX Note: 32-bit String contains a multi_payload_enum which projectEnumValue cannot yet handle.
-// CHECK32-NEXT: swift_reflection_projectEnumValue failed.
-
-//reflect(enum: ManyCasesManyPayloads.a("hi, world"))
+// CHECKALL-NEXT: (enum_value name=otherB index=2)
 
 doneReflecting()
 
