5.9: [stdlib] Delete bad @_effects(readonly) annotation #68222
Merged
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description: Programs built with release/5.9 crash when back-deploying to before 10.15 if they use an empty dictionary literal.
Since 2014,
Dictionary.init(dictionaryLiteral:)
has been annotated@_effects(readonly)
. Among other things, that annotation means the function must not release any values. At the time, the default convention forinit
s was that they "borrowed" the values passed to them--they could not release them. Subsequently, that default convention changed so thatinit
s instead "consumed" the values passed to them--starting then, theinit
s released the values if they were not consumed by some other mechanism (storing them, calling some other function which consumed them, etc.). From that time on, this annotation has been incorrect.In release/5.9, this incorrectness was exposed as a runtime crash. In particular, given a program involving a class with a field of dictionary type which is initialized to be the empty dictionary, building that program with release/5.9 and running that program on pre-10.15 will result in a runtime crash.
Here's what's happening: the empty dictionary literal
[:]
is turned into a callDictionary.init(dictionaryLiteral: [])
. The empty array is passed toDictionary.init
which consumes it. BecauseDictionary.init
does not store (or otherwise consume) the empty array, it releases it. BecauseDictionary.init
consumes its argument, when compiling the function containing the call to it (i.e. containing[:]
), the compiler retains the empty array[]
that's passed to it. Now, to decrease unnecessary ARC traffic, the compiler sinks retains and hoists releases so that they can be eliminated altogether. When the compiler sinks the retain of[]
, it is allowed to move it past the call toDictionary.init
because the call is explicitly annotated@_effects(readonly)
which means that it can't release the empty array. BecauseDictionary.init
actually releases its argument[]
, the call to retain is a use-after-free.The crashes only occur on older macOSes because more recently (since 2019) empty collection singletons like the empty array have been made immortal.
Risk: Very low. The fix is to delete an annotation from a stdlib function. The annotation only communicates (false) information about optimization opportunities to the compiler.
Scope: Very narrow. Deletes a single line of code from a stdlib function.
Reward: High. Fixes runtime crashes for many programs built with release/5.9 compilers when running on old macOSes.
Origin: Primeval.
Original PR: #68223
Reviewed By: Andrew Trick ( @atrick )
Testing: Added a test demonstrating that retain of
[]
is not sunk to below the call toDictionary.init
. Verified that runtime crash doesn't occur with fix applied.Resolves: rdar://114699006