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

Jump to bottom

Consider using __forceinline and __attribute__((always_inline)) on static inline functions (Py_INCREF, Py_TYPE) for debug builds #89257

Closed
vstinner opened this issue Sep 3, 2021 · 8 comments
Closed

Consider using __forceinline and __attribute__((always_inline)) on static inline functions (Py_INCREF, Py_TYPE) for debug builds #89257

vstinner opened this issue Sep 3, 2021 · 8 comments
Labels
3.11 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs)

Comments

@vstinner
Copy link
Member

vstinner commented Sep 3, 2021

BPO 45094
Nosy @vstinner, @corona10, @pablogsal, @erlend-aasland, @Fidget-Spinner
PRs
  • bpo-45094: Add Py_NO_INLINE macro #28140
  • bpo-45094: Add Py_ALWAYS_INLINE macro #28141
    		•

    Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
closed_at = <Date 2021-09-07.13:45:12.964>
created_at = <Date 2021-09-03.13:26:39.988>
labels = ['interpreter-core', '3.11']
title = 'Consider using __forceinline and __attribute__((always_inline)) on static inline functions (Py_INCREF, Py_TYPE) for debug builds'
updated_at = <Date 2021-09-07.13:45:12.964>
user = 'https://github.com/vstinner'

    bugs.python.org fields:

    activity = <Date 2021-09-07.13:45:12.964>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2021-09-07.13:45:12.964>
closer = 'vstinner'
components = ['Interpreter Core']
creation = <Date 2021-09-03.13:26:39.988>
creator = 'vstinner'
dependencies = []
files = []
hgrepos = []
issue_num = 45094
keywords = ['patch']
message_count = 8.0
messages = ['400990', '400994', '400995', '400996', '400997', '400999', '401144', '401272']
nosy_count = 5.0
nosy_names = ['vstinner', 'corona10', 'pablogsal', 'erlendaasland', 'kj']
pr_nums = ['28140', '28141']
priority = 'normal'
resolution = 'rejected'
stage = 'resolved'
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue45094'
versions = ['Python 3.11']
    @vstinner
    Copy link
    Member Author

    vstinner commented Sep 3, 2021

    I converted many C macros to static inline functions to reduce the risk of programming bugs (macros pitfalls), limit variable scopes to the function, have a more readable function, and benefit of the function name even when it's inlined in debuggers and profilers.

    When Py_INCREF() macro was converted to a static inline function, using __attribute__((always_inline)) was considered, but the idea was rejected. See bpo-35059.

    I'm now trying to convert the Py_TYPE() macro to a static inline function. The problem is that by default, MSC disables inlining and test_exceptions does crash with a stack overflow, since my PR 28128 increases the usage of the stack memory: see bpo-44348.

    For the specific case of CPython built by MSC, we can increase the stack size, or change compiler optimizations to enable inlining. But the problem is wider than just CPython built by MSC in debug mode. Third party C extensions built by distutils may get the same issue. Building CPython on other platforms on debug mode with all compiler optimizations disabled (ex: gcc -O0) can also have the same issue.

    I propose to reconsider the usage __forceinline (MSC) and __attribute__((always_inline)) (GCC, clang) on the most important static inline functions, like Py_INCREF() and Py_TYPE(), to avoid this issue.

    @vstinner vstinner added 3.11 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) labels Sep 3, 2021
    @vstinner vstinner changed the title Consider using __forceinline and __attribute__((always_inline)) for debug builds Consider using __forceinline and __attribute__((always_inline)) on static inline functions (Py_INCREF, Py_TYPE) for debug builds Sep 3, 2021
    @vstinner vstinner changed the title Consider using __forceinline and __attribute__((always_inline)) for debug builds Consider using __forceinline and __attribute__((always_inline)) on static inline functions (Py_INCREF, Py_TYPE) for debug builds Sep 3, 2021
    @vstinner
    Copy link
    Member Author

    vstinner commented Sep 3, 2021

    New changeset 7974c30 by Victor Stinner in branch 'main':
    bpo-45094: Add Py_NO_INLINE macro (GH-28140)
    7974c30

    3 similar comments
    @vstinner
    Copy link
    Member Author

    vstinner commented Sep 3, 2021

    New changeset 7974c30 by Victor Stinner in branch 'main':
    bpo-45094: Add Py_NO_INLINE macro (GH-28140)
    7974c30

    @vstinner
    Copy link
    Member Author

    vstinner commented Sep 3, 2021

    New changeset 7974c30 by Victor Stinner in branch 'main':
    bpo-45094: Add Py_NO_INLINE macro (GH-28140)
    7974c30

    @vstinner
    Copy link
    Member Author

    vstinner commented Sep 3, 2021

    New changeset 7974c30 by Victor Stinner in branch 'main':
    bpo-45094: Add Py_NO_INLINE macro (GH-28140)
    7974c30

    @corona10
    Copy link
    Member

    corona10 commented Sep 3, 2021

    I like the idea of Py_ALWAYS_INLINE personally if we have to do that.

    @vstinner
    Copy link
    Member Author

    vstinner commented Sep 6, 2021

    Sadly, Py_ALWAYS_INLINE does *not* prevent test_exceptions to crash with my PR 28128 (convert Py_TYPE macro to a static inline function). Even if the Py_TYPE() static inline function is inlined, the stack memory still increases. MSC produces inefficient machine code. It allocates useless variables on the stack which requires more stack memory.

    I propose a different approach: bpo-45115 "Windows: enable compiler optimizations when building Python in debug mode".

    @vstinner
    Copy link
    Member Author

    vstinner commented Sep 7, 2021

    Using __forceinline didn't fix test_exceptions: see https://bugs.python.org/issue44348#msg401185

    Since I created the issue to fix bpo-44348 and it doesn't work, I close the issue.

    Moreover, always inlining can have a negative impact on release builds.

    @vstinner vstinner closed this as completed Sep 7, 2021
    @vstinner vstinner closed this as completed Sep 7, 2021
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    @eduardo-elizondo eduardo-elizondo mentioned this issue Apr 10, 2023
    Merged
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Assignees
    No one assigned
    Labels
    3.11 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs)
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    2 participants
    @vstinner @corona10