GeezerVison

Copy link

Copy Markdown

Member

avloss reacted with eyes emoji

markshannon requested a review from mdickinson January 24, 2023 17:23

markshannon requested review from rhettinger and tiran as code owners January 24, 2023 17:23

mdickinson approved these changes Jan 29, 2023

View reviewed changes

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

…r and PyLongValue struct. (pythonGH-101292)

Needed after python/cpython#101292

See cython#5238

Needed after python/cpython#101292

See cython#5238

Needed after python/cpython#101292

See #5238

markshannon deleted the split-long branch September 26, 2023 12:55

- generated c files are outdated due to [GH-101291](python/cpython#101292)

- generated c files are outdated due to [GH-101291](python/cpython#101292)

- generated c files are outdated due to [GH-101291](python/cpython#101292)

- generated c files are outdated due to [GH-101291](python/cpython#101292)

…,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 <[email protected]>