From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out2.suse.de (smtp-out2.suse.de [IPv6:2001:67c:2178:6::1d]) by sourceware.org (Postfix) with ESMTPS id A0AF03857BAB for ; Thu, 11 Aug 2022 07:40:51 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org A0AF03857BAB Received: from relay2.suse.de (relay2.suse.de [149.44.160.134]) by smtp-out2.suse.de (Postfix) with ESMTP id E6B6E5CBAE; Thu, 11 Aug 2022 07:40:49 +0000 (UTC) Received: from wotan.suse.de (wotan.suse.de [10.160.0.1]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by relay2.suse.de (Postfix) with ESMTPS id C95DE2C212; Thu, 11 Aug 2022 07:40:49 +0000 (UTC) Date: Thu, 11 Aug 2022 07:40:49 +0000 (UTC) From: Richard Biener To: Qing Zhao cc: gcc-patches Subject: Re: [GCC13][Patch][PR106457]improve array_at_struct_end_p for array objects (PR106457) In-Reply-To: Message-ID: References: User-Agent: Alpine 2.22 (LSU 394 2020-01-19) MIME-Version: 1.0 X-Spam-Status: No, score=-5.0 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, KAM_SHORT, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8BIT X-Content-Filtered-By: Mailman/MimeDel 2.1.29 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: Thu, 11 Aug 2022 07:40:53 -0000 On Wed, 10 Aug 2022, Qing Zhao wrote: > Hi, > > As mentioned in the bug report, I reopened this bug since the previous patch: > > commit r13-1875-gff26f0ba68fe6e870f315d0601b596f889b89680 > Author: Richard Biener > Date: Thu Jul 28 10:07:32 2022 +0200 > > middle-end/106457 - improve array_at_struct_end_p for array objects > Array references to array objects are never at struct end. > > > Didn’t resolve this bug. > > This is a new patch, and my current work on -fstrict-flex-array depends on this patch. > > Please take a look at the patch and let me know whether it’s good for committing. > > Thanks. > > Qing. > > > ====================================== > > [PATCH] middle-end/106457 - improve array_at_struct_end_p for array > objects (PR106457) > > Array references are not handled correctly by current array_at_struct_end_p, > for the following array references: > > Example 1: (from gcc/testsuite/gcc.dg/torture/pr50067-[1|2].c): > > short a[32] = { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, > 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31 }; > ... = (*((char(*)[32])&a[0]))[i+8]; // this array reference For this one we have (gdb) p debug_generic_expr (ref_to_array) MEM[(char[32] *)&a] at the 12782 if (DECL_P (ref_to_array)) check. The array referenced (char[32]) isn't the decl (which is short[32]), so the referenced array (char[32]) _is_ at struct end since accessing index 48 is OK - it won't exceed the decl boundary. In fact that -Waggressive-loop-optimizations triggers with your patch shows it is wrong in this case - the code is perfectly OK, no out-of-bound access occurs. When this warning triggers it hints at the code being miscompiled (we usually elide the exit test). > Example 2: (from gcc/testsuite/gcc.target/aarch64/vadd_reduc-2.c): > > int test (uint8_t *p, uint32_t t[1][1], int n) { > for (int i = 0; i < 4; i++, p++) > t[i][0] = ...; // this array reference > ... > } The parameter declaration uint32_t t[1][1] doesn't guarantee anything - it's just a pointer. So we cannot reasonably constrain accesses to sizeof(uint32_t). That means array_at_struct_end_p has to return true. > Example 3: (from gcc/testsuite/g++.dg/debug/debug5.C): > > int a = 1; > int b = 1; > int e[a][b]; > e[0][0] = 0; // this array reference That might be the only case we'd be allowed to say false. I see a single call with MEM[(int[0:D.1989][0:D.1986] *)&e.4][0]{lb: 0 sz: 4} that is, the caller has stripped the outermost [0] already. The issue here is unfortunate lowering of the alloca with constant size we originally have here. The array typed decl has type int[1] but we access it with type int[0:D.1989][0:D.1986]. We'd now have to prove that D.1989 and D.1986 are '1' (and not less). That doesn't look possible in general. Not sure if it is worth the trobble here. Eventually array_at_struct_end_p isn't the correct vehicle for diagnostics if you desparately need to avoid 'true' for the above cases. Note for the first case it's more about treating pointer to array with bounds in a stricter way than we do at the moment, not so much about arrays at struct end. But array_at_struct_end_p is used to query whether we have to assume there's storage beyond the declared bound of an array that is "valid" to access (and it's valid from the middle-end point of view in this case). Richard. > All the above array references are identified as TRUE by the current > array_at_struct_end_p, therefore treated as flexible array members. > Obviously, they are just simple array references, not an array refs > to the last field of a struture. The current array_at_struct_end_p handles > such array references incorrectly. > > In order to handle array references correctly, we could recursively check > its first operand if it's a MEM_REF or COMPONENT_REF and stop as FALSE > when otherwise. This resolved all the issues for ARRAY_REF. > > bootstrapped and regression tested on both X86 and Aarch64. > Multiple testing cases behave differently due to array_at_struct_end_p now > behave correctly (return FALSE now, then they are not flexible array member > anymore). Adjust these testing cases. > > There is one regression for gcc/target/aarch64/vadd_reduc-2.c is left > unresolved since the loop transformation is changed due to the changed behavior > of array_at_struct_end_p, simple adjustment of the testing case doesnt work. > I will file a bug to record this regression. > > gcc/ChangeLog: > > PR middle-end/106457 > * tree.cc (array_at_struct_end_p): Handle array objects recursively > through its first operand. > > gcc/testsuite/ChangeLog: > > PR middle-end/106457 > * gcc.dg/torture/pr50067-1.c: Add -Wno-aggressive-loop-optimizations > to suppress warnings. > * gcc.dg/torture/pr50067-2.c: Likewise. > * gcc.target/aarch64/vadd_reduc-2.c: Likewise. > * gcc.target/i386/pr104059.c: Likewise. > > > The complete patch is at: > > > -- Richard Biener SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg, Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman; HRB 36809 (AG Nuernberg)