[Distributed] Protocol mangling and primary associated type fixes #77584

ktoso · 2024-11-13T14:56:57Z

The commits are now properly staged, please check their descriptions for details.

This solves:

rdar://139332556 - so now we're handling primary associated types in protocols with @resolvable -- and this would have then crashed during mangling (!), which is why the follow up commits
rdar://139781083 - which is solved in 2a3ac44 (#77584) which was about how sometimes with generics involved or more complex type signatures of a distributed func we'd wrongly mangle and crash during thunk mangling.

Both changes go hand in hand and allow correct handling of more situations with generics and complex types in distributed funcs

xedin · 2024-11-27T07:46:12Z

@ktoso I see that there is a bunch of leftover comments and code, could you please clean that up and ping me when done so I can take a look when this is ready to merge?

ktoso · 2024-11-27T07:56:13Z

Oh yeah don't review this at all yet, I'm cleaning up and will make it into some nice commits. Marked as draft now, sorry!

ktoso · 2024-11-27T11:06:40Z

@swift-ci please smoke test

ktoso · 2024-11-27T11:20:29Z

@swift-ci please smoke test

ktoso · 2024-11-27T14:50:33Z

@swift-ci please smoke test

ktoso · 2024-11-28T01:53:01Z

Unrelated, Cxx interop fails compiling on Windows:
https://ci-external.swift.org/job/swift-PR-windows/33738/console

inalloca argument for call has mismatched alloca

  %7 = alloca %TSo3stdO0077basic_stringCChar32char_traitsCChar32allocatorCChar32_jlHEvsaGGdtaEBguasEitaaV, align 4

  %9 = call i32 @"?__swift_interopComputeHashOfU32String@@YAIV?$basic_string@_UU?$char_traits@_U@std@@V?$allocator@_U@2@@std@@@Z"(ptr %7) #16

<unknown>:0: error: fatal error encountered during compilation; please submit a bug report (https://swift.org/contributing/#reporting-bugs)

ktoso · 2024-11-28T01:53:17Z

@swift-ci please smoke test Windows

ktoso · 2024-11-28T02:17:02Z

...d/Runtime/distributed_actor_remoteCall_protocol_method_in_presence_of_multiple_systems.swift

+    // CHECK-NEXT: > encode return type: Swift.String
+    // CHECK-NEXT: > done recording
+    // CHECK-NEXT: >> remoteCall: on:main.$GreeterProtocol<main.FakeRoundtripActorSystem>, target:main.$GreeterProtocol.greet(), invocation:FakeInvocationEncoder(genericSubs: [main.$GreeterProtocol<main.FakeRoundtripActorSystem>], arguments: [], returnType: Optional(Swift.String), errorType: nil), throwing:Swift.Never, returning:Swift.String
+    // CHECK-NEXT: > execute distributed target: main.$GreeterProtocol.greet(), identifier: $s4main16$GreeterProtocolC5greetSSyYaKFTE


note: here we keep the same mangling, good 👍

ktoso · 2024-11-28T02:17:32Z

test/Distributed/Runtime/distributed_actor_func_calls_remoteCall_extension.swift

@@ -40,7 +40,9 @@ func test() async throws {
  let ref = try KappaProtocolImpl.resolve(id: local.id, using: system)

  let reply = try await ref.echo(name: "Caplin")
-  // CHECK: >> remoteCall: on:main.KappaProtocolImpl, target:main.$KappaProtocol.echo(name:), invocation:FakeInvocationEncoder(genericSubs: [main.KappaProtocolImpl], arguments: ["Caplin"], returnType: Optional(Swift.String), errorType: nil), throwing:Swift.Never, returning:Swift.String
+  // CHECK: >> remoteCall: on:main.KappaProtocolImpl, target:echo(name:), invocation:FakeInvocationEncoder(genericSubs: [main.KappaProtocolImpl], arguments: ["Caplin"], returnType: Optional(Swift.String), errorType: nil), throwing:Swift.Never, returning:Swift.String
+  // CHECK: > execute distributed target: echo(name:), identifier: $s4main13KappaProtocolPAAE4echo4nameS2S_tYaKFTE


added a mangling check as well, we didnt before

ktoso · 2024-12-02T02:04:58Z

@swift-ci please smoke test

xedin

LGTM! Could you add an example with array type and optional as parameters to make sure that it works as expected too?

lib/AST/ASTMangler.cpp

ktoso · 2024-12-03T01:48:26Z

Thanks for review! Huh seems I missed to git add my array test :) Adding some more

…tocols Previously we would not propagate those into the generated distributed actor, making a lot of generic distributed actor protocols impossible to express. We indeed cannot handle protocols WITHOUT primary associated types, but we certainly can handle them with! This resolves rdar://139332556

This is a crucial fix without which we can crash on some distributed protocol declarations with @resolvable. We cannot "just" use a String to represent the "fake base" of the thunks, and must instead find the $Target macro generated type and use it as the base of the thunk's mangling. Calls are made in such way that record for the protocol requirement: `$s4main28GreeterDefinedSystemProtocolP5greetSSyYaKFTEHF` points at `$$s4main29$GreeterDefinedSystemProtocolC5greetSSyYaKFTE` which makes a dispatch through the _apropriate_ witness table. And the record for the $witness named e.g. `$s4main29$GreeterDefinedSystemProtocolC5greetSSyYaKFTEHF` points to `$s4main28GreeterDefinedSystemProtocolPAA11Distributed01_F9ActorStubRzrlE5greetSSyYaKFTE` which is an extension method: `distributed thunk (extension in main):main.GreeterDefinedSystemProtocol< where A: Distributed._DistributedActorStub>.greet() async throws -> Swift.String`, this very specific design allows us to call the "right method" on the recieving end of a remote call where we do not know the recipient type.

ktoso · 2024-12-03T12:07:58Z

@swift-ci please smoke test

ktoso · 2024-12-04T05:06:11Z

@swift-ci please smoke test

ktoso · 2024-12-04T06:18:32Z

@swift-ci please smoke test

ktoso · 2024-12-04T06:19:18Z

@swift-ci please smoke test

ktoso changed the title ~~checking resolvable and array calls~~ [Distributed] checking resolvable and array calls Nov 18, 2024

ktoso added the distributed Feature → concurrency: distributed actor label Nov 18, 2024

ktoso force-pushed the wip-check-array-calls branch from ff19248 to 6812658 Compare November 27, 2024 07:37

ktoso requested review from zoecarver, hyp, egorzhdan, Xazax-hun, rjmccall, mikeash, al45tair, DougGregor, hborla, slavapestov and xedin as code owners November 27, 2024 07:37

ktoso marked this pull request as draft November 27, 2024 07:56

ktoso force-pushed the wip-check-array-calls branch 2 times, most recently from 5cff8a5 to 86f4820 Compare November 27, 2024 11:04

ktoso force-pushed the wip-check-array-calls branch from 86f4820 to 0906d32 Compare November 27, 2024 11:17

ktoso marked this pull request as ready for review November 27, 2024 11:20

ktoso changed the title ~~[Distributed] checking resolvable and array calls~~ [Distributed] Protocol mangling and primary associated type fixes Nov 27, 2024

ktoso commented Nov 28, 2024

View reviewed changes

ktoso mentioned this pull request Dec 2, 2024

[6.1][Distributed] Protocol mangling and primary associated type fixes #77895

Merged

xedin approved these changes Dec 3, 2024

View reviewed changes

lib/AST/ASTMangler.cpp Show resolved Hide resolved

lib/AST/ASTMangler.cpp Outdated Show resolved Hide resolved

ktoso added 6 commits December 3, 2024 14:59

[Distributed] Introduce SWIFT_DUMP_ACCESSIBLE_FUNCTIONS

6e9224c

[Distributed] More tests for primary associated types in protocols

a98a30d

[Distributed] minor cleanups, remove dump=always from tests

7189ade

[DistributedResolvableMacro] cannot use if expressions

4da5b64

ktoso force-pushed the wip-check-array-calls branch from 96c1266 to a48b4f8 Compare December 3, 2024 12:07

ktoso force-pushed the wip-check-array-calls branch from a48b4f8 to d098618 Compare December 4, 2024 06:11

[Distributed] additional test coverage

746720c

ktoso force-pushed the wip-check-array-calls branch from d098618 to 746720c Compare December 4, 2024 06:19

ktoso enabled auto-merge December 4, 2024 06:19

ktoso merged commit ed5007f into swiftlang:main Dec 4, 2024
3 checks passed

ktoso deleted the wip-check-array-calls branch December 10, 2024 09:29

ktoso mentioned this pull request Jan 21, 2025

Distributed: allow @resolvable with primary associated types #77449

Closed

[Distributed] Protocol mangling and primary associated type fixes #77584

[Distributed] Protocol mangling and primary associated type fixes #77584

Uh oh!

Conversation

ktoso commented Nov 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xedin commented Nov 27, 2024

Uh oh!

ktoso commented Nov 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ktoso commented Nov 27, 2024

Uh oh!

ktoso commented Nov 27, 2024

Uh oh!

ktoso commented Nov 27, 2024

Uh oh!

ktoso commented Nov 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ktoso commented Nov 28, 2024

Uh oh!

ktoso Nov 28, 2024

Choose a reason for hiding this comment

Uh oh!

ktoso Nov 28, 2024

Choose a reason for hiding this comment

Uh oh!

ktoso commented Dec 2, 2024

Uh oh!

xedin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ktoso commented Dec 3, 2024

Uh oh!

ktoso commented Dec 3, 2024

Uh oh!

ktoso commented Dec 4, 2024

Uh oh!

ktoso commented Dec 4, 2024

Uh oh!

ktoso commented Dec 4, 2024

Uh oh!

Uh oh!

Uh oh!

ktoso commented Nov 13, 2024 •

edited

Loading

ktoso commented Nov 27, 2024 •

edited

Loading

ktoso commented Nov 28, 2024 •

edited

Loading