[DiscardingTG] Undo pointer auth workaround; fix memory leaks #65613

ktoso · 2023-05-03T14:10:24Z

There are two leaks in the group.

"first error task leaks"
(a) the "the first error task that we RETAIN" and later on use to automatically re-throw the "first thrown by a child task error" task was not released;

"all values leak"
(b) due to the "hot fix" for pointer auth #65220 done for rdar://107574868 where we changed the signature of addTask from ()->() (what it must be), to ()->T in order to work around pointer-auth issues we introduced a leak and ALL "discarded" values would be leaking since we would not release that T value. The correct way to solve this is to undo this workaround and use correct pointer auth, which this PR does.

We must fix the method signature and undo the hack which #65223 was.

Resolves: rdar://108829466

test/Concurrency/Runtime/async_taskgroup_discarding_noLeaks.swift

stdlib/public/Concurrency/TaskGroup.cpp

ktoso · 2023-05-08T14:01:43Z

@swift-ci please smoke test

include/swift/ABI/MetadataValues.h

ktoso · 2023-05-16T08:46:06Z

stdlib/public/Concurrency/DiscardingTaskGroup.swift

    priority: TaskPriority? = nil,
-    operation: __owned @Sendable @escaping () async -> DiscardedResult


This entire change to () -> T was a hotfix to temporarily unbreak but it is very wrong, we must get back to -> Void

✅ This fixes the (b) leaks the by undoing what introduced them in the first place.

stdlib/public/Concurrency/Task.cpp

stdlib/public/Concurrency/TaskGroup.cpp

ktoso · 2023-05-16T08:53:08Z

This is primarily stuck on pointer auth issues

include/swift/Runtime/Concurrency.h

ktoso · 2023-05-24T10:57:47Z

@swift-ci please smoke test

[lit] cleanup lit leaks target

…efault

ktoso · 2023-05-24T16:10:04Z

stdlib/public/Concurrency/TaskGroup.cpp

+          resumeWaitingTask(readyErrorItem.getTask(), assumed,
+                            /*hadErrorResult=*/true,
+                            alreadyDecrementedStatus,
+                            /*taskWasRetained=*/true);


hunting this one down took way too long... this is another case where we dequeue the retained error task and need to release it later

ktoso · 2023-05-24T16:55:52Z

@swift-ci please smoke test

ktoso · 2023-05-24T17:01:22Z

stdlib/public/Concurrency/TaskGroup.cpp

@@ -1369,6 +1383,11 @@ void TaskGroupBase::resumeWaitingTask(
        // locks) because we know that the child task is completed and
        // we can't be holding its locks ourselves.
        _swift_taskGroup_detachChild(asAbstract(this), completedTask);
+        if (isDiscardingResults() && hadErrorResult && taskWasRetained) {


✅ This fixes the (a) leaks because we need to balance the extra retain we do when we store the task for future use like this.

In other words, in this case of the discarding group we were missing this balancing release:

// Retain the task while it is in the queue; it must remain alive until // it is found by poll. This retain will be balanced by the release in waitAll. assert(hadErrorResult); // a discarding group may only store an errored task. swift_retain(completedTask);

ktoso · 2023-05-24T17:05:26Z

@swift-ci please smoke test

ktoso · 2023-05-24T17:08:28Z

stdlib/public/Concurrency/Task.cpp

-  std::tie(taskEntry, initialContextSize) =
+  TaskCreateFlags taskCreateFlags(rawTaskCreateFlags);
+
+  if (taskCreateFlags.isDiscardingTask()) {


the pointer auth dance for the nullary function type:

rjmccall

Okay. I think this seems reasonable.

ktoso · 2023-05-25T10:31:55Z

Thanks for review John!

A bit confused why same commits worked on Windows 5.9 but not here, in a lit configuration failure. Let's try again

ktoso · 2023-05-25T10:32:08Z

@swift-ci please smoke test Windows

ktoso · 2023-05-25T17:33:01Z

Silly main + windows only issue in lit config:

  File "C:/Users/swift-ci/jenkins/workspace/swift-PR-windows/swift\test\lit.cfg", line 2324, in <module>
    escape_for_substitute_captures(config.target_run_with_leaks))
AttributeError: 'TestingConfig' object has no attribute 'target_run_with_leaks'

Fixed I hope;

@swift-ci please smoke test and merge

ktoso · 2023-05-25T17:42:40Z

@swift-ci please smoke test and merge

ktoso · 2023-05-25T21:36:10Z

Pending on the CI unbreak: #66138

ktoso · 2023-05-25T22:43:22Z

@swift-ci please smoke test and merge

… Void closure Since swiftlang#65613, DiscardingTG started to accept `() -> Void` instead of `() -> T`, but it also changed the number of arguments accepted by the closure from 3 to 2. So it should use `non_future_adapter` instead of `future_adapter` to avoid runtime signature mismatch.

ktoso commented May 3, 2023

View reviewed changes

test/Concurrency/Runtime/async_taskgroup_discarding_noLeaks.swift Outdated Show resolved Hide resolved

ktoso commented May 3, 2023

View reviewed changes

test/Concurrency/Runtime/async_taskgroup_discarding_noLeaks.swift Outdated Show resolved Hide resolved

ktoso commented May 3, 2023

View reviewed changes

stdlib/public/Concurrency/TaskGroup.cpp Outdated Show resolved Hide resolved

ktoso added the concurrency Feature: umbrella label for concurrency language features label May 3, 2023

ktoso marked this pull request as draft May 3, 2023 14:19

ktoso force-pushed the wip-discarding-error-task-leak branch 2 times, most recently from 8f660b4 to 72f14b5 Compare May 8, 2023 08:42

ktoso commented May 16, 2023

View reviewed changes

include/swift/ABI/MetadataValues.h Outdated Show resolved Hide resolved

ktoso commented May 16, 2023

View reviewed changes

ktoso requested review from rjmccall and DougGregor May 16, 2023 08:52

ktoso changed the title ~~[DiscardingTG] Don't leak the first error task if we had to retain it~~ [DiscardingTG] Don't leak the first error task if we retained it, and fix addTask signature to prevent leaks May 16, 2023

ktoso changed the title ~~[DiscardingTG] Don't leak the first error task if we retained it, and fix addTask signature to prevent leaks~~ [DiscardingTG] Undo pointer auth workaround, prevent leaks May 17, 2023

ktoso commented May 18, 2023

View reviewed changes

include/swift/Runtime/Concurrency.h Outdated Show resolved Hide resolved

ktoso force-pushed the wip-discarding-error-task-leak branch 2 times, most recently from 8ba48c6 to 679a24a Compare May 24, 2023 10:57

[lit] introduce target-run-simple-leaks-swift to run with leaks

44aff27

[lit] cleanup lit leaks target

ktoso force-pushed the wip-discarding-error-task-leak branch from 679a24a to ae58811 Compare May 24, 2023 15:28

ktoso added 3 commits May 24, 2023 18:03

[Discarding] Don't leak retained "first error" task when retaining it

cad608e

make (normal) task group "dont leak" test use leaks and enable by d…

b039691

…efault

back to println tests

9f5a707

ktoso force-pushed the wip-discarding-error-task-leak branch from ae58811 to 9f5a707 Compare May 24, 2023 16:04

ktoso commented May 24, 2023

View reviewed changes

ktoso marked this pull request as ready for review May 24, 2023 16:28

less code duplication

d82de55

ktoso force-pushed the wip-discarding-error-task-leak branch from cdd127b to d82de55 Compare May 24, 2023 16:47

ktoso force-pushed the wip-discarding-error-task-leak branch 2 times, most recently from ce1c880 to 0b088e2 Compare May 24, 2023 16:58

remove extra debug statements

760cb44

ktoso force-pushed the wip-discarding-error-task-leak branch from 0b088e2 to 760cb44 Compare May 24, 2023 16:59

ktoso changed the title ~~[DiscardingTG] Undo pointer auth workaround, prevent leaks~~ [DiscardingTG] Undo pointer auth workaround; fix memory leaks May 24, 2023

ktoso commented May 24, 2023

View reviewed changes

ktoso mentioned this pull request May 24, 2023

🍒[5.9][DiscardingTG] Undo pointer auth workaround; fix memory leaks #66103

Merged

rjmccall approved these changes May 24, 2023

View reviewed changes

fix lit config on windows

e3d4b55

ktoso force-pushed the wip-discarding-error-task-leak branch from 68a5e09 to e3d4b55 Compare May 25, 2023 17:42

swift-ci merged commit 9698128 into swiftlang:main May 26, 2023

ktoso mentioned this pull request May 26, 2023

[Distributed] Relax ordering of checks in async_taskgroup_discarding_dontLeak #66166

Merged

kateinoigakukun mentioned this pull request May 30, 2023

[DiscardingTG] Fix runtime signature mismatch for statically known -> Void closure #66219

Merged

		priority: TaskPriority? = nil,
		operation: __owned @Sendable @escaping () async -> DiscardedResult

[DiscardingTG] Undo pointer auth workaround; fix memory leaks #65613

[DiscardingTG] Undo pointer auth workaround; fix memory leaks #65613

Uh oh!

Conversation

ktoso commented May 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ktoso commented May 8, 2023

Uh oh!

Uh oh!

ktoso May 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ktoso commented May 16, 2023

Uh oh!

Uh oh!

ktoso commented May 24, 2023

Uh oh!

ktoso May 24, 2023

Choose a reason for hiding this comment

Uh oh!

ktoso commented May 24, 2023

Uh oh!

ktoso May 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ktoso commented May 24, 2023

Uh oh!

ktoso May 24, 2023

Choose a reason for hiding this comment

Uh oh!

rjmccall left a comment

Choose a reason for hiding this comment

Uh oh!

ktoso commented May 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ktoso commented May 25, 2023

Uh oh!

ktoso commented May 25, 2023

Uh oh!

ktoso commented May 25, 2023

Uh oh!

ktoso commented May 25, 2023

Uh oh!

ktoso commented May 25, 2023

Uh oh!

Uh oh!

ktoso commented May 3, 2023 •

edited

Loading

ktoso May 16, 2023 •

edited

Loading

ktoso May 24, 2023 •

edited

Loading

ktoso commented May 25, 2023 •

edited

Loading