From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx0b-00364e01.pphosted.com (mx0b-00364e01.pphosted.com [148.163.139.74]) by sourceware.org (Postfix) with ESMTPS id 123A83858D20 for ; Fri, 1 Sep 2023 21:07:20 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 123A83858D20 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=columbia.edu Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=columbia.edu Received: from pps.filterd (m0167075.ppops.net [127.0.0.1]) by mx0b-00364e01.pphosted.com (8.17.1.19/8.17.1.19) with ESMTP id 381Jnwwn022987 for ; Fri, 1 Sep 2023 17:07:19 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=columbia.edu; h=mime-version : references : in-reply-to : from : date : message-id : subject : to : cc : content-type : content-transfer-encoding; s=pps01; bh=JFHkyWI8Gjky9+7/werYWnekioqQbqHH2/6zEeat7es=; b=KNQitnTPDjC+JpUquwstprjvsJkeQUzx8U56gRBHa4KlwgtaICmgxL+u98yytoYZv5Qh k6N7wTdDv2Rxd5e504+5U2Bk86s66jaC+Yi3SQDG0hf8SoLBhzX+AU6Hc3ljD/L0IxDk xIm3H4ajO0wWxNcp/hFuZ3LWG0OXSPutDXVHtwenLrSLhNk6N9J7btlw/krfCJT6n5UW Fxeh6QrZNZ7BzmKemPV0e/EpAkG/zekSoP04FDKPXUZDymlvwel4ZPVnNVdfVB88/wv0 1Ohjj4VkA7DS98FS7f60n+641pun4wzwuzYSydhoLxSJqnBGN4rg/v3zRZw4eI1nJLc+ Ug== Received: from mail-ua1-f69.google.com (mail-ua1-f69.google.com [209.85.222.69]) by mx0b-00364e01.pphosted.com (PPS) with ESMTPS id 3sug6nv4bw-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128 verify=NOT) for ; Fri, 01 Sep 2023 17:07:19 -0400 Received: by mail-ua1-f69.google.com with SMTP id a1e0cc1a2514c-7a27786f99cso949441241.0 for ; Fri, 01 Sep 2023 14:07:19 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1693602438; x=1694207238; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=JFHkyWI8Gjky9+7/werYWnekioqQbqHH2/6zEeat7es=; b=TwSjqG5LDTjZKcXPnUO191NWprakJi2BaL7Q84P0jUGeQlno2zyLNGxt0a2jLdPIGc oh/BbTBDUS4+YDwAusSZscuEvX8ZTNSr/tPUe1aQ/G8kzwexlZ0D+dye26t8ehBSLaVB Oc8iNsw7h8dM9ye+SYi6vI1pdqp3U8LRuqO3err5QhtqbUk8pHW+IErksLwqUz7SWSHI 4BQ985MZbIdMpx5PB/FCe6rxVHu/KOe6NMfIkfopoCnPtuSgLhHLVo0KLsu0AqQpioI5 f/SXbDkZUfx9BJRDEAgJuw1s4TXhK3eGULbw4M0DFO4X/BVLVKtV2r1lsTwYU/Q4aECv SC0g== X-Gm-Message-State: AOJu0YxyuYoJ5Jue1AYkeEPH/d+T8blievi9S6crjvnngNyc8aLI+87k 5/NC5AyFPfc3eBa3DSOitJdRoSdBcms8P9eK/KNocjNEiGQLEc58V7im/xNiUes/MpVlcs0EJPe qNkoBGWxbQZ/lvE5tfhas X-Received: by 2002:a67:b646:0:b0:44e:9cdc:6fe5 with SMTP id e6-20020a67b646000000b0044e9cdc6fe5mr4474279vsm.17.1693602438526; Fri, 01 Sep 2023 14:07:18 -0700 (PDT) X-Google-Smtp-Source: AGHT+IHnTf/nj97vds6atoEKbavTi0bGl7q8TjCdCSyJ3JMhGc4eJbasDbbL6bfIc7gKmv96dbpNKKZ4Pkvp7WuBcsI= X-Received: by 2002:a67:b646:0:b0:44e:9cdc:6fe5 with SMTP id e6-20020a67b646000000b0044e9cdc6fe5mr4474265vsm.17.1693602438220; Fri, 01 Sep 2023 14:07:18 -0700 (PDT) MIME-Version: 1.0 References: <20230829043155.17651-1-ef2648@columbia.edu> <20230901024948.8496820427@pchp3.se.axis.com> <456ca70cfade6081f770310423e4e8926c10dd77.camel@redhat.com> In-Reply-To: <456ca70cfade6081f770310423e4e8926c10dd77.camel@redhat.com> From: Eric Feng Date: Fri, 1 Sep 2023 17:07:07 -0400 Message-ID: Subject: Re: [PATCH] analyzer: implement reference count checking for CPython plugin [PR107646] To: David Malcolm Cc: Hans-Peter Nilsson , gcc@gcc.gnu.org, gcc-patches@gcc.gnu.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Proofpoint-ORIG-GUID: JNcGV37mT0tPOcoT-i9zbA-SQr9124Ep X-Proofpoint-GUID: JNcGV37mT0tPOcoT-i9zbA-SQr9124Ep X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.267,Aquarius:18.0.957,Hydra:6.0.601,FMLib:17.11.176.26 definitions=2023-09-01_17,2023-08-31_01,2023-05-22_02 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 mlxscore=0 phishscore=0 impostorscore=10 adultscore=0 lowpriorityscore=10 spamscore=0 clxscore=1015 mlxlogscore=999 suspectscore=0 priorityscore=1501 bulkscore=10 malwarescore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2308100000 definitions=main-2309010197 X-Spam-Status: No, score=-11.6 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,RCVD_IN_DNSWL_LOW,SPF_HELO_NONE,SPF_NONE,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Thank you for the patch! On Fri, Sep 1, 2023 at 10:51=E2=80=AFAM 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-patches@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 =3D 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 =3D 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..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 =3D hash_map (); > > - auto seen_regions =3D 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 =3D cd.get_model (); > > - auto region_to_refcnt =3D 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 > > +} > Best, Eric