Hi Dave,
Just wanted to give you and everyone else a short update on how
reference count checking is going — we can now observe the refcnt
diagnostic being emitted:
rc3.c:22:10: warning: REF COUNT PROBLEM
22 | return list;
| ^~~~
‘create_py_object’: events 1-4
|
|4 | PyObject* item = PyLong_FromLong(3);
| |^~
| ||
| |(1) when ‘PyLong_FromLong’ succeeds
|5 | PyObject* list = PyList_New(1);
| |~
| ||
| |(2) when ‘PyList_New’ succeeds
|..
| 14 | PyList_Append(list, item);
| | ~
| | |
| | (3) when ‘PyList_Append’ fails
|..
| 22 | return list;
| |
| | |
| | (4) here
|
I will fix up and refactor the logic for counting the actual ref count
before coming back and refining the diagnostic to give much more
detailed information.
Best,
Eric
On Wed, Aug 16, 2023 at 9:47 PM Eric Feng wrote:
>
> Hi Dave,
>
> Thanks for the feedback!
>
>
> On Wed, Aug 16, 2023 at 5:29 PM David Malcolm wrote:
> >
> > On Wed, 2023-08-16 at 15:17 -0400, Eric Feng via Gcc wrote:
> > > Hi everyone,
> >
> > [fixing typo in my email address]
> >
> > Hi Eric, thanks for the update, and the WIP patch.
> >
> > >
> > > After pushing the code that supports various known function classes last
> > > week,
> > > I've turned my attention back to the core reference count checking
> > > functionality. This functionality used to reside in region_model, which
> > > wasn't ideal. To address this, I've introduced a hook to register
> > > callbacks
> > > to pop_frame. Specifically, this allows the code that checks the reference
> > > count and emits diagnostics to be housed within the plugin, rather than
> > > the
> > > core analyzer.
> > >
> > > As of now, the parameters of pop_frame_callback are tailored specifically
> > > to
> > > our needs. If the use of callbacks at the end of pop_frame becomes more
> > > prevalent, we can revisit the setup to potentially make it more general.
> > >
> > > Moreover, the core reference count checking logic was previously somewhat
> > > bloated, contained in one extensive function. I've since refactored it,
> > > breaking it down into several helper functions to simplify and reduce
> > > complexity. There are still some aspects that need refinement, especially
> > > since the plugin has seen changes since I last worked on this logic.
> > > However,
> > > I believe that there aren't any significant problems.
> >
> > Suggestion: introduce some more decls into analyzer-decls.h and
> > known_functions for them into the plugin so that you can run/test/debug
> > the helper functions independently (similar to the existing ones in kf-
> > analyzer.cc).
> >
> > e.g.
> > extern void __analyzer_cpython_dump_real_refcounts (void);
> > extern void __analyzer_cpython_dump_ob_refcnt (void);
> >
> > >
> Thanks for the suggestion. This will be even more helpful now that we
> have split the logic into helper functions. I will look into these
> when I come back to the "is there a refcount problem" side of the
> equation.
> > > Currently, I've started working a custom stmt_finder similar to
> > > leak_stmt_finder
> > > to address the issue of m_stmt and m_stmt_finder being NULL at the time of
> > > region_model::pop_frame. This approach was discussed as a viable solution
> > > in
> > > a previous email, and I'll keep everyone posted on my progress.
> > > Afterwards, I
> > > will go back to address the refinements necessary mentioned above.
> >
> > You might want to experiment with splitting out
> > (a) "is there a refcount problem" from
> > (b) "emit a refcount problem".
> >
> > For example, you could hardcode (a) to true, so we always complain with
> > (b) on every heap-allocated object, just to debug the stmt_finder
> > workaround.
> >
> >
> > [...snip...]
> >
> > BTW, you don't need to bother to write ChangeLog entries if you're just
> > sending a work-in-progress for me.
> >
> > > diff --git a/gcc/testsuite/gcc.dg/plugin/analyzer_cpython_plugin.c
> > > b/gcc/testsuite/gcc.dg/plugin/analyzer_cpython_plugin.c
> > > index 7cd72e8a886..918bb5a5587 100644
> > > --- a/gcc/testsuite/gcc.dg/plugin/analyzer_cpython_plugin.c
> > > +++ b/gcc/testsuite/gcc.dg/plugin/analyzer_cpython_plugin.c
> >
> > [...]
> >
> > > +/* For PyListObjects: processes the ob_item field within the current
> > > region and
> > > + * increments the reference count if conditions are met. */
> > > +void
> > > +process_ob_item_region (const region_model *model, region_model_manager
> > > *mgr,
> > > + region_model_context *ctxt, const region
> > > *curr_region,
> > > + const svalue *pylist_type_ptr, const region
> > > *
