GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-112535: Update _Py_ThreadId() to support PowerPC, IBM Z, etc by corona10 · Pull Request #112553 · python/cpython

Conversation

@corona10

@corona10 corona10 commented Nov 30, 2023 •
edited by bedevere-app Bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member
  • Issue: Make _Py_ThreadId() work on PowerPC, IBM Z, etc. #112535

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@corona10
pythongh-112535: Update _Py_ThreadId() to support PowerPC, IBM Z, etc
54070e1
@corona10
corona10 requested review from colesbury and vstinner November 30, 2023 13:41
@corona10
corona10 requested a review from erlend-aasland as a code owner November 30, 2023 13:41
@bedevere-app bedevere-app Bot mentioned this pull request Nov 30, 2023
Make _Py_ThreadId() work on PowerPC, IBM Z, etc. #112535
Closed
@bedevere-app bedevere-app Bot added the awaiting core review label Nov 30, 2023
@corona10 corona10 added skip news topic-free-threading and removed awaiting core review labels Nov 30, 2023
vstinner
vstinner reviewed Nov 30, 2023
View reviewed changes
Comment thread Include/object.h
__asm__ ("mrs %0, tpidrro_el0" : "=r" (tid));
#elif defined(__aarch64__)
__asm__ ("mrs %0, tpidr_el0" : "=r" (tid));
#elif define(HAVE_BUILTIN_THREAD_POINTER)

@vstinner vstinner Nov 30, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This line of the public C API which can be used by a C compiler different than the one used to build Python. I don't think that if define(HAVE_BUILTIN_THREAD_POINTER) check is reliable, according to what @colesbury wrote in the issue.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@vstinner vstinner Nov 30, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I wrote "C compiler", but the C API can be parsed by parsers which are not C compiler, but written in any programming language. Moreover, the C API is used in C++.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@corona10 corona10 Nov 30, 2023

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Ah got it, let me check :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@corona10 corona10 Nov 30, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

. I don't think that if define(HAVE_BUILTIN_THREAD_POINTER) check is reliable
but the C API can be parsed by parsers which are not C compiler

Yeah, I thought that configuring time checking would be enough but it looks like we should consider 3rd party case.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@colesbury colesbury Nov 30, 2023

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Yeah, I'm concerned that the compiler for CPython may be not exactly the same as the compiler for third-party extensions. For example, if we want to use __builtin_thread_pointer() for x86-64, it's available for GCC 11.1+, but an extension might be compiling with an older version of GCC that doesn't support it for that architecture.

Additionally, one thing to be careful of, is that macOS arm64 will successfully compile and run __builtin_thread_pointer(), but it returns the wrong value (CPU id).

PowerPC on GCC still requires a "register variable" https://github.com/bminor/glibc/blob/master/sysdeps/powerpc/nptl/thread_pointer.h (or it can fallback to a portable thread-local implementation, but that would be slower.)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
erlend-aasland and corona10 reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
@vstinner

vstinner commented Nov 30, 2023

Copy link
Copy Markdown
Member

Maybe we should have a more global configuration: "if the compiler doesn't support _Py_ThreadId(), use opaque function calls for Py_INCREF/Py_DECREF". Maybe even give a way to opt-out from inline assembly code.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@colesbury

colesbury commented Nov 30, 2023

Copy link
Copy Markdown
Contributor

@vstinner, that might be a better strategy. It just seems more difficult than making _Py_ThreadId() an opaque function if it's not supported.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@corona10 corona10 closed this Dec 5, 2023
@vstinner

vstinner commented Dec 5, 2023

Copy link
Copy Markdown
Member

@vstinner, that might be a better strategy. It just seems more difficult than making _Py_ThreadId() an opaque function if it's not supported.

One step forward: with PR #112747, it becomes possible to use the limited C API on a free threading build. It's implemented with opaque function calls for functions which use too "low-level" functions such at the private _Py_atomic API or _Py_ThreadId().

Later, we can modify the #if checks to also use this code path for platforms which don't support _Py_ThreadId(). A "platform" can also be a combination of {C/C++ compiler, C/C++ compiler options, operating system name/version, CPU architecture}. I'm not sure that inline assembly code is allowed by all C/C++ compiler configurations.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner vstinner mentioned this pull request Dec 5, 2023
gh-111506: Implement Py_REFCNT() as opaque function call #112747
Closed
@corona10
corona10 deleted the gh-112535 branch February 15, 2024 01:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner vstinner left review comments
@colesbury colesbury Awaiting requested review from colesbury
@erlend-aasland erlend-aasland Awaiting requested review from erlend-aasland erlend-aasland is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news topic-free-threading

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@corona10 @vstinner @colesbury
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.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.