Skip to content

Disable distributed_actor_assume_executor.swift #64435

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
Mar 16, 2023

Conversation

tshortli
Copy link
Contributor

@tshortli tshortli commented Mar 16, 2023

It is failing due to a compiler assertion in IRGen on Windows:

0.	Program arguments: t:\\swift\\bin\\swiftc.exe -frontend -c -primary-file C:\\Users\\swift-ci\\jenkins\\workspace\\swift-PR-windows\\swift\\test\\Distributed\\Runtime\\distributed_actor_assume_executor.swift C:\\Users\\swift-ci\\jenkins\\workspace\\swift-PR-windows\\swift\\test\\Distributed\\Runtime/../Inputs/FakeDistributedActorSystems.swift -target x86_64-unknown-windows-msvc -disable-objc-interop -I T:\\swift\\test-windows-x86_64\\Distributed\\Runtime\\Output\\distributed_actor_assume_executor.swift.tmp -vfsoverlay T:/swift\\stdlib\\windows-vfs-overlay.yaml -swift-version 4 -define-availability "SwiftStdlib 9999:macOS 9999, iOS 9999, watchOS 9999, tvOS 9999" -define-availability "SwiftStdlib 5.0:macOS 10.14.4, iOS 12.2, watchOS 5.2, tvOS 12.2" -define-availability "SwiftStdlib 5.1:macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0" -define-availability "SwiftStdlib 5.2:macOS 10.15.4, iOS 13.4, watchOS 6.2, tvOS 13.4" -define-availability "SwiftStdlib 5.3:macOS 11.0, iOS 14.0, watchOS 7.0, tvOS 14.0" -define-availability "SwiftStdlib 5.4:macOS 11.3, iOS 14.5, watchOS 7.4, tvOS 14.5" -define-availability "SwiftStdlib 5.5:macOS 12.0, iOS 15.0, watchOS 8.0, tvOS 15.0" -define-availability "SwiftStdlib 5.6:macOS 12.3, iOS 15.4, watchOS 8.5, tvOS 15.4" -define-availability "SwiftStdlib 5.7:macOS 13.0, iOS 16.0, watchOS 9.0, tvOS 16.0" -define-availability "SwiftStdlib 5.8:macOS 13.3, iOS 16.4, watchOS 9.4, tvOS 16.4" -define-availability "SwiftStdlib 5.9:macOS 9999, iOS 9999, watchOS 9999, tvOS 9999" -disable-availability-checking -autolink-library oldnames -autolink-library msvcrt -Xcc -D_MT -Xcc -D_DLL -parse-as-library -module-name a -o T:\\tmp\\lit-tmp-ev0tg5ng\\distributed_actor_assume_executor-80a30f.o

1.	Swift version 5.9-dev (LLVM f2296de74014c53, Swift 9b89a948d27c5ac)

2.	Compiling with effective version 4.1.50

3.	While evaluating request IRGenRequest(IR Generation for file "C:\Users\swift-ci\jenkins\workspace\swift-PR-windows\swift\test\Distributed\Runtime\distributed_actor_assume_executor.swift")

4.	While emitting IR for source file C:\Users\swift-ci\jenkins\workspace\swift-PR-windows\swift\test\Distributed\Runtime\distributed_actor_assume_executor.swift

5.	While emitting class metadata for 'MainFriend' (at C:\Users\swift-ci\jenkins\workspace\swift-PR-windows\swift\test\Distributed\Runtime\distributed_actor_assume_executor.swift:42:13)

6.	While emitting metadata for 'MainFriend' (at C:\Users\swift-ci\jenkins\workspace\swift-PR-windows\swift\test\Distributed\Runtime\distributed_actor_assume_executor.swift:42:13)

@tshortli tshortli requested a review from ktoso as a code owner March 16, 2023 20:00
@tshortli
Copy link
Contributor Author

@swift-ci please smoke test and merge

Copy link
Member

@compnerd compnerd left a comment

Choose a reason for hiding this comment

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

This is the wrong way to handle this. Please disable this test globally

@tshortli tshortli force-pushed the disable-assume-actor-test-windows branch from 43c5ab4 to b2a1e16 Compare March 16, 2023 20:17
@tshortli
Copy link
Contributor Author

@swift-ci please smoke test

@tshortli tshortli requested a review from compnerd March 16, 2023 20:17
Copy link
Member

@compnerd compnerd left a comment

Choose a reason for hiding this comment

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

Thanks!

@tshortli tshortli changed the title Disable distributed_actor_assume_executor.swift on Windows Disable distributed_actor_assume_executor.swift Mar 16, 2023
@jckarter jckarter merged commit 8fa394f into swiftlang:main Mar 16, 2023
@tshortli tshortli deleted the disable-assume-actor-test-windows branch March 16, 2023 20:31
@compnerd
Copy link
Member

From a local build:

The decl causing the issue:

distributed actor MainFriend {
  nonisolated var unownedExecutor: UnownedSerialExecutor { get }
  @_hasStorage @_hasInitialValue final let isolatedProperty: String { get }
  distributed func test(x: Int) async throws
  static func resolve(id: ActorAddress, using system: FakeRoundtripActorSystem) throws -> MainFriend
  typealias ActorSystem = FakeRoundtripActorSystem
  typealias ID = ActorAddress
  typealias SerializationRequirement = any Decodable & Encodable
  @_hasStorage nonisolated final let actorSystem: FakeRoundtripActorSystem { get }
  deinit
  nonisolated var hashValue: Int { get }
  @_compilerInitialized @_hasStorage nonisolated final let id: ActorAddress { get }
  init(actorSystem system: FakeRoundtripActorSystem)
}

At the point of assertion: index = 4, numFields = 3.

The iteration encounters the following fields:

  1. $nonDefaultDistributedActor
  2. id
  3. actorSystem
  4. isolatedProperty

@compnerd
Copy link
Member

@ktoso
Copy link
Contributor

ktoso commented May 15, 2023

I missed this; why do you consider that bit odd? That bit is definitely right: We store the non-default executor in there.

But the order of fields you show here is bad -- it should always be:

  • id
  • actorsystem
  • nondefaultdistributedactor

we have an assertion for this:

/// Asserts that the synthesized fields appear in the expected order.
///
/// The `id` and `actorSystem` MUST be the first two fields of a distributed actor,
/// because we assume their location in IRGen, and also when we allocate a distributed remote actor,
/// we're able to allocate memory ONLY for those and without allocating any of the storage for the actor's
/// properties.
///         [id, actorSystem]
/// followed by the executor fields for a default distributed actor.
///
static void assertRequiredSynthesizedPropertyOrder(DerivedConformance &derived, ValueDecl *derivedValue) {

so if it was wrong on windows somehow perhaps the logic is still flawed and that would be a real bug

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.

4 participants