Thank you for the patch! On Fri, Sep 1, 2023 at 10:51 AM David Malcolm <dmalc...@redhat.com> 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 > > > <ef2...@columbia.edu> > > > From: Eric Feng via Gcc <gcc@gcc.gnu.org> > > > > > 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<bar>()" > > 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<const ana::region*, int>::hash_map(hash_map<const ana::region*, int>)' auto region_to_refcnt = hash_map<const region *, int> (); 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 <h...@axis.com> > > 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<const region *, int> (); > > - auto seen_regions = hash_set<const region *> (); > > + hash_map<const region *, int> region_to_refcnt; > > + hash_set<const region *> 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<const region *, int> (); > > + hash_map<const region *, int> 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 > > +} > Best, Eric
