From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id E491F385841B for ; Fri, 11 Feb 2022 19:39:38 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org E491F385841B Received: from mail-qv1-f71.google.com (mail-qv1-f71.google.com [209.85.219.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-631-qocAuNdTNLGrJnx82n6Kfg-1; Fri, 11 Feb 2022 14:39:37 -0500 X-MC-Unique: qocAuNdTNLGrJnx82n6Kfg-1 Received: by mail-qv1-f71.google.com with SMTP id eu2-20020ad44f42000000b0042bfcac4a52so6977934qvb.16 for ; Fri, 11 Feb 2022 11:39:37 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:date:mime-version:user-agent:subject :content-language:to:cc:references:from:in-reply-to :content-transfer-encoding; bh=Ywa3zQziIJC7m+SYDZf4T+/8Cwlz7LwcKM+IFQb9nyM=; b=Fms06v2rGavm3HL9cI8bsHbBVJlUx+gl9V6pqvH2KLgIj17fgE+Gvv9vNVa0hlEL+b V2lp3py5EpUZ7MJ0dwyyOA4ddReBpvxVlPXGFhA7zVsMkXOznjny31Thnj4JpaQXilL4 oE3FVcd3d55iLUu1QS4gMLWQiHddVrqcQl7RxumaFKoJLolihX3L2uLzKL6LC9QLfAEF rcTFquR942sehnQscORaPpVD4ch1vWhysbWoFXHSCgGmaDtLmnro3JeqRl8kIcefocGb pw6A0t+tKMmWCa5n1k/khMINWx96l48N91bz4tYYMH8KLpziDam2AGjgtZSMjZTudfhs nS+g== X-Gm-Message-State: AOAM5312k+eOc8phEWPAQrSJtZ/YNkWjKCL4UqSVDF4Hmax+s7+wr0OU J4/aepARlSpwEy8f2rQfGbJshJlsOp+wRpwmNWk/lCgrBJgjn5awb4qQHkITBhMcZd8/cyBofCq YC3Tqp4QkN8MOLu5D/w== X-Received: by 2002:a05:6214:2386:: with SMTP id fw6mr2173147qvb.89.1644608376971; Fri, 11 Feb 2022 11:39:36 -0800 (PST) X-Google-Smtp-Source: ABdhPJxHWjcOsu8xNxKwrxpVS6d5xVK4oYvTVXUj3CmTlb47bdiA4DMt22nDLmHIHPgJDxatHqvR4g== X-Received: by 2002:a05:6214:2386:: with SMTP id fw6mr2173131qvb.89.1644608376602; Fri, 11 Feb 2022 11:39:36 -0800 (PST) Received: from [192.168.1.149] (130-44-159-43.s15913.c3-0.arl-cbr1.sbo-arl.ma.cable.rcncustomer.com. [130.44.159.43]) by smtp.gmail.com with ESMTPSA id f16sm13800720qtk.8.2022.02.11.11.39.35 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 11 Feb 2022 11:39:35 -0800 (PST) Message-ID: Date: Fri, 11 Feb 2022 14:39:34 -0500 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.5.1 Subject: Re: [PATCH] Fix PR 101515 (ICE in pp_cxx_unqualified_id, at cp/cxx-pretty-print.c:128) To: Qing Zhao Cc: Jakub Jelinek , gcc-patches@gcc.gnu.org References: <946D3718-32CD-45B6-8EF5-C41DDC3CA06E@oracle.com> <87823a36-93f3-5541-dc76-5a8c32e51c03@redhat.com> <319ad931-a975-e29c-7b8a-51534d657e01@redhat.com> <9B1729F5-964A-4A12-93B3-148BFE4D96F5@oracle.com> <01B0A297-CC89-4666-9684-BE04BE17E66E@oracle.com> From: Jason Merrill In-Reply-To: X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-6.9 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, NICE_REPLY_A, RCVD_IN_DNSWL_LOW, 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 11 Feb 2022 19:39:40 -0000 On 2/11/22 13:11, Qing Zhao wrote: > Hi, Jason, > >> On Feb 11, 2022, at 11:27 AM, Jason Merrill wrote: >>>>> >>>>> Sure, we might as well make this code more robust. But we can do better than 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 (*) (void) _1; > ... > [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