Skip to content

Concurrency: add Windows support to SWIFT_TASK_PRINTF_DEBUG #39348

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
Sep 22, 2021

Conversation

compnerd
Copy link
Member

pthread_self is not portable to all platforms. Introduce a
_swift_get_current_thread_id to abstract over accessing the current
thread ID. On Windows, the thread ID and thread handle are two separate
entities, unlike POSIX threads which treats them the same.

Replace this paragraph with a description of your changes and rationale. Provide links to external references/discussions if appropriate.

Resolves SR-NNNN.

`pthread_self` is not portable to all platforms.  Introduce a
`_swift_get_current_thread_id` to abstract over accessing the current
thread ID.  On Windows, the thread ID and thread handle are two separate
entities, unlike POSIX threads which treats them the same.
@compnerd compnerd requested a review from a team as a code owner September 17, 2021 21:22
@compnerd
Copy link
Member Author

@swift-ci please test

@compnerd
Copy link
Member Author

CC: @ktoso @DougGregor

@compnerd
Copy link
Member Author

This is a cherry-pick of #39348 to the 5.5 branch, though there were conflicts as there were changes that were applied out of order.

@shahmishal
Copy link
Member

@swift-ci test

@shahmishal
Copy link
Member

Failed Tests (1):
  Swift(windows-x86_64) :: Interop/Cxx/extern-var/extern-var-irgen.swift

@compnerd is this known failure?

@ktoso ktoso self-requested a review September 18, 2021 00:35
Copy link
Contributor

@ktoso ktoso left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @compnerd !

For reference I don't think this would have been breaking anything since it was only in a macro that is only conditionally enabled during debugging... 🤔

@compnerd
Copy link
Member Author

@shahmishal - no, its not, I don't see that in the results though?

@compnerd
Copy link
Member Author

@ktoso - its the missing include that causes problems - ThreadHandle uses some fancy C++ trickery to avoid spelling out the types.

@ktoso
Copy link
Contributor

ktoso commented Sep 18, 2021

Oh I see, thank you for explaining! 🙏

Copy link
Member

@DougGregor DougGregor left a comment

Choose a reason for hiding this comment

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

This looks fine to me

@ktoso ktoso merged commit a6b3322 into release/5.5 Sep 22, 2021
@ktoso ktoso deleted the compnerd/5.5-task-logging branch September 22, 2021 20:31
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