Skip to content
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

gh-99069: Consolidate checks for static_assert #94766

Merged
merged 9 commits into from
Apr 5, 2023

Conversation

jmroot
Copy link
Contributor

@jmroot jmroot commented Jul 12, 2022

Several platforms don't define the static_assert macro despite having
compiler support for the _Static_assert keyword. The macro needs to be
defined since it is used unconditionally in the Python code. So it
should always be safe to define it if undefined and not in C++11 (or
later) mode.

Hence, remove the checks for particular platforms or libc versions,
and just define static_assert anytime it needs to be defined but isn't.
That way, all platforms that need the fix will get it, regardless of
whether someone specifically thought of them.

Also document that certain macOS versions are among the platforms that
need this.

@jmroot jmroot changed the title gh-91731: Consolidate checks for static_assert gh-99069: Consolidate checks for static_assert Nov 3, 2022
Include/pymacro.h Outdated Show resolved Hide resolved
@jmroot
Copy link
Contributor Author

jmroot commented Nov 7, 2022

@vstinner Is there something that is defined iff Python itself is being built? Other users of the headers don't necessarily need static_assert, so it would be nice to avoid defining it as an unexpected side effect. I guess that would allow removing the __cplusplus checks too, since Python won't be built as C++?

@vstinner
Copy link
Member

vstinner commented Nov 7, 2022

Sadly, static_assert() is now part of the Python 3.11 C API. Removing it from its public C API in a 3.11.x bugfix release would be worse IMO.

@vstinner
Copy link
Member

vstinner commented Nov 7, 2022

In practice, which platform/libc/compiler is impacted by this change? It would be nice to add a NEWS entry to discuss which platforms are affected.

@vstinner
Copy link
Member

vstinner commented Nov 7, 2022

cc @pablogsal who modified this macro recently.

