Re: [PATCH] analyzer: implement reference count checking for CPython plugin [PR107646]
Thank you for the patch! On Fri, Sep 1, 2023 at 10:51 AM David Malcolm wrote: > > On Fri, 2023-09-01 at 04:49 +0200, Hans-Peter Nilsson wrote: > > (Looks like this was committed as r14-3580-g597b9ec69bca8a) > > > > > Cc: gcc@gcc.gnu.org, gcc-patc...@gcc.gnu.org, Eric Feng > > > > > > From: Eric Feng via Gcc > > > > > 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. > > > > It seems this was more or less a rewrite, but that said, > > it's generally preferable to always *add* tests, never *modify* them. > > > > > .../gcc.dg/plugin/analyzer_cpython_plugin.c | 376 > > > +- > > > > ^^^ Ouch! Was it not within reason to keep that test as it > > was, and just add another test? Thanks for the feedback. To clarify, 'analyzer_cpython_plugin.c' is not a test itself but rather a plugin that currently lives within the testsuite. The core of the test cases were also not modified, rather I renamed certain filenames containing them for clarity (unless this is what you meant in terms of modification, in which case noted) and added to them. However, I understand the preference and will keep that in mind. > > > > Anyway, the test after rewrite fails, and for some targets > > like cris-elf and apparently m68k-linux, yields an error. > > I see a PR was already opened. > > > > Also, mostly for future reference, several files in the > > patch miss a final newline, as seen by a "\ No newline at > > end of file"-marker. Noted. > > > > I think I found the problem; a mismatch between default C++ > > language standard between host-gcc and target-gcc. > > > > (It's actually *not* as simple as "auto var = typeofvar()" > > not being recognized in C++11 --or else there'd be an error > > for the hash_set declaration too, which I just changed for > > consistency-- but it's close enough for me.) > > > > With this, retesting plugin.exp for cris-elf works. Sounds good, thanks again! I was also curious about why hash_map had an issue here with that syntax whilst hash_set did not, so I tried to investigate a bit further. I believe the issue was due to the compiler having trouble disambiguating between the hash_map constructors in C++11. >From the error message we received: test/gcc/testsuite/gcc.dg/plugin/analyzer_cpython_plugin.c:480:58: error: no matching function for call to 'hash_map::hash_map(hash_map)' auto region_to_refcnt = hash_map (); I think the compiler is mistakenly interpreting the call here as we would like to create a new hash_map object using its copy constructor, but we "forgot" to provide the object to be copied, rather than our intention of using the default constructor. Looking at hash_map.h and hash_set.h seems to support this hypothesis, as hash_map has two constructors, one of which resembles a copy constructor with additional arguments: https://github.com/gcc-mirror/gcc/blob/master/gcc/hash-map.h#L147. Perhaps the default arguments here further complicated the ambiguity as to which constructor to use in the presence of the empty parenthesis. On the other hand, hash_set has only the default constructor with default parameters, and thus there is no ambiguity: https://github.com/gcc-mirror/gcc/blob/master/gcc/hash-set.h#L40. I assume this ambiguity was cleared up by later versions, and thus we observed no problems in C++17. However, I am certainly still a relative newbie of C++, so please anyone feel free to correct my reasoning and chime in! > > > > Ok to commit? > > Sorry about the failing tests. > > Thanks for the patch; please go ahead and commit. > > Dave > > > > > -- >8 -- > > From: Hans-Peter Nilsson > > Date: Fri, 1 Sep 2023 04:36:03 +0200 > > Subject: [PATCH] testsuite: Fix analyzer_cpython_plugin.c > > declarations, PR testsuite/111264 > > > > Also, add missing newline at end of file. > > > > PR testsuite/111264 > > * gcc.dg/plugin/analyzer_cpython_plugin.c: Make declarations > > C++11-compatible. > > --- > > gcc/testsuite/gcc.dg/plugin/analyzer_cpython_plugin.c | 8 > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/gcc/testsuite/gcc.dg/plugin/analyzer_cpython_plugin.c > > b/gcc/testsuite/gcc.dg/plugin/analyzer_cpython_plugin.c > > index 7af520436549..bf1
Re: [PATCH] analyzer: implement reference count checking for CPython plugin [PR107646]
On Fri, 2023-09-01 at 04:49 +0200, Hans-Peter Nilsson wrote: > (Looks like this was committed as r14-3580-g597b9ec69bca8a) > > > Cc: gcc@gcc.gnu.org, gcc-patc...@gcc.gnu.org, Eric Feng > > > > From: Eric Feng via Gcc > > > 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. > > It seems this was more or less a rewrite, but that said, > it's generally preferable to always *add* tests, never *modify* them. > > > .../gcc.dg/plugin/analyzer_cpython_plugin.c | 376 > > +- > > ^^^ Ouch! Was it not within reason to keep that test as it > was, and just add another test? > > Anyway, the test after rewrite fails, and for some targets > like cris-elf and apparently m68k-linux, yields an error. > I see a PR was already opened. > > Also, mostly for future reference, several files in the > patch miss a final newline, as seen by a "\ No newline at > end of file"-marker. > > I think I found the problem; a mismatch between default C++ > language standard between host-gcc and target-gcc. > > (It's actually *not* as simple as "auto var = typeofvar()" > not being recognized in C++11 --or else there'd be an error > for the hash_set declaration too, which I just changed for > consistency-- but it's close enough for me.) > > With this, retesting plugin.exp for cris-elf works. > > Ok to commit? Sorry about the failing tests. Thanks for the patch; please go ahead and commit. Dave > > -- >8 -- > From: Hans-Peter Nilsson > Date: Fri, 1 Sep 2023 04:36:03 +0200 > Subject: [PATCH] testsuite: Fix analyzer_cpython_plugin.c > declarations, PR testsuite/111264 > > Also, add missing newline at end of file. > > PR testsuite/111264 > * gcc.dg/plugin/analyzer_cpython_plugin.c: Make declarations > C++11-compatible. > --- > gcc/testsuite/gcc.dg/plugin/analyzer_cpython_plugin.c | 8 > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/gcc/testsuite/gcc.dg/plugin/analyzer_cpython_plugin.c > b/gcc/testsuite/gcc.dg/plugin/analyzer_cpython_plugin.c > index 7af520436549..bf1982e79c37 100644 > --- a/gcc/testsuite/gcc.dg/plugin/analyzer_cpython_plugin.c > +++ b/gcc/testsuite/gcc.dg/plugin/analyzer_cpython_plugin.c > @@ -477,8 +477,8 @@ pyobj_refcnt_checker (const region_model *model, > if (!ctxt) > return; > > - auto region_to_refcnt = hash_map (); > - auto seen_regions = hash_set (); > + hash_map region_to_refcnt; > + hash_set seen_regions; > > count_pyobj_references (model, region_to_refcnt, retval, > seen_regions); > check_refcnts (model, old_model, retval, ctxt, region_to_refcnt); > @@ -561,7 +561,7 @@ public: > if (!ctxt) > return; > region_model *model = cd.get_model (); > - auto region_to_refcnt = hash_map (); > + hash_map region_to_refcnt; > count_all_references(model, region_to_refcnt); > dump_refcnt_info(region_to_refcnt, model, ctxt); > } > @@ -1330,4 +1330,4 @@ plugin_init (struct plugin_name_args > *plugin_info, > sorry_no_analyzer (); > #endif > return 0; > -} > \ No newline at end of file > +}
Re: [PATCH] analyzer: implement reference count checking for CPython plugin [PR107646]
On Thu, 2023-08-31 at 21:25 -0400, Eric Feng wrote: > On Thu, Aug 31, 2023 at 4:19 PM David Malcolm > wrote: > > > > On Thu, 2023-08-31 at 15:09 -0400, Eric Feng wrote: > > > On Thu, Aug 31, 2023 at 1:01 PM David Malcolm > > > > > > wrote: > > > > > > > > On Wed, 2023-08-30 at 18:15 -0400, Eric Feng wrote: > > > > [...] > > > > > > > > > > > > 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? > > > Happy to add it myself, but I'm not finding an option to edit the > > > page > > > (created an account under ef...@gcc.gnu.org). Do I need to be > > > added > > > to > > > the EditorGroup (https://gcc.gnu.org/wiki/EditorGroup) to do so? > > > > I can do this. What's your account's WikiName ? > Thank you — it is EricFeng. > I've added EricFeng to that page, so hopefully you should be able to make wiki edits now. Dave
Re: [PATCH] analyzer: implement reference count checking for CPython plugin [PR107646]
(Looks like this was committed as r14-3580-g597b9ec69bca8a) > Cc: gcc@gcc.gnu.org, gcc-patc...@gcc.gnu.org, Eric Feng > From: Eric Feng via Gcc > 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. It seems this was more or less a rewrite, but that said, it's generally preferable to always *add* tests, never *modify* them. > .../gcc.dg/plugin/analyzer_cpython_plugin.c | 376 +- ^^^ Ouch! Was it not within reason to keep that test as it was, and just add another test? Anyway, the test after rewrite fails, and for some targets like cris-elf and apparently m68k-linux, yields an error. I see a PR was already opened. Also, mostly for future reference, several files in the patch miss a final newline, as seen by a "\ No newline at end of file"-marker. I think I found the problem; a mismatch between default C++ language standard between host-gcc and target-gcc. (It's actually *not* as simple as "auto var = typeofvar()" not being recognized in C++11 --or else there'd be an error for the hash_set declaration too, which I just changed for consistency-- but it's close enough for me.) With this, retesting plugin.exp for cris-elf works. Ok to commit? -- >8 -- From: Hans-Peter Nilsson Date: Fri, 1 Sep 2023 04:36:03 +0200 Subject: [PATCH] testsuite: Fix analyzer_cpython_plugin.c declarations, PR testsuite/111264 Also, add missing newline at end of file. PR testsuite/111264 * gcc.dg/plugin/analyzer_cpython_plugin.c: Make declarations C++11-compatible. --- gcc/testsuite/gcc.dg/plugin/analyzer_cpython_plugin.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/gcc/testsuite/gcc.dg/plugin/analyzer_cpython_plugin.c b/gcc/testsuite/gcc.dg/plugin/analyzer_cpython_plugin.c index 7af520436549..bf1982e79c37 100644 --- a/gcc/testsuite/gcc.dg/plugin/analyzer_cpython_plugin.c +++ b/gcc/testsuite/gcc.dg/plugin/analyzer_cpython_plugin.c @@ -477,8 +477,8 @@ pyobj_refcnt_checker (const region_model *model, if (!ctxt) return; - auto region_to_refcnt = hash_map (); - auto seen_regions = hash_set (); + hash_map region_to_refcnt; + hash_set seen_regions; count_pyobj_references (model, region_to_refcnt, retval, seen_regions); check_refcnts (model, old_model, retval, ctxt, region_to_refcnt); @@ -561,7 +561,7 @@ public: if (!ctxt) return; region_model *model = cd.get_model (); -auto region_to_refcnt = hash_map (); +hash_map region_to_refcnt; count_all_references(model, region_to_refcnt); dump_refcnt_info(region_to_refcnt, model, ctxt); } @@ -1330,4 +1330,4 @@ plugin_init (struct plugin_name_args *plugin_info, sorry_no_analyzer (); #endif return 0; -} \ No newline at end of file +} -- 2.30.2 brgds, H-P
Re: [PATCH] analyzer: implement reference count checking for CPython plugin [PR107646]
On Thu, Aug 31, 2023 at 4:19 PM David Malcolm wrote: > > On Thu, 2023-08-31 at 15:09 -0400, Eric Feng wrote: > > On Thu, Aug 31, 2023 at 1:01 PM David Malcolm > > wrote: > > > > > > On Wed, 2023-08-30 at 18:15 -0400, Eric Feng wrote: > > [...] > > > > > > > > > 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? > > Happy to add it myself, but I'm not finding an option to edit the > > page > > (created an account under ef...@gcc.gnu.org). Do I need to be added > > to > > the EditorGroup (https://gcc.gnu.org/wiki/EditorGroup) to do so? > > I can do this. What's your account's WikiName ? Thank you — it is EricFeng. > > Thanks > Dave >
Re: [PATCH] analyzer: implement reference count checking for CPython plugin [PR107646]
On Thu, 2023-08-31 at 15:09 -0400, Eric Feng wrote: > On Thu, Aug 31, 2023 at 1:01 PM David Malcolm > wrote: > > > > On Wed, 2023-08-30 at 18:15 -0400, Eric Feng wrote: [...] > > > > > > 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? > Happy to add it myself, but I'm not finding an option to edit the > page > (created an account under ef...@gcc.gnu.org). Do I need to be added > to > the EditorGroup (https://gcc.gnu.org/wiki/EditorGroup) to do so? I can do this. What's your account's WikiName ? Thanks Dave
Re: [PATCH] analyzer: implement reference count checking for CPython plugin [PR107646]
On Thu, Aug 31, 2023 at 1:01 PM David Malcolm wrote:
>
> On Wed, 2023-08-30 at 18:15 -0400, Eric Feng wrote:
> > On Tue, Aug 29, 2023 at 5:14 PM David Malcolm
> > 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?
Happy to add it myself, but I'm not finding an option to edit the page
(created an account under ef...@gcc.gnu.org). Do I need to be added to
the EditorGroup (https://gcc.gnu.org/wiki/EditorGroup) to do so?
>
> >
> > 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
>
>
Re: [PATCH] analyzer: implement reference count checking for CPython plugin [PR107646]
On Wed, 2023-08-30 at 18:15 -0400, Eric Feng wrote:
> On Tue, Aug 29, 2023 at 5:14 PM David Malcolm
> 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
Re: [PATCH] analyzer: implement reference count checking for CPython plugin [PR107646]
On Tue, Aug 29, 2023 at 5:14 PM David Malcolm 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).
Re: [PATCH] analyzer: implement reference count checking for CPython plugin [PR107646]
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
Re: [PATCH] analyzer: implement reference count checking for CPython plugin [PR107646]
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 d) override
> + bool warn (std::unique_ptr 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
[PATCH] analyzer: implement reference count checking for CPython plugin [PR107646]
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 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::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_representativ
Re: [PATCH] analyzer: implement reference count checking for CPython plugin [PR107646]
On Tue, Aug 29, 2023 at 12:32 AM Eric Feng wrote:
>
> Hi Dave,
>
> Thanks for the feedback. I've addressed the changes you mentioned in
> addition to adding more test cases. I've also taken this chance to
> split the test files according to known function subclasses, as you previously
> suggested. Since there were also some changes to the core analyzer, I've done
> a
> bootstrap and regtested the patch as well. Does it look OK for trunk?
Apologies — I forgot to mention that bootstrap and regtest was done on
aarch64-unknown-linux-gnu.
>
> Best,
> Eric
>
> ---
>
> This patch introduces initial support for reference count checking of
> PyObjects in relation to the Python/C API for the CPython plugin.
> Additionally, the core analyzer underwent several modifications to
> accommodate this feature. These include:
>
> - Introducing support for callbacks at the end of
> region_model::pop_frame. This is our current point of validation for
> the reference count of PyObjects.
> - An added optional custom stmt_finder parameter to
> region_model_context::warn. This aids in emitting a diagnostic
> concerning the reference count, especially when the stmt_finder is
> NULL, which is currently the case during region_model::pop_frame.
>
> The current diagnostic we emit relating to the reference count
> appears as follows:
>
> rc3.c:23:10: warning: expected 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
> |
>
> This is a WIP in several ways:
> - Enhancing the diagnostic for better clarity. For instance, users should
> expect to see the variable name 'item' instead of the placeholder in the
> diagnostic above.
> - Currently, functions returning PyObject * are assumed to always produce
> a new reference.
> - The validation of reference count is only for PyObjects created within a
> function body. Verifying reference counts for PyObjects passed as
> parameters is not supported in this patch.
>
> 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.
>
> Signed-off-by: Eric Feng
>
> ---
> gcc/analyzer/engine.cc| 8 +-
> gcc/analyzer/exploded-graph.h | 4 +-
> gcc/analyzer/region-model.cc | 3 +
> gcc/analyzer/region-model.h | 48 ++-
> .../gcc.dg/plugin/analyzer_cpython_plugin.c | 376 +-
> c => cpython-plugin-test-PyList_Append.c} | 56 +--
> .../plugin/cpython-plugin-test-PyList_New.c | 38 ++
> .../cpython-plugin-test-PyLong_FromLong.c | 38 ++
> ...st-1.c => cpython-plugin-test-no-plugin.c} | 0
> .../cpython-plugin-test-refcnt-checking.c | 78
> gcc/testsuite/gcc.dg/plugin/plugin.exp| 5 +-
> 11 files changed, 612 insertions(+), 42 deletions(-)
> rename gcc/testsuite/gcc.dg/plugin/{cpython-plugin-test-2.c =>
> cpython-plugin-test-PyList_Append.c} (64%)
> create mode 100644
> gcc/testsuite/gcc.dg/plugin/cpython-plugin-
[PATCH] analyzer: implement reference count checking for CPython plugin [PR107646]
Hi Dave,
Thanks for the feedback. I've addressed the changes you mentioned in
addition to adding more test cases. I've also taken this chance to
split the test files according to known function subclasses, as you previously
suggested. Since there were also some changes to the core analyzer, I've done a
bootstrap and regtested the patch as well. Does it look OK for trunk?
Best,
Eric
---
This patch introduces initial support for reference count checking of
PyObjects in relation to the Python/C API for the CPython plugin.
Additionally, the core analyzer underwent several modifications to
accommodate this feature. These include:
- Introducing support for callbacks at the end of
region_model::pop_frame. This is our current point of validation for
the reference count of PyObjects.
- An added optional custom stmt_finder parameter to
region_model_context::warn. This aids in emitting a diagnostic
concerning the reference count, especially when the stmt_finder is
NULL, which is currently the case during region_model::pop_frame.
The current diagnostic we emit relating to the reference count
appears as follows:
rc3.c:23:10: warning: expected 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
|
This is a WIP in several ways:
- Enhancing the diagnostic for better clarity. For instance, users should
expect to see the variable name 'item' instead of the placeholder in the
diagnostic above.
- Currently, functions returning PyObject * are assumed to always produce
a new reference.
- The validation of reference count is only for PyObjects created within a
function body. Verifying reference counts for PyObjects passed as
parameters is not supported in this patch.
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.
Signed-off-by: Eric Feng
---
gcc/analyzer/engine.cc| 8 +-
gcc/analyzer/exploded-graph.h | 4 +-
gcc/analyzer/region-model.cc | 3 +
gcc/analyzer/region-model.h | 48 ++-
.../gcc.dg/plugin/analyzer_cpython_plugin.c | 376 +-
c => cpython-plugin-test-PyList_Append.c} | 56 +--
.../plugin/cpython-plugin-test-PyList_New.c | 38 ++
.../cpython-plugin-test-PyLong_FromLong.c | 38 ++
...st-1.c => cpython-plugin-test-no-plugin.c} | 0
.../cpython-plugin-test-refcnt-checking.c | 78
gcc/testsuite/gcc.dg/plugin/plugin.exp| 5 +-
11 files changed, 612 insertions(+), 42 deletions(-)
rename gcc/testsuite/gcc.dg/plugin/{cpython-plugin-test-2.c =>
cpython-plugin-test-PyList_Append.c} (64%)
create mode 100644 gcc/testsuite/gcc.dg/plugin/cpython-plugin-test-PyList_New.c
create mode 100644
gcc/testsuite/gcc.dg/plugin/cpython-plugin-test-PyLong_FromLong.c
rename gcc/testsuite/gcc.dg/plugin/{cpython-plugin-test-1.c =>
cpython-plugin-test-no-plugin.c} (100%)
create mode 100644
gcc/testsuite/gcc.dg/plugin/cpython-plugin-test-refcnt-checking.c
diff --git a/gcc/analyzer/engine.cc b/gcc/analyzer/engine.cc
in
