Skip to content

Fix the IR attributes of swift_getObjectType. #13390

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

Merged
merged 1 commit into from
Dec 13, 2017

Conversation

eeckstein
Copy link
Contributor

It's not readnone, because it reads the metatype from an object.
Readnone would let the llvm ARC optimizer reschedule the call with a release-call for the object.

fixes SR-6560.
rdar://problem/35998785

It's not readnone, because it reads the metatype from an object.
Readnone would let the llvm ARC optimizer reschedule the call with a release-call for the object.

fixes SR-6560.
@eeckstein eeckstein requested a review from rjmccall December 12, 2017 19:18
@eeckstein
Copy link
Contributor Author

@swift-ci test

Copy link
Contributor

@rjmccall rjmccall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your reasoning seems sound. Essentially, anything that depends on object liveness can be thought of as reading the reference count of those objects.

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 70aeb71

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 70aeb71

@eeckstein
Copy link
Contributor Author

@swift-ci test

@jrose-apple
Copy link
Contributor

If only we had a notion of "this can be done earlier, but not later".

@eeckstein
Copy link
Contributor Author

@swift-ci nominate

• Explanation: Fixes a use-after-free issue related to class types
• Scope of Issue: may show up if dealing with class types
• Origination: it's an old bug
• Risk: Low
• Testing: CI bots

@eeckstein eeckstein merged commit 82e08e0 into swiftlang:swift-4.1-branch Dec 13, 2017
@eeckstein eeckstein deleted the fix-attrs-4.1 branch December 13, 2017 02:59
@atrick
Copy link
Contributor

atrick commented Dec 13, 2017

I'm not opposed to the conservative fix, but it does indicate a problem or deficiency in the LLVM ARC pass. Reference counts are part of the "runtime memory", so LLVM doesn't see them. Any pass that reorders runtime calls needs a different interpretation of the memory effects.

@eeckstein
Copy link
Contributor Author

Any LLVM pass could reorder this operations if the function is marked as readnone.
Memory effects of ref count operations are visible in LLVM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants