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

Includes/internal/pycore_code.h uses static_assert() but does not inlcude pymacro.h #123747

Closed
dnicolodi opened this issue Sep 5, 2024 · 17 comments
Labels
release-blocker type-bug An unexpected behavior, bug, or error

Comments

@dnicolodi
Copy link

dnicolodi commented Sep 5, 2024

Bug report

Bug description:

Includes/internal/pycore_code.h uses static_assert() but does not inlcude pymacro.h, should it?

AFAIU pymacro.h makes sure that static_assert() is correctly defined for all supported compilers and platforms. Not including it in Includes/internal/pycore_code.h implicitly relies on pymacro.h being included before or via transitive includes.

I've found this while investigating a Cython extension module build failure. Cython includes the private header and thus requires static_macro() to be defined.

CPython versions tested on:

3.13, CPython main branch

Operating systems tested on:

No response

Linked PRs

@dnicolodi dnicolodi added the type-bug An unexpected behavior, bug, or error label Sep 5, 2024
@vstinner
Copy link
Member

vstinner commented Sep 5, 2024

In general, I would suggest to only include <pycore_xxx.h> headers after including <Python.h>.

@dnicolodi
Copy link
Author

Sure, the compilation error is unrelated (Cython requires only C99 which does not have static_assert()). This report is more about a question of principle: should CPython internal headers be independent or can they rely on #include order for the definitions they use?

@vstinner
Copy link
Member

vstinner commented Sep 5, 2024

Do you get the error if you include <Python.h> first?

@dnicolodi
Copy link
Author

Yes. But the error is unrelated to what I am trying to say.

@vstinner
Copy link
Member

vstinner commented Sep 5, 2024

This report is more about a question of principle: should CPython internal headers be independent or can they rely on #include order for the definitions they use?

They are not independent. <Python.h> must always be included first.

@dnicolodi
Copy link
Author

Thanks. There is nothing wrong then.

@scoder
Copy link
Contributor

scoder commented Sep 6, 2024

It turns out that this is an issue, even after #include "Python.h":
cython/cython#6385 (comment)
This issue was found in Cython generated code, which always includes Python.h but then also needs pycore_frame.h later, which includes pycore_code.h.

Since Cython code is not commonly compiled with C11 (we currently require C99), there are platforms where the resulting code does not compile

@scoder
Copy link
Contributor

scoder commented Sep 6, 2024

Since it is really just an assertion that gets in the way here, I propose to comment it out for the final release.

@vstinner
Copy link
Member

vstinner commented Sep 6, 2024

I don't understand how to reproduce the issue. Can somone provide a reproducer? Does the issue only affect macOS? With which C compiler and which compiler options?

@picnixz
Copy link
Member

picnixz commented Sep 6, 2024

FTR, here's the comment concerning static_assert (it may help people that are too lazy to find it in the codebase):

// gh-91782: On FreeBSD 12, if the _POSIX_C_SOURCE and _XOPEN_SOURCE macros are
// defined, <sys/cdefs.h> disables C11 support and <assert.h> does not define
// the static_assert() macro.
// https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=255290
//
// macOS <= 10.10 doesn't define static_assert in assert.h at all despite
// having C11 compiler support.
//
// static_assert is defined in glibc from version 2.16. Compiler support for
// the C11 _Static_assert keyword is in gcc >= 4.6.
//
// MSVC makes static_assert a keyword in C11-17, contrary to the standards.
//
// In C++11 and C2x, static_assert is a keyword, redefining is undefined
// behaviour. So only define if building as C, not C++ (if __cplusplus is
// not defined), and only for C11-17.

@dnicolodi
Copy link
Author

I don't understand how to reproduce the issue. Can somone provide a reproducer? Does the issue only affect macOS? With which C compiler and which compiler options?

I can reproduce the issue compiling the most simple Cython extension module:

def foo():
    return 42

with Cython 3.0.11 with clang from Xtools:

$ cc --version
Apple clang version 14.0.0 (clang-1400.0.29.202)

using an explicit -std=c99 compilation option.

A more synthetic reproducer is trying to compile something like:

#include "Python.h"
#include "internal/pycore_frame.h"
int main() { }

In this way:

$ cc foo.c -o foo.o -DPy_BUILD_CORE=1 $(python3.13-config --cflags) -std=c99

Omitting the -std=c99 or setting it to -std=c11 or higher the snippet compiles.

As pointed out before, the issue is that static_assert() is a C11 concept. CPython requires C11 thus it is fine. However, Cython requires only C99 and it includes internal CPython headers which assume C11 features.

For some reason that I have not investigated, static_assert() is defined when compiling with GCC on Linux or MSVC on Windows when requesting C99 compliance.

@vstinner
Copy link
Member

vstinner commented Sep 6, 2024

Apple clang version 14.0.0 (clang-1400.0.29.202)

I cannot reproduce the issue on Linux (Fedora 40) with clang 18.1.6.

See also discussion capi-workgroup/decisions#30

@vstinner
Copy link
Member

vstinner commented Sep 6, 2024

I wrote PR gh-123779 to avoid static_assert() in the internal header files.

@encukou
Copy link
Member

encukou commented Sep 6, 2024

Could Cython re-#define static_assert(EXPR, MSG) to nothing if compiling in C99 mode?

vstinner added a commit to vstinner/cpython that referenced this issue Sep 6, 2024
Yhg1s pushed a commit that referenced this issue Sep 6, 2024
…3779) (#123785)

gh-123747: Avoid static_assert() in internal header files (#123779)

(cherry picked from commit ef4b69d)
@vstinner
Copy link
Member

vstinner commented Sep 6, 2024

Fixed by ef4b69d.

@vstinner
Copy link
Member

vstinner commented Sep 6, 2024

Thanks for your bug report @dnicolodi!

@scoder
Copy link
Contributor

scoder commented Sep 6, 2024

Thanks @vstinner

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-blocker type-bug An unexpected behavior, bug, or error
Projects
Development

No branches or pull requests

5 participants