[5.1] SILOptimier: Fix a miscompile in COWArrayOpt. #23431
Conversation
If a make_mutable operation is done conditionally in a loop, the hoisting of this operation can cause an over-release of the array buffer in some cases. rdar://problem/48906146
|
@swift-ci smoke test and merge |
|
@swift-ci test |
|
Build failed |
| // conditionally (i.e. doesn't dominate all exit blocks). | ||
| // The test SILOptimizer/cowarray_opt.sil: dont_hoist_if_executed_conditionally | ||
| // shows the problem. | ||
| if (hasLoopOnlyDestructorSafeArrayOperations() && dominatesExits) { | ||
| // Done. We can hoist the make_mutable. |
There was a problem hiding this comment.
I don't see an obvious connection between the comment and the test case. Here's my guess at how it works... For potentially aliased arrays, I think it's this pass' job to determine that a potential alias can't be accessed on any path from the loop header that does not pass through the original make_mutable. I guess we already do that within the loop but never checked outside the loop?
That isn't clear from the comment. Shouldn't it say something like:
The hoisted make_mutable releases the original array storage. If an
alias of that storage is accessed on any path reachable from the
header that doesn't already pass through make_mutable, then hoisting
is illegal. hasLoopOnlyDestructorSafeArrayOperations checks that the
array storage is not accessed within the loop. However, the array may
also be accessed after exiting the loop. Rather than analyzing code
outside the loop, simply check that the original make_mutable
dominates all exits.
I'm not sure if the above is actually true but at least it makes sense to me.
There was a problem hiding this comment.
Yes, that's probably a better way to describe it.
|
@swift-ci test linux |
If a make_mutable operation is done conditionally in a loop, the hoisting of this operation can cause an over-release of the array buffer in some cases.
rdar://problem/48906146