Merge pull request #4469 from slavapestov/reflection-crash-invalid-single-payload-enum-3.0

Reflection: Fix crash in single-payload enum layout if payload type c…

Author: tkremenek

include/swift/Reflection/TypeLowering.h

@@ -19,6 +19,7 @@
 #define SWIFT_REFLECTION_TYPELOWERING_H
 
 #include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/DenseSet.h"
 #include "llvm/Support/Casting.h"
 
 #include <iostream>
@@ -206,6 +207,7 @@ class TypeConverter {
   TypeRefBuilder &Builder;
   std::vector<std::unique_ptr<const TypeInfo>> Pool;
   llvm::DenseMap<const TypeRef *, const TypeInfo *> Cache;
+  llvm::DenseSet<const TypeRef *> RecursionCheck;
   llvm::DenseMap<std::pair<unsigned, unsigned>,
                  const ReferenceTypeInfo *> ReferenceCache;
 

stdlib/public/Reflection/TypeLowering.cpp

@@ -908,19 +908,23 @@ class EnumTypeInfoBuilder {
       Kind = RecordKind::SinglePayloadEnum;
       addCase(PayloadCases[0].Name, CaseTR, CaseTI);
 
-      // Below logic should match the runtime function
-      // swift_initEnumValueWitnessTableSinglePayload().
-      NumExtraInhabitants = CaseTI->getNumExtraInhabitants();
-      if (NumExtraInhabitants >= NoPayloadCases) {
-        // Extra inhabitants can encode all no-payload cases.
-        NumExtraInhabitants -= NoPayloadCases;
-      } else {
-        // Not enough extra inhabitants for all cases. We have to add an
-        // extra tag field.
-        NumExtraInhabitants = 0;
-        Size += getNumTagBytes(Size,
-                               NoPayloadCases - NumExtraInhabitants,
-                               /*payloadCases=*/1);
+      // If we were unable to lower the payload type, do not proceed
+      // further.
+      if (CaseTI != nullptr) {
+        // Below logic should match the runtime function
+        // swift_initEnumValueWitnessTableSinglePayload().
+        NumExtraInhabitants = CaseTI->getNumExtraInhabitants();
+        if (NumExtraInhabitants >= NoPayloadCases) {
+          // Extra inhabitants can encode all no-payload cases.
+          NumExtraInhabitants -= NoPayloadCases;
+        } else {
+          // Not enough extra inhabitants for all cases. We have to add an
+          // extra tag field.
+          NumExtraInhabitants = 0;
+          Size += getNumTagBytes(Size,
+                                 NoPayloadCases - NumExtraInhabitants,
+                                 /*payloadCases=*/1);
+        }
       }
 
     // MultiPayloadEnumImplStrategy
@@ -1213,27 +1217,29 @@ class LowerType
   }
 
   const TypeInfo *visitOpaqueTypeRef(const OpaqueTypeRef *O) {
-    unreachable("Can't lower opaque TypeRef");
+    DEBUG(std::cerr << "Can't lower opaque TypeRef");
     return nullptr;
   }
 };
 
 const TypeInfo *TypeConverter::getTypeInfo(const TypeRef *TR) {
+  // See if we already computed the result
   auto found = Cache.find(TR);
-  if (found != Cache.end()) {
-    auto *TI = found->second;
-    assert(TI != nullptr && "TypeRef recursion detected");
-    return TI;
-  }
+  if (found != Cache.end())
+    return found->second;
 
-  // Detect recursion
-  Cache[TR] = nullptr;
+  // Detect invalid recursive value types (IRGen should not emit
+  // them in the first place, but there might be bugs)
+  if (!RecursionCheck.insert(TR).second) {
+    DEBUG(std::cerr << "TypeRef recursion detected");
+    return nullptr;
+  }
 
+  // Compute the result and cache it
   auto *TI = LowerType(*this).visit(TR);
+  Cache[TR] = TI;
 
-  // Cache the result
-  if (TI != nullptr)
-    Cache[TR] = TI;
+  RecursionCheck.erase(TR);
 
   return TI;
 }

test/Reflection/typeref_lowering_missing.swift

@@ -0,0 +1,15 @@
+// RUN: rm -rf %t && mkdir -p %t
+// RUN: %target-build-swift %S/Inputs/ConcreteTypes.swift -parse-as-library -emit-module -emit-library -module-name TypeLowering -Xfrontend -disable-reflection-metadata -o %t/libTypesToReflect
+// RUN: %target-swift-reflection-dump -binary-filename %t/libTypesToReflect -binary-filename %platform-module-dir/libswiftCore.dylib -dump-type-lowering < %s | %FileCheck %s
+
+// REQUIRES: objc_interop
+// REQUIRES: CPU=x86_64
+
+V12TypeLowering11BasicStruct
+// CHECK: (struct TypeLowering.BasicStruct)
+// CHECK: Invalid lowering
+
+GSqV12TypeLowering11BasicStruct_
+// CHECK: (bound_generic_enum Swift.Optional
+// CHECK:   (struct TypeLowering.BasicStruct))
+// CHECK: Invalid lowering