-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[semantic-arc-opts] When performing load [copy] -> load_borrow on classes, do not ignore forwarding uses. #28989
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[semantic-arc-opts] When performing load [copy] -> load_borrow on classes, do not ignore forwarding uses. #28989
Conversation
@swift-ci test |
Build failed |
Thanks for tracking this down! |
@swift-ci test os x platform |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
I started to play with this a bit locally, and I found a smaller simpler change to cherry-pick. I am going to first commit that. |
I tried to cherry-pick this change and I found it to be a bit difficult since the LiveRange infrastructure doesn't exist on 5.2. I am about to push an even smaller fix that only uses things in that older code to make it slightly easier to cherry-pick. In a subsequent PR, I am going to re-add the LiveRange code. |
eb0e969
to
26af4ab
Compare
@swift-ci test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
…sses, do not ignore forwarding uses. This is the first of two commits. This commit is a very simple, easily cherry-pickable fix but does not use the LiveRange infrastructure so that we handle forwarding uses here. Instead, we just bail if all uses of our load [copy] are not destroy_value. In a subsequent commit, I am going to change this to use the LiveRange infrastructure so we will handle these cases. Sadly doing so doesn't cherry-pick well. = /. rdar://58289320
26af4ab
to
23f36a0
Compare
@swift-ci test |
1 similar comment
@swift-ci test |
Build failed |
Java failure... |
@swift-ci test os x platform |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix looks good
Otherwise in cases like the test case attached, we will not properly look
through the destroy.
In a subsequent commit, I am going to do some more fixes here by adding memory
use verification around begin_borrow. I have wanted to enable this for a minute,
but this seems like a good time to do so given that would have caught this.
rdar://58289320
EDIT! I am doing a smaller cherry-picking fix. I am still using this PR to not confuse the automation. See message below in conversation!