Skip to content
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.

Sign up for GitHub

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

Jump to bottom

SIL: some improvements/fixes around assign_by_wrapper #36253

Merged
merged 1 commit into from Mar 4, 2021
Merged

SIL: some improvements/fixes around assign_by_wrapper #36253

merged 1 commit into from Mar 4, 2021

Conversation

eeckstein
Copy link
Member

  • Refactoring: replace "Destination" and the ownership qualifier by a single "Mode". This represents much better the mode how the instruction is to be lowered. NFC
  • Make assign_by_wrapper printable and parseable.
  • Fix lowering of the assign modes for indirect results of the init-closure: The indirect result was initialized and not assigned to. The fix is to insert a destroy_addr before calling the init closure. This fixes a memory lifetime error and/or a memory leak. Found by inspection.
  • Fix an iterator-invalidation crash in RawSILInstLowering
  • Add tests for lowering assign_by_wrapper (a follow-up of SIL: support store_borrow and partial_apply in memory lifetime verification #36211)

@eeckstein
SIL: some improvements/fixes around assign_by_wrapper
9055e93 
* Refactoring: replace "Destination" and the ownership qualifier by a single "Mode".  This represents much better the mode how the instruction is to be lowered. NFC
* Make assign_by_wrapper printable and parseable.
* Fix lowering of the assign modes for indirect results of the init-closure: The indirect result was initialized and not assigned to. The fix is to insert a destroy_addr before calling the init closure. This fixes a memory lifetime error and/or a memory leak. Found by inspection.
* Fix an iterator-invalidation crash in RawSILInstLowering
* Add tests for lowering assign_by_wrapper.
@eeckstein eeckstein requested a review from hborla March 3, 2021 17:58
@eeckstein
Copy link
Member Author

@swift-ci test

hborla
hborla approved these changes Mar 3, 2021
View reviewed changes
Copy link
Member

@hborla hborla left a comment

Choose a reason for hiding this comment

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

This looks a lot cleaner than what I wrote (not to mention the other memory leak that I missed). Thank you!

Removing the unused initializer/setter closure in lowerAssignByWrapperInstruction should also resolve rdar://73406156

@eeckstein
Copy link
Member Author

@hborla Yes, this should resolve rdar://73406156, which is https://bugs.swift.org/browse/SR-14070

@swift-ci
Copy link
Collaborator

swift-ci commented Mar 3, 2021

Build failed
Swift Test OS X Platform
Git Sha - 9055e93

@eeckstein
Copy link
Member Author

@swift-ci test macOS

@swift-ci
Copy link
Collaborator

swift-ci commented Mar 4, 2021

Build failed
Swift Test OS X Platform
Git Sha - 9055e93

@eeckstein
Copy link
Member Author

@swift-ci test macOS

@swift-ci
Copy link
Collaborator

swift-ci commented Mar 4, 2021

Build failed
Swift Test OS X Platform
Git Sha - 9055e93

@eeckstein
Copy link
Member Author

@swift-ci smoke test macOS

@eeckstein eeckstein merged commit c4f6fc3 into apple:main Mar 4, 2021
@eeckstein eeckstein deleted the fix-assign-by-wrapper branch March 4, 2021 19:07
@hborla hborla mentioned this pull request Mar 3, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hborla hborla hborla approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@eeckstein @swift-ci @hborla