Skip to content

gh-110014: Fix inconsistent struct definition in pycore_semaphore.h #110030

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

Closed
wants to merge 1 commit into from

Conversation

colesbury
Copy link
Contributor

@colesbury colesbury commented Sep 28, 2023

The pycore_semaphore.h header is included by Python/lock.c and Python/parking_lot.c. The macro _POSIX_SEMAPHORES was not consistently defined across the two files (due to a missing include of <unistd.h>) leading to different struct definitions. The RHEL8 ppc64le LTO buildbot correctly warned due to this issue.

…re.h

The pycore_semaphore.h header is included by Python/lock.c and
Python/parking_lot.c. The macro `_POSIX_SEMAPHORES` was not consistently
defined across the two files (due to a missing include of `<unistd.h>`)
leading to different struct definitions. The RHEL8 ppc64le LTO buildbot
correctly warned due to this issue.
@colesbury
Copy link
Contributor Author

!buildbot ppc64le

@bedevere-bot
Copy link

The regex 'ppc64le' did not match any buildbot builder.Is the requested builder in the list of stable builders?

@colesbury
Copy link
Contributor Author

!buildbot lto

@bedevere-bot
Copy link

The regex 'lto' did not match any buildbot builder.Is the requested builder in the list of stable builders?

@colesbury colesbury added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Sep 28, 2023
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @colesbury for commit 6e0228b 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Sep 28, 2023
@colesbury
Copy link
Contributor Author

  1. I verified locally that pycore_semaphore.h now consistently defines _PySemaphore (by temporarily added #warning)
  2. https://buildbot.python.org/all/#/builders/256/builds/1238 (ppc64, lto) does not have any warnings related to pycore_semaphore.h. It's not identical to the buildbot running on the main branch, but it's the closest configuration I could find for PRs.

@vstinner
Copy link
Member

The regex 'ppc64le' did not match any buildbot builder.Is the requested builder in the list of stable builders?

Oh, maybe the search is case sensitive and you should use PPC64LE or PPC64LE RHEL8.

@colesbury
Copy link
Contributor Author

!buildbot PPC64LE

@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @colesbury for commit 6e0228b 🤖

The command will test the builders whose names match following regular expression: PPC64LE

The builders matched are:

  • PPC64LE Fedora Stable LTO + PGO PR
  • PPC64LE RHEL8 PR
  • PPC64LE RHEL7 Refleaks PR
  • PPC64LE RHEL7 LTO + PGO PR
  • PPC64LE RHEL8 LTO + PGO PR
  • PPC64LE Fedora Stable Clang Installed PR
  • PPC64LE Fedora Stable LTO PR
  • PPC64LE RHEL7 PR
  • PPC64LE Fedora Stable PR
  • PPC64LE Fedora Stable Refleaks PR
  • PPC64LE RHEL7 LTO PR
  • PPC64LE RHEL8 Refleaks PR
  • PPC64LE Fedora Stable Clang PR
  • PPC64LE RHEL8 LTO PR

@vstinner
Copy link
Member

Oh, maybe the search is case sensitive and you should use PPC64LE or PPC64LE RHEL8.

I wrote python/buildmaster-config#409 to make the search ignores the case.

I got bitten multiple times by the !buildbot command which said nothing. So I added the "The regex 'lto' did not match any buildbot builder.Is the requested builder in the list of stable builders?" message (I just added the missing space ;-)).

@colesbury
Copy link
Contributor Author

Thanks!

@colesbury
Copy link
Contributor Author

@vstinner, would you please review this when you get a chance?

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

Oh wait. The problem is way wider than pycore_semaphore.h! _POSIX_THREADS and _POSIX_SEMAPHORES macros are checked in Python/pthread_thread.h, pycore_condvar.h. and pycore_pythread.h without explicitly including <unistd.h>.

I wrote a wider change to attempt to fix all cases: PR gh-110139.

@vstinner
Copy link
Member

I wrote a more complete fix: PR #110139. Thanks for working on fixing these warnings!

@vstinner vstinner closed this Sep 30, 2023
@colesbury colesbury deleted the issue_110014 branch October 4, 2023 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting core review build The build process and cross-build skip news
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants