OSSA: Rewrite address cloning code to fix issues and fix an infinite loop in the ownership verifier. #36152
Conversation
This was an obvious thinko that must not have been hit yet in practice.
So I can test interesting ownership RAUW cases.
Very long unreadable names are not a substitute for describing a test.
atrick
force-pushed
the
fix-rauwclone
branch
from
4fa4ad1 to
4c4ed03
Compare
Generalize the AccessUseDefChainCloner in MemAccessUtils. It was always meant to work this way, just needed a client. Add a new API AccessUseDefChainCloner::canCloneUseDefChain(). Add a bailout for begin_borrow and mark_dependence. Those projections may appear on an access path, but they can't be individually cloned without compensating. Delete InteriorPointerAddressRebaseUseDefChainCloner. Add a check in OwnershipRAUWHelper for canCloneUseDefChain. Add test cases for begin_borrow and mark_dependence.
atrick
force-pushed
the
fix-rauwclone
branch
from
4c4ed03 to
66752e9
Compare
|
@swift-ci test |
|
@swift-ci test source compatibility |
|
Build failed |
|
An unrelated lldb test failed on macos. |
|
@swift-ci smoke test macos |
|
@swift-ci test macOS |
| /// Clone all projections and casts on the access use-def chain until either the | ||
| /// specified predicate is true or the access base is reached. | ||
| /// Clone all projections and casts on the access use-def chain until the | ||
| /// checkBase predicate returns a valid base. |
There was a problem hiding this comment.
In a subsequent commit, can you describe the function signature of checkBase here and specify what it means to return a "valid" base. Without that I don't know what that means. I would maybe do something like this:
Clone all projections and cases on the access use-def chain until the checkBase predicate returns a valid SILValue as a base.
There was a problem hiding this comment.
The next line below says "See comments on the cloneUseDefChain() API."
| /// This will not clone ref_element_addr or ref_tail_addr because those aren't | ||
| /// part of the access chain. | ||
| /// | ||
| /// CheckBase is a unary predicate that takes the next source address and either |
There was a problem hiding this comment.
This is the comment on CheckBase that I wanted above. Maybe add a note above to point here for more information about CheckBase?
There was a problem hiding this comment.
I had all these comments duplicated, decided the was a bad idea, so added See comments on the cloneUseDefChain() API.
| @@ -768,7 +768,7 @@ bool InteriorPointerOperand::findTransitiveUsesForAddress( | |||
| } | |||
|
|
|||
| // If we are the value use, look through it. | |||
| llvm::copy(mdi->getValue()->getUses(), std::back_inserter(worklist)); | |||
| llvm::copy(mdi->getUses(), std::back_inserter(worklist)); | |||
There was a problem hiding this comment.
I am guessing this was the infinite loop? Such a thinko = /.
4 commits...
Fix an infinite loop in the OSSA verifier exposed by the new mark_dependence test cases.
Generalize the AccessUseDefChainCloner in MemAccessUtils. It was
always meant to work this way, just needed a client.
Add a new API AccessUseDefChainCloner::canCloneUseDefChain().
Add a bailout for begin_borrow and mark_dependence. Those
projections may appear on an access path, but they can't be
individually cloned without compensating.
Delete InteriorPointerAddressRebaseUseDefChainCloner.
Add a check in OwnershipRAUWHelper for canCloneUseDefChain.
Add test cases for begin_borrow and mark_dependence.