[Distributed] Ensure to swift_release during destroying decoded distributed call arguments #65952
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@swift-ci please smoke test |
|
Why? |
ktoso
force-pushed
the
wip-destroy-dist-args-reverse-order
branch
from
b58b86e
to
842771c
Compare
|
@DougGregor asked for it, I guess so it's the same as other things in this func, also in reverse? "Shouldn't matter but" I guess |
|
The other thing is stack allocations which should be in reverse order, this one is just calls to destroy function that’s why I am asking the question here :) |
|
@swift-ci please smoke test |
|
Yeah it really shouldn't matter for this one... I'll ask @DougGregor if he thinks we'd better do it like this anyway or nah. |
|
note to self; add some more tests to check various combinations of types |
ktoso
force-pushed
the
wip-destroy-dist-args-reverse-order
branch
from
842771c
to
73cd2c9
Compare
|
I was adding more tests and actually see that deinits DON'T run and we have an unbalanced retain here still -- even if the memory is properly destroyed so we don't leak the object itself. We're in the Direct_Guaranteed case and take the param with |
ktoso
force-pushed
the
wip-destroy-dist-args-reverse-order
branch
from
73cd2c9
to
6649804
Compare
ktoso
changed the title
ktoso
force-pushed
the
wip-destroy-dist-args-reverse-order
branch
from
6649804
to
abdfb15
Compare
|
@swift-ci please smoke test |
ktoso
force-pushed
the
wip-destroy-dist-args-reverse-order
branch
from
abdfb15
to
cd3e243
Compare
This was referenced May 19, 2023
|
@swift-ci please smoke test |
xedin
reviewed
May 19, 2023
xedin
approved these changes
May 19, 2023
|
Fixed formatting |
|
@swift-ci please smoke test and merge |
ktoso
added a commit
to ktoso/swift
that referenced
this pull request
Jan 10, 2024
We recently fixed a similar issue where the objects created from decodeNextArgument in a distributed thunk were not properly destroyed. The fix was in swiftlang#65952 In that fix, we missed handling of Indirect_In_Guaranteed which also need to be destroyed, this patch corrects that omission. Resolves swiftlang#70004 Resolves rdar://119943672 Resolves rdar://119446012
ktoso
added a commit
to ktoso/swift
that referenced
this pull request
Jan 10, 2024
We recently fixed a similar issue where the objects created from decodeNextArgument in a distributed thunk were not properly destroyed. The fix was in swiftlang#65952 In that fix, we missed handling of Indirect_In_Guaranteed which also need to be destroyed, this patch corrects that omission. Resolves swiftlang#70004 Resolves rdar://119943672 Resolves rdar://119446012
ktoso
added a commit
to ktoso/swift
that referenced
this pull request
Jan 10, 2024
We recently fixed a similar issue where the objects created from decodeNextArgument in a distributed thunk were not properly destroyed. The fix was in swiftlang#65952 In that fix, we missed handling of Indirect_In_Guaranteed which also need to be destroyed, this patch corrects that omission. Resolves swiftlang#70004 Resolves rdar://119943672 Resolves rdar://119446012
ktoso
added a commit
to ktoso/swift
that referenced
this pull request
Jan 10, 2024
We recently fixed a similar issue where the objects created from decodeNextArgument in a distributed thunk were not properly destroyed. The fix was in swiftlang#65952 In that fix, we missed handling of Indirect_In_Guaranteed which also need to be destroyed, this patch corrects that omission. Resolves swiftlang#70004 Resolves rdar://119943672 Resolves rdar://119446012
ktoso
added a commit
to ktoso/swift
that referenced
this pull request
Jan 10, 2024
We recently fixed a similar issue where the objects created from decodeNextArgument in a distributed thunk were not properly destroyed. The fix was in swiftlang#65952 In that fix, we missed handling of Indirect_In_Guaranteed which also need to be destroyed, this patch corrects that omission. Resolves swiftlang#70004 Resolves rdar://119943672 Resolves rdar://119446012
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Follow up to #65874
Resolves: #65853 now also ensuring deinits properly run.
Thanks for review @DougGregor
Huge thanks to @xedin for helping debug the issue.
We must destroy before we deallocate, otherwise the destroy does nothing to refcount and we would not run deinitializers as expected.