{{ message }}
GH-101291: Refactor the PyLongObject struct - #101292
Merged
Merged
Conversation
mdickinson
approved these changes
Jan 29, 2023
mdickinson
left a comment
Member
There was a problem hiding this comment.
Changes seem fine to me, though it's a little bit disturbing to see how much the longobject internals have leaked into the rest of the codebase.
Have we looked into the possible impact of these changes on 3rd party libraries? Most shouldn't care, but I'd imagine it'll affect Cython at least, and possibly also things like NumPy and gmpy2.
Member
Author
mdboom
pushed a commit
to mdboom/cpython
that referenced
this pull request
Jan 31, 2023
…r and PyLongValue struct. (pythonGH-101292)
Contributor
Does indeed hit cython! Can not tell if it hits numpy independent of cython because the cythonized files fail to build. |
This was referenced Feb 7, 2023
scoder
added a commit
to scoder/cython
that referenced
this pull request
Feb 13, 2023
Needed after python/cpython#101292 See cython#5238
scoder
added a commit
to scoder/cython
that referenced
this pull request
Feb 13, 2023
Needed after python/cpython#101292 See cython#5238
scoder
added a commit
to cython/cython
that referenced
this pull request
Feb 13, 2023
Needed after python/cpython#101292 See #5238
This was referenced Mar 1, 2023
Member
This was referenced Jul 3, 2023
MartinNowak
added a commit
to 7Learnings/ssh-python
that referenced
this pull request
Jun 25, 2024
- generated c files are outdated due to [GH-101291](python/cpython#101292)
EliusSolis
added a commit
to EliusSolis/charm
that referenced
this pull request
Jul 28, 2024
MartinNowak
added a commit
to 7Learnings/ssh-python
that referenced
this pull request
Dec 26, 2024
- generated c files are outdated due to [GH-101291](python/cpython#101292)
MartinNowak
added a commit
to 7Learnings/ssh-python
that referenced
this pull request
Dec 27, 2024
- generated c files are outdated due to [GH-101291](python/cpython#101292)
MartinNowak
added a commit
to 7Learnings/ssh2-python
that referenced
this pull request
Dec 27, 2024
- generated c files are outdated due to [GH-101291](python/cpython#101292)
jakinyele
pushed a commit
to JHUISI/charm
that referenced
this pull request
Jan 23, 2025
hubot
pushed a commit
to gcc-mirror/gcc
that referenced
this pull request
Mar 7, 2026
…,PR112520] In GCC 14 the testsuite gained a plugin that "teaches" the analyzer about the CPython API, trying for find common mistakes: https://gcc.gnu.org/wiki/StaticAnalyzer/CPython Unfortunately, this has been crashing for more recent versions of CPython. Specifically, in Python 3.11, PyObject's ob_refcnt was moved to an anonymous union (as part of PEP 683 "Immortal Objects, Using a Fixed Refcount"). The plugin attempts to find the field but fails, but has no error-handling, leading to a null pointer dereference. Also, python/cpython#101292 moved the "ob_digit" from struct _longobject to a new field long_value of a new struct _PyLongValue, leading to similar analyzer crashes when not finding the field. The following patch fixes this by * looking within the anonymous union for the ob_refcnt field if it can't find it directly * gracefully handling the case of not finding "ob_digit" in PyLongObject * doing more lookups once at plugin startup, rather than continuously on analyzing API calls * adding diagnostics and more error-handling to the plugin startup, so that if it can't find something in the Python headers it emits a useful note when disabling itself, e.g. cc1: note: could not find field 'ob_digit' of CPython type 'PyLongObject' {aka 'struct _longobject'} * replacing some copy-and-pasted code with member functions of a new "class api" (though various other cleanups could be done) Tested with: * CPython 3.8: all tests continue to PASS * CPython 3.13: fixes the ICEs, 2 FAILs remain (reference counting false negatives) Given that this is already a large patch, I'm opting to only fix the crashes and defer the 2 remainings FAILs and other cleanups to followup work. gcc/analyzer/ChangeLog: PR testsuite/112520 * region-model-manager.cc (region_model_manager::get_field_region): Assert that the args are non-null. gcc/testsuite/ChangeLog: PR analyzer/107646 PR testsuite/112520 * gcc.dg/plugin/analyzer_cpython_plugin.cc: Move everything from namespace ana:: into ana::cpython_plugin. Move global tree values into a new "class api". (pyobj_record): Replace with api.m_type_PyObject. (pyobj_ptr_tree): Replace with api.m_type_PyObject_ptr. (pyobj_ptr_ptr): Replace with api.m_type_PyObject_ptr_ptr. (varobj_record): Replace with api.m_type_PyVarObject. (pylistobj_record): Replace with api.m_type_PyListObject. (pylongobj_record): Replace with api.m_type_PyLongObject. (pylongtype_vardecl): Replace with api.m_vardecl_PyLong_Type. (pylisttype_vardecl): Replace with api.m_vardecl_PyList_Type. (get_field_by_name): Add "complain" param and use it to issue a note on failure. Assert that type and name are non-null. Don't crash on fields that are anonymous unions, and special-case looking within them for "ob_refcnt" to work around the Python 3.11 change for PEP 683 (immortal objects). (get_sizeof_pyobjptr): Convert to... (api::get_sval_sizeof_PyObject_ptr): ...this (init_ob_refcnt_field): Convert to... (api::init_ob_refcnt_field): ...this. (set_ob_type_field): Convert to... (api::set_ob_type_field): ..this. (api::init_PyObject_HEAD): New. (api::get_region_PyObject_ob_refcnt): New. (api::do_Py_INCREF): New. (api::get_region_PyVarObject_ob_size): New. (api::get_region_PyLongObject_ob_digit): New. (inc_field_val): Convert to... (api::inc_field_val): ...this. (refcnt_mismatch::refcnt_mismatch): Add tree params for refcounts and initialize corresponding fields. Fix whitespace. (refcnt_mismatch::emit): Use stored tree values, rather than assuming we have constants, and crashing non-constants. Delete commented-out dead code. (refcnt_mismatch::foo): Delete. (refcnt_mismatch::m_expected_refcnt_tree): New field. (refcnt_mismatch::m_actual_refcnt_tree): New field. (retrieve_ob_refcnt_sval): Simplify using class api. (count_pyobj_references): Likewise. (check_refcnt): Likewise. Don't warn on UNKNOWN values. Use get_representative_tree for the expected and actual values and skip the warning if it fails, rather than assuming we have constants and crashing on non-constants. (count_all_references): Update comment. (kf_PyList_Append::impl_call_pre): Simplify using class api. (kf_PyList_Append::impl_call_post): Likewise. (kf_PyList_New::impl_call_post): Likewise. (kf_PyLong_FromLong::impl_call_post): Likewise. (get_stashed_type_by_name): Emit note if the type couldn't be found. (get_stashed_global_var_by_name): Likewise for globals. (init_py_structs): Convert to... (api::init_from_stashed_types): ...this. Bail out with an error code if anything fails. Look up more things at startup, rather than during analysis of calls. (ana::cpython_analyzer_events_subscriber): Rename to... (ana::cpython_plugin::analyzer_events_subscriber): ...this. (analyzer_events_subscriber::analyzer_events_subscriber): Initialize m_init_failed. (analyzer_events_subscriber::on_message<on_tu_finished>): Update for conversion of init_py_structs to api::init_from_stashed_types and bail if it fails. (analyzer_events_subscriber::on_message<on_frame_popped): Don't run if plugin initialization failed. (analyzer_events_subscriber::m_init_failed): New field. Signed-off-by: David Malcolm <dmalcolm@redhat.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

There is no change to the implementation, apart from the refactoring of the struct. Even the binary layout remains the same.
This is the first step in improving int performance. The real changes will come later, but this PR will help keep later PR small and manageable.