Expand the work from #73491 to support more MPE layouts. #74145

tbkka · 2024-06-05T17:19:56Z

This is also switches the MPE layout code to exclusively use the new approach. 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.

There is some risk that the old code may have handled some enums that aren't (yet) fully supported by the new code. However, I did a bunch of experiments and found that our existing test cases have decent coverage of the major capabilities and I've also added a new test case specifically to exercise enum layouts being accessed resiliently. So if there are regressions from this change, they should be minor and easily fixed.

Resolves rdar://129281368

…s 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

tbkka · 2024-06-05T17:20:32Z

@swift-ci Please test

tbkka · 2024-06-05T17:21:42Z

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) {


This is just here to further harden the bit mask code against allocation failures. Allocation failures can occur in this code when clients try to use RemoteMirror to decipher type data from random chunks of memory.

Should we also have a limit on sizeInBytes so we don't read garbage and then successfully, but unhappily, allocate 500TB of memory?

tbkka · 2024-06-05T17:22:17Z

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


There are doubtless other pointer types that need to expose spare bits. I expect some further iteration of this specific test here.

tbkka · 2024-06-05T17:22:56Z

stdlib/public/RemoteInspection/TypeLowering.cpp

@@ -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()));


Bug fix: Non-payload enums do in fact export spare bits from the tag value.

tbkka · 2024-06-05T17:24:20Z

stdlib/public/RemoteInspection/TypeLowering.cpp

@@ -648,7 +651,17 @@ class SinglePayloadEnumTypeInfo: public EnumTypeInfo {
  }

  BitMask getSpareBits(TypeConverter &TC, bool &hasAddrOnly) const override {
-    return BitMask::zeroMask(getSize());
+    FieldInfo PayloadCase = getCases()[0];


Single payload enums also export spare bits from the tag. This is a bit more complex than the non-payload case above just because of the manipulations needed to ensure we have a tag and to ensure we don't inadvertently try to export spare bits from the payload.

tbkka · 2024-06-05T17:26:51Z

stdlib/public/RemoteInspection/TypeLowering.cpp

@@ -2103,44 +2120,11 @@ class EnumTypeInfoBuilder {
      }
    }

-    // See if we have MPE bit mask information from the compiler...


Previously, I tried having the compiler export a "multi-payload enum descriptor" with a complete copy of the spare bit mask. I was never happy with this approach because it potentially adds a lot of bulk to the reflection metadata. Fortunately, the new code has shown we shouldn't need such information from the compiler and this PR rips all of that logic out of RemoteMirror. I'll file a separate PR to remove the never-completely-implemented compiler support for generating this data.

tbkka · 2024-06-05T17:27:26Z

validation-test/Reflection/reflect_Enum_values_resilient.swift

@@ -0,0 +1,74 @@
+// RUN: %empty-directory(%t)


New test case builds a separate resilient library and verifies that we can project values from enums defined in that library.

mikeash · 2024-06-05T17:25:13Z

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) {


Should we also have a limit on sizeInBytes so we don't read garbage and then successfully, but unhappily, allocate 500TB of memory?

tbkka · 2024-06-05T17:35:04Z

Should we also have a limit on sizeInBytes so we don't read garbage and then successfully, but unhappily, allocate 500TB of memory?

These bit masks can never be larger than the Size of the type in question, so I think the right place for such a check would be in the top-level layout code. Maybe reject any attempt to lay out a type whose Size is larger than 1TB?

tbkka · 2024-06-05T17:51:54Z

Should we also have a limit on sizeInBytes so we don't read garbage and then successfully, but unhappily, allocate 500TB of memory?

These bit masks can never be larger than the Size of the type in question, so I think the right place for such a check would be in the top-level layout code. Maybe reject any attempt to lay out a type whose Size is larger than 1TB?

And since Size here is unsigned int, we already have an implicit limit that no type here can be larger than 4GiB. We could put a lower limit as a sanity-check.

kastiglione · 2024-06-05T17:55:47Z

Thanks! I appreciate the extra comments.

mikeash · 2024-06-05T18:03:12Z

I'd suggest a limit more like 1MB, but if we already have a an implicit 4GB limit then that doesn't have to be done right here and now.

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.

…nums 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.

tbkka · 2024-06-05T23:12:13Z

@swift-ci Please test Linux Platform

tbkka · 2024-06-05T23:13:30Z

I added a 1MiB limit and fixed a mis-handling of broken integer type information. The Linux test failed for some reason I don't understand. I may have to spin up a Linux VM here and see if I can duplicate it.

tbkka · 2024-06-06T00:48:02Z

@swift-ci Please test

tbkka · 2024-06-06T14:32:24Z

I noted that a lot of resilience tests are limited to only run on macOS. I may take a poke at this particular one later, but for now I've limited it to macOS only.

…fixes Expand the work from swiftlang#73491 to support more MPE layouts. This is also switches the MPE layout code to exclusively use the new approach. 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. There is some risk that the old code may have handled some enums that aren't (yet) fully supported by the new code. However, I did a bunch of experiments and found that our existing test cases have decent coverage of the major capabilities and I've also added a new test case specifically to exercise enum layouts being accessed resiliently. So if there are regressions from this change, they should be minor and easily fixed. Resolves rdar://129281368

tbkka requested a review from a team as a code owner June 5, 2024 17:19

tbkka requested review from mikeash and kastiglione and removed request for a team June 5, 2024 17:20

tbkka commented Jun 5, 2024

View reviewed changes

mikeash approved these changes Jun 5, 2024

View reviewed changes

tbkka added 2 commits June 5, 2024 14:58

Limit this test to macOS

eba980a

tbkka merged commit 6c2b772 into swiftlang:main Jun 6, 2024
5 checks passed

This was referenced Jun 6, 2024

[6.0] Use direct spare bit calculation for more MPE layouts #74194

Merged

[RemoteMirror] Remove compiler code to emit spare bit info in reflection metadata #74230

Open

Remove RemoteInspection code to fetch no-longer-used reflection metadata #74394

Merged

tbkka deleted the tbkka-remotemirror-mpe-fixes branch August 1, 2024 16:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expand the work from #73491 to support more MPE layouts. #74145

Expand the work from #73491 to support more MPE layouts. #74145

tbkka commented Jun 5, 2024

tbkka commented Jun 5, 2024

tbkka Jun 5, 2024

mikeash Jun 5, 2024

tbkka Jun 5, 2024

tbkka Jun 5, 2024

tbkka Jun 5, 2024

tbkka Jun 5, 2024

tbkka Jun 5, 2024

mikeash Jun 5, 2024

tbkka commented Jun 5, 2024

tbkka commented Jun 5, 2024

kastiglione commented Jun 5, 2024

mikeash commented Jun 5, 2024

tbkka commented Jun 5, 2024

tbkka commented Jun 5, 2024

tbkka commented Jun 6, 2024

tbkka commented Jun 6, 2024

Expand the work from #73491 to support more MPE layouts. #74145

Expand the work from #73491 to support more MPE layouts. #74145

Conversation

tbkka commented Jun 5, 2024

tbkka commented Jun 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbkka commented Jun 5, 2024

tbkka commented Jun 5, 2024

kastiglione commented Jun 5, 2024

mikeash commented Jun 5, 2024

tbkka commented Jun 5, 2024

tbkka commented Jun 5, 2024

tbkka commented Jun 6, 2024

tbkka commented Jun 6, 2024