Skip to content

Fix an assert in canonicalizeFunctionArgument. #41460

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 2 commits into from
Feb 19, 2022

Conversation

atrick
Copy link
Contributor

@atrick atrick commented Feb 18, 2022

Assertion failed: (succeed && "should be filtered by FindBorrowScopeUses"), function canonicalizeFunctionArgument, file CanonicalizeBorrowScope.cpp, line 798

Canonicalization for guaranteed function arguments is triggered by
SILCombine without any up-front analysis. Because the canonicalization
rewrites the function argument's copies in place, it must always
succeed.

Fix the visitBorrowScopeUses utility to be aware that it is being
invoked on a function argument and avoid bailing out.


To make this problem reproduce reliably, I enabled it in the CopyPropagation pass. This way, the code in question runs over all SIL. Previously, it happened to be invoked during certain SILCombine optimization and could only assert in a rare case involving unmanaged references.

@atrick atrick requested a review from nate-chandler February 18, 2022 22:11
@atrick
Copy link
Contributor Author

atrick commented Feb 18, 2022

@swift-ci smoke test

Copy link
Contributor

@nate-chandler nate-chandler left a comment

Choose a reason for hiding this comment

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

Great to be doing borrow canonicalizations for all guaranteed function arguments!

@atrick atrick force-pushed the fix-canonical-func-arg branch from 837b39f to 2d1ca32 Compare February 18, 2022 23:58
Mandatory copy propagation was primarily a stop-gap until lexcial
lifetimes were implemented. It supposedly made variables lifetimes
more consistent between -O and -Onone builds. Now that lexical
lifetimes are enabled, it is no longer needed for that purpose (and
will never satisfactorily meet that goal anyway).

Mandatory copy propagation may be enabled again later as a -Onone "
optimization. But that requires a more careful audit of the effect on
debug information.

For now, it should be disabled.
Assertion failed: (succeed && "should be filtered by
FindBorrowScopeUses"), function canonicalizeFunctionArgument, file
CanonicalizeBorrowScope.cpp, line 798

Canonicalization for guaranteed function arguments is triggered by
SILCombine without any up-front analysis. Because the canonicalization
rewrites the function argument's copies in place, it must always
succeed.

Fix the visitBorrowScopeUses utility to be aware that it is being
invoked on a function argument and avoid bailing out.
@atrick atrick force-pushed the fix-canonical-func-arg branch from 2d1ca32 to 4a30118 Compare February 19, 2022 00:52
@atrick
Copy link
Contributor Author

atrick commented Feb 19, 2022

Sorry for the delay. This is currently blocked by PR: #41469

@atrick
Copy link
Contributor Author

atrick commented Feb 19, 2022

@swift-ci test

@atrick
Copy link
Contributor Author

atrick commented Feb 19, 2022

SwiftREPL/BreakpointSimple.test

@atrick
Copy link
Contributor Author

atrick commented Feb 19, 2022

@swift-ci smoke test and merge

@swift-ci swift-ci merged commit e45bb6f into swiftlang:main Feb 19, 2022
@atrick atrick deleted the fix-canonical-func-arg branch February 20, 2022 00:56
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.

3 participants