From 40eb3c084d54c1f271e75cc70bfdb3b9e993cc4f Mon Sep 17 00:00:00 2001 From: Tim Kientzle Date: Wed, 5 Jun 2024 07:08:47 -0700 Subject: [PATCH 1/4] Expand the work from #73491 to support more MPE layouts. This also switches the MPE layout code to exclusively use the new code. The key observation: existing reflection metadata seems to already provide enough information in all cases, so we can abandon an earlier effort to add spare bitmask data. Resolves rdar://129281368 --- include/swift/RemoteInspection/BitMask.h | 11 ++- .../public/RemoteInspection/TypeLowering.cpp | 68 +++++++---------- .../reflect_Enum_values_resilient_enums.swift | 10 +++ .../reflect_Enum_values_resilient.swift | 74 +++++++++++++++++++ 4 files changed, 120 insertions(+), 43 deletions(-) create mode 100644 validation-test/Reflection/Inputs/reflect_Enum_values_resilient_enums.swift create mode 100644 validation-test/Reflection/reflect_Enum_values_resilient.swift diff --git a/include/swift/RemoteInspection/BitMask.h b/include/swift/RemoteInspection/BitMask.h index f64f2126996a1..866e0e5f24a7e 100644 --- a/include/swift/RemoteInspection/BitMask.h +++ b/include/swift/RemoteInspection/BitMask.h @@ -74,7 +74,16 @@ class BitMask { BitMask(unsigned sizeInBytes, uint64_t sourceMask): size(sizeInBytes) { mask = (uint8_t *)calloc(1, sizeInBytes); - memcpy(mask, &sourceMask, sizeInBytes); + if (!mask) { + assert(false && "Failed to allocate Bitmask"); + size = 0; + return; + } + size_t toCopy = sizeInBytes; + if (toCopy > sizeof(sourceMask)) { + toCopy = sizeof(sourceMask); + } + memcpy(mask, &sourceMask, toCopy); } // Construct a bitmask of the appropriate number of bytes diff --git a/stdlib/public/RemoteInspection/TypeLowering.cpp b/stdlib/public/RemoteInspection/TypeLowering.cpp index 8a2d36dfeedda..25c360a2f890a 100644 --- a/stdlib/public/RemoteInspection/TypeLowering.cpp +++ b/stdlib/public/RemoteInspection/TypeLowering.cpp @@ -315,6 +315,7 @@ BitMask BuiltinTypeInfo::getSpareBits(TypeConverter &TC, bool &hasAddrOnly) cons mask.keepOnlyMostSignificantBits(getSize() * 8 - intSize); return mask; } else if ( + Name == "ypXp" || // Any.Type Name == "yyXf" // 'yyXf' = @thin () -> Void function ) { // Builtin types that expose pointer spare bits @@ -578,7 +579,9 @@ class NoPayloadEnumTypeInfo: public EnumTypeInfo { } BitMask getSpareBits(TypeConverter &TC, bool &hasAddrOnly) const override { - return BitMask::zeroMask(getSize()); + auto mask = BitMask(getSize(), maskForCount(getNumCases())); + mask.complement(); + return mask; } bool projectEnumValue(remote::MemoryReader &reader, @@ -648,7 +651,17 @@ class SinglePayloadEnumTypeInfo: public EnumTypeInfo { } BitMask getSpareBits(TypeConverter &TC, bool &hasAddrOnly) const override { - return BitMask::zeroMask(getSize()); + FieldInfo PayloadCase = getCases()[0]; + size_t payloadSize = PayloadCase.TI.getSize(); + if (getSize() <= payloadSize) { + return BitMask::zeroMask(getSize()); + } + size_t tagSize = getSize() - payloadSize; + auto mask = BitMask::oneMask(getSize()); + mask.keepOnlyMostSignificantBits(tagSize * 8); // Clear payload bits + auto tagMaskUsedBits = BitMask(getSize(), maskForCount(getNumCases())); + mask.andNotMask(tagMaskUsedBits, payloadSize); // Clear used tag bits + return mask; } // Think of a single-payload enum as being encoded in "pages". @@ -2046,11 +2059,10 @@ class EnumTypeInfoBuilder { // // Do we have a fixed layout? - // TODO: Test whether a missing FixedDescriptor is actually relevant. auto FixedDescriptor = TC.getBuilder().getBuiltinTypeDescriptor(TR); if (!FixedDescriptor || GenericPayloadCases > 0) { // This is a "dynamic multi-payload enum". For example, - // this occurs with: + // this occurs with generics such as: // ``` // class ClassWithEnum { // enum E { @@ -2060,6 +2072,12 @@ class EnumTypeInfoBuilder { // var e: E? // } // ``` + // and when we have a resilient inner enum, such as: + // ``` + // enum E2 { + // case y(E1_resilient) + // case z(Int) + // } auto tagCounts = getEnumTagCounts(Size, EffectiveNoPayloadCases, EffectivePayloadCases); Size += tagCounts.numTagBytes; @@ -2091,7 +2109,6 @@ class EnumTypeInfoBuilder { unsigned Stride = ((Size + Alignment - 1) & ~(Alignment - 1)); if (Stride == 0) Stride = 1; - auto PayloadSize = EnumTypeInfo::getPayloadSizeForCases(Cases); // Compute the spare bit mask and determine if we have any address-only fields auto localSpareBitMask = BitMask::oneMask(Size); @@ -2103,44 +2120,11 @@ class EnumTypeInfoBuilder { } } - // See if we have MPE bit mask information from the compiler... - // TODO: drop this? - - // Uncomment the following line to dump the MPE section every time we come through here... - //TC.getBuilder().dumpMultiPayloadEnumSection(std::cerr); // DEBUG helper - - auto MPEDescriptor = TC.getBuilder().getMultiPayloadEnumDescriptor(TR); - if (MPEDescriptor && MPEDescriptor->usesPayloadSpareBits()) { - // We found compiler-provided spare bit data... - auto PayloadSpareBitMaskByteCount = MPEDescriptor->getPayloadSpareBitMaskByteCount(); - auto PayloadSpareBitMaskByteOffset = MPEDescriptor->getPayloadSpareBitMaskByteOffset(); - auto SpareBitMask = MPEDescriptor->getPayloadSpareBits(); - BitMask compilerSpareBitMask(PayloadSize, SpareBitMask, - PayloadSpareBitMaskByteCount, PayloadSpareBitMaskByteOffset); - - if (compilerSpareBitMask.isZero() || hasAddrOnly) { - // If there are no spare bits, use the "simple" tag-only implementation. - return TC.makeTypeInfo( - Size, Alignment, Stride, NumExtraInhabitants, - BitwiseTakable, Cases, EffectivePayloadCases); - } - -#if 0 // TODO: This should be !defined(NDEBUG) - // Verify that compiler provided and local spare bit info agree... - // TODO: If we could make this actually work, then we wouldn't need the - // bulky compiler-provided info, would we? - assert(localSpareBitMask == compilerSpareBitMask); -#endif - - // Use compiler-provided spare bit information - return TC.makeTypeInfo( - Size, Alignment, Stride, NumExtraInhabitants, - BitwiseTakable, Cases, compilerSpareBitMask, - EffectivePayloadCases); - } - if (localSpareBitMask.isZero() || hasAddrOnly) { - // Simple case that does not use spare bits + // Simple tag-only layout does not use spare bits. + // Either: + // * There are no spare bits, or + // * We can't copy it to strip spare bits. return TC.makeTypeInfo( Size, Alignment, Stride, NumExtraInhabitants, BitwiseTakable, Cases, EffectivePayloadCases); diff --git a/validation-test/Reflection/Inputs/reflect_Enum_values_resilient_enums.swift b/validation-test/Reflection/Inputs/reflect_Enum_values_resilient_enums.swift new file mode 100644 index 0000000000000..b41e6fa45a864 --- /dev/null +++ b/validation-test/Reflection/Inputs/reflect_Enum_values_resilient_enums.swift @@ -0,0 +1,10 @@ + +public enum E1_resilient { +case a +case b +} + +public enum E2_resilient { +case c(E1_resilient) +case d(E1_resilient) +} diff --git a/validation-test/Reflection/reflect_Enum_values_resilient.swift b/validation-test/Reflection/reflect_Enum_values_resilient.swift new file mode 100644 index 0000000000000..02a5653836591 --- /dev/null +++ b/validation-test/Reflection/reflect_Enum_values_resilient.swift @@ -0,0 +1,74 @@ +// RUN: %empty-directory(%t) + +// RUN: %target-build-swift-dylib(%t/%target-library-name(resilient_enums)) -enable-library-evolution %S/Inputs/reflect_Enum_values_resilient_enums.swift -emit-module -emit-module-path %t/resilient_enums.swiftmodule -module-name resilient_enums +// RUN: %target-codesign %t/%target-library-name(resilient_enums) + +// RUN: %target-build-swift -lswiftSwiftReflectionTest %s -L %t -I %t -lresilient_enums -o %t/reflect_Enum_values_resilient %target-rpath(%t) +// RUN: %target-codesign %t/reflect_Enum_values_resilient + +// RUN: %target-run %target-swift-reflection-test %t/reflect_Enum_values_resilient | tee /dev/stderr | %FileCheck %s --dump-input=fail + +// REQUIRES: executable_test +// UNSUPPORTED: use_os_stdlib + +import SwiftReflectionTest + +import resilient_enums + +// Non-resilient enum wrapping a resilient enum +// This doesn't use spare bits of the inner enum +enum E2 { +case y(E1_resilient) +case z(E1_resilient) +} + +// Contrast: +// E2_resilient is a resilient enum wrapping a resilient enum +// This does use spare bits of the inner enum + + +// CHECK: Reflecting an enum value. +// CHECK-NEXT: Type reference: +// CHECK-NEXT: (enum reflect_Enum_values_resilient.E2) +// CHECK-NEXT: Value: .y(.a) + +reflect(enumValue: E2.y(.a)) + +// CHECK: Reflecting an enum value. +// CHECK-NEXT: Type reference: +// CHECK-NEXT: (enum reflect_Enum_values_resilient.E2) +// CHECK-NEXT: Value: .z(.b) + +reflect(enumValue: E2.z(.b)) + +// CHECK: Reflecting an enum value. +// CHECK-NEXT: Type reference: +// CHECK-NEXT: (enum resilient_enums.E1_resilient) +// CHECK-NEXT: Value: .a + +reflect(enumValue: E1_resilient.a) + +// CHECK: Reflecting an enum value. +// CHECK-NEXT: Type reference: +// CHECK-NEXT: (enum resilient_enums.E1_resilient) +// CHECK-NEXT: Value: .b + +reflect(enumValue: E1_resilient.b) + +// CHECK: Reflecting an enum value. +// CHECK-NEXT: Type reference: +// CHECK-NEXT: (enum resilient_enums.E2_resilient) +// CHECK-NEXT: Value: .c(.a) + +reflect(enumValue: E2_resilient.c(.a)) + +// CHECK: Reflecting an enum value. +// CHECK-NEXT: Type reference: +// CHECK-NEXT: (enum resilient_enums.E2_resilient) +// CHECK-NEXT: Value: .d(.b) + +reflect(enumValue: E2_resilient.d(.b)) + +doneReflecting() + +// CHECK: Done. From 346d5b22c4d713b971f93fa29c7a1c132de3cfc8 Mon Sep 17 00:00:00 2001 From: Tim Kientzle Date: Wed, 5 Jun 2024 14:58:46 -0700 Subject: [PATCH 2/4] Correct handling of nonsensical Int types If an Int type makes no sense (it has a nonsensical number of extra inhabitants, for example), ignore it rather than treating it as a non-Int type. Rename `isIntType()` to `intTypeBitSize()` to better document what this function actually returns. --- .../public/RemoteInspection/TypeLowering.cpp | 29 ++++++++++++++----- 1 file changed, 21 insertions(+), 8 deletions(-) diff --git a/stdlib/public/RemoteInspection/TypeLowering.cpp b/stdlib/public/RemoteInspection/TypeLowering.cpp index 25c360a2f890a..7fa9e0c63af88 100644 --- a/stdlib/public/RemoteInspection/TypeLowering.cpp +++ b/stdlib/public/RemoteInspection/TypeLowering.cpp @@ -251,7 +251,7 @@ BuiltinTypeInfo::BuiltinTypeInfo(unsigned Size, unsigned Alignment, // Builtin.Int is mangled as 'Bi' N '_' // Returns 0 if this isn't an Int -static unsigned isIntType(std::string name) { +static unsigned intTypeBitSize(std::string name) { llvm::StringRef nameRef(name); if (nameRef.starts_with("Bi") && nameRef.ends_with("_")) { llvm::StringRef naturalRef = nameRef.drop_front(2).drop_back(); @@ -272,8 +272,22 @@ bool BuiltinTypeInfo::readExtraInhabitantIndex( *extraInhabitantIndex = -1; return true; } - unsigned intSize = isIntType(Name); - if (intSize > 0 && intSize < 64 && getSize() <= 8 && intSize < getSize() * 8) { + // If it has extra inhabitants, it could be an integer type with extra + // inhabitants (such as a bool) or a pointer. + unsigned intSize = intTypeBitSize(Name); + if (intSize > 0) { + // This is an integer type + + // If extra inhabitants are impossible, return early... + // (assert in debug builds) + assert(intSize < getSize() * 8 + && "Standard-sized int cannot have extra inhabitants"); + if (intSize > 64 || getSize() > 8 || intSize >= getSize() * 8) { + *extraInhabitantIndex = -1; + return true; + } + + // Compute range of extra inhabitants uint64_t maxValidValue = (((uint64_t)1) << intSize) - 1; uint64_t maxAvailableValue = (((uint64_t)1) << (getSize() * 8)) - 1; uint64_t computedExtraInhabitants = maxAvailableValue - maxValidValue; @@ -281,14 +295,13 @@ bool BuiltinTypeInfo::readExtraInhabitantIndex( computedExtraInhabitants = ValueWitnessFlags::MaxNumExtraInhabitants; } assert(getNumExtraInhabitants() == computedExtraInhabitants && - "Unexpected number of extra inhabitants in an odd-sized integer"); + "Unexpected number of extra inhabitants in an odd-sized integer"); + // Example: maxValidValue is 1 for a 1-bit bool, so any larger value + // is an extra inhabitant. uint64_t rawValue; if (!reader.readInteger(address, getSize(), &rawValue)) return false; - - // Example: maxValidValue is 1 for a 1-bit bool, so any larger value - // is an extra inhabitant. if (maxValidValue < rawValue) { *extraInhabitantIndex = rawValue - maxValidValue - 1; } else { @@ -307,7 +320,7 @@ bool BuiltinTypeInfo::readExtraInhabitantIndex( } BitMask BuiltinTypeInfo::getSpareBits(TypeConverter &TC, bool &hasAddrOnly) const { - unsigned intSize = isIntType(Name); + unsigned intSize = intTypeBitSize(Name); if (intSize > 0) { // Odd-sized integers export spare bits // In particular: bool fields are Int1 and export 7 spare bits From 3895908c678bbcaf9e75364235605fa75041ffa9 Mon Sep 17 00:00:00 2001 From: Tim Kientzle Date: Wed, 5 Jun 2024 16:00:05 -0700 Subject: [PATCH 3/4] Sanity: Don't spend too much time (or memory) on rediculously large enums RemoteMirror gets called on lots of malformed type information, due to memory corruption bugs or even clients that ask RemoteMirror to decode a chunk of memory to test whether or not it might be valid type data. In any case, we need to be a little cautious here. In this case, I've chosen to ignore any enum whose in-memory size (according to the metadata) is over 1 MiB. We can easily adjust this limit up if experience shows there really are legitimate enums this large in the wild. --- stdlib/public/RemoteInspection/TypeLowering.cpp | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/stdlib/public/RemoteInspection/TypeLowering.cpp b/stdlib/public/RemoteInspection/TypeLowering.cpp index 7fa9e0c63af88..2bdb4c85b85b2 100644 --- a/stdlib/public/RemoteInspection/TypeLowering.cpp +++ b/stdlib/public/RemoteInspection/TypeLowering.cpp @@ -1987,6 +1987,15 @@ class EnumTypeInfoBuilder { default: Kind = EnumKind::MultiPayloadEnum; break; } + // Sanity: Ignore any enum that claims to have a size more than 1MiB + // This avoids allocating lots of memory for spare bit mask calculations + // when clients try to interpret random chunks of memory as type descriptions. + if (Size > (1024ULL * 1024)) { + unsigned Stride = ((Size + Alignment - 1) & ~(Alignment - 1)); + return TC.makeTypeInfo( + Size, Alignment, Stride, NumExtraInhabitants, BitwiseTakable, Kind, Cases); + } + if (Cases.size() == 1) { if (EffectivePayloadCases == 0) { // Zero-sized enum with only one empty case From eba980af03d5fdb5a61f8f48b41e01cb6e850286 Mon Sep 17 00:00:00 2001 From: Tim Kientzle Date: Wed, 5 Jun 2024 17:47:30 -0700 Subject: [PATCH 4/4] Limit this test to macOS --- validation-test/Reflection/reflect_Enum_values_resilient.swift | 1 + 1 file changed, 1 insertion(+) diff --git a/validation-test/Reflection/reflect_Enum_values_resilient.swift b/validation-test/Reflection/reflect_Enum_values_resilient.swift index 02a5653836591..814339169be2d 100644 --- a/validation-test/Reflection/reflect_Enum_values_resilient.swift +++ b/validation-test/Reflection/reflect_Enum_values_resilient.swift @@ -9,6 +9,7 @@ // RUN: %target-run %target-swift-reflection-test %t/reflect_Enum_values_resilient | tee /dev/stderr | %FileCheck %s --dump-input=fail // REQUIRES: executable_test +// REQUIRES: objc_interop, OS=macosx // UNSUPPORTED: use_os_stdlib import SwiftReflectionTest