From: David Malcolm <dmalcolm@redhat.com>
To: Eric Feng <ef2648@columbia.edu>
Cc: gcc@gcc.gnu.org, gcc-patches@gcc.gnu.org
Subject: Re: [PATCH] analyzer: implement reference count checking for CPython plugin [PR107646]
Date: Tue, 29 Aug 2023 17:08:40 -0400 [thread overview]
Message-ID: <aced6b444fe6caef23df86f2a806b170fdcfeb0f.camel@redhat.com> (raw)
In-Reply-To: <20230829043155.17651-1-ef2648@columbia.edu>
On Tue, 2023-08-29 at 00:31 -0400, Eric Feng wrote:
> Hi Dave,
Hi Eric.
Thanks for the updated patch.
A few nits below; this is OK for trunk with them fixed...
[...snip...]
>
> gcc/analyzer/ChangeLog:
> PR analyzer/107646
> * engine.cc (impl_region_model_context::warn): New optional parameter.
> * exploded-graph.h (class impl_region_model_context): Likewise.
> * region-model.cc (region_model::pop_frame): New callback feature for
> * region_model::pop_frame.
> * region-model.h (struct append_regions_cb_data): Likewise.
> (class region_model): Likewise.
> (class region_model_context): New optional parameter.
> (class region_model_context_decorator): Likewise.
>
> gcc/testsuite/ChangeLog:
> PR analyzer/107646
> * gcc.dg/plugin/analyzer_cpython_plugin.c: Implements reference count
> * checking for PyObjects.
> * gcc.dg/plugin/cpython-plugin-test-2.c: Moved to...
> * gcc.dg/plugin/cpython-plugin-test-PyList_Append.c: ...here (and
> * added more tests).
> * gcc.dg/plugin/cpython-plugin-test-1.c: Moved to...
> * gcc.dg/plugin/cpython-plugin-test-no-plugin.c: ...here (and added
> * more tests).
> * gcc.dg/plugin/plugin.exp: New tests.
> * gcc.dg/plugin/cpython-plugin-test-PyList_New.c: New test.
> * gcc.dg/plugin/cpython-plugin-test-PyLong_FromLong.c: New test.
> * gcc.dg/plugin/cpython-plugin-test-refcnt-checking.c: New test.
The ChangeLog formatting here seems wrong; lines starting with a '*'
should refer to a filename. Continuation lines begin with just a tab
character.
[...snip...]
> diff --git a/gcc/analyzer/region-model.h b/gcc/analyzer/region-model.h
> index 10b2a59e787..440ea6d828d 100644
> --- a/gcc/analyzer/region-model.h
> +++ b/gcc/analyzer/region-model.h
[...snip...]
> @@ -840,7 +865,8 @@ private:
> class region_model_context_decorator : public region_model_context
> {
> public:
> - bool warn (std::unique_ptr<pending_diagnostic> d) override
> + bool warn (std::unique_ptr<pending_diagnostic> d,
> + const stmt_finder *custom_finder)
> {
> if (m_inner)
> return m_inner->warn (std::move (d));
This should presumably pass the custom_finder on to the 2nd argument of
m_inner->warn, rather than have the inner call to warn implicitly use
the NULL default arg.
[...snip...]
> diff --git a/gcc/testsuite/gcc.dg/plugin/cpython-plugin-test-1.c b/gcc/testsuite/gcc.dg/plugin/cpython-plugin-test-no-plugin.c
> similarity index 100%
> rename from gcc/testsuite/gcc.dg/plugin/cpython-plugin-test-1.c
> rename to gcc/testsuite/gcc.dg/plugin/cpython-plugin-test-no-plugin.c
Looks like
"-no-Python-h.c"
would be a better suffix than
"-no-plugin.c"
as it's the include that's missing, not the plugin.
[...snip...]
> diff --git a/gcc/testsuite/gcc.dg/plugin/plugin.exp b/gcc/testsuite/gcc.dg/plugin/plugin.exp
> index e1ed2d2589e..cbef6da8d86 100644
> --- a/gcc/testsuite/gcc.dg/plugin/plugin.exp
> +++ b/gcc/testsuite/gcc.dg/plugin/plugin.exp
> @@ -161,8 +161,9 @@ set plugin_test_list [list \
> taint-CVE-2011-0521-6.c \
> taint-antipatterns-1.c } \
> { analyzer_cpython_plugin.c \
> - cpython-plugin-test-1.c \
> - cpython-plugin-test-2.c } \
> + cpython-plugin-test-PyList_Append.c \
> + cpython-plugin-test-PyList_New.c \
> + cpython-plugin-test-PyLong_FromLong.c } \
Looks like this is missing:
cpython-plugin-test-no-plugin.c
and
cpython-plugin-test-refcnt-checking.c
(though as noted above"cpython-plugin-test-no-Python-h.c" would be a
better name for the former)
so it wasn't actually compiling these tests.
Be sure to doublecheck that these tests pass when updating.
[...snip...]
OK for trunk with the above nits fixed.
Dave
next prev parent reply other threads:[~2023-08-29 21:08 UTC|newest]
Thread overview: 50+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-07-25 4:49 Update and Questions on CPython Extension Module -fanalyzer plugin development Eric Feng
2023-07-25 14:41 ` David Malcolm
2023-07-27 22:13 ` Eric Feng
2023-07-27 22:35 ` David Malcolm
2023-07-30 17:52 ` Eric Feng
2023-07-30 23:44 ` David Malcolm
2023-08-01 13:57 ` Eric Feng
2023-08-01 17:06 ` David Malcolm
2023-08-04 15:02 ` Eric Feng
2023-08-04 15:39 ` David Malcolm
2023-08-04 20:48 ` Eric Feng
2023-08-04 22:42 ` David Malcolm
2023-08-04 22:46 ` David Malcolm
2023-08-07 18:31 ` Eric Feng
2023-08-07 23:16 ` David Malcolm
2023-08-08 16:51 ` [PATCH] WIP for dg-require-python-h [PR107646] Eric Feng
2023-08-08 18:08 ` David Malcolm
2023-08-08 18:51 ` David Malcolm
2023-08-09 19:22 ` [PATCH v2] analyzer: More features for CPython analyzer plugin [PR107646] Eric Feng
2023-08-09 21:36 ` David Malcolm
2023-08-11 17:47 ` [COMMITTED] " Eric Feng
2023-08-11 20:23 ` Eric Feng
2023-08-16 19:17 ` Update on CPython Extension Module -fanalyzer plugin development Eric Feng
2023-08-16 21:28 ` David Malcolm
2023-08-17 1:47 ` Eric Feng
2023-08-21 14:05 ` Eric Feng
2023-08-21 15:04 ` David Malcolm
2023-08-23 21:15 ` Eric Feng
2023-08-23 23:16 ` David Malcolm
2023-08-24 14:45 ` Eric Feng
2023-08-25 12:50 ` Eric Feng
2023-08-25 19:50 ` David Malcolm
2023-08-29 4:31 ` [PATCH] analyzer: implement reference count checking for CPython plugin [PR107646] Eric Feng
2023-08-29 4:35 ` Eric Feng
2023-08-29 17:28 ` Eric Feng
2023-08-29 21:14 ` David Malcolm
2023-08-30 22:15 ` Eric Feng
2023-08-31 17:01 ` David Malcolm
2023-08-31 19:09 ` Eric Feng
2023-08-31 20:19 ` David Malcolm
2023-09-01 1:25 ` Eric Feng
2023-09-01 11:57 ` David Malcolm
2023-09-05 2:13 ` [PATCH] analyzer: implement symbolic value support for CPython plugin's refcnt checker [PR107646] Eric Feng
2023-09-07 17:28 ` David Malcolm
2023-09-11 2:12 ` Eric Feng
2023-09-11 19:00 ` David Malcolm
2023-08-29 21:08 ` David Malcolm [this message]
2023-09-01 2:49 ` [PATCH] analyzer: implement reference count checking for CPython plugin [PR107646] 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=aced6b444fe6caef23df86f2a806b170fdcfeb0f.camel@redhat.com \
--to=dmalcolm@redhat.com \
--cc=ef2648@columbia.edu \
--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).