analyzer: implement reference count checking for CPython plugin [PR107646]
Checks
Commit Message
Additionally, by using the old model and the pointer per your suggestion,
we are able to find the representative tree and emit a more accurate diagnostic!
rc3.c:23:10: warning: expected ‘item’ to have reference count: ‘1’ but ob_refcnt field is: ‘2’
23 | 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’ succeeds, moving buffer
|......
| 23 | return list;
| | ~~~~
| | |
| | (4) here
|
If a representative tree is not found, I decided we should just bail out
of emitting a diagnostic for now, to avoid confusing the user on what
the problem is.
I've attached the patch for this (on top of the previous one) below. If
it also looks good, I can merge it with the last patch and push it in at
the same time.
Best,
Eric
---
gcc/analyzer/region-model.cc | 3 +-
gcc/analyzer/region-model.h | 7 ++--
.../gcc.dg/plugin/analyzer_cpython_plugin.c | 35 +++++++++++--------
3 files changed, 27 insertions(+), 18 deletions(-)
Comments
On Tue, 2023-08-29 at 13:28 -0400, Eric Feng wrote:
> Additionally, by using the old model and the pointer per your
> suggestion,
> we are able to find the representative tree and emit a more accurate
> diagnostic!
>
> rc3.c:23:10: warning: expected ‘item’ to have reference count: ‘1’
> but ob_refcnt field is: ‘2’
> 23 | 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’ succeeds, moving buffer
> |......
> | 23 | return list;
> | | ~~~~
> | | |
> | | (4) here
> |
Excellent, that's a big improvement.
>
> If a representative tree is not found, I decided we should just bail
> out
> of emitting a diagnostic for now, to avoid confusing the user on what
> the problem is.
Fair enough.
>
> I've attached the patch for this (on top of the previous one) below.
> If
> it also looks good, I can merge it with the last patch and push it in
> at
> the same time.
I don't mind either way, but please can you update the tests so that we
have some automated test coverage that the correct name is being
printed in the warning.
Thanks
Dave
On Tue, Aug 29, 2023 at 5:14 PM David Malcolm <dmalcolm@redhat.com> wrote:
>
> On Tue, 2023-08-29 at 13:28 -0400, Eric Feng wrote:
> > Additionally, by using the old model and the pointer per your
> > suggestion,
> > we are able to find the representative tree and emit a more accurate
> > diagnostic!
> >
> > rc3.c:23:10: warning: expected ‘item’ to have reference count: ‘1’
> > but ob_refcnt field is: ‘2’
> > 23 | 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’ succeeds, moving buffer
> > |......
> > | 23 | return list;
> > | | ~~~~
> > | | |
> > | | (4) here
> > |
>
> Excellent, that's a big improvement.
>
> >
> > If a representative tree is not found, I decided we should just bail
> > out
> > of emitting a diagnostic for now, to avoid confusing the user on what
> > the problem is.
>
> Fair enough.
>
> >
> > I've attached the patch for this (on top of the previous one) below.
> > If
> > it also looks good, I can merge it with the last patch and push it in
> > at
> > the same time.
>
> I don't mind either way, but please can you update the tests so that we
> have some automated test coverage that the correct name is being
> printed in the warning.
>
> Thanks
> Dave
>
Sorry — forgot to hit 'reply all' in the previous e-mail. Resending to
preserve our chain on the list:
---
Thanks; pushed to trunk with nits fixed:
https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=597b9ec69bca8acb7a3d65641c0a730de8b27ed4.
Incidentally, I updated my formatting settings in VSCode, which I've
previously mentioned in passing. In case anyone is interested:
"C_Cpp.clang_format_style": "{ BasedOnStyle: GNU, UseTab: Always,
TabWidth: 8, IndentWidth: 2, BinPackParameters: false,
AlignAfterOpenBracket: Align,
AllowAllParametersOfDeclarationOnNextLine: true }",
This fixes some issues with the indent width and also ensures function
parameters of appropriate length are aligned properly and on a new
line each (like the rest of the analyzer code).
On Wed, 2023-08-30 at 18:15 -0400, Eric Feng wrote:
> On Tue, Aug 29, 2023 at 5:14 PM David Malcolm <dmalcolm@redhat.com>
> wrote:
> >
> > On Tue, 2023-08-29 at 13:28 -0400, Eric Feng wrote:
> > > Additionally, by using the old model and the pointer per your
> > > suggestion,
> > > we are able to find the representative tree and emit a more
> > > accurate
> > > diagnostic!
> > >
> > > rc3.c:23:10: warning: expected ‘item’ to have reference count:
> > > ‘1’
> > > but ob_refcnt field is: ‘2’
> > > 23 | 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’ succeeds, moving buffer
> > > |......
> > > | 23 | return list;
> > > | | ~~~~
> > > | | |
> > > | | (4) here
> > > |
> >
> > Excellent, that's a big improvement.
> >
> > >
> > > If a representative tree is not found, I decided we should just
> > > bail
> > > out
> > > of emitting a diagnostic for now, to avoid confusing the user on
> > > what
> > > the problem is.
> >
> > Fair enough.
> >
> > >
> > > I've attached the patch for this (on top of the previous one)
> > > below.
> > > If
> > > it also looks good, I can merge it with the last patch and push
> > > it in
> > > at
> > > the same time.
> >
> > I don't mind either way, but please can you update the tests so
> > that we
> > have some automated test coverage that the correct name is being
> > printed in the warning.
> >
> > Thanks
> > Dave
> >
>
> Sorry — forgot to hit 'reply all' in the previous e-mail. Resending
> to
> preserve our chain on the list:
>
> ---
>
> Thanks; pushed to trunk with nits fixed:
> https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=597b9ec69bca8acb7a3d65641c0a730de8b27ed4
> .
Thanks; looks good.
Do you want to add this to the GCC 14 part of the "History" section on
the wiki page:
https://gcc.gnu.org/wiki/StaticAnalyzer
or should I?
>
> Incidentally, I updated my formatting settings in VSCode, which I've
> previously mentioned in passing. In case anyone is interested:
>
> "C_Cpp.clang_format_style": "{ BasedOnStyle: GNU, UseTab: Always,
> TabWidth: 8, IndentWidth: 2, BinPackParameters: false,
> AlignAfterOpenBracket: Align,
> AllowAllParametersOfDeclarationOnNextLine: true }",
>
> This fixes some issues with the indent width and also ensures
> function
> parameters of appropriate length are aligned properly and on a new
> line each (like the rest of the analyzer code).
Thanks
Dave
@@ -5391,6 +5391,7 @@ region_model::pop_frame (tree result_lvalue,
{
gcc_assert (m_current_frame);
+ const region_model pre_popped_model = *this;
const frame_region *frame_reg = m_current_frame;
/* Notify state machines. */
@@ -5424,7 +5425,7 @@ region_model::pop_frame (tree result_lvalue,
}
unbind_region_and_descendents (frame_reg,POISON_KIND_POPPED_STACK);
- notify_on_pop_frame (this, retval, ctxt);
+ notify_on_pop_frame (this, &pre_popped_model, retval, ctxt);
}
/* Get the number of frames in this region_model's stack. */
@@ -237,6 +237,7 @@ public:
struct append_regions_cb_data;
typedef void (*pop_frame_callback) (const region_model *model,
+ const region_model *prev_model,
const svalue *retval,
region_model_context *ctxt);
@@ -543,11 +544,13 @@ class region_model
}
static void
- notify_on_pop_frame (const region_model *model, const svalue *retval,
+ notify_on_pop_frame (const region_model *model,
+ const region_model *prev_model,
+ const svalue *retval,
region_model_context *ctxt)
{
for (auto &callback : pop_frame_callbacks)
- callback (model, retval, ctxt);
+ callback (model, prev_model, retval, ctxt);
}
private:
@@ -318,11 +318,10 @@ public:
auto actual_refcnt
= m_actual_refcnt->dyn_cast_constant_svalue ()->get_constant ();
auto ob_refcnt = m_ob_refcnt->dyn_cast_constant_svalue ()->get_constant ();
- warned = warning_meta (
- rich_loc, m, get_controlling_option (),
- "expected <variable name belonging to m_base_region> to have "
- "reference count: %qE but ob_refcnt field is: %qE",
- actual_refcnt, ob_refcnt);
+ warned = warning_meta (rich_loc, m, get_controlling_option (),
+ "expected %qE to have "
+ "reference count: %qE but ob_refcnt field is: %qE",
+ m_reg_tree, actual_refcnt, ob_refcnt);
// location_t loc = rich_loc->get_loc ();
// foo (loc);
@@ -425,7 +424,8 @@ count_expected_pyobj_references (const region_model *model,
/* Compare ob_refcnt field vs the actual reference count of a region */
static void
-check_refcnt (const region_model *model, region_model_context *ctxt,
+check_refcnt (const region_model *model, const region_model *old_model,
+ region_model_context *ctxt,
const hash_map<const ana::region *,
int>::iterator::reference_pair region_refcnt)
{
@@ -438,8 +438,11 @@ check_refcnt (const region_model *model, region_model_context *ctxt,
if (ob_refcnt_sval != actual_refcnt_sval)
{
- // todo: fix this
- tree reg_tree = model->get_representative_tree (curr_region);
+ const svalue *curr_reg_sval
+ = mgr->get_ptr_svalue (pyobj_ptr_tree, curr_region);
+ tree reg_tree = old_model->get_representative_tree (curr_reg_sval);
+ if (!reg_tree)
+ return;
const auto &eg = ctxt->get_eg ();
refcnt_stmt_finder finder (*eg, reg_tree);
@@ -451,20 +454,22 @@ check_refcnt (const region_model *model, region_model_context *ctxt,
}
static void
-check_refcnts (const region_model *model, const svalue *retval,
- region_model_context *ctxt,
- hash_map<const region *, int> ®ion_to_refcnt)
+check_refcnts (const region_model *model, const region_model *old_model,
+ const svalue *retval, region_model_context *ctxt,
+ hash_map<const region *, int> ®ion_to_refcnt)
{
for (const auto ®ion_refcnt : region_to_refcnt)
{
- check_refcnt(model, ctxt, region_refcnt);
+ check_refcnt(model, old_model, ctxt, region_refcnt);
}
}
/* Validates the reference count of all Python objects. */
void
-pyobj_refcnt_checker (const region_model *model, const svalue *retval,
- region_model_context *ctxt)
+pyobj_refcnt_checker (const region_model *model,
+ const region_model *old_model,
+ const svalue *retval,
+ region_model_context *ctxt)
{
if (!ctxt)
return;
@@ -473,7 +478,7 @@ pyobj_refcnt_checker (const region_model *model, const svalue *retval,
auto seen_regions = hash_set<const region *> ();
count_expected_pyobj_references (model, region_to_refcnt, retval, seen_regions);
- check_refcnts (model, retval, ctxt, region_to_refcnt);
+ check_refcnts (model, old_model, retval, ctxt, region_to_refcnt);
}
/* Counts the actual pyobject references from all clusters in the model's