[Runtime] Don't use the low bit of a WitnessTable pointer in the conformance cache #80844
Conversation
…ormance cache With relative witness tables, the low bit of a witness table pointer is an indicator that we need to load from the given pointer. We were also using the low bit of the witness table pointer in the conformance cache entry as part of a pointer union. Hilarity ensures [*]. Switch to another low bit by exploding the conformance cache key into separate fields and taking the low bit of one of those pointers that isn't reserved. Fixes the remainder of rdar://149326058, I hope. [*] No, I am not laughing.
|
@swift-ci please smoke test |
|
@swift-ci please smoke test Linux |
This can happen when running against an older version of a library that doesn't have the protocol defined.
|
@swift-ci please smoke test |
| if (result.globalActorIsolationType) { | ||
| WitnessTableOrLookupResult = new ConformanceLookupResult(result); | ||
| ProtoOrStorage = new ExtendedStorage{ |
There was a problem hiding this comment.
Do we want to use swift_cxx_newObject() here? (Also, this will leak whenever we dynamically load a new image, since we clear the conformance cache at that point and there's no destructor for ConformanceCacheEntry.)
There was a problem hiding this comment.
I can switch to swift_cxx_newObject(), sure. And... I had not realized that we clear the conformance cache, ever. If we do... well, let's figure out a way to free this memory. I don't want to add a destructor to ConformanceCacheEntry, though, because only a tiny fraction of protocol conformances actual have this extra storage. Maybe an intrusive linked list of the extended storage pointers instead.
| return ConformanceLookupResult( | ||
| Witness, storage->globalActorIsolationType, | ||
| storage->globalActorIsolationWitnessTable); | ||
| } | ||
|
|
||
| return nullptr; |
There was a problem hiding this comment.
(Nothing to do with this PR)
This line made me raise my eyebrows a bit — ConformanceLookupResult is not a pointer. But it turns out we have
ConformanceLookupResult(std::nullptr_t) { }
Not sure what I think about that :-) It works, I suppose.
There was a problem hiding this comment.
Yeah, so it used to be that this was just a pointer (const WitnessTable *) in a number of places, so I added this to keep return nullptr; working from the lookup routines that now return a ConformanceLookupResult.
|
@swift-ci please smoke test Linux |
With relative witness tables, the low bit of a witness table pointer is an indicator that we need to load from the given pointer. We were also using the low bit of the witness table pointer in the conformance cache entry as part of a pointer union. Hilarity ensures [*].
Switch to another low bit by exploding the conformance cache key into separate fields and taking the low bit of one of those pointers that isn't reserved.
Fixes the remainder of rdar://149326058, I hope.
[*] No, I am not laughing.