cc @ambv who may have an opinion on the macOS 10.10 support (issue #99069), purpose of this PR if I got it correctly

@jmroot
Copy link
Contributor Author

jmroot commented Nov 7, 2022

In practice, which platform/libc/compiler is impacted by this change?

I only know of the ones mentioned in the comment, i.e. FreeBSD <= 12, macOS <= 10.10, and glibc < 2.16.

@vstinner
Copy link
Member

vstinner commented Nov 7, 2022

I was very excited to be able to get rid of Py_BUILD_ASSERT() "hack" with a well defined (!) static_assert() function of the C11 standard. But month after month, I see that: weeeeeell, it depends, maybe it's available, maybe not. It depends on the libc, on the compiler, on the OS, etc.

Maybe we should give up with static_assert() and use _Static_assert(). Coming from Python, for me it's weird to use a name starting with an underscore :-( It reminds me non-standard APIs on Windows which looks like POSIX but are different, like _open().

Maybe Py_BUILD_ASSERT() is just better since it's battle tested (in Python) and it "just works" :-) I didn't expect that it would be so complicated to get static_assert() on all compilers on all platforms in 2022 :-(

A tradeoff might be to only use static_assert() inside Python and remove it from the public Python C API. Then the funny part is that depending if the proper (hypothetical) Python internal header file is included or not, using static_assert() might work or not. The problem is that some Python macros use assert(), so Python.h includes <assert.h>. But as you can see, static_assert() may or may not be available, Python currently has a complicated macro to make sure that it's always present. What if an internal Python C file uses <Python.h> but forgets to include "pycore_assert.h"? Maybe it works on all platforms. Maybe it fails on some specific platforms. Fun. Isn't it? That's why I put it in the public C API.


"Well defined": people are now discussing static_assert() with a single argument. I'm curious when it will be standard and when all compilers on all platforms will support it :-)

@jmroot
Copy link
Contributor Author

jmroot commented Nov 20, 2022

@vstinner @pablogsal Is there a consensus for how you want to proceed here?

@vstinner
Copy link
Member

@pablogsal asked to add __has_feature(c_static_assert) check. I don't see this check in your PR yet. Does it mean that you are against using it, and if yes, why?

@jmroot
Copy link
Contributor Author

jmroot commented Nov 21, 2022

@vstinner Sorry if I misinterpreted your comments above; it looked to me like you were disagreeing with @pablogsal's proposal.

My own view is that a clang-specific check is not necessary, because the existence of _Static_assert does not depend on the stdlib at all, and all versions of clang that accept -std=c11 or -std=c1x provide that keyword. Restricting the define to clang and gcc would be reasonable though, just in case other compilers do something weird.

Several platforms don't define the static_assert macro despite having
compiler support for the _Static_assert keyword. The macro needs to be
defined since it is used unconditionally in the Python code. So it
should always be safe to define it if undefined and not in C++11 (or
later) mode.

Hence, remove the checks for particular platforms or libc versions,
and just define static_assert anytime it needs to be defined but isn't.
That way, all platforms that need the fix will get it, regardless of
whether someone specifically thought of them.

Also document that certain macOS versions are among the platforms that
need this.
@jmroot
Copy link
Contributor Author

jmroot commented Nov 29, 2022

@pablogsal Is this acceptable now?

@pablogsal
Copy link
Member

Yup, this looks good to me. Thanks for working on this!

Include/pymacro.h Outdated Show resolved Hide resolved
Include/pymacro.h Outdated Show resolved Hide resolved
@jmroot jmroot requested a review from vstinner December 3, 2022 13:52
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.

For me, it's strange to check for GNUC with clang. The first code was added for FreeBSD which uses clang.

Include/pymacro.h Outdated Show resolved Hide resolved
@jmroot
Copy link
Contributor Author

jmroot commented Jan 1, 2023

@vstinner What is still needed for this to be ready to merge?

blurb-it bot and others added 2 commits February 11, 2023 05:31
The C2x draft (currently expected to become C23) makes static_assert
a keyword to match C++. So only define the macro for up to C17.
@vstinner vstinner merged commit 96e1901 into python:main Apr 5, 2023
@vstinner vstinner added the needs backport to 3.11 only security fixes label Apr 5, 2023
@miss-islington
Copy link
Contributor

Thanks @jmroot for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Apr 5, 2023
Several platforms don't define the static_assert macro despite having
compiler support for the _Static_assert keyword. The macro needs to be
defined since it is used unconditionally in the Python code. So it
should always be safe to define it if undefined and not in C++11 (or
later) mode.

Hence, remove the checks for particular platforms or libc versions,
and just define static_assert anytime it needs to be defined but isn't.
That way, all platforms that need the fix will get it, regardless of
whether someone specifically thought of them.

Also document that certain macOS versions are among the platforms that
need this.

The C2x draft (currently expected to become C23) makes static_assert
a keyword to match C++. So only define the macro for up to C17.

(cherry picked from commit 96e1901)

Co-authored-by: Joshua Root <jmr@macports.org>
Co-authored-by: Victor Stinner <vstinner@python.org>
@bedevere-bot
Copy link

GH-103282 is a backport of this pull request to the 3.11 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.11 only security fixes label Apr 5, 2023
@vstinner
Copy link
Member

vstinner commented Apr 5, 2023

Merged, thanks.

miss-islington added a commit that referenced this pull request Apr 5, 2023
Several platforms don't define the static_assert macro despite having
compiler support for the _Static_assert keyword. The macro needs to be
defined since it is used unconditionally in the Python code. So it
should always be safe to define it if undefined and not in C++11 (or
later) mode.

Hence, remove the checks for particular platforms or libc versions,
and just define static_assert anytime it needs to be defined but isn't.
That way, all platforms that need the fix will get it, regardless of
whether someone specifically thought of them.

Also document that certain macOS versions are among the platforms that
need this.

The C2x draft (currently expected to become C23) makes static_assert
a keyword to match C++. So only define the macro for up to C17.

(cherry picked from commit 96e1901)

Co-authored-by: Joshua Root <jmr@macports.org>
Co-authored-by: Victor Stinner <vstinner@python.org>
@jmroot jmroot deleted the static_assert branch April 5, 2023 18:53
gaogaotiantian pushed a commit to gaogaotiantian/cpython that referenced this pull request Apr 8, 2023
Several platforms don't define the static_assert macro despite having
compiler support for the _Static_assert keyword. The macro needs to be
defined since it is used unconditionally in the Python code. So it
should always be safe to define it if undefined and not in C++11 (or
later) mode.

Hence, remove the checks for particular platforms or libc versions,
and just define static_assert anytime it needs to be defined but isn't.
That way, all platforms that need the fix will get it, regardless of
whether someone specifically thought of them.

Also document that certain macOS versions are among the platforms that
need this.

The C2x draft (currently expected to become C23) makes static_assert
a keyword to match C++. So only define the macro for up to C17.

Co-authored-by: Victor Stinner <vstinner@python.org>
warsaw pushed a commit to warsaw/cpython that referenced this pull request Apr 11, 2023
Several platforms don't define the static_assert macro despite having
compiler support for the _Static_assert keyword. The macro needs to be
defined since it is used unconditionally in the Python code. So it
should always be safe to define it if undefined and not in C++11 (or
later) mode.

Hence, remove the checks for particular platforms or libc versions,
and just define static_assert anytime it needs to be defined but isn't.
That way, all platforms that need the fix will get it, regardless of
whether someone specifically thought of them.

Also document that certain macOS versions are among the platforms that
need this.

The C2x draft (currently expected to become C23) makes static_assert
a keyword to match C++. So only define the macro for up to C17.

Co-authored-by: Victor Stinner <vstinner@python.org>
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.

5 participants