Message ID | 946D3718-32CD-45B6-8EF5-C41DDC3CA06E@oracle.com |
---|---|
State | New |
Headers |
Return-Path: <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 998E13858414 for <patchwork@sourceware.org>; Tue, 8 Feb 2022 20:12:45 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 998E13858414 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1644351165; bh=u4TTc6EfGXWrMq1dm1uODx+GV1ORy+57U5ZTNB9zdzY=; h=To:Subject:Date:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:Cc:From; b=J0/6kGEmtNG5zN3EpJv5NhXg+nhdY5JCSg4X4tsz9Uq4YktaRim7VGfpOEB0q5Gpj GoI1WToSV4h9J/XB5At2doJvtHlXQe6mVBUkJ586wI7PUp+Eps6yblzk9t/RlMsyvW 9+pddpMZJPeeN03OkKLp5XcmMzbddSWthzJZxqvo= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from mx0b-00069f02.pphosted.com (mx0b-00069f02.pphosted.com [205.220.177.32]) by sourceware.org (Postfix) with ESMTPS id 26D0A3857C47 for <gcc-patches@gcc.gnu.org>; Tue, 8 Feb 2022 20:11:44 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 26D0A3857C47 Received: from pps.filterd (m0246631.ppops.net [127.0.0.1]) by mx0b-00069f02.pphosted.com (8.16.1.2/8.16.1.2) with SMTP id 218Iv0u6011047; Tue, 8 Feb 2022 20:11:43 GMT Received: from aserp3020.oracle.com (aserp3020.oracle.com [141.146.126.70]) by mx0b-00069f02.pphosted.com with ESMTP id 3e3hdsth6t-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Tue, 08 Feb 2022 20:11:43 +0000 Received: from pps.filterd (aserp3020.oracle.com [127.0.0.1]) by aserp3020.oracle.com (8.16.1.2/8.16.1.2) with SMTP id 218KBZHS083557; Tue, 8 Feb 2022 20:11:42 GMT Received: from nam11-bn8-obe.outbound.protection.outlook.com (mail-bn8nam11lp2169.outbound.protection.outlook.com [104.47.58.169]) by aserp3020.oracle.com with ESMTP id 3e1h26wxya-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Tue, 08 Feb 2022 20:11:42 +0000 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=A7nhpy8PKWnchObWaX4LxVPPjO+739UMTLWVew9TUfnEhoCK+opL6OoUBwYpnPhUiy3g7PSXo5djpxDfYwu2qGzAGkg6I5ylndHvu3e4cMTtUYqIo8WGmW7lcvCu2+YrNEP27QcEaG5NmL2GHZhnEfS0Oho903HheQtFRYLcfnVmtEXKanjdminEegAJtXUy5FIP6Dwy+YK2pk/8ljpMR+z4pZDAC//O1HmgQbeMZ3aHackw3GTQ9BTl9FgMBlRbMDBRvJX6rtYhkM+svOKLCFiMyJA2/Q55exl+BXk16V5RWXc/K61epJ5YWTKA9ac/8mKLig12QjYBRikRc6JlBQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=u4TTc6EfGXWrMq1dm1uODx+GV1ORy+57U5ZTNB9zdzY=; b=esMNWjeXjlD8WNGyiGKjS8kIeq9L1i/IlOMVoOSHIPOPXuNM1NYsUMCMzTAUpvjt7Cp4IcQn15atWUB5669+Y92p94AJFCA1zVhebNz26IWPW06E7i9WCf/VP3b0Mcb778nqmHTt1cXKpAlTnPwIjkZ5eEVWM64q4li1lJWYx2mgMfVgKIkLlvJ/sAaP5psh4Y1bcbhL/rgHwMyCQ/d77QV3cXZAUu94mM7rysULwD7vnwafdFXd9RG+PTLe0HKKIgwAQCOsYC9z68deVCFIGyKmot03yMBlmQvFbmP07jR5YqH4AqP1ZrSHqszRfYh5XnBUse9bfG+asysp1xNrWQ== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=none; dmarc=none; dkim=none; arc=none Received: from CH2PR10MB4344.namprd10.prod.outlook.com (2603:10b6:610:af::19) by MN2PR10MB4000.namprd10.prod.outlook.com (2603:10b6:208:1b8::31) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4951.17; Tue, 8 Feb 2022 20:11:39 +0000 Received: from CH2PR10MB4344.namprd10.prod.outlook.com ([fe80::50e0:10b6:4c07:3728]) by CH2PR10MB4344.namprd10.prod.outlook.com ([fe80::50e0:10b6:4c07:3728%8]) with mapi id 15.20.4975.011; Tue, 8 Feb 2022 20:11:39 +0000 To: "jason@redhat.com" <jason@redhat.com> Subject: [PATCH] Fix PR 101515 (ICE in pp_cxx_unqualified_id, at cp/cxx-pretty-print.c:128) Thread-Topic: [PATCH] Fix PR 101515 (ICE in pp_cxx_unqualified_id, at cp/cxx-pretty-print.c:128) Thread-Index: AQHYHSgUwivW/WCgVU68v7XgX39s8g== Date: Tue, 8 Feb 2022 20:11:39 +0000 Message-ID: <946D3718-32CD-45B6-8EF5-C41DDC3CA06E@oracle.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-mailer: Apple Mail (2.3608.120.23.2.7) x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: c15b0994-056e-40ba-6188-08d9eb3f3775 x-ms-traffictypediagnostic: MN2PR10MB4000:EE_ x-microsoft-antispam-prvs: <MN2PR10MB4000D0D8819D6B26532FFF9E802D9@MN2PR10MB4000.namprd10.prod.outlook.com> x-ms-oob-tlc-oobclassifiers: OLM:6790; x-ms-exchange-senderadcheck: 1 x-ms-exchange-antispam-relay: 0 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: VUc2kYgWYI9rIKg6x+BtfA587X9mJvgFUkQfYhfK0H253PbCUuvRSan4ZlnyyvYmWNtOERkcRdTYYxfrMe/vLwIjPokXErBcxLr5N3DfPJ0gnRPvMn4rjugg5FQGR0Z4Fpus2hTxF6txw6LaLzcZhynuFD3t+wdUlGXBIoDKzxqXDeVi4OtvjhJrzjyCfG0CPrkRKklYuB6zuCbGqxwYPsX31UI/stwWGO9wAh00r3zyDMpfCQzHmPyDAyHaQikelPQw062SNfdkqftbxr4whwjgkjdmRDaECtcNaJ138LGLHHk0UcxSSYHJ/UAMvZMJze7EifysGKz+r3Op7hVMJo0Mh9wj05eNrVP+FUQzWCcS1ySo4RWkzQ9ZsqLHKkLg8yFKSobTj+kFPArR3ZB2uCf1RbXwt33nYSL0rzFxp28Maygbu7bu0mBZkjgmQ+WmeKxodGcXByeELyoGhhHrwRzPC8Zj81jV76UJv9Xv1RnQ/y8ICy4XrzzSbqlhaP9AWyihneP4kypzbahsxf+Hf6Iq0H6VSYTsb+lAu+s4J4P9/ObTyyc+yjZDdxXfcypICtbbqLZ7v5nxQ9dnHCf+Sp+90RzcZm1tgqsFi57Cin6mANmKbX5kTkKj36Rn8idRMgvMdvjOS0R/nhbS4ClM4CJY0EUmtt7xDe6hCDbkmkALC+I1gQxHuezKE7A1J5Wxs9Mamo/Gks2ekVjP/RYN/J/l8sYMA+iylq+H+UkSlpM28RMj4z7O+63SRldziaBW758xx/n3kexome9tVKouk2jKfL8EBMHVq7IZUcFY43KVvY2scxxEXCqer4IR02bN x-forefront-antispam-report: CIP:255.255.255.255; CTRY:; LANG:en; SCL:1; SRV:; IPV:NLI; SFV:NSPM; H:CH2PR10MB4344.namprd10.prod.outlook.com; PTR:; CAT:NONE; SFS:(13230001)(366004)(53546011)(26005)(186003)(966005)(6486002)(66946007)(83380400001)(86362001)(71200400001)(6506007)(6512007)(64756008)(8936002)(33656002)(316002)(6916009)(5660300002)(38100700002)(76116006)(4326008)(44832011)(66446008)(66556008)(122000001)(66476007)(8676002)(91956017)(2616005)(508600001)(38070700005)(36756003)(2906002)(45980500001); DIR:OUT; SFP:1101; x-ms-exchange-antispam-messagedata-chunkcount: 1 x-ms-exchange-antispam-messagedata-0: /7s/m3UyHGyAC5Kk4hDfbn8RIcXQXKWjNqCEhfAlfLzVc1jo754CgO4us6eBO4gW5XGmzOCAqMVcgifx6Ecm9fksMfy0YfDb2kWtN1/W86bCsk8XwUnI1A2QhI/2/YOogpYhazWnGPCNqeo3uWcGj2PV7Xh6Kwt1HEVWxeCwCuMCX1rTOulVCmys3B2k3F6uQjwCpBbOQyDHAFu1mWhz5fAvZ7N/4R8BSZYekrc4eWW/XAOegog6FMremz+mTxGIVQ5HYe5SnwOtEgg41rDdGXAMNt/PKATfyWqMwudPOJhLIDQl02pxT4gKWSG143HMXVJGTnR5jNDQwvWJY0J71bjtnplw0MRnnIc194dPMcYv6hSFxvwKAYJ1eRjK5jQTqTxpAdPeMb3peQlHzeA9vfso3qMa+WRrFECab3aP+qxxnRei8ZYsTufGfAVaxD3iHSb+KZzoiUuerhJSZpeOORQk3yfmu1TbqMMEeOTfzb7KBKGGi1uovfw/LMpY0fVavRmGq3zdOGYbLHOc37SVRHNRovOwC7yAABvXcbDvIVkaDr0gP1S3PPWRMoHuyI6TesUC5nmzRvIMsdj7cjDDpeZsuU//c4YwtcNUw9DmGdHJRCc/VhbzVRF2/c/4qrEYlv2f0vTuSIgYaBMAtXaH0EA7n10quncPbFl/HKkU7CSn+JfiHyd+FXkEmq5My54rP75BnoIAzGbZL3zOgbefV0UV+ZjHlLytRwk54ZCvaDQPKypbJMuNNfBHaqJi13nR0LokCM2iAn+qlYXOI8Mwn39HNIq9J+dle4XKaQ2NFP9R3z46n3UIe1oPVfdvYQBEPpqKHXqQgM58MxCYe/Rr+JZPris4Sv03X5rsBBo0IjLXYQvJJcS59bt63wcU/SRls2iXaF0HK1Zf3qo+pIfWlY+tt5GfmUQcukqDgkBhka73lURLkEJWGazSCeO7cq2Y9lbW1wsDgGlLMaCWnbO56IVvtkgCxtDgy3Fx2ypUyXy8spN/EvGR3F5Ra1VaxvxA7yuAK7BJSbsvEXMnOA8bt+hnBmXUsVXGxFHy7SIMgYEErQ4z7nD1dTYzIn1TydKkPCM9ggybk6slyRWShIRm/wMMR8HBRZBUZLs23SWz0vBVCu8XXuCDTO2UXBoPK/7mH5wLM2FE/5U7gI1MGbZOHHRMG2NPHF7dnkbiYfsEwz3/F1CCssv5/eS89PZnRdmrD+XQcWPogLGmgllB8adv/R+9CWAFmEu8jtffDMlGlrV3DZAJ5jxVkWkzNhVV01Yx/wOObZQelys6BSxQxixtSMxsBm+FhXn9IEcN/zSARLjkFKk90j0p5zsDL8zc6F2SH2PfV94Zt82FHs8lRumtTJWkU9svgriiplEIgMOaD4NodHYL9GdRWTFf+EfC17XEj+6S8W/GWkJ8epqHgriGzg00GIyxwBMNa661my4G9aWcDU8iglSYP/m0cKbgkPBQKpJdRnc6gHkaF3gYXYy9cBqxtMHMq6qCEUVK4JfbPT+PDnChJD9g+n+cthJ41vfcY0TSOrnVOhB4fA47GBbJ76c8DUHo699jj2Ox1hGBfcw4BO3QZaBEDv/zps1rVWlBRAR0seedrte63jPk6mLKEARSgbQvC/RgVdbn0eY4j0U= Content-Type: text/plain; charset="us-ascii" Content-ID: <3A4E0810EFC7214687DE000C2B86787A@namprd10.prod.outlook.com> Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-OriginatorOrg: oracle.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-AuthSource: CH2PR10MB4344.namprd10.prod.outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: c15b0994-056e-40ba-6188-08d9eb3f3775 X-MS-Exchange-CrossTenant-originalarrivaltime: 08 Feb 2022 20:11:39.4136 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: //AEFrHWmFXopIU+GazexhzNFKULrUVFxqsao2xi/1zcBuO3jCimetpgMtkfML/J+Zxn3k0aCDqCsbHBbu1k8A== X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN2PR10MB4000 X-Proofpoint-Virus-Version: vendor=nai engine=6300 definitions=10252 signatures=673431 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=0 mlxscore=0 bulkscore=0 phishscore=0 malwarescore=0 mlxlogscore=999 adultscore=0 spamscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2201110000 definitions=main-2202080119 X-Proofpoint-GUID: NiI0WGjGsHvwCYMm1fmd3wksUSbZXsGv X-Proofpoint-ORIG-GUID: NiI0WGjGsHvwCYMm1fmd3wksUSbZXsGv X-Spam-Status: No, score=-11.3 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H5, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list <gcc-patches.gcc.gnu.org> List-Unsubscribe: <https://gcc.gnu.org/mailman/options/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=unsubscribe> List-Archive: <https://gcc.gnu.org/pipermail/gcc-patches/> List-Post: <mailto:gcc-patches@gcc.gnu.org> List-Help: <mailto:gcc-patches-request@gcc.gnu.org?subject=help> List-Subscribe: <https://gcc.gnu.org/mailman/listinfo/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=subscribe> From: Qing Zhao via Gcc-patches <gcc-patches@gcc.gnu.org> Reply-To: Qing Zhao <qing.zhao@oracle.com> Cc: gcc-patches Paul A Clarke via <gcc-patches@gcc.gnu.org> Errors-To: gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org Sender: "Gcc-patches" <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> |
Series |
Fix PR 101515 (ICE in pp_cxx_unqualified_id, at cp/cxx-pretty-print.c:128)
|
|
Commit Message
Qing Zhao
Feb. 8, 2022, 8:11 p.m. UTC
Hi,
This is the patch to fix PR101515 (ICE in pp_cxx_unqualified_id, at cp/cxx-pretty-print.c:128)
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101515
It's possible that the TYPE_NAME of a record_type is NULL, therefore when
printing the TYPE_NAME, we should check and handle this special case.
Please see the comment of pr101515 for more details.
The fix is very simple, just check and special handle cases when TYPE_NAME is NULL.
Bootstrapped and regression tested on both x86 and aarch64, no issues.
Okay for commit?
Thanks.
Qing
=====================================
From f37ee8d21b80cb77d8108cb97a487c84c530545b Mon Sep 17 00:00:00 2001
From: Qing Zhao <qing.zhao@oracle.com>
Date: Tue, 8 Feb 2022 16:10:37 +0000
Subject: [PATCH] Fix PR 101515 ICE in pp_cxx_unqualified_id, at
cp/cxx-pretty-print.c:128.
It's possible that the TYPE_NAME of a record_type is NULL, therefore when
printing the TYPE_NAME, we should check and handle this special case.
gcc/cp/ChangeLog:
* cxx-pretty-print.cc (pp_cxx_unqualified_id): Check and handle
the case when TYPE_NAME is NULL.
gcc/testsuite/ChangeLog:
* g++.dg/pr101515.C: New test.
---
gcc/cp/cxx-pretty-print.cc | 5 ++++-
gcc/testsuite/g++.dg/pr101515.C | 25 +++++++++++++++++++++++++
2 files changed, 29 insertions(+), 1 deletion(-)
create mode 100644 gcc/testsuite/g++.dg/pr101515.C
Comments
On 2/8/22 15:11, Qing Zhao wrote: > Hi, > > This is the patch to fix PR101515 (ICE in pp_cxx_unqualified_id, at cp/cxx-pretty-print.c:128) > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101515 > > It's possible that the TYPE_NAME of a record_type is NULL, therefore when > printing the TYPE_NAME, we should check and handle this special case. > > Please see the comment of pr101515 for more details. > > The fix is very simple, just check and special handle cases when TYPE_NAME is NULL. > > Bootstrapped and regression tested on both x86 and aarch64, no issues. > > Okay for commit? > > Thanks. > > Qing > > ===================================== > > From f37ee8d21b80cb77d8108cb97a487c84c530545b Mon Sep 17 00:00:00 2001 > From: Qing Zhao <qing.zhao@oracle.com> > Date: Tue, 8 Feb 2022 16:10:37 +0000 > Subject: [PATCH] Fix PR 101515 ICE in pp_cxx_unqualified_id, at > cp/cxx-pretty-print.c:128. > > It's possible that the TYPE_NAME of a record_type is NULL, therefore when > printing the TYPE_NAME, we should check and handle this special case. > > gcc/cp/ChangeLog: > > * cxx-pretty-print.cc (pp_cxx_unqualified_id): Check and handle > the case when TYPE_NAME is NULL. > > gcc/testsuite/ChangeLog: > > * g++.dg/pr101515.C: New test. > --- > gcc/cp/cxx-pretty-print.cc | 5 ++++- > gcc/testsuite/g++.dg/pr101515.C | 25 +++++++++++++++++++++++++ > 2 files changed, 29 insertions(+), 1 deletion(-) > create mode 100644 gcc/testsuite/g++.dg/pr101515.C > > diff --git a/gcc/cp/cxx-pretty-print.cc b/gcc/cp/cxx-pretty-print.cc > index 4f9a090e520d..744ed0add5ba 100644 > --- a/gcc/cp/cxx-pretty-print.cc > +++ b/gcc/cp/cxx-pretty-print.cc > @@ -171,7 +171,10 @@ pp_cxx_unqualified_id (cxx_pretty_printer *pp, tree t) > case ENUMERAL_TYPE: > case TYPENAME_TYPE: > case UNBOUND_CLASS_TEMPLATE: > - pp_cxx_unqualified_id (pp, TYPE_NAME (t)); > + if (TYPE_NAME (t)) > + pp_cxx_unqualified_id (pp, TYPE_NAME (t)); > + else > + pp_string (pp, "<unnamed type>"); Hmm, but it's not an unnamed class, it's a pointer to member function type, and it would be better to avoid dumping compiler internal representations like the __pfn field name. I think the real problem comes sooner, when c_fold_indirect_ref_for_warn turns a MEM_REF with RECORD_TYPE into a COMPONENT_REF with POINTER_TYPE. > if (tree ti = TYPE_TEMPLATE_INFO_MAYBE_ALIAS (t)) > if (PRIMARY_TEMPLATE_P (TI_TEMPLATE (ti))) > { > diff --git a/gcc/testsuite/g++.dg/pr101515.C b/gcc/testsuite/g++.dg/pr101515.C > new file mode 100644 > index 000000000000..898c7e003c22 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/pr101515.C > @@ -0,0 +1,25 @@ > +/* PR101515 - ICE in pp_cxx_unqualified_id, at cp/cxx-pretty-print.c:128 > + { dg-do compile } > + { dg-options "-Wuninitialized -O1" } */ > + > +struct S > +{ > + int j; > +}; > +struct T : public S > +{ > + virtual void h () {} > +}; > +struct ptrmemfunc > +{ > + void (*ptr) (); > +}; > +typedef void (S::*sp)(); > +int main () > +{ > + T t; > + sp x; > + ptrmemfunc *xp = (ptrmemfunc *) &x; > + if (xp->ptr != ((void (*)())(sizeof(void *)))) /* { dg-warning "is used uninitialized" } */ > + return 1; > +}
> On Feb 8, 2022, at 4:20 PM, Jason Merrill <jason@redhat.com> wrote: > > On 2/8/22 15:11, Qing Zhao wrote: >> Hi, >> This is the patch to fix PR101515 (ICE in pp_cxx_unqualified_id, at cp/cxx-pretty-print.c:128) >> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101515 >> It's possible that the TYPE_NAME of a record_type is NULL, therefore when >> printing the TYPE_NAME, we should check and handle this special case. >> Please see the comment of pr101515 for more details. >> The fix is very simple, just check and special handle cases when TYPE_NAME is NULL. >> Bootstrapped and regression tested on both x86 and aarch64, no issues. >> Okay for commit? >> Thanks. >> Qing >> ===================================== >> From f37ee8d21b80cb77d8108cb97a487c84c530545b Mon Sep 17 00:00:00 2001 >> From: Qing Zhao <qing.zhao@oracle.com> >> Date: Tue, 8 Feb 2022 16:10:37 +0000 >> Subject: [PATCH] Fix PR 101515 ICE in pp_cxx_unqualified_id, at >> cp/cxx-pretty-print.c:128. >> It's possible that the TYPE_NAME of a record_type is NULL, therefore when >> printing the TYPE_NAME, we should check and handle this special case. >> gcc/cp/ChangeLog: >> * cxx-pretty-print.cc (pp_cxx_unqualified_id): Check and handle >> the case when TYPE_NAME is NULL. >> gcc/testsuite/ChangeLog: >> * g++.dg/pr101515.C: New test. >> --- >> gcc/cp/cxx-pretty-print.cc | 5 ++++- >> gcc/testsuite/g++.dg/pr101515.C | 25 +++++++++++++++++++++++++ >> 2 files changed, 29 insertions(+), 1 deletion(-) >> create mode 100644 gcc/testsuite/g++.dg/pr101515.C >> diff --git a/gcc/cp/cxx-pretty-print.cc b/gcc/cp/cxx-pretty-print.cc >> index 4f9a090e520d..744ed0add5ba 100644 >> --- a/gcc/cp/cxx-pretty-print.cc >> +++ b/gcc/cp/cxx-pretty-print.cc >> @@ -171,7 +171,10 @@ pp_cxx_unqualified_id (cxx_pretty_printer *pp, tree t) >> case ENUMERAL_TYPE: >> case TYPENAME_TYPE: >> case UNBOUND_CLASS_TEMPLATE: >> - pp_cxx_unqualified_id (pp, TYPE_NAME (t)); >> + if (TYPE_NAME (t)) >> + pp_cxx_unqualified_id (pp, TYPE_NAME (t)); >> + else >> + pp_string (pp, "<unnamed type>"); > > Hmm, but it's not an unnamed class, it's a pointer to member function type, and it would be better to avoid dumping compiler internal representations like the __pfn field name. Yes, It’s not an unnamed class, but the ICE happened when try to print the compiler generated member function type “__ptrmemfunc_type”, whose TYPE_NAME is NULLed during building this type in c++ FE and the c++ FE does not handle the case when TYPE_NAME is NULL correctly. So, there are two levels of issues: 1. The first level issue is that the current C++ FE does not handle the case TYPE_NAME being NULL correctly, this is indeed a bug in the current code and should be fixed as in the current patch. 2. The second level issue is what you suggested in the above, shall we print the “compiler generated internal type” to the user? And I agree with you that it might not be a good idea to print such compiler internal names to the user. Are we do this right now in general? (i.e, check whether the current TYPE is a source level TYPE or a compiler internal TYPE, and then only print out the name of TYPE for the source level TYPE?) and is there a bit in the TYPE to distinguish whether a TYPE is user -level type or a compiler generated internal type? > I think the real problem comes sooner, when c_fold_indirect_ref_for_warn turns a MEM_REF with RECORD_TYPE into a COMPONENT_REF with POINTER_TYPE. What’s the major issue for this transformation? (I will study this in more details). thanks. Qing > >> if (tree ti = TYPE_TEMPLATE_INFO_MAYBE_ALIAS (t)) >> if (PRIMARY_TEMPLATE_P (TI_TEMPLATE (ti))) >> { >> diff --git a/gcc/testsuite/g++.dg/pr101515.C b/gcc/testsuite/g++.dg/pr101515.C >> new file mode 100644 >> index 000000000000..898c7e003c22 >> --- /dev/null >> +++ b/gcc/testsuite/g++.dg/pr101515.C >> @@ -0,0 +1,25 @@ >> +/* PR101515 - ICE in pp_cxx_unqualified_id, at cp/cxx-pretty-print.c:128 >> + { dg-do compile } >> + { dg-options "-Wuninitialized -O1" } */ >> + >> +struct S >> +{ >> + int j; >> +}; >> +struct T : public S >> +{ >> + virtual void h () {} >> +}; >> +struct ptrmemfunc >> +{ >> + void (*ptr) (); >> +}; >> +typedef void (S::*sp)(); >> +int main () >> +{ >> + T t; >> + sp x; >> + ptrmemfunc *xp = (ptrmemfunc *) &x; >> + if (xp->ptr != ((void (*)())(sizeof(void *)))) /* { dg-warning "is used uninitialized" } */ >> + return 1; >> +} >
On 2/9/22 10:51, Qing Zhao wrote: > > >> On Feb 8, 2022, at 4:20 PM, Jason Merrill <jason@redhat.com> wrote: >> >> On 2/8/22 15:11, Qing Zhao wrote: >>> Hi, >>> This is the patch to fix PR101515 (ICE in pp_cxx_unqualified_id, at cp/cxx-pretty-print.c:128) >>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101515 >>> It's possible that the TYPE_NAME of a record_type is NULL, therefore when >>> printing the TYPE_NAME, we should check and handle this special case. >>> Please see the comment of pr101515 for more details. >>> The fix is very simple, just check and special handle cases when TYPE_NAME is NULL. >>> Bootstrapped and regression tested on both x86 and aarch64, no issues. >>> Okay for commit? >>> Thanks. >>> Qing >>> ===================================== >>> From f37ee8d21b80cb77d8108cb97a487c84c530545b Mon Sep 17 00:00:00 2001 >>> From: Qing Zhao <qing.zhao@oracle.com> >>> Date: Tue, 8 Feb 2022 16:10:37 +0000 >>> Subject: [PATCH] Fix PR 101515 ICE in pp_cxx_unqualified_id, at >>> cp/cxx-pretty-print.c:128. >>> It's possible that the TYPE_NAME of a record_type is NULL, therefore when >>> printing the TYPE_NAME, we should check and handle this special case. >>> gcc/cp/ChangeLog: >>> * cxx-pretty-print.cc (pp_cxx_unqualified_id): Check and handle >>> the case when TYPE_NAME is NULL. >>> gcc/testsuite/ChangeLog: >>> * g++.dg/pr101515.C: New test. >>> --- >>> gcc/cp/cxx-pretty-print.cc | 5 ++++- >>> gcc/testsuite/g++.dg/pr101515.C | 25 +++++++++++++++++++++++++ >>> 2 files changed, 29 insertions(+), 1 deletion(-) >>> create mode 100644 gcc/testsuite/g++.dg/pr101515.C >>> diff --git a/gcc/cp/cxx-pretty-print.cc b/gcc/cp/cxx-pretty-print.cc >>> index 4f9a090e520d..744ed0add5ba 100644 >>> --- a/gcc/cp/cxx-pretty-print.cc >>> +++ b/gcc/cp/cxx-pretty-print.cc >>> @@ -171,7 +171,10 @@ pp_cxx_unqualified_id (cxx_pretty_printer *pp, tree t) >>> case ENUMERAL_TYPE: >>> case TYPENAME_TYPE: >>> case UNBOUND_CLASS_TEMPLATE: >>> - pp_cxx_unqualified_id (pp, TYPE_NAME (t)); >>> + if (TYPE_NAME (t)) >>> + pp_cxx_unqualified_id (pp, TYPE_NAME (t)); >>> + else >>> + pp_string (pp, "<unnamed type>"); >> >> Hmm, but it's not an unnamed class, it's a pointer to member function type, and it would be better to avoid dumping compiler internal representations like the __pfn field name. > Yes, It’s not an unnamed class, but the ICE happened when try to print the compiler generated member function type “__ptrmemfunc_type”, whose TYPE_NAME is NULLed during building this type in c++ FE and the c++ FE does not handle the case when TYPE_NAME is NULL correctly. > > So, there are two levels of issues: > > 1. The first level issue is that the current C++ FE does not handle the case TYPE_NAME being NULL correctly, this is indeed a bug in the current code and should be fixed as in the current patch. Sure, we might as well make this code more robust. But we can do better than <unnamed type> if we check TYPE_PTRMEMFUNC_P. > 2. The second level issue is what you suggested in the above, shall we print the “compiler generated internal type” to the user? And I agree with you that it might not be a good idea to print such compiler internal names to the user. Are we do this right now in general? (i.e, check whether the current TYPE is a source level TYPE or a compiler internal TYPE, and then only print out the name of TYPE for the source level TYPE?) and is there a bit in the TYPE to distinguish whether a TYPE is user -level type or a compiler generated internal type? >> I think the real problem comes sooner, when c_fold_indirect_ref_for_warn turns a MEM_REF with RECORD_TYPE into a COMPONENT_REF with POINTER_TYPE. > What’s the major issue for this transformation? (I will study this in more details). We told c_fold_indirect_ref that we want a RECORD_TYPE (the PMF as a whole) and it gave us back a POINTER_TYPE instead (the __pmf member). Folding shouldn't change the type of an expression like that. Jason
> On Feb 9, 2022, at 12:23 PM, Jason Merrill <jason@redhat.com> wrote: > > On 2/9/22 10:51, Qing Zhao wrote: >>> On Feb 8, 2022, at 4:20 PM, Jason Merrill <jason@redhat.com> wrote: >>> >>> On 2/8/22 15:11, Qing Zhao wrote: >>>> Hi, >>>> This is the patch to fix PR101515 (ICE in pp_cxx_unqualified_id, at cp/cxx-pretty-print.c:128) >>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101515 >>>> It's possible that the TYPE_NAME of a record_type is NULL, therefore when >>>> printing the TYPE_NAME, we should check and handle this special case. >>>> Please see the comment of pr101515 for more details. >>>> The fix is very simple, just check and special handle cases when TYPE_NAME is NULL. >>>> Bootstrapped and regression tested on both x86 and aarch64, no issues. >>>> Okay for commit? >>>> Thanks. >>>> Qing >>>> ===================================== >>>> From f37ee8d21b80cb77d8108cb97a487c84c530545b Mon Sep 17 00:00:00 2001 >>>> From: Qing Zhao <qing.zhao@oracle.com> >>>> Date: Tue, 8 Feb 2022 16:10:37 +0000 >>>> Subject: [PATCH] Fix PR 101515 ICE in pp_cxx_unqualified_id, at >>>> cp/cxx-pretty-print.c:128. >>>> It's possible that the TYPE_NAME of a record_type is NULL, therefore when >>>> printing the TYPE_NAME, we should check and handle this special case. >>>> gcc/cp/ChangeLog: >>>> * cxx-pretty-print.cc (pp_cxx_unqualified_id): Check and handle >>>> the case when TYPE_NAME is NULL. >>>> gcc/testsuite/ChangeLog: >>>> * g++.dg/pr101515.C: New test. >>>> --- >>>> gcc/cp/cxx-pretty-print.cc | 5 ++++- >>>> gcc/testsuite/g++.dg/pr101515.C | 25 +++++++++++++++++++++++++ >>>> 2 files changed, 29 insertions(+), 1 deletion(-) >>>> create mode 100644 gcc/testsuite/g++.dg/pr101515.C >>>> diff --git a/gcc/cp/cxx-pretty-print.cc b/gcc/cp/cxx-pretty-print.cc >>>> index 4f9a090e520d..744ed0add5ba 100644 >>>> --- a/gcc/cp/cxx-pretty-print.cc >>>> +++ b/gcc/cp/cxx-pretty-print.cc >>>> @@ -171,7 +171,10 @@ pp_cxx_unqualified_id (cxx_pretty_printer *pp, tree t) >>>> case ENUMERAL_TYPE: >>>> case TYPENAME_TYPE: >>>> case UNBOUND_CLASS_TEMPLATE: >>>> - pp_cxx_unqualified_id (pp, TYPE_NAME (t)); >>>> + if (TYPE_NAME (t)) >>>> + pp_cxx_unqualified_id (pp, TYPE_NAME (t)); >>>> + else >>>> + pp_string (pp, "<unnamed type>"); >>> >>> Hmm, but it's not an unnamed class, it's a pointer to member function type, and it would be better to avoid dumping compiler internal representations like the __pfn field name. >> Yes, It’s not an unnamed class, but the ICE happened when try to print the compiler generated member function type “__ptrmemfunc_type”, whose TYPE_NAME is NULLed during building this type in c++ FE and the c++ FE does not handle the case when TYPE_NAME is NULL correctly. >> So, there are two levels of issues: >> 1. The first level issue is that the current C++ FE does not handle the case TYPE_NAME being NULL correctly, this is indeed a bug in the current code and should be fixed as in the current patch. > > Sure, we might as well make this code more robust. But we can do better than <unnamed type> if we check TYPE_PTRMEMFUNC_P. Okay, so what should we print to the user if it's “TYPE_PTRMEMFUNC_P”? Print nothing or some special string? > >> 2. The second level issue is what you suggested in the above, shall we print the “compiler generated internal type” to the user? And I agree with you that it might not be a good idea to print such compiler internal names to the user. Are we do this right now in general? (i.e, check whether the current TYPE is a source level TYPE or a compiler internal TYPE, and then only print out the name of TYPE for the source level TYPE?) and is there a bit in the TYPE to distinguish whether a TYPE is user -level type or a compiler generated internal type? > >>> I think the real problem comes sooner, when c_fold_indirect_ref_for_warn turns a MEM_REF with RECORD_TYPE into a COMPONENT_REF with POINTER_TYPE. > >> What’s the major issue for this transformation? (I will study this in more details). > > We told c_fold_indirect_ref that we want a RECORD_TYPE (the PMF as a whole) and it gave us back a POINTER_TYPE instead (the __pmf member). Folding shouldn't change the type of an expression like that. Yes, this is not correct transformation, will study in more detail and try to fix it. Qing > > Jason
On 2/9/22 16:01, Qing Zhao wrote: > > >> On Feb 9, 2022, at 12:23 PM, Jason Merrill <jason@redhat.com> wrote: >> >> On 2/9/22 10:51, Qing Zhao wrote: >>>> On Feb 8, 2022, at 4:20 PM, Jason Merrill <jason@redhat.com> wrote: >>>> >>>> On 2/8/22 15:11, Qing Zhao wrote: >>>>> Hi, >>>>> This is the patch to fix PR101515 (ICE in pp_cxx_unqualified_id, at cp/cxx-pretty-print.c:128) >>>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101515 >>>>> It's possible that the TYPE_NAME of a record_type is NULL, therefore when >>>>> printing the TYPE_NAME, we should check and handle this special case. >>>>> Please see the comment of pr101515 for more details. >>>>> The fix is very simple, just check and special handle cases when TYPE_NAME is NULL. >>>>> Bootstrapped and regression tested on both x86 and aarch64, no issues. >>>>> Okay for commit? >>>>> Thanks. >>>>> Qing >>>>> ===================================== >>>>> From f37ee8d21b80cb77d8108cb97a487c84c530545b Mon Sep 17 00:00:00 2001 >>>>> From: Qing Zhao <qing.zhao@oracle.com> >>>>> Date: Tue, 8 Feb 2022 16:10:37 +0000 >>>>> Subject: [PATCH] Fix PR 101515 ICE in pp_cxx_unqualified_id, at >>>>> cp/cxx-pretty-print.c:128. >>>>> It's possible that the TYPE_NAME of a record_type is NULL, therefore when >>>>> printing the TYPE_NAME, we should check and handle this special case. >>>>> gcc/cp/ChangeLog: >>>>> * cxx-pretty-print.cc (pp_cxx_unqualified_id): Check and handle >>>>> the case when TYPE_NAME is NULL. >>>>> gcc/testsuite/ChangeLog: >>>>> * g++.dg/pr101515.C: New test. >>>>> --- >>>>> gcc/cp/cxx-pretty-print.cc | 5 ++++- >>>>> gcc/testsuite/g++.dg/pr101515.C | 25 +++++++++++++++++++++++++ >>>>> 2 files changed, 29 insertions(+), 1 deletion(-) >>>>> create mode 100644 gcc/testsuite/g++.dg/pr101515.C >>>>> diff --git a/gcc/cp/cxx-pretty-print.cc b/gcc/cp/cxx-pretty-print.cc >>>>> index 4f9a090e520d..744ed0add5ba 100644 >>>>> --- a/gcc/cp/cxx-pretty-print.cc >>>>> +++ b/gcc/cp/cxx-pretty-print.cc >>>>> @@ -171,7 +171,10 @@ pp_cxx_unqualified_id (cxx_pretty_printer *pp, tree t) >>>>> case ENUMERAL_TYPE: >>>>> case TYPENAME_TYPE: >>>>> case UNBOUND_CLASS_TEMPLATE: >>>>> - pp_cxx_unqualified_id (pp, TYPE_NAME (t)); >>>>> + if (TYPE_NAME (t)) >>>>> + pp_cxx_unqualified_id (pp, TYPE_NAME (t)); >>>>> + else >>>>> + pp_string (pp, "<unnamed type>"); >>>> >>>> Hmm, but it's not an unnamed class, it's a pointer to member function type, and it would be better to avoid dumping compiler internal representations like the __pfn field name. >>> Yes, It’s not an unnamed class, but the ICE happened when try to print the compiler generated member function type “__ptrmemfunc_type”, whose TYPE_NAME is NULLed during building this type in c++ FE and the c++ FE does not handle the case when TYPE_NAME is NULL correctly. >>> So, there are two levels of issues: >>> 1. The first level issue is that the current C++ FE does not handle the case TYPE_NAME being NULL correctly, this is indeed a bug in the current code and should be fixed as in the current patch. >> >> Sure, we might as well make this code more robust. But we can do better than <unnamed type> if we check TYPE_PTRMEMFUNC_P. > Okay, so what should we print to the user if it's “TYPE_PTRMEMFUNC_P”? Print nothing or some special string? Maybe call pp->type_id in that case. >>> 2. The second level issue is what you suggested in the above, shall we print the “compiler generated internal type” to the user? And I agree with you that it might not be a good idea to print such compiler internal names to the user. Are we do this right now in general? (i.e, check whether the current TYPE is a source level TYPE or a compiler internal TYPE, and then only print out the name of TYPE for the source level TYPE?) and is there a bit in the TYPE to distinguish whether a TYPE is user -level type or a compiler generated internal type? >> >>>> I think the real problem comes sooner, when c_fold_indirect_ref_for_warn turns a MEM_REF with RECORD_TYPE into a COMPONENT_REF with POINTER_TYPE. >> >>> What’s the major issue for this transformation? (I will study this in more details). >> >> We told c_fold_indirect_ref that we want a RECORD_TYPE (the PMF as a whole) and it gave us back a POINTER_TYPE instead (the __pmf member). Folding shouldn't change the type of an expression like that. > > Yes, this is not correct transformation, will study in more detail and try to fix it. > > Qing >> >> Jason >
Hi, Jason,
On Feb 9, 2022, at 3:01 PM, Qing Zhao via Gcc-patches <gcc-patches@gcc.gnu.org<mailto:gcc-patches@gcc.gnu.org>> wrote:
On Feb 9, 2022, at 12:23 PM, Jason Merrill <jason@redhat.com<mailto:jason@redhat.com>> wrote:
On 2/9/22 10:51, Qing Zhao wrote:
On Feb 8, 2022, at 4:20 PM, Jason Merrill <jason@redhat.com<mailto:jason@redhat.com>> wrote:
On 2/8/22 15:11, Qing Zhao wrote:
Hi,
This is the patch to fix PR101515 (ICE in pp_cxx_unqualified_id, at cp/cxx-pretty-print.c:128)
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101515
It's possible that the TYPE_NAME of a record_type is NULL, therefore when
printing the TYPE_NAME, we should check and handle this special case.
Please see the comment of pr101515 for more details.
The fix is very simple, just check and special handle cases when TYPE_NAME is NULL.
Bootstrapped and regression tested on both x86 and aarch64, no issues.
Okay for commit?
Thanks.
Qing
=====================================
From f37ee8d21b80cb77d8108cb97a487c84c530545b Mon Sep 17 00:00:00 2001
From: Qing Zhao <qing.zhao@oracle.com>
Date: Tue, 8 Feb 2022 16:10:37 +0000
Subject: [PATCH] Fix PR 101515 ICE in pp_cxx_unqualified_id, at
cp/cxx-pretty-print.c:128.
It's possible that the TYPE_NAME of a record_type is NULL, therefore when
printing the TYPE_NAME, we should check and handle this special case.
gcc/cp/ChangeLog:
* cxx-pretty-print.cc (pp_cxx_unqualified_id): Check and handle
the case when TYPE_NAME is NULL.
gcc/testsuite/ChangeLog:
* g++.dg/pr101515.C: New test.
---
gcc/cp/cxx-pretty-print.cc | 5 ++++-
gcc/testsuite/g++.dg/pr101515.C | 25 +++++++++++++++++++++++++
2 files changed, 29 insertions(+), 1 deletion(-)
create mode 100644 gcc/testsuite/g++.dg/pr101515.C
diff --git a/gcc/cp/cxx-pretty-print.cc b/gcc/cp/cxx-pretty-print.cc
index 4f9a090e520d..744ed0add5ba 100644
--- a/gcc/cp/cxx-pretty-print.cc
+++ b/gcc/cp/cxx-pretty-print.cc
@@ -171,7 +171,10 @@ pp_cxx_unqualified_id (cxx_pretty_printer *pp, tree t)
case ENUMERAL_TYPE:
case TYPENAME_TYPE:
case UNBOUND_CLASS_TEMPLATE:
- pp_cxx_unqualified_id (pp, TYPE_NAME (t));
+ if (TYPE_NAME (t))
+ pp_cxx_unqualified_id (pp, TYPE_NAME (t));
+ else
+ pp_string (pp, "<unnamed type>");
Hmm, but it's not an unnamed class, it's a pointer to member function type, and it would be better to avoid dumping compiler internal representations like the __pfn field name.
Yes, It’s not an unnamed class, but the ICE happened when try to print the compiler generated member function type “__ptrmemfunc_type”, whose TYPE_NAME is NULLed during building this type in c++ FE and the c++ FE does not handle the case when TYPE_NAME is NULL correctly.
So, there are two levels of issues:
1. The first level issue is that the current C++ FE does not handle the case TYPE_NAME being NULL correctly, this is indeed a bug in the current code and should be fixed as in the current patch.
Sure, we might as well make this code more robust. But we can do better than <unnamed type> if we check TYPE_PTRMEMFUNC_P.
Okay, so what should we print to the user if it's “TYPE_PTRMEMFUNC_P”? Print nothing or some special string?
2. The second level issue is what you suggested in the above, shall we print the “compiler generated internal type” to the user? And I agree with you that it might not be a good idea to print such compiler internal names to the user. Are we do this right now in general? (i.e, check whether the current TYPE is a source level TYPE or a compiler internal TYPE, and then only print out the name of TYPE for the source level TYPE?) and is there a bit in the TYPE to distinguish whether a TYPE is user -level type or a compiler generated internal type?
I think the real problem comes sooner, when c_fold_indirect_ref_for_warn turns a MEM_REF with RECORD_TYPE into a COMPONENT_REF with POINTER_TYPE.
What’s the major issue for this transformation? (I will study this in more details).
We told c_fold_indirect_ref that we want a RECORD_TYPE (the PMF as a whole) and it gave us back a POINTER_TYPE instead (the __pmf member). Folding shouldn't change the type of an expression like that.
Yes, this is not correct transformation, will study in more detail and try to fix it.
After a deeper study of commit r11-6729-gadb520606ce3e1e1 (which triggered the ICE and introduced the new routine “c_fold_indirect_ref_for_warn”), from my understanding, the above transformation from a RECORD_TYPE (the PMF as a whole) to POINTER_TYPE (the __pmf member) is what the function intended to do as following:
1823 static tree
1824 c_fold_indirect_ref_for_warn (location_t loc, tree type, tree op,
1825 offset_int &off)
1826 {
…
1870 /* ((foo *)&struct_with_foo_field)[x] => COMPONENT_REF */
1871 else if (TREE_CODE (optype) == RECORD_TYPE)
1872 {
1873 for (tree field = TYPE_FIELDS (optype);
1874 field; field = DECL_CHAIN (field))
1875 if (TREE_CODE (field) == FIELD_DECL
…
1886 if (upos <= off && off < upos + el_sz)
1887 {
1888 tree cop = build3_loc (loc, COMPONENT_REF, TREE_TYPE (field),
1889 op, field, NULL_TREE);
1890 off = off - upos;
The above code was used to transform a MEM_REF to a RECORD_TYPE to a COMPONENT_REF to the corresponding FIELD. And then finally print out this COMPONENT_REF as a BASE + offsetof (field), in which the “field” is the compiler generated __pmf member field of the class, and its TYPE_NAME is NULL. Since the current code of “cp/cxx-pretty-print.cc<http://cxx-pretty-print.cc>” does NOT handle the case when TYPE_NAME==NULL, therefore the ICE.
So, I still think that the major issue with this bug is cp/cxx-pretty-print.cc<http://cxx-pretty-print.cc> does not handle the case when TYPE_NAME==NULL. This clearly is a bug that need to be fixed.
Let me know if you have more comments and suggestions.
thanks.
Qing
Qing
Jason
On 2/11/22 11:07, Qing Zhao wrote: > Hi, Jason, > >> On Feb 9, 2022, at 3:01 PM, Qing Zhao via Gcc-patches >> <gcc-patches@gcc.gnu.org <mailto:gcc-patches@gcc.gnu.org>> wrote: >> >> >> >>> On Feb 9, 2022, at 12:23 PM, Jason Merrill <jason@redhat.com >>> <mailto:jason@redhat.com>> wrote: >>> >>> On 2/9/22 10:51, Qing Zhao wrote: >>>>> On Feb 8, 2022, at 4:20 PM, Jason Merrill <jason@redhat.com >>>>> <mailto:jason@redhat.com>> wrote: >>>>> >>>>> On 2/8/22 15:11, Qing Zhao wrote: >>>>>> Hi, >>>>>> This is the patch to fix PR101515 (ICE in pp_cxx_unqualified_id, >>>>>> at cp/cxx-pretty-print.c:128) >>>>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101515 >>>>>> <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101515> >>>>>> It's possible that the TYPE_NAME of a record_type is NULL, >>>>>> therefore when >>>>>> printing the TYPE_NAME, we should check and handle this special case. >>>>>> Please see the comment of pr101515 for more details. >>>>>> The fix is very simple, just check and special handle cases when >>>>>> TYPE_NAME is NULL. >>>>>> Bootstrapped and regression tested on both x86 and aarch64, no issues. >>>>>> Okay for commit? >>>>>> Thanks. >>>>>> Qing >>>>>> ===================================== >>>>>> From f37ee8d21b80cb77d8108cb97a487c84c530545b Mon Sep 17 00:00:00 2001 >>>>>> From: Qing Zhao <qing.zhao@oracle.com> >>>>>> Date: Tue, 8 Feb 2022 16:10:37 +0000 >>>>>> Subject: [PATCH] Fix PR 101515 ICE in pp_cxx_unqualified_id, at >>>>>> cp/cxx-pretty-print.c:128. >>>>>> It's possible that the TYPE_NAME of a record_type is NULL, >>>>>> therefore when >>>>>> printing the TYPE_NAME, we should check and handle this special case. >>>>>> gcc/cp/ChangeLog: >>>>>> * cxx-pretty-print.cc (pp_cxx_unqualified_id): Check and handle >>>>>> the case when TYPE_NAME is NULL. >>>>>> gcc/testsuite/ChangeLog: >>>>>> * g++.dg/pr101515.C: New test. >>>>>> --- >>>>>> gcc/cp/cxx-pretty-print.cc | 5 ++++- >>>>>> gcc/testsuite/g++.dg/pr101515.C | 25 +++++++++++++++++++++++++ >>>>>> 2 files changed, 29 insertions(+), 1 deletion(-) >>>>>> create mode 100644 gcc/testsuite/g++.dg/pr101515.C >>>>>> diff --git a/gcc/cp/cxx-pretty-print.cc b/gcc/cp/cxx-pretty-print.cc >>>>>> index 4f9a090e520d..744ed0add5ba 100644 >>>>>> --- a/gcc/cp/cxx-pretty-print.cc >>>>>> +++ b/gcc/cp/cxx-pretty-print.cc >>>>>> @@ -171,7 +171,10 @@ pp_cxx_unqualified_id (cxx_pretty_printer >>>>>> *pp, tree t) >>>>>> case ENUMERAL_TYPE: >>>>>> case TYPENAME_TYPE: >>>>>> case UNBOUND_CLASS_TEMPLATE: >>>>>> - pp_cxx_unqualified_id (pp, TYPE_NAME (t)); >>>>>> + if (TYPE_NAME (t)) >>>>>> +pp_cxx_unqualified_id (pp, TYPE_NAME (t)); >>>>>> + else >>>>>> +pp_string (pp, "<unnamed type>"); >>>>> >>>>> Hmm, but it's not an unnamed class, it's a pointer to member >>>>> function type, and it would be better to avoid dumping compiler >>>>> internal representations like the __pfn field name. >>>> Yes, It’s not an unnamed class, but the ICE happened when try to >>>> print the compiler generated member function type >>>> “__ptrmemfunc_type”, whose TYPE_NAME is NULLed during building this >>>> type in c++ FE and the c++ FE does not handle the case when >>>> TYPE_NAME is NULL correctly. >>>> So, there are two levels of issues: >>>> 1. The first level issue is that the current C++ FE does not handle >>>> the case TYPE_NAME being NULL correctly, this is indeed a bug in the >>>> current code and should be fixed as in the current patch. >>> >>> Sure, we might as well make this code more robust. But we can do >>> better than <unnamed type> if we check TYPE_PTRMEMFUNC_P. >> Okay, so what should we print to the user if it's “TYPE_PTRMEMFUNC_P”? >> Print nothing or some special string? >>> >>>> 2. The second level issue is what you suggested in the above, shall >>>> we print the “compiler generated internal type” to the user? And I >>>> agree with you that it might not be a good idea to print such >>>> compiler internal names to the user. Are we do this right now in >>>> general? (i.e, check whether the current TYPE is a source level TYPE >>>> or a compiler internal TYPE, and then only print out the name of >>>> TYPE for the source level TYPE?) and is there a bit in the TYPE to >>>> distinguish whether a TYPE is user -level type or a compiler >>>> generated internal type? >>> >>>>> I think the real problem comes sooner, when >>>>> c_fold_indirect_ref_for_warn turns a MEM_REF with RECORD_TYPE into >>>>> a COMPONENT_REF with POINTER_TYPE. >>> >>>> What’s the major issue for this transformation? (I will study this >>>> in more details). >>> >>> We told c_fold_indirect_ref that we want a RECORD_TYPE (the PMF as a >>> whole) and it gave us back a POINTER_TYPE instead (the __pmf member). >>> Folding shouldn't change the type of an expression like that. >> >> Yes, this is not correct transformation, will study in more detail and >> try to fix it. > > After a deeper study of commit r11-6729-gadb520606ce3e1e1 (which > triggered the ICE and introduced the new routine > “c_fold_indirect_ref_for_warn”), from my understanding, the above > transformation from a RECORD_TYPE (the PMF as a whole) to POINTER_TYPE > (the __pmf member) is what the function intended to do as following: > > 1823 static tree > 1824 c_fold_indirect_ref_for_warn (location_t loc, tree type, tree op, > 1825 offset_int &off) > 1826 { > … > 1870 */* ((foo *)&struct_with_foo_field)[x] => COMPONENT_REF */* > 1871 else if (TREE_CODE (optype) == RECORD_TYPE) > 1872 { > 1873 for (tree field = TYPE_FIELDS (optype); > 1874 field; field = DECL_CHAIN (field)) > 1875 if (TREE_CODE (field) == FIELD_DECL > … > 1886 if(upos <= off && off < upos + el_sz) > 1887 { > 1888 tree cop = build3_loc (loc, COMPONENT_REF, > TREE_TYPE (field), > 1889 op, field, NULL_TREE); > 1890 off = off - upos; > > The above code was used to transform a MEM_REF to a RECORD_TYPE to a > COMPONENT_REF to the corresponding FIELD. Yes, that's what the above code would correctly do if TYPE were the pointer-to-method type. It's wrong for this case because TYPE is unrelated to TREE_TYPE (field). I think the problem is just this line: > if (tree ret = c_fold_indirect_ref_for_warn (loc, type, cop, > off)) > return ret; > return cop; ^^^^^^^^^^ The recursive call does the proper type checking, but then the "return cop" line returns the COMPONENT_REF even though the type check failed. The parallel code in cxx_fold_indirect_ref_1 doesn't have this line, and removing it fixes the testcase, so I see warning: ‘*(ptrmemfunc*)&x.ptrmemfunc::ptr’ is used uninitialized Jason
Hi, Jason, > On Feb 11, 2022, at 11:27 AM, Jason Merrill <jason@redhat.com> wrote: >>>> >>>> Sure, we might as well make this code more robust. But we can do better than <unnamed type> if we check TYPE_PTRMEMFUNC_P. >>> Okay, so what should we print to the user if it's “TYPE_PTRMEMFUNC_P”? Print nothing or some special string? >>>> >>>>> 2. The second level issue is what you suggested in the above, shall we print the “compiler generated internal type” to the user? And I agree with you that it might not be a good idea to print such compiler internal names to the user. Are we do this right now in general? (i.e, check whether the current TYPE is a source level TYPE or a compiler internal TYPE, and then only print out the name of TYPE for the source level TYPE?) and is there a bit in the TYPE to distinguish whether a TYPE is user -level type or a compiler generated internal type? >>>> >>>>>> I think the real problem comes sooner, when c_fold_indirect_ref_for_warn turns a MEM_REF with RECORD_TYPE into a COMPONENT_REF with POINTER_TYPE. >>>> >>>>> What’s the major issue for this transformation? (I will study this in more details). >>>> >>>> We told c_fold_indirect_ref that we want a RECORD_TYPE (the PMF as a whole) and it gave us back a POINTER_TYPE instead (the __pmf member). Folding shouldn't change the type of an expression like that. >>> >>> Yes, this is not correct transformation, will study in more detail and try to fix it. >> After a deeper study of commit r11-6729-gadb520606ce3e1e1 (which triggered the ICE and introduced the new routine “c_fold_indirect_ref_for_warn”), from my understanding, the above transformation from a RECORD_TYPE (the PMF as a whole) to POINTER_TYPE (the __pmf member) is what the function intended to do as following: >> 1823 static tree >> 1824 c_fold_indirect_ref_for_warn (location_t loc, tree type, tree op, >> 1825 offset_int &off) >> 1826 { >> … >> 1870 */* ((foo *)&struct_with_foo_field)[x] => COMPONENT_REF */* >> 1871 else if (TREE_CODE (optype) == RECORD_TYPE) >> 1872 { >> 1873 for (tree field = TYPE_FIELDS (optype); >> 1874 field; field = DECL_CHAIN (field)) >> 1875 if (TREE_CODE (field) == FIELD_DECL >> … >> 1886 if(upos <= off && off < upos + el_sz) >> 1887 { >> 1888 tree cop = build3_loc (loc, COMPONENT_REF, TREE_TYPE (field), >> 1889 op, field, NULL_TREE); >> 1890 off = off - upos; >> The above code was used to transform a MEM_REF to a RECORD_TYPE to a COMPONENT_REF to the corresponding FIELD. > > Yes, that's what the above code would correctly do if TYPE were the pointer-to-method type. It's wrong for this case because TYPE is unrelated to TREE_TYPE (field). > > I think the problem is just this line: > >> if (tree ret = c_fold_indirect_ref_for_warn (loc, type, cop, >> off)) >> return ret; >> return cop; > ^^^^^^^^^^ > > The recursive call does the proper type checking, but then the "return cop" line returns the COMPONENT_REF even though the type check failed. The parallel code in cxx_fold_indirect_ref_1 doesn't have this line, Just compared the routine “cxx_fold_indirect_ref_1” and “c_fold_indirect_ref_for_warn”, looks like there are more places that have such difference, for example, In “cxx_fold_indirect_ref_1”: /* ((foo *)&fooarray)[x] => fooarray[x] */ else if (TREE_CODE (optype) == ARRAY_TYPE && tree_fits_uhwi_p (TYPE_SIZE_UNIT (TREE_TYPE (optype))) && !integer_zerop (TYPE_SIZE_UNIT (TREE_TYPE (optype)))) … if (tree_fits_uhwi_p (min_val)) { tree index = size_int (idx + tree_to_uhwi (min_val)); op = build4_loc (loc, ARRAY_REF, TREE_TYPE (optype), op, index, NULL_TREE, NULL_TREE); return cxx_fold_indirect_ref_1 (ctx, loc, type, op, rem, empty_base); } However, in “c_fold_indirect_ref_for_warn”, the corresponding part is: /* ((foo *)&fooarray)[x] => fooarray[x] */ if (TREE_CODE (optype) == ARRAY_TYPE && TYPE_SIZE_UNIT (TREE_TYPE (optype)) && TREE_CODE (TYPE_SIZE_UNIT (TREE_TYPE (optype))) == INTEGER_CST && !integer_zerop (TYPE_SIZE_UNIT (TREE_TYPE (optype)))) … if (TREE_CODE (min_val) == INTEGER_CST) { tree index = wide_int_to_tree (sizetype, idx + wi::to_offset (min_val)); op = build4_loc (loc, ARRAY_REF, TREE_TYPE (optype), op, index, NULL_TREE, NULL_TREE); off = rem; if (tree ret = c_fold_indirect_ref_for_warn (loc, type, op, off)) return ret; return op; } The exactly same difference as for “RECORD_TYPE”. So, I suspect that it’s a typo for “RECORD_TYPE” in “c_fold_indirect_ref_for_warn”. > and removing it fixes the testcase, so I see > > warning: ‘*(ptrmemfunc*)&x.ptrmemfunc::ptr’ is used uninitialized The question is: For the following IR: struct sp x; void (*<T389>) (void) _1; ... <bb 2> [local count: 1073741824]: _1 = MEM[(struct ptrmemfunc_U *)&x].ptr; _7 = _1 != 8B; Which message is better: 1. warning: ‘*(ptrmemfunc*)&x.ptrmemfunc::ptr’ is used uninitialized Or 2. warning: ‘*(ptrmemfunc*)((char*)&x + offsetof(void (S::*)(),__PTRMEMFUNC)).ptrmemfunc::ptr’ is used uninitialized From the source code: ==== struct S { int j; }; struct T : public S { virtual void h () {} }; struct ptrmemfunc { void (*ptr) (); }; typedef void (S::*sp)(); int main () { T t; sp x; ptrmemfunc *xp = (ptrmemfunc *) &x; if (xp->ptr != ((void (*)())(sizeof(void *)))) return 1; } ==== The reference “xp->ptr” went through from “x” to “xp”, and there is a clear type casting from S::__PTRMEMFUNC to ptrmemfunc::ptr. Shall we emit such type casting to the user? Qing > Jason >
On 2/11/22 13:11, Qing Zhao wrote: > Hi, Jason, > >> On Feb 11, 2022, at 11:27 AM, Jason Merrill <jason@redhat.com> wrote: >>>>> >>>>> Sure, we might as well make this code more robust. But we can do better than <unnamed type> if we check TYPE_PTRMEMFUNC_P. >>>> Okay, so what should we print to the user if it's “TYPE_PTRMEMFUNC_P”? Print nothing or some special string? >>>>> >>>>>> 2. The second level issue is what you suggested in the above, shall we print the “compiler generated internal type” to the user? And I agree with you that it might not be a good idea to print such compiler internal names to the user. Are we do this right now in general? (i.e, check whether the current TYPE is a source level TYPE or a compiler internal TYPE, and then only print out the name of TYPE for the source level TYPE?) and is there a bit in the TYPE to distinguish whether a TYPE is user -level type or a compiler generated internal type? >>>>> >>>>>>> I think the real problem comes sooner, when c_fold_indirect_ref_for_warn turns a MEM_REF with RECORD_TYPE into a COMPONENT_REF with POINTER_TYPE. >>>>> >>>>>> What’s the major issue for this transformation? (I will study this in more details). >>>>> >>>>> We told c_fold_indirect_ref that we want a RECORD_TYPE (the PMF as a whole) and it gave us back a POINTER_TYPE instead (the __pmf member). Folding shouldn't change the type of an expression like that. >>>> >>>> Yes, this is not correct transformation, will study in more detail and try to fix it. >>> After a deeper study of commit r11-6729-gadb520606ce3e1e1 (which triggered the ICE and introduced the new routine “c_fold_indirect_ref_for_warn”), from my understanding, the above transformation from a RECORD_TYPE (the PMF as a whole) to POINTER_TYPE (the __pmf member) is what the function intended to do as following: >>> 1823 static tree >>> 1824 c_fold_indirect_ref_for_warn (location_t loc, tree type, tree op, >>> 1825 offset_int &off) >>> 1826 { >>> … >>> 1870 */* ((foo *)&struct_with_foo_field)[x] => COMPONENT_REF */* >>> 1871 else if (TREE_CODE (optype) == RECORD_TYPE) >>> 1872 { >>> 1873 for (tree field = TYPE_FIELDS (optype); >>> 1874 field; field = DECL_CHAIN (field)) >>> 1875 if (TREE_CODE (field) == FIELD_DECL >>> … >>> 1886 if(upos <= off && off < upos + el_sz) >>> 1887 { >>> 1888 tree cop = build3_loc (loc, COMPONENT_REF, TREE_TYPE (field), >>> 1889 op, field, NULL_TREE); >>> 1890 off = off - upos; >>> The above code was used to transform a MEM_REF to a RECORD_TYPE to a COMPONENT_REF to the corresponding FIELD. >> >> Yes, that's what the above code would correctly do if TYPE were the pointer-to-method type. It's wrong for this case because TYPE is unrelated to TREE_TYPE (field). >> >> I think the problem is just this line: >> >>> if (tree ret = c_fold_indirect_ref_for_warn (loc, type, cop, >>> off)) >>> return ret; >>> return cop; >> ^^^^^^^^^^ >> >> The recursive call does the proper type checking, but then the "return cop" line returns the COMPONENT_REF even though the type check failed. The parallel code in cxx_fold_indirect_ref_1 doesn't have this line, > > Just compared the routine “cxx_fold_indirect_ref_1” and “c_fold_indirect_ref_for_warn”, looks like there are more places that have such difference, for example, > In “cxx_fold_indirect_ref_1”: > > /* ((foo *)&fooarray)[x] => fooarray[x] */ > else if (TREE_CODE (optype) == ARRAY_TYPE > && tree_fits_uhwi_p (TYPE_SIZE_UNIT (TREE_TYPE (optype))) > && !integer_zerop (TYPE_SIZE_UNIT (TREE_TYPE (optype)))) > … > if (tree_fits_uhwi_p (min_val)) > { > tree index = size_int (idx + tree_to_uhwi (min_val)); > op = build4_loc (loc, ARRAY_REF, TREE_TYPE (optype), op, index, > NULL_TREE, NULL_TREE); > return cxx_fold_indirect_ref_1 (ctx, loc, type, op, rem, > empty_base); > } > However, in “c_fold_indirect_ref_for_warn”, the corresponding part is: > > /* ((foo *)&fooarray)[x] => fooarray[x] */ > if (TREE_CODE (optype) == ARRAY_TYPE > && TYPE_SIZE_UNIT (TREE_TYPE (optype)) > && TREE_CODE (TYPE_SIZE_UNIT (TREE_TYPE (optype))) == INTEGER_CST > && !integer_zerop (TYPE_SIZE_UNIT (TREE_TYPE (optype)))) > … > if (TREE_CODE (min_val) == INTEGER_CST) > { > tree index > = wide_int_to_tree (sizetype, idx + wi::to_offset (min_val)); > op = build4_loc (loc, ARRAY_REF, TREE_TYPE (optype), op, index, > NULL_TREE, NULL_TREE); > off = rem; > if (tree ret = c_fold_indirect_ref_for_warn (loc, type, op, off)) > return ret; > return op; > } > > The exactly same difference as for “RECORD_TYPE”. So, I suspect that it’s a typo for “RECORD_TYPE” in “c_fold_indirect_ref_for_warn”. > >> and removing it fixes the testcase, so I see >> >> warning: ‘*(ptrmemfunc*)&x.ptrmemfunc::ptr’ is used uninitialized > > > The question is: > > For the following IR: > > struct sp x; > void (*<T389>) (void) _1; > ... > <bb 2> [local count: 1073741824]: > _1 = MEM[(struct ptrmemfunc_U *)&x].ptr; > _7 = _1 != 8B; > > Which message is better: > > 1. warning: ‘*(ptrmemfunc*)&x.ptrmemfunc::ptr’ is used uninitialized > Or > 2. warning: ‘*(ptrmemfunc*)((char*)&x + offsetof(void (S::*)(),__PTRMEMFUNC)).ptrmemfunc::ptr’ is used uninitialized > > From the source code: > > ==== > struct S > { > int j; > }; > struct T : public S > { > virtual void h () {} > }; > struct ptrmemfunc > { > void (*ptr) (); > }; > typedef void (S::*sp)(); > int main () > { > T t; > sp x; > ptrmemfunc *xp = (ptrmemfunc *) &x; > if (xp->ptr != ((void (*)())(sizeof(void *)))) > return 1; > } > ==== > > The reference “xp->ptr” went through from “x” to “xp”, and there is a clear type casting from S::__PTRMEMFUNC to ptrmemfunc::ptr. > Shall we emit such type casting to the user? But there is no such cast in the source; the cast is (ptrmemfunc*)&x, which appears in the fixed message. Though *(ptrmemfunc*)&x.ptrmemfunc::ptr is wrong syntax, it should be ((ptrmemfunc*)&x)->ptr Jakub, this is your code from r11-6729; from the comment on that commit it sounds like you were deliberately ignoring type incompatibility here, and my suggested fix changes two lines in uninit-40.c. What do you think should happen for this testcase? Jason
> On Feb 11, 2022, at 1:39 PM, Jason Merrill <jason@redhat.com> wrote: > > On 2/11/22 13:11, Qing Zhao wrote: >> Hi, Jason, >>> On Feb 11, 2022, at 11:27 AM, Jason Merrill <jason@redhat.com> wrote: >>>>>> >>>>>> Sure, we might as well make this code more robust. But we can do better than <unnamed type> if we check TYPE_PTRMEMFUNC_P. >>>>> Okay, so what should we print to the user if it's “TYPE_PTRMEMFUNC_P”? Print nothing or some special string? >>>>>> >>>>>>> 2. The second level issue is what you suggested in the above, shall we print the “compiler generated internal type” to the user? And I agree with you that it might not be a good idea to print such compiler internal names to the user. Are we do this right now in general? (i.e, check whether the current TYPE is a source level TYPE or a compiler internal TYPE, and then only print out the name of TYPE for the source level TYPE?) and is there a bit in the TYPE to distinguish whether a TYPE is user -level type or a compiler generated internal type? >>>>>> >>>>>>>> I think the real problem comes sooner, when c_fold_indirect_ref_for_warn turns a MEM_REF with RECORD_TYPE into a COMPONENT_REF with POINTER_TYPE. >>>>>> >>>>>>> What’s the major issue for this transformation? (I will study this in more details). >>>>>> >>>>>> We told c_fold_indirect_ref that we want a RECORD_TYPE (the PMF as a whole) and it gave us back a POINTER_TYPE instead (the __pmf member). Folding shouldn't change the type of an expression like that. >>>>> >>>>> Yes, this is not correct transformation, will study in more detail and try to fix it. >>>> After a deeper study of commit r11-6729-gadb520606ce3e1e1 (which triggered the ICE and introduced the new routine “c_fold_indirect_ref_for_warn”), from my understanding, the above transformation from a RECORD_TYPE (the PMF as a whole) to POINTER_TYPE (the __pmf member) is what the function intended to do as following: >>>> 1823 static tree >>>> 1824 c_fold_indirect_ref_for_warn (location_t loc, tree type, tree op, >>>> 1825 offset_int &off) >>>> 1826 { >>>> … >>>> 1870 */* ((foo *)&struct_with_foo_field)[x] => COMPONENT_REF */* >>>> 1871 else if (TREE_CODE (optype) == RECORD_TYPE) >>>> 1872 { >>>> 1873 for (tree field = TYPE_FIELDS (optype); >>>> 1874 field; field = DECL_CHAIN (field)) >>>> 1875 if (TREE_CODE (field) == FIELD_DECL >>>> … >>>> 1886 if(upos <= off && off < upos + el_sz) >>>> 1887 { >>>> 1888 tree cop = build3_loc (loc, COMPONENT_REF, TREE_TYPE (field), >>>> 1889 op, field, NULL_TREE); >>>> 1890 off = off - upos; >>>> The above code was used to transform a MEM_REF to a RECORD_TYPE to a COMPONENT_REF to the corresponding FIELD. >>> >>> Yes, that's what the above code would correctly do if TYPE were the pointer-to-method type. It's wrong for this case because TYPE is unrelated to TREE_TYPE (field). >>> >>> I think the problem is just this line: >>> >>>> if (tree ret = c_fold_indirect_ref_for_warn (loc, type, cop, >>>> off)) >>>> return ret; >>>> return cop; >>> ^^^^^^^^^^ >>> >>> The recursive call does the proper type checking, but then the "return cop" line returns the COMPONENT_REF even though the type check failed. The parallel code in cxx_fold_indirect_ref_1 doesn't have this line, >> Just compared the routine “cxx_fold_indirect_ref_1” and “c_fold_indirect_ref_for_warn”, looks like there are more places that have such difference, for example, >> In “cxx_fold_indirect_ref_1”: >> /* ((foo *)&fooarray)[x] => fooarray[x] */ >> else if (TREE_CODE (optype) == ARRAY_TYPE >> && tree_fits_uhwi_p (TYPE_SIZE_UNIT (TREE_TYPE (optype))) >> && !integer_zerop (TYPE_SIZE_UNIT (TREE_TYPE (optype)))) >> … >> if (tree_fits_uhwi_p (min_val)) >> { >> tree index = size_int (idx + tree_to_uhwi (min_val)); >> op = build4_loc (loc, ARRAY_REF, TREE_TYPE (optype), op, index, >> NULL_TREE, NULL_TREE); >> return cxx_fold_indirect_ref_1 (ctx, loc, type, op, rem, >> empty_base); >> } >> However, in “c_fold_indirect_ref_for_warn”, the corresponding part is: >> /* ((foo *)&fooarray)[x] => fooarray[x] */ >> if (TREE_CODE (optype) == ARRAY_TYPE >> && TYPE_SIZE_UNIT (TREE_TYPE (optype)) >> && TREE_CODE (TYPE_SIZE_UNIT (TREE_TYPE (optype))) == INTEGER_CST >> && !integer_zerop (TYPE_SIZE_UNIT (TREE_TYPE (optype)))) >> … >> if (TREE_CODE (min_val) == INTEGER_CST) >> { >> tree index >> = wide_int_to_tree (sizetype, idx + wi::to_offset (min_val)); >> op = build4_loc (loc, ARRAY_REF, TREE_TYPE (optype), op, index, >> NULL_TREE, NULL_TREE); >> off = rem; >> if (tree ret = c_fold_indirect_ref_for_warn (loc, type, op, off)) >> return ret; >> return op; >> } >> The exactly same difference as for “RECORD_TYPE”. So, I suspect that it’s a typo for “RECORD_TYPE” in “c_fold_indirect_ref_for_warn”. >>> and removing it fixes the testcase, so I see >>> >>> warning: ‘*(ptrmemfunc*)&x.ptrmemfunc::ptr’ is used uninitialized >> The question is: >> For the following IR: >> struct sp x; >> void (*<T389>) (void) _1; >> ... >> <bb 2> [local count: 1073741824]: >> _1 = MEM[(struct ptrmemfunc_U *)&x].ptr; >> _7 = _1 != 8B; >> Which message is better: >> 1. warning: ‘*(ptrmemfunc*)&x.ptrmemfunc::ptr’ is used uninitialized >> Or >> 2. warning: ‘*(ptrmemfunc*)((char*)&x + offsetof(void (S::*)(),__PTRMEMFUNC)).ptrmemfunc::ptr’ is used uninitialized >> From the source code: >> ==== >> struct S >> { >> int j; >> }; >> struct T : public S >> { >> virtual void h () {} >> }; >> struct ptrmemfunc >> { >> void (*ptr) (); >> }; >> typedef void (S::*sp)(); >> int main () >> { >> T t; >> sp x; >> ptrmemfunc *xp = (ptrmemfunc *) &x; >> if (xp->ptr != ((void (*)())(sizeof(void *)))) >> return 1; >> } >> ==== >> The reference “xp->ptr” went through from “x” to “xp”, and there is a clear type casting from S::__PTRMEMFUNC to ptrmemfunc::ptr. >> Shall we emit such type casting to the user? > > But there is no such cast in the source; the cast is (ptrmemfunc*)&x, which appears in the fixed message. still a little confused here: the original type for “x” is “sp” (is “sp” equal to “S::__PTRMEMFUNC”?), then it was casted to “ptrmemfunc *”. So, should this type conversion from “S::__PTRMEMFUNC” to “ptrmemfunc *” be exposed to the user in the message? Qing > > Though *(ptrmemfunc*)&x.ptrmemfunc::ptr is wrong syntax, it should be ((ptrmemfunc*)&x)->ptr > > Jakub, this is your code from r11-6729; from the comment on that commit it sounds like you were deliberately ignoring type incompatibility here, and my suggested fix changes two lines in uninit-40.c. What do you think should happen for this testcase? > > Jason
On 2/11/22 15:29, Qing Zhao wrote: > > >> On Feb 11, 2022, at 1:39 PM, Jason Merrill <jason@redhat.com> wrote: >> >> On 2/11/22 13:11, Qing Zhao wrote: >>> Hi, Jason, >>>> On Feb 11, 2022, at 11:27 AM, Jason Merrill <jason@redhat.com> wrote: >>>>>>> >>>>>>> Sure, we might as well make this code more robust. But we can do better than <unnamed type> if we check TYPE_PTRMEMFUNC_P. >>>>>> Okay, so what should we print to the user if it's “TYPE_PTRMEMFUNC_P”? Print nothing or some special string? >>>>>>> >>>>>>>> 2. The second level issue is what you suggested in the above, shall we print the “compiler generated internal type” to the user? And I agree with you that it might not be a good idea to print such compiler internal names to the user. Are we do this right now in general? (i.e, check whether the current TYPE is a source level TYPE or a compiler internal TYPE, and then only print out the name of TYPE for the source level TYPE?) and is there a bit in the TYPE to distinguish whether a TYPE is user -level type or a compiler generated internal type? >>>>>>> >>>>>>>>> I think the real problem comes sooner, when c_fold_indirect_ref_for_warn turns a MEM_REF with RECORD_TYPE into a COMPONENT_REF with POINTER_TYPE. >>>>>>> >>>>>>>> What’s the major issue for this transformation? (I will study this in more details). >>>>>>> >>>>>>> We told c_fold_indirect_ref that we want a RECORD_TYPE (the PMF as a whole) and it gave us back a POINTER_TYPE instead (the __pmf member). Folding shouldn't change the type of an expression like that. >>>>>> >>>>>> Yes, this is not correct transformation, will study in more detail and try to fix it. >>>>> After a deeper study of commit r11-6729-gadb520606ce3e1e1 (which triggered the ICE and introduced the new routine “c_fold_indirect_ref_for_warn”), from my understanding, the above transformation from a RECORD_TYPE (the PMF as a whole) to POINTER_TYPE (the __pmf member) is what the function intended to do as following: >>>>> 1823 static tree >>>>> 1824 c_fold_indirect_ref_for_warn (location_t loc, tree type, tree op, >>>>> 1825 offset_int &off) >>>>> 1826 { >>>>> … >>>>> 1870 */* ((foo *)&struct_with_foo_field)[x] => COMPONENT_REF */* >>>>> 1871 else if (TREE_CODE (optype) == RECORD_TYPE) >>>>> 1872 { >>>>> 1873 for (tree field = TYPE_FIELDS (optype); >>>>> 1874 field; field = DECL_CHAIN (field)) >>>>> 1875 if (TREE_CODE (field) == FIELD_DECL >>>>> … >>>>> 1886 if(upos <= off && off < upos + el_sz) >>>>> 1887 { >>>>> 1888 tree cop = build3_loc (loc, COMPONENT_REF, TREE_TYPE (field), >>>>> 1889 op, field, NULL_TREE); >>>>> 1890 off = off - upos; >>>>> The above code was used to transform a MEM_REF to a RECORD_TYPE to a COMPONENT_REF to the corresponding FIELD. >>>> >>>> Yes, that's what the above code would correctly do if TYPE were the pointer-to-method type. It's wrong for this case because TYPE is unrelated to TREE_TYPE (field). >>>> >>>> I think the problem is just this line: >>>> >>>>> if (tree ret = c_fold_indirect_ref_for_warn (loc, type, cop, >>>>> off)) >>>>> return ret; >>>>> return cop; >>>> ^^^^^^^^^^ >>>> >>>> The recursive call does the proper type checking, but then the "return cop" line returns the COMPONENT_REF even though the type check failed. The parallel code in cxx_fold_indirect_ref_1 doesn't have this line, >>> Just compared the routine “cxx_fold_indirect_ref_1” and “c_fold_indirect_ref_for_warn”, looks like there are more places that have such difference, for example, >>> In “cxx_fold_indirect_ref_1”: >>> /* ((foo *)&fooarray)[x] => fooarray[x] */ >>> else if (TREE_CODE (optype) == ARRAY_TYPE >>> && tree_fits_uhwi_p (TYPE_SIZE_UNIT (TREE_TYPE (optype))) >>> && !integer_zerop (TYPE_SIZE_UNIT (TREE_TYPE (optype)))) >>> … >>> if (tree_fits_uhwi_p (min_val)) >>> { >>> tree index = size_int (idx + tree_to_uhwi (min_val)); >>> op = build4_loc (loc, ARRAY_REF, TREE_TYPE (optype), op, index, >>> NULL_TREE, NULL_TREE); >>> return cxx_fold_indirect_ref_1 (ctx, loc, type, op, rem, >>> empty_base); >>> } >>> However, in “c_fold_indirect_ref_for_warn”, the corresponding part is: >>> /* ((foo *)&fooarray)[x] => fooarray[x] */ >>> if (TREE_CODE (optype) == ARRAY_TYPE >>> && TYPE_SIZE_UNIT (TREE_TYPE (optype)) >>> && TREE_CODE (TYPE_SIZE_UNIT (TREE_TYPE (optype))) == INTEGER_CST >>> && !integer_zerop (TYPE_SIZE_UNIT (TREE_TYPE (optype)))) >>> … >>> if (TREE_CODE (min_val) == INTEGER_CST) >>> { >>> tree index >>> = wide_int_to_tree (sizetype, idx + wi::to_offset (min_val)); >>> op = build4_loc (loc, ARRAY_REF, TREE_TYPE (optype), op, index, >>> NULL_TREE, NULL_TREE); >>> off = rem; >>> if (tree ret = c_fold_indirect_ref_for_warn (loc, type, op, off)) >>> return ret; >>> return op; >>> } >>> The exactly same difference as for “RECORD_TYPE”. So, I suspect that it’s a typo for “RECORD_TYPE” in “c_fold_indirect_ref_for_warn”. >>>> and removing it fixes the testcase, so I see >>>> >>>> warning: ‘*(ptrmemfunc*)&x.ptrmemfunc::ptr’ is used uninitialized >>> The question is: >>> For the following IR: >>> struct sp x; >>> void (*<T389>) (void) _1; >>> ... >>> <bb 2> [local count: 1073741824]: >>> _1 = MEM[(struct ptrmemfunc_U *)&x].ptr; >>> _7 = _1 != 8B; >>> Which message is better: >>> 1. warning: ‘*(ptrmemfunc*)&x.ptrmemfunc::ptr’ is used uninitialized >>> Or >>> 2. warning: ‘*(ptrmemfunc*)((char*)&x + offsetof(void (S::*)(),__PTRMEMFUNC)).ptrmemfunc::ptr’ is used uninitialized >>> From the source code: >>> ==== >>> struct S >>> { >>> int j; >>> }; >>> struct T : public S >>> { >>> virtual void h () {} >>> }; >>> struct ptrmemfunc >>> { >>> void (*ptr) (); >>> }; >>> typedef void (S::*sp)(); >>> int main () >>> { >>> T t; >>> sp x; >>> ptrmemfunc *xp = (ptrmemfunc *) &x; >>> if (xp->ptr != ((void (*)())(sizeof(void *)))) >>> return 1; >>> } >>> ==== >>> The reference “xp->ptr” went through from “x” to “xp”, and there is a clear type casting from S::__PTRMEMFUNC to ptrmemfunc::ptr. >>> Shall we emit such type casting to the user? >> >> But there is no such cast in the source; the cast is (ptrmemfunc*)&x, which appears in the fixed message. > > still a little confused here: the original type for “x” is “sp” Yes. > (is “sp” equal to “S::__PTRMEMFUNC”?) No. > then it was casted to “ptrmemfunc *”. Yes. > So, should this type conversion from “S::__PTRMEMFUNC” to “ptrmemfunc *” be exposed to the user in the message? The conversion from sp* to ptrmemfunc* is exposed as (ptrmemfunc*), which is normal C++ syntax; a cast only names the target type, not the source type. >> Though *(ptrmemfunc*)&x.ptrmemfunc::ptr is wrong syntax, it should be ((ptrmemfunc*)&x)->ptr >> >> Jakub, this is your code from r11-6729; from the comment on that commit it sounds like you were deliberately ignoring type incompatibility here, and my suggested fix changes two lines in uninit-40.c. What do you think should happen for this testcase?
> On Feb 11, 2022, at 3:54 PM, Jason Merrill <jason@redhat.com> wrote: > > On 2/11/22 15:29, Qing Zhao wrote: >>> On Feb 11, 2022, at 1:39 PM, Jason Merrill <jason@redhat.com> wrote: >>> >>> On 2/11/22 13:11, Qing Zhao wrote: >>>> Hi, Jason, >>>>> On Feb 11, 2022, at 11:27 AM, Jason Merrill <jason@redhat.com> wrote: >>>>>>>> >>>>>>>> Sure, we might as well make this code more robust. But we can do better than <unnamed type> if we check TYPE_PTRMEMFUNC_P. >>>>>>> Okay, so what should we print to the user if it's “TYPE_PTRMEMFUNC_P”? Print nothing or some special string? >>>>>>>> >>>>>>>>> 2. The second level issue is what you suggested in the above, shall we print the “compiler generated internal type” to the user? And I agree with you that it might not be a good idea to print such compiler internal names to the user. Are we do this right now in general? (i.e, check whether the current TYPE is a source level TYPE or a compiler internal TYPE, and then only print out the name of TYPE for the source level TYPE?) and is there a bit in the TYPE to distinguish whether a TYPE is user -level type or a compiler generated internal type? >>>>>>>> >>>>>>>>>> I think the real problem comes sooner, when c_fold_indirect_ref_for_warn turns a MEM_REF with RECORD_TYPE into a COMPONENT_REF with POINTER_TYPE. >>>>>>>> >>>>>>>>> What’s the major issue for this transformation? (I will study this in more details). >>>>>>>> >>>>>>>> We told c_fold_indirect_ref that we want a RECORD_TYPE (the PMF as a whole) and it gave us back a POINTER_TYPE instead (the __pmf member). Folding shouldn't change the type of an expression like that. >>>>>>> >>>>>>> Yes, this is not correct transformation, will study in more detail and try to fix it. >>>>>> After a deeper study of commit r11-6729-gadb520606ce3e1e1 (which triggered the ICE and introduced the new routine “c_fold_indirect_ref_for_warn”), from my understanding, the above transformation from a RECORD_TYPE (the PMF as a whole) to POINTER_TYPE (the __pmf member) is what the function intended to do as following: >>>>>> 1823 static tree >>>>>> 1824 c_fold_indirect_ref_for_warn (location_t loc, tree type, tree op, >>>>>> 1825 offset_int &off) >>>>>> 1826 { >>>>>> … >>>>>> 1870 */* ((foo *)&struct_with_foo_field)[x] => COMPONENT_REF */* >>>>>> 1871 else if (TREE_CODE (optype) == RECORD_TYPE) >>>>>> 1872 { >>>>>> 1873 for (tree field = TYPE_FIELDS (optype); >>>>>> 1874 field; field = DECL_CHAIN (field)) >>>>>> 1875 if (TREE_CODE (field) == FIELD_DECL >>>>>> … >>>>>> 1886 if(upos <= off && off < upos + el_sz) >>>>>> 1887 { >>>>>> 1888 tree cop = build3_loc (loc, COMPONENT_REF, TREE_TYPE (field), >>>>>> 1889 op, field, NULL_TREE); >>>>>> 1890 off = off - upos; >>>>>> The above code was used to transform a MEM_REF to a RECORD_TYPE to a COMPONENT_REF to the corresponding FIELD. >>>>> >>>>> Yes, that's what the above code would correctly do if TYPE were the pointer-to-method type. It's wrong for this case because TYPE is unrelated to TREE_TYPE (field). >>>>> >>>>> I think the problem is just this line: >>>>> >>>>>> if (tree ret = c_fold_indirect_ref_for_warn (loc, type, cop, >>>>>> off)) >>>>>> return ret; >>>>>> return cop; >>>>> ^^^^^^^^^^ >>>>> >>>>> The recursive call does the proper type checking, but then the "return cop" line returns the COMPONENT_REF even though the type check failed. The parallel code in cxx_fold_indirect_ref_1 doesn't have this line, >>>> Just compared the routine “cxx_fold_indirect_ref_1” and “c_fold_indirect_ref_for_warn”, looks like there are more places that have such difference, for example, >>>> In “cxx_fold_indirect_ref_1”: >>>> /* ((foo *)&fooarray)[x] => fooarray[x] */ >>>> else if (TREE_CODE (optype) == ARRAY_TYPE >>>> && tree_fits_uhwi_p (TYPE_SIZE_UNIT (TREE_TYPE (optype))) >>>> && !integer_zerop (TYPE_SIZE_UNIT (TREE_TYPE (optype)))) >>>> … >>>> if (tree_fits_uhwi_p (min_val)) >>>> { >>>> tree index = size_int (idx + tree_to_uhwi (min_val)); >>>> op = build4_loc (loc, ARRAY_REF, TREE_TYPE (optype), op, index, >>>> NULL_TREE, NULL_TREE); >>>> return cxx_fold_indirect_ref_1 (ctx, loc, type, op, rem, >>>> empty_base); >>>> } >>>> However, in “c_fold_indirect_ref_for_warn”, the corresponding part is: >>>> /* ((foo *)&fooarray)[x] => fooarray[x] */ >>>> if (TREE_CODE (optype) == ARRAY_TYPE >>>> && TYPE_SIZE_UNIT (TREE_TYPE (optype)) >>>> && TREE_CODE (TYPE_SIZE_UNIT (TREE_TYPE (optype))) == INTEGER_CST >>>> && !integer_zerop (TYPE_SIZE_UNIT (TREE_TYPE (optype)))) >>>> … >>>> if (TREE_CODE (min_val) == INTEGER_CST) >>>> { >>>> tree index >>>> = wide_int_to_tree (sizetype, idx + wi::to_offset (min_val)); >>>> op = build4_loc (loc, ARRAY_REF, TREE_TYPE (optype), op, index, >>>> NULL_TREE, NULL_TREE); >>>> off = rem; >>>> if (tree ret = c_fold_indirect_ref_for_warn (loc, type, op, off)) >>>> return ret; >>>> return op; >>>> } >>>> The exactly same difference as for “RECORD_TYPE”. So, I suspect that it’s a typo for “RECORD_TYPE” in “c_fold_indirect_ref_for_warn”. >>>>> and removing it fixes the testcase, so I see >>>>> >>>>> warning: ‘*(ptrmemfunc*)&x.ptrmemfunc::ptr’ is used uninitialized >>>> The question is: >>>> For the following IR: >>>> struct sp x; >>>> void (*<T389>) (void) _1; >>>> ... >>>> <bb 2> [local count: 1073741824]: >>>> _1 = MEM[(struct ptrmemfunc_U *)&x].ptr; >>>> _7 = _1 != 8B; >>>> Which message is better: >>>> 1. warning: ‘*(ptrmemfunc*)&x.ptrmemfunc::ptr’ is used uninitialized >>>> Or >>>> 2. warning: ‘*(ptrmemfunc*)((char*)&x + offsetof(void (S::*)(),__PTRMEMFUNC)).ptrmemfunc::ptr’ is used uninitialized >>>> From the source code: >>>> ==== >>>> struct S >>>> { >>>> int j; >>>> }; >>>> struct T : public S >>>> { >>>> virtual void h () {} >>>> }; >>>> struct ptrmemfunc >>>> { >>>> void (*ptr) (); >>>> }; >>>> typedef void (S::*sp)(); >>>> int main () >>>> { >>>> T t; >>>> sp x; >>>> ptrmemfunc *xp = (ptrmemfunc *) &x; >>>> if (xp->ptr != ((void (*)())(sizeof(void *)))) >>>> return 1; >>>> } >>>> ==== >>>> The reference “xp->ptr” went through from “x” to “xp”, and there is a clear type casting from S::__PTRMEMFUNC to ptrmemfunc::ptr. >>>> Shall we emit such type casting to the user? >>> >>> But there is no such cast in the source; the cast is (ptrmemfunc*)&x, which appears in the fixed message. >> still a little confused here: the original type for “x” is “sp” > > Yes. > >> (is “sp” equal to “S::__PTRMEMFUNC”?) > > No. > >> then it was casted to “ptrmemfunc *”. > > Yes. > >> So, should this type conversion from “S::__PTRMEMFUNC” to “ptrmemfunc *” be exposed to the user in the message? > > The conversion from sp* to ptrmemfunc* is exposed as (ptrmemfunc*), which is normal C++ syntax; a cast only names the target type, not the source type. Okay, I see now. thanks. Qing > >>> Though *(ptrmemfunc*)&x.ptrmemfunc::ptr is wrong syntax, it should be ((ptrmemfunc*)&x)->ptr >>> >>> Jakub, this is your code from r11-6729; from the comment on that commit it sounds like you were deliberately ignoring type incompatibility here, and my suggested fix changes two lines in uninit-40.c. What do you think should happen for this testcase?
On Fri, Feb 11, 2022 at 12:27:49PM -0500, Jason Merrill wrote: > Yes, that's what the above code would correctly do if TYPE were the > pointer-to-method type. It's wrong for this case because TYPE is unrelated > to TREE_TYPE (field). > > I think the problem is just this line: > > > if (tree ret = c_fold_indirect_ref_for_warn (loc, type, cop, > > off)) > > return ret; > > return cop; > ^^^^^^^^^^ > > The recursive call does the proper type checking, but then the "return cop" > line returns the COMPONENT_REF even though the type check failed. The > parallel code in cxx_fold_indirect_ref_1 doesn't have this line, and > removing it fixes the testcase, so I see > > warning: ‘*(ptrmemfunc*)&x.ptrmemfunc::ptr’ is used uninitialized The intent of r11-6729 is that it prints something that helps user to figure out what exactly is being accessed. When we find a unique non-static data member that is being accessed, even when we can't fold it nicely, IMNSHO it is better to print ((sometype *)&var)->field or (*(sometype *)&var).field instead of *(fieldtype *)((char *)&var + 56) because the user doesn't know what is at offset 56, we shouldn't ask user to decipher structure layout etc. One question is if we could return something better for the TYPE_PTRMEMFUNC_FLAG RECORD_TYPE members here (something that would print it more naturally/readably in a C++ way), though the fact that the routine is in c-family makes it harder. Another one is whether we shouldn't punt for FIELD_DECLs that don't have nicely printable name of its containing scope, something like: if (tree scope = get_containing_scope (field)) if (TYPE_P (scope) && TYPE_NAME (scope) == NULL_TREE) break; return cop; or so. Note the returned cop is a COMPONENT_REF where the first argument has a nicely printable type name (x with type sp), but sp's TYPE_MAIN_VARIANT is the unnamed TYPE_PTRMEMFUNC_FLAG. So another possibility would be if we see such a problem for the FIELD_DECL's scope, check if TYPE_MAIN_VARIANT of the first COMPONENT_REF's argument is equal to that scope and in that case use TREE_TYPE of the first COMPONENT_REF's argument as the scope instead. Jakub
On 3/15/22 08:32, Jakub Jelinek wrote: > On Fri, Feb 11, 2022 at 12:27:49PM -0500, Jason Merrill wrote: >> Yes, that's what the above code would correctly do if TYPE were the >> pointer-to-method type. It's wrong for this case because TYPE is unrelated >> to TREE_TYPE (field). >> >> I think the problem is just this line: >> >>> if (tree ret = c_fold_indirect_ref_for_warn (loc, type, cop, >>> off)) >>> return ret; >>> return cop; >> ^^^^^^^^^^ >> >> The recursive call does the proper type checking, but then the "return cop" >> line returns the COMPONENT_REF even though the type check failed. The >> parallel code in cxx_fold_indirect_ref_1 doesn't have this line, and >> removing it fixes the testcase, so I see >> >> warning: ‘*(ptrmemfunc*)&x.ptrmemfunc::ptr’ is used uninitialized > > The intent of r11-6729 is that it prints something that helps user to figure > out what exactly is being accessed. > When we find a unique non-static data member that is being accessed, even > when we can't fold it nicely, IMNSHO it is better to print > ((sometype *)&var)->field > or > (*(sometype *)&var).field > instead of > *(fieldtype *)((char *)&var + 56) > because the user doesn't know what is at offset 56, we shouldn't ask user > to decipher structure layout etc. The problem is that the reference is *not* to any non-static data member, it's to the PMF as a whole. But c_fold_indirect_ref_for_warn wrongly turns it into a reference to the first non-static data member. We asked c_fold_indirect_ref_warn to fold a MEM_REF with RECORD_TYPE, and it gave us back a COMPONENT_REF with POINTER_TYPE. That seems clearly wrong. > One question is if we could return something better for the TYPE_PTRMEMFUNC_FLAG > RECORD_TYPE members here (something that would print it more naturally/readably > in a C++ way), though the fact that the routine is in c-family makes it > harder. > > Another one is whether we shouldn't punt for FIELD_DECLs that don't have > nicely printable name of its containing scope, something like: > if (tree scope = get_containing_scope (field)) > if (TYPE_P (scope) && TYPE_NAME (scope) == NULL_TREE) > break; > return cop; > or so. > Note the returned cop is a COMPONENT_REF where the first argument has a > nicely printable type name (x with type sp), but sp's TYPE_MAIN_VARIANT > is the unnamed TYPE_PTRMEMFUNC_FLAG. So another possibility would be if > we see such a problem for the FIELD_DECL's scope, check if TYPE_MAIN_VARIANT > of the first COMPONENT_REF's argument is equal to that scope and in that > case use TREE_TYPE of the first COMPONENT_REF's argument as the scope > instead. > > Jakub >
On Tue, Mar 15, 2022 at 11:57:22AM -0400, Jason Merrill wrote: > > The intent of r11-6729 is that it prints something that helps user to figure > > out what exactly is being accessed. > > When we find a unique non-static data member that is being accessed, even > > when we can't fold it nicely, IMNSHO it is better to print > > ((sometype *)&var)->field > > or > > (*(sometype *)&var).field > > instead of > > *(fieldtype *)((char *)&var + 56) > > because the user doesn't know what is at offset 56, we shouldn't ask user > > to decipher structure layout etc. > > The problem is that the reference is *not* to any non-static data member, > it's to the PMF as a whole. But c_fold_indirect_ref_for_warn wrongly turns > it into a reference to the first non-static data member. > > We asked c_fold_indirect_ref_warn to fold a MEM_REF with RECORD_TYPE, and it > gave us back a COMPONENT_REF with POINTER_TYPE. That seems clearly wrong. That is not what I see on the testcase. I see the outer c_fold_indirect_ref_for_warn call with type ptrmemfunc which is a 64-bit RECORD_TYPE containing a single ptr member which has pointer to function type, and op which is the x VAR_DECL with sp type which is 128-bit RECORD_TYPE containing 64-bit __pfn member and 64-bit __delta member. As all the bits of the ptrmemfunc RECORD_TYPE fit within the __pfn member (they are equal size), it wants to print (cast)(something.__pfn). Jakub
On 3/15/22 12:06, Jakub Jelinek wrote: > On Tue, Mar 15, 2022 at 11:57:22AM -0400, Jason Merrill wrote: >>> The intent of r11-6729 is that it prints something that helps user to figure >>> out what exactly is being accessed. >>> When we find a unique non-static data member that is being accessed, even >>> when we can't fold it nicely, IMNSHO it is better to print >>> ((sometype *)&var)->field >>> or >>> (*(sometype *)&var).field >>> instead of >>> *(fieldtype *)((char *)&var + 56) >>> because the user doesn't know what is at offset 56, we shouldn't ask user >>> to decipher structure layout etc. >> >> The problem is that the reference is *not* to any non-static data member, >> it's to the PMF as a whole. But c_fold_indirect_ref_for_warn wrongly turns >> it into a reference to the first non-static data member. >> >> We asked c_fold_indirect_ref_warn to fold a MEM_REF with RECORD_TYPE, and it >> gave us back a COMPONENT_REF with POINTER_TYPE. That seems clearly wrong. > > That is not what I see on the testcase. > I see the outer c_fold_indirect_ref_for_warn call with type ptrmemfunc > which is a 64-bit RECORD_TYPE containing a single ptr member which has > pointer to function type, and op which is the x VAR_DECL with sp type which > is 128-bit RECORD_TYPE containing 64-bit __pfn member and 64-bit __delta > member. Exactly: TYPE is RECORD_TYPE, TREE_TYPE (field) is POINTER_TYPE. > As all the bits of the ptrmemfunc RECORD_TYPE fit within the __pfn member > (they are equal size), it wants to print (cast)(something.__pfn). I disagree that this is what we want; we asked to fold an expression with class type, it seems unlikely that we want to get back an expression with pointer type. Jason
On Fri, Mar 18, 2022 at 01:35:53PM -0400, Jason Merrill wrote: > On 3/15/22 12:06, Jakub Jelinek wrote: > > On Tue, Mar 15, 2022 at 11:57:22AM -0400, Jason Merrill wrote: > > > > The intent of r11-6729 is that it prints something that helps user to figure > > > > out what exactly is being accessed. > > > > When we find a unique non-static data member that is being accessed, even > > > > when we can't fold it nicely, IMNSHO it is better to print > > > > ((sometype *)&var)->field > > > > or > > > > (*(sometype *)&var).field > > > > instead of > > > > *(fieldtype *)((char *)&var + 56) > > > > because the user doesn't know what is at offset 56, we shouldn't ask user > > > > to decipher structure layout etc. > > > > > > The problem is that the reference is *not* to any non-static data member, > > > it's to the PMF as a whole. But c_fold_indirect_ref_for_warn wrongly turns > > > it into a reference to the first non-static data member. > > > > > > We asked c_fold_indirect_ref_warn to fold a MEM_REF with RECORD_TYPE, and it > > > gave us back a COMPONENT_REF with POINTER_TYPE. That seems clearly wrong. > > > > That is not what I see on the testcase. > > I see the outer c_fold_indirect_ref_for_warn call with type ptrmemfunc > > which is a 64-bit RECORD_TYPE containing a single ptr member which has > > pointer to function type, and op which is the x VAR_DECL with sp type which > > is 128-bit RECORD_TYPE containing 64-bit __pfn member and 64-bit __delta > > member. > > Exactly: TYPE is RECORD_TYPE, TREE_TYPE (field) is POINTER_TYPE. > > > As all the bits of the ptrmemfunc RECORD_TYPE fit within the __pfn member > > (they are equal size), it wants to print (cast)(something.__pfn). > > I disagree that this is what we want; we asked to fold an expression with > class type, it seems unlikely that we want to get back an expression with > pointer type. That doesn't matter. What c_fold_indirect_ref_warn returns is something that can help the user understand what is actually being accessed. Consider slightly modified testcase (which doesn't use the PMFs so that we don't ICE on those too): // PR c++/101515 // { dg-do compile } // { dg-options "-O1 -Wuninitialized" } struct S { int j; }; struct T : public S { virtual void h () {} }; struct U { char buf[32]; void (*ptr) (); }; struct sp { char a[14]; char b[50]; void (*pfn) (); long delta; }; int main () { T t; sp x; U *xp = (U *) &x.b[18]; if (xp->ptr != ((void (*) ()) (sizeof (void *)))) return 1; } Trunk emits: pr101515-2.C: In function ‘int main()’: pr101515-2.C:16:11: warning: ‘*(U*)((char*)&x + offsetof(sp, sp::b[18])).U::ptr’ is used uninitialized [-Wuninitialized] 16 | if (xp->ptr != ((void (*) ()) (sizeof (void *)))) | ~~~~^~~ pr101515-2.C:14:6: note: ‘x’ declared here 14 | sp x; | ^ here, which is indeed quite long expression, but valid C++ and helps the user to narrow down what exactly is being accessed. If I comment out the c_fold_indirect_ref_warn RECORD_TYPE handling so that it punts on it, it prints: pr101515-2.C: In function ‘int main()’: pr101515-2.C:16:11: warning: ‘*(U*)((char*)&x + 32).U::ptr’ is used uninitialized [-Wuninitialized] 16 | if (xp->ptr != ((void (*) ()) (sizeof (void *)))) | ~~~~^~~ pr101515-2.C:14:6: note: ‘x’ declared here 14 | sp x; | ^ That is also correct C++ expression, but user probably has no idea what is present at offset 32 into the variable. Of course if there is a type match and not any kind of type punning, it will try to print much shorter and more readable expressions. Jakub
On 3/18/22 14:20, Jakub Jelinek wrote: > On Fri, Mar 18, 2022 at 01:35:53PM -0400, Jason Merrill wrote: >> On 3/15/22 12:06, Jakub Jelinek wrote: >>> On Tue, Mar 15, 2022 at 11:57:22AM -0400, Jason Merrill wrote: >>>>> The intent of r11-6729 is that it prints something that helps user to figure >>>>> out what exactly is being accessed. >>>>> When we find a unique non-static data member that is being accessed, even >>>>> when we can't fold it nicely, IMNSHO it is better to print >>>>> ((sometype *)&var)->field >>>>> or >>>>> (*(sometype *)&var).field >>>>> instead of >>>>> *(fieldtype *)((char *)&var + 56) >>>>> because the user doesn't know what is at offset 56, we shouldn't ask user >>>>> to decipher structure layout etc. >>>> >>>> The problem is that the reference is *not* to any non-static data member, >>>> it's to the PMF as a whole. But c_fold_indirect_ref_for_warn wrongly turns >>>> it into a reference to the first non-static data member. >>>> >>>> We asked c_fold_indirect_ref_warn to fold a MEM_REF with RECORD_TYPE, and it >>>> gave us back a COMPONENT_REF with POINTER_TYPE. That seems clearly wrong. >>> >>> That is not what I see on the testcase. >>> I see the outer c_fold_indirect_ref_for_warn call with type ptrmemfunc >>> which is a 64-bit RECORD_TYPE containing a single ptr member which has >>> pointer to function type, and op which is the x VAR_DECL with sp type which >>> is 128-bit RECORD_TYPE containing 64-bit __pfn member and 64-bit __delta >>> member. >> >> Exactly: TYPE is RECORD_TYPE, TREE_TYPE (field) is POINTER_TYPE. >> >>> As all the bits of the ptrmemfunc RECORD_TYPE fit within the __pfn member >>> (they are equal size), it wants to print (cast)(something.__pfn). >> >> I disagree that this is what we want; we asked to fold an expression with >> class type, it seems unlikely that we want to get back an expression with >> pointer type. > > That doesn't matter. What c_fold_indirect_ref_warn returns is something > that can help the user understand what is actually being accessed. > Consider slightly modified testcase (which doesn't use the PMFs so that > we don't ICE on those too): > // PR c++/101515 > // { dg-do compile } > // { dg-options "-O1 -Wuninitialized" } > > struct S { int j; }; > struct T : public S { virtual void h () {} }; > struct U { char buf[32]; void (*ptr) (); }; > struct sp { char a[14]; char b[50]; void (*pfn) (); long delta; }; > > int > main () > { > T t; > sp x; > U *xp = (U *) &x.b[18]; > if (xp->ptr != ((void (*) ()) (sizeof (void *)))) > return 1; > } > Trunk emits: > pr101515-2.C: In function ‘int main()’: > pr101515-2.C:16:11: warning: ‘*(U*)((char*)&x + offsetof(sp, sp::b[18])).U::ptr’ is used uninitialized [-Wuninitialized] > 16 | if (xp->ptr != ((void (*) ()) (sizeof (void *)))) > | ~~~~^~~ > pr101515-2.C:14:6: note: ‘x’ declared here > 14 | sp x; > | ^ > here, which is indeed quite long expression, but valid C++ and helps the > user to narrow down what exactly is being accessed. > If I comment out the c_fold_indirect_ref_warn RECORD_TYPE handling so that > it punts on it, it prints: > pr101515-2.C: In function ‘int main()’: > pr101515-2.C:16:11: warning: ‘*(U*)((char*)&x + 32).U::ptr’ is used uninitialized [-Wuninitialized] > 16 | if (xp->ptr != ((void (*) ()) (sizeof (void *)))) > | ~~~~^~~ > pr101515-2.C:14:6: note: ‘x’ declared here > 14 | sp x; > | ^ > That is also correct C++ expression, but user probably has no idea what is > present at offset 32 into the variable. > Of course if there is a type match and not any kind of type punning, > it will try to print much shorter and more readable expressions. One important point about the OP's testcase is that we're dealing with offset 0, which corresponds to both the PMF as a whole and its first element. Since we're looking for a RECORD_TYPE, choosing to interpret the desired object as the (RECORD_TYPE) PMF as a whole seems like the better choice; being more flexible probably does make sense at non-0 offsets. Jason
On Fri, Mar 18, 2022 at 02:27:30PM -0400, Jason Merrill wrote: > > That is also correct C++ expression, but user probably has no idea what is > > present at offset 32 into the variable. > > Of course if there is a type match and not any kind of type punning, > > it will try to print much shorter and more readable expressions. > > One important point about the OP's testcase is that we're dealing with > offset 0, which corresponds to both the PMF as a whole and its first > element. Since we're looking for a RECORD_TYPE, choosing to interpret the > desired object as the (RECORD_TYPE) PMF as a whole seems like the better > choice; being more flexible probably does make sense at non-0 offsets. Even if we punt for 0 offsets in RECORD_TYPEs, we'd still run into the ICE if we tried to access something overlapping the __delta member. For 0 offsets and type puning, the question is what will help user more, it is true we don't need to print those + offsetof(...) parts because it is the start of it, on the other side the user might not know what is the first member of the struct and printing that might sometimes help. Jakub
On 3/18/22 14:47, Jakub Jelinek wrote: > On Fri, Mar 18, 2022 at 02:27:30PM -0400, Jason Merrill wrote: >>> That is also correct C++ expression, but user probably has no idea what is >>> present at offset 32 into the variable. >>> Of course if there is a type match and not any kind of type punning, >>> it will try to print much shorter and more readable expressions. >> >> One important point about the OP's testcase is that we're dealing with >> offset 0, which corresponds to both the PMF as a whole and its first >> element. Since we're looking for a RECORD_TYPE, choosing to interpret the >> desired object as the (RECORD_TYPE) PMF as a whole seems like the better >> choice; being more flexible probably does make sense at non-0 offsets. > > Even if we punt for 0 offsets in RECORD_TYPEs, we'd still run into > the ICE if we tried to access something overlapping the __delta member. Good point. Your patch is OK, then. > For 0 offsets and type puning, the question is what will help user more, > it is true we don't need to print those + offsetof(...) parts because it > is the start of it, on the other side the user might not know what is the > first member of the struct and printing that might sometimes help. Or it might not, as in this case. Going with the enclosing class if none of the types match seems like a better default to me, but I guess let's not worry about it now. Jason
diff --git a/gcc/cp/cxx-pretty-print.cc b/gcc/cp/cxx-pretty-print.cc index 4f9a090e520d..744ed0add5ba 100644 --- a/gcc/cp/cxx-pretty-print.cc +++ b/gcc/cp/cxx-pretty-print.cc @@ -171,7 +171,10 @@ pp_cxx_unqualified_id (cxx_pretty_printer *pp, tree t) case ENUMERAL_TYPE: case TYPENAME_TYPE: case UNBOUND_CLASS_TEMPLATE: - pp_cxx_unqualified_id (pp, TYPE_NAME (t)); + if (TYPE_NAME (t)) + pp_cxx_unqualified_id (pp, TYPE_NAME (t)); + else + pp_string (pp, "<unnamed type>"); if (tree ti = TYPE_TEMPLATE_INFO_MAYBE_ALIAS (t)) if (PRIMARY_TEMPLATE_P (TI_TEMPLATE (ti))) { diff --git a/gcc/testsuite/g++.dg/pr101515.C b/gcc/testsuite/g++.dg/pr101515.C new file mode 100644 index 000000000000..898c7e003c22 --- /dev/null +++ b/gcc/testsuite/g++.dg/pr101515.C @@ -0,0 +1,25 @@ +/* PR101515 - ICE in pp_cxx_unqualified_id, at cp/cxx-pretty-print.c:128 + { dg-do compile } + { dg-options "-Wuninitialized -O1" } */ + +struct S +{ + int j; +}; +struct T : public S +{ + virtual void h () {} +}; +struct ptrmemfunc +{ + void (*ptr) (); +}; +typedef void (S::*sp)(); +int main () +{ + T t; + sp x; + ptrmemfunc *xp = (ptrmemfunc *) &x; + if (xp->ptr != ((void (*)())(sizeof(void *)))) /* { dg-warning "is used uninitialized" } */ + return 1; +}