Skip to content

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

Closed
Closed
Includes/internal/pycore_code.h uses static_assert() but does not inlcude pymacro.h#123747
Labels
release-blockertype-bugAn unexpected behavior, bug, or error

Description

@dnicolodi
dnicolodi
opened on 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

Activity

  1. dnicolodi
    added
    type-bugAn unexpected behavior, bug, or error
    on Sep 5, 2024

  2. vstinner commented on Sep 5, 2024

    @vstinner
    vstinner
    on Sep 5, 2024
    Member

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

  3. dnicolodi commented on Sep 5, 2024

    @dnicolodi
    dnicolodi
    on Sep 5, 2024
    ContributorAuthor

    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?

  4. vstinner commented on Sep 5, 2024

    @vstinner
    vstinner
    on Sep 5, 2024
    Member

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

  5. dnicolodi commented on Sep 5, 2024

    @dnicolodi
    dnicolodi
    on Sep 5, 2024
    ContributorAuthor

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

  6. vstinner commented on Sep 5, 2024

    @vstinner
    vstinner
    on Sep 5, 2024
    Member

    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.

  7. dnicolodi commented on Sep 5, 2024

    @dnicolodi
    dnicolodi
    on Sep 5, 2024
    ContributorAuthor

    Thanks. There is nothing wrong then.

  8. dnicolodi
    closed this as completedon Sep 5, 2024
  9. picnixz
    closed this as not plannedon Sep 5, 2024
  10. scoder
    added
    release-blocker
    on Sep 6, 2024
  11. scoder
    reopened this on Sep 6, 2024
  12. ezio-melotti
    added this to Release and Deferred blockers 🚫on Sep 6, 2024
  13. github-project-automation
    moved this to Todo in Release and Deferred blockers 🚫on Sep 6, 2024

  14. scoder commented on Sep 6, 2024

    @scoder
    scoder
    on Sep 6, 2024
    Contributor

    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

  15. scoder commented on Sep 6, 2024

    @scoder
    scoder
    on Sep 6, 2024
    Contributor

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

  16. 1 remaining item

  17. picnixz commented on Sep 6, 2024

    @picnixz
    picnixz
    on Sep 6, 2024
    Member

    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.

  18. dnicolodi commented on Sep 6, 2024

    @dnicolodi
    dnicolodi
    on Sep 6, 2024
    ContributorAuthor

    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.

  19. vstinner commented on Sep 6, 2024

    @vstinner
    vstinner
    on Sep 6, 2024
    Member

    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

  20. vstinner
    added 2 commits that reference this issue on Sep 6, 2024

    pythongh-123747: Avoid static_assert() in internal header files

    a496bfa

    pythongh-123747: Avoid static_assert() in internal header files

    c1f05e9

  21. vstinner commented on Sep 6, 2024

    @vstinner
    vstinner
    on Sep 6, 2024
    Member

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

  22. encukou commented on Sep 6, 2024

    @encukou
    encukou
    on Sep 6, 2024
    Member

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

  23. vstinner
    added 2 commits that reference this issue on Sep 6, 2024

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

    ef4b69d

    pythongh-123747: Avoid static_assert() in internal header files (pyth…

    5ad0120
  24. Yhg1s
    added a commit that references this issue on Sep 6, 2024

    [3.13] gh-123747: Avoid static_assert() in internal header files (#12…

    d053204

  25. vstinner commented on Sep 6, 2024

    @vstinner
    vstinner
    on Sep 6, 2024
    Member

    Fixed by ef4b69d.

  26. vstinner
    closed this as completedon Sep 6, 2024
  27. github-project-automation
    moved this from Todo to Done in Release and Deferred blockers 🚫on Sep 6, 2024

  28. vstinner commented on Sep 6, 2024

    @vstinner
    vstinner
    on Sep 6, 2024
    Member

    Thanks for your bug report @dnicolodi!

  29. scoder commented on Sep 6, 2024

    @scoder
    scoder
    on Sep 6, 2024
    Contributor

    Thanks @vstinner

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    release-blockertype-bugAn unexpected behavior, bug, or error

    Projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions