public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: Eric Feng <ef2648@columbia.edu>
To: dmalcolm@redhat.com
Cc: gcc@gcc.gnu.org, gcc-patches@gcc.gnu.org,
	Eric Feng <ef2648@columbia.edu>
Subject: [PATCH] analyzer: implement reference count checking for CPython plugin [PR107646]
Date: Tue, 29 Aug 2023 13:28:18 -0400	[thread overview]
Message-ID: <20230829172818.3264-1-ef2648@columbia.edu> (raw)
In-Reply-To: <CANGHATV4fXHOfew8ZC0btUYpoNFFTy1-7U9drDULM0g+_cAc8A@mail.gmail.com>

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(-)

diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-model.cc
index eb4f976b83a..c1d266d351b 100644
--- a/gcc/analyzer/region-model.cc
+++ b/gcc/analyzer/region-model.cc
@@ -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.  */
diff --git a/gcc/analyzer/region-model.h b/gcc/analyzer/region-model.h
index 440ea6d828d..b89c6f6c649 100644
--- a/gcc/analyzer/region-model.h
+++ b/gcc/analyzer/region-model.h
@@ -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:
diff --git a/gcc/testsuite/gcc.dg/plugin/analyzer_cpython_plugin.c b/gcc/testsuite/gcc.dg/plugin/analyzer_cpython_plugin.c
index b2caed8fc1b..6f0a355fe30 100644
--- a/gcc/testsuite/gcc.dg/plugin/analyzer_cpython_plugin.c
+++ b/gcc/testsuite/gcc.dg/plugin/analyzer_cpython_plugin.c
@@ -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> &region_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> &region_to_refcnt)
 {
   for (const auto &region_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
-- 
2.30.2


  reply	other threads:[~2023-08-29 17:28 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <c720fc39d86cb3ae8041fe5c86dad34d71466cec.camel@redhat.com>
2023-08-29  4:31 ` Eric Feng
2023-08-29  4:35   ` Eric Feng
2023-08-29 17:28     ` Eric Feng [this message]
2023-08-29 21:14       ` David Malcolm
2023-08-30 22:15         ` Eric Feng
2023-08-31 17:01           ` David Malcolm
2023-08-29 21:08   ` David Malcolm
2023-09-01  2:49   ` Hans-Peter Nilsson
2023-09-01 14:51     ` David Malcolm
2023-09-01 21:07       ` Eric Feng

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20230829172818.3264-1-ef2648@columbia.edu \
    --to=ef2648@columbia.edu \
    --cc=dmalcolm@redhat.com \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=gcc@gcc.gnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).