Skip to content

Fix infinite recursion in ClosureSpecialize #61956

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
Nov 9, 2022

Conversation

atrick
Copy link
Contributor

@atrick atrick commented Nov 7, 2022

Fixes getSpecializationLevelRecursive to handle recursive manglings caused by interleaving CapturePropagation and ClosureSpecialize passes.

For some reason, only the first closure parameter was checked for recursion. We need to handle patterns like this:

kind=FunctionSignatureSpecialization
kind=SpecializationPassID, index=3
kind=FunctionSignatureSpecializationParam
kind=FunctionSignatureSpecializationParam
kind=FunctionSignatureSpecializationParamKind, index=0
kind=FunctionSignatureSpecializationParamPayload, text="$s4test10ExpressionO8contains5whereS3bXE_tFSbACXEfU_S2bXEfU_36$s4test12IndirectEnumVACycfcS2bXEfU_Tf3npf_n"

I fixed the logic so we now check for recursion on all closure parameters and bail out on unrecognized mangling formats.

For reference, see summary.sil in
Infinitely recursive closure specialization #61955 #61955

Fixes rdar://101589190 (Swift Compiler hangs when building this code for release)

@atrick atrick requested a review from eeckstein November 7, 2022 08:23
@atrick
Copy link
Contributor Author

atrick commented Nov 7, 2022

@swift-ci test

@atrick
Copy link
Contributor Author

atrick commented Nov 7, 2022

@swift-ci benchmark

Copy link
Contributor

@eeckstein eeckstein left a comment

Choose a reason for hiding this comment

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

Basically LGTM.
Can you add a test case?

@atrick atrick force-pushed the fix-closurespecialize-hang branch from 2301aed to 856386c Compare November 7, 2022 22:43
Fixes getSpecializationLevelRecursive to handle recursive manglings caused by interleaving CapturePropagation and ClosureSpecialize passes.

For some reason, only the first closure parameter was checked for recursion. We need to handle patterns like this:

  kind=FunctionSignatureSpecialization
    kind=SpecializationPassID, index=3
    kind=FunctionSignatureSpecializationParam
    kind=FunctionSignatureSpecializationParam
      kind=FunctionSignatureSpecializationParamKind, index=0
      kind=FunctionSignatureSpecializationParamPayload, text="$s4test10ExpressionO8contains5whereS3bXE_tFSbACXEfU_S2bXEfU_36$s4test12IndirectEnumVACycfcS2bXEfU_Tf3npf_n"

I fixed the logic so we now check for recursion on all closure parameters and bail out on unrecognized mangling formats.

For reference, see summary.sil in
Infinitely recursive closure specialization swiftlang#61955
swiftlang#61955

Fixes rdar://101589190 (Swift Compiler hangs when building this code for release)
@atrick atrick force-pushed the fix-closurespecialize-hang branch from 856386c to c665311 Compare November 7, 2022 22:44
@atrick
Copy link
Contributor Author

atrick commented Nov 7, 2022

Basically LGTM.
Can you add a test case?

Test case included

@atrick
Copy link
Contributor Author

atrick commented Nov 7, 2022

@swift-ci smoke test and merge

@atrick
Copy link
Contributor Author

atrick commented Nov 7, 2022

No benchmark regressions!

Copy link
Contributor

@eeckstein eeckstein left a comment

Choose a reason for hiding this comment

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

thanks!

@atrick
Copy link
Contributor Author

atrick commented Nov 8, 2022

@swift-ci smoke test macOS

@atrick
Copy link
Contributor Author

atrick commented Nov 8, 2022

@swift-ci smoke test

@atrick atrick merged commit 0592979 into swiftlang:main Nov 9, 2022
@atrick atrick deleted the fix-closurespecialize-hang branch November 9, 2022 00:41
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.

2 participants