[Reflection] Hoist computation of error existentials to the reader. #18242
Conversation
|
It's quite possible that it is wrong, because AFAIK, CoreSymbolication never made use of the functionality for projecting existential payloads. |
| // Round up to alignment, and we have the start address of the | ||
| // instance payload. | ||
| auto Alignment = VWT->getAlignment(); | ||
| InstanceAddress += Alignment - InstanceAddress % Alignment; |
There was a problem hiding this comment.
This actually was the computation I was referring to as wrong, but well, the same argument applies.
There was a problem hiding this comment.
If we had a type for this, we could make the rounding-up operation be a method on it! Just saying. :)
Incidentally, the type we use for this in the runtime is SwiftError in ErrorObject.h, and you can see the offset computation it does in getValue().
| } else { | ||
| // Otherwise, we can check to see if this is a class metadata with the | ||
| // kind value's least significant bit set, which indicates a pure | ||
| // Swift class. |
There was a problem hiding this comment.
Wait, does this happen? And if so, when? Is it just when ObjCInterop is disabled, because I feel like that's something that we ought to be able to check for (and arguably ought to be part of the Runtime template argument).
There was a problem hiding this comment.
This doesn't seem to ever happen (I tried to put an assertion and the tests still pass). I might consider removing this but I want to try a little harder to see whether I can exercise this.
|
|
||
|
|
||
| /// Given a pointer to the metadata, attempt to read the value | ||
| /// witness table. |
There was a problem hiding this comment.
You should probably mention here in this comment that it's not safe to access any non-mandatory members of the VWT, like extra inhabitants or enum members.
|
|
||
| // Read the value witness table. | ||
| auto VWT = | ||
| readValueWitnessTable(cast<StoredPointer>(*InstanceMetadataAddress)); |
There was a problem hiding this comment.
...I'm confused that cast<> works with StoredPointer as its argument.
There was a problem hiding this comment.
I'm surprised this works too, but given it's unnecessary, I'll remove it anyway.
| // and userInfo. | ||
| StoredPointer InstanceMetadataAddressAddress = | ||
| ExistentialAddress.getAddressData() + | ||
| (isObjC ? 5 : 2) * sizeof(StoredPointer); |
There was a problem hiding this comment.
Hmm. It'd be nice to just have a type for this structure.
There was a problem hiding this comment.
Indeed. Will try to introduce one.
| // Round up to alignment, and we have the start address of the | ||
| // instance payload. | ||
| auto Alignment = VWT->getAlignment(); | ||
| InstanceAddress += Alignment - InstanceAddress % Alignment; |
There was a problem hiding this comment.
If we had a type for this, we could make the rounding-up operation be a method on it! Just saying. :)
Incidentally, the type we use for this in the runtime is SwiftError in ErrorObject.h, and you can see the offset computation it does in getValue().
| TargetOpaqueExistentialContainer<Runtime> Container; | ||
| if (!Reader->readBytes(RemoteAddress(ExistentialAddress), | ||
| (uint8_t *)&Container, sizeof(Container))) | ||
| return llvm::None; |
There was a problem hiding this comment.
Incidentally, is llvm::None really not in scope here? I thought we had a common header imported everywhere that usinged them into namespace swift.
There was a problem hiding this comment.
I'll try removing the full namespace qualification and see if it sticks together.
| return llvm::None; | ||
|
|
||
| auto AlignmentMask = VWT->getAlignmentMask(); | ||
| auto Offset = (sizeof(HeapObject) + AlignmentMask) & ~AlignmentMask; |
There was a problem hiding this comment.
This is the right computation; you can just extract it out as a utility if you need to. (But it would also make sense to have a TargetBoxedValue struct with some sort of getDataOffsetFromAlignmentMask static method on it.)
There was a problem hiding this comment.
Going to do that (as well as introducing a type for the Error in a follow-up).
|
@swift-ci test and merge |
<rdar://problem/41546568>
|
@swift-ci test and merge |
rdar://problem/41546568
Needs to be rebased on top of
masteronce the depending patch goes in.All the logic is in the second commit: fd0f5f1