RemoteMirror: Project some multi-payload enums #30357
Conversation
…nums According to the compiler code, the payload area is reused by treating the first min(4, payload size) bytes as an n-byte integer. The previous code didn't handle 3-byte integers. Mostly, this just means simplifying the `readInteger` function to handle arbitrary byte lengths (requiring only that it's less than the length of the destination integer variable). I've also made a tentative stab at handling big-endian architectures here.
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.)
tbkka
force-pushed
the
tbkka-remoteMirror-projectEnum
branch
from
76f29fb to
13853c1
Compare
|
@swift-ci Please test |
|
Basically, if all payload cases have a fixed size known at compile time in all resilience domains where the enum is visible (this is a bit tricky), you want to use spare bits. Since the compiler makes the decision it should also be the one to emit the descriptor that tells you where the spare bits are. Otherwise, you use the much simpler dynamic layout algorithm that the runtime uses in Also do we have a way to project single payload enums yet? The interesting thing to do here is to know which non-payload case is active. |
|
Build failed |
|
@slavapestov -- PR #30161 added the |
|
@slavapestov -- If spare bits handling requires associated compiler changes, any concerns about merging this partial support and tackling that in a separate PR? Users of this API just need to be confident that this code will either project the enum correctly or return |
|
Swift-CI failure looks entirely unrelated, so I'll retry. |
|
@swift-ci Please test macOS |
|
Windows CI caught a typo. Restarting CI bots... |
|
@swift-ci Please test |
|
Build failed |
|
Build failed |
| case RecordKind::MultiPayloadEnum:// XXX TODO | ||
| return false; | ||
|
|
||
| case RecordKind::MultiPayloadEnum: { |
There was a problem hiding this comment.
This block of code counts XIs in MultiPayloadEnums and returns the digested result for an enclosing single-payload enum to use.
| /// 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) { |
There was a problem hiding this comment.
A more flexible readInteger that can read integers with an arbitrary number of bytes, from 1 to sizeof(*dest). In particular, this is needed for handling 3-byte integers that can occur when storing enum tag indexes in the payload area.
| @@ -801,7 +801,50 @@ class ReflectionContext | |||
| } | |||
|
|
|||
| case RecordKind::MultiPayloadEnum: { | |||
There was a problem hiding this comment.
This block of code actually identifies the current case for multi-payload enums. Today, it only handles enums that use a separate tag (as inferred from the size data exposed in the type information). For other enum layouts it returns false to indicate that it is unable to parse that particular enum.
| // CHECK-NEXT: (enum reflect_Enum_value.SinglePayloadEnumWithMultiPayloadEnumPayload) | ||
| // CHECK-NEXT: Value: .alsoC | ||
|
|
||
| reflect(enumValue: Optional<Optional<SinglePayloadEnumWithMultiPayloadEnumPayload>>.some(.none)) |
There was a problem hiding this comment.
This test, in particular, verifies that XIs are being properly counted in the multi-payload enum for use by the surrounding single-payload optionals.
| // CHECK-NEXT: (enum reflect_Enum_value.SinglePayloadEnumWithMultiPayloadEnumPayload))) | ||
| // CHECK-NEXT: Value: .some(.none) | ||
|
|
||
| enum SmallMultiPayload { |
There was a problem hiding this comment.
The code internally handles payloads with size >= 4 in a different path (to avoid overflow issues on 32-bit platforms). So the test above is repeated with a multi-payload enum that has a 2-byte payload area.
|
I added a few more comments just to summarize our chat discussion for other reviewers. Nothing new for you here. |
|
It might be fun to deploy the |
This builds on #30161 by adding support for the most basic form of multi-payload enum.
As before, the
swift_reflection_projectEnumcall should returnfalsefor any enum that it is unable to project, so adopters should continue to fall back to whatever mechanism they were using before if it fails.This is one part of rdar://31666592