[Runtime] Fix race condition in protocol conformance lookups that caused false negatives. #35836
Conversation
|
@swift-ci please test |
…sed false negatives. In the uncached case, we'd scan conformances, cache them, then re-query the cache. This worked fine when the cache always grew, but now we clear the cache when loading new Swift images into the process. If that happens between the scan and the re-query, we lose the entry and return a false negative. Instead, track what we've found in the scan in a separate local table, then query that after completing the scan. While we're in there, fix a bug in TypeLookupError where operator= accidentally copied this->Context instead of other.Context. This caused the runtime to crash when trying to print error messages due to the false negative. Add a no-parameter constructor to TypeLookupErrorOr<> to distinguish the case where it's being initialized with nothing from the case where it's being initialized with nullptr.
mikeash
force-pushed
the
fix-protocol-conformance-race-condition
branch
from
076146c to
3a2ea08
Compare
|
|
||
| // If it's for a superclass or if we didn't find anything, then add an | ||
| // authoritative entry for this type. | ||
| if (searchType != type) |
There was a problem hiding this comment.
Shouldn't we call cacheResult regardless of whether the types match here? Isn't that the point of snapshot.count()?
There was a problem hiding this comment.
When the types match, then we'll have already cached this result up above during the conformance scan. Caching here would be slightly wasteful, although otherwise harmless.
This whole thing is a little weird. searchInConformanceCache checks superclasses, and we don't add cache entries for subclasses when we find a superclass conformance in the cache. Whether a subclass gets its own cache entry depends on whether you looked up a conformance on a superclass first. We should probably figure out the right way to do this (always add cache entries for subclasses, or never) and fix it up, but another time.... For now, this matches the previous behavior.
|
Build failed |
In the uncached case, we'd scan conformances, cache them, then re-query the cache. This worked fine when the cache always grew, but now we clear the cache when loading new Swift images into the process. If that happens between the scan and the re-query, we lose the entry and return a false negative.
Instead, track what we've found in the scan in a separate local table, then query that after completing the scan.
While we're in there, fix a bug in
TypeLookupErrorwhereoperator=accidentally copiedthis->Contextinstead ofother.Context. This caused the runtime to crash when trying to print error messages due to the false negative.Add a no-parameter constructor to
TypeLookupErrorOr<>to distinguish the case where it's being initialized with nothing from the case where it's being initialized withnullptr.rdar://72865822