From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by sourceware.org (Postfix) with ESMTP id DE77C3861802 for ; Wed, 11 Aug 2021 09:03:00 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org DE77C3861802 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 47941106F; Wed, 11 Aug 2021 02:03:00 -0700 (PDT) Received: from localhost (e121540-lin.manchester.arm.com [10.32.98.126]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 73F883F70D; Wed, 11 Aug 2021 02:02:59 -0700 (PDT) From: Richard Sandiford To: Qing Zhao via Gcc-patches Mail-Followup-To: Qing Zhao via Gcc-patches , Qing Zhao , Richard Biener , Jakub Jelinek , Kees Cook , richard.sandiford@arm.com Cc: Qing Zhao , Richard Biener , Jakub Jelinek , Kees Cook Subject: Re: [patch][version 6] add -ftrivial-auto-var-init and variable attribute "uninitialized" to gcc References: <52E29277-1403-4755-901A-528116C43FB8@oracle.com> <58ADBC0C-9D44-485B-BB5A-B072664B9C4F@oracle.com> <6FD42B95-F73D-4B75-B83A-BAC4925B1714@oracle.com> Date: Wed, 11 Aug 2021 10:02:58 +0100 In-Reply-To: (Qing Zhao via Gcc-patches's message of "Tue, 10 Aug 2021 22:26:37 +0000") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-Spam-Status: No, score=-12.4 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_STATUS, SPF_HELO_NONE, SPF_PASS, TXREP 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: Wed, 11 Aug 2021 09:03:03 -0000 Qing Zhao via Gcc-patches writes: >> On Aug 10, 2021, at 3:16 PM, Qing Zhao via Gcc-patches wrote: >> >> Hi, Richard, >> >>> On Aug 10, 2021, at 10:22 AM, Richard Biener wrote: >>>>> >>>>> Especially in the VLA case but likely also in general (though unlikely >>>>> since usually the receiver of initializations are simple enough). I'd >>>>> expect the VLA case end up as >>>>> >>>>> *ptr_to_decl = .DEFERRED_INIT (...); >>>>> >>>>> where *ptr_to_decl is the DECL_VALUE_EXPR of the decl. >>>> >>>> So, for the following small testing case: >>>> >>>> ==== >>>> extern void bar (int); >>>> >>>> void foo(int n) >>>> { >>>> int arr[n]; >>>> bar (arr[2]); >>>> return; >>>> } >>>> ===== >>>> >>>> If I compile it with -ftrivial-auto-var-init=zero -fdump-tree-gimple -S -o auto-init-11.s -fdump-rtl-expand, the *.gimple dump is: >>>> >>>> ===== >>>> void foo (int n) >>>> { >>>> int n.0; >>>> sizetype D.1950; >>>> bitsizetype D.1951; >>>> sizetype D.1952; >>>> bitsizetype D.1953; >>>> sizetype D.1954; >>>> int[0:D.1950] * arr.1; >>>> void * saved_stack.2; >>>> int arr[0:D.1950] [value-expr: *arr.1]; >>>> >>>> saved_stack.2 = __builtin_stack_save (); >>>> try >>>> { >>>> n.0 = n; >>>> _1 = (long int) n.0; >>>> _2 = _1 + -1; >>>> _3 = (sizetype) _2; >>>> D.1950 = _3; >>>> _4 = (sizetype) n.0; >>>> _5 = (bitsizetype) _4; >>>> _6 = _5 * 32; >>>> D.1951 = _6; >>>> _7 = (sizetype) n.0; >>>> _8 = _7 * 4; >>>> D.1952 = _8; >>>> _9 = (sizetype) n.0; >>>> _10 = (bitsizetype) _9; >>>> _11 = _10 * 32; >>>> D.1953 = _11; >>>> _12 = (sizetype) n.0; >>>> _13 = _12 * 4; >>>> D.1954 = _13; >>>> arr.1 = __builtin_alloca_with_align (D.1954, 32); >>>> arr = .DEFERRED_INIT (D.1952, 2, 1); >>>> _14 = (*arr.1)[2]; >>>> bar (_14); >>>> return; >>>> } >>>> finally >>>> { >>>> __builtin_stack_restore (saved_stack.2); >>>> } >>>> } >>>> >>>> ==== >>>> >>>> You think that the above .DEFEERED_INIT is not correct? >>>> It should be: >>>> >>>> *arr.1 = .DEFERRED_INIT (D.1952. 2, 1); >>>> >>>> ? >>> >>> Yes. >>> >> >> I updated gimplify.c for VLA and now it emits the call to .DEFERRED_INIT as: >> >> arr.1 = __builtin_alloca_with_align (D.1954, 32); >> *arr.1 = .DEFERRED_INIT (D.1952, 2, 1); >> >> However, this call triggered the assertion failure in verify_gimple_call of tree-cfg.c because the LHS is not a valid LHS. >> Then I modify tree-cfg.c as: >> >> diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c >> index 330eb7dd89bf..180d4f1f9e32 100644 >> --- a/gcc/tree-cfg.c >> +++ b/gcc/tree-cfg.c >> @@ -3375,7 +3375,11 @@ verify_gimple_call (gcall *stmt) >> } >> >> tree lhs = gimple_call_lhs (stmt); >> + /* For .DEFERRED_INIT call, the LHS might be an indirection of >> + a pointer for the VLA variable, which is not a valid LHS of >> + a gimple call, we ignore the asssertion on this. */ >> if (lhs >> + && (!gimple_call_internal_p (stmt, IFN_DEFERRED_INIT)) >> && (!is_gimple_reg (lhs) >> && (!is_gimple_lvalue (lhs) >> || verify_types_in_gimple_reference >> >> The assertion failure in tree-cfg.c got resolved, but I got another assertion failure in operands_scanner::get_expr_operands (tree *expr_p, int flags), line 945: >> >> 939 /* If we get here, something has gone wrong. */ >> 940 if (flag_checking) >> 941 { >> 942 fprintf (stderr, "unhandled expression in get_expr_operands():\n"); >> 943 debug_tree (expr); >> 944 fputs ("\n", stderr); >> 945 gcc_unreachable (); >> 946 } >> >> Looks like that the gimple statement: >> *arr.1 = .DEFERRED_INIT (D.1952, 2, 1); >> >> Is not valid. i.e, the LHS should not be an indirection to a pointer. >> >> How to resolve this issue? > > I came up with the following solution: > > Define the IFN_DEFERRED_INIT function as: > > LHS = DEFERRED_INIT (SIZE of the DECL, INIT_TYPE, IS_VLA); > > if IS_VLA is false, the LHS is the DECL itself, > if IS_VLA is true, the LHS is the pointer to this DECL that created by > gimplify_vla_decl. > > > The benefit of this solution are: > > 1. Resolved the invalid IR issue; > 2. The call stmt carries the address of the VLA natually; > > The issue with this solution is: > > For VLA and non-VLA, the LHS will be different, > > Do you see any other potential issues with this solution? The idea behind the DECL version of the .DEFERRED_INIT semantics was that .DEFERRED_INIT just returns a SIZE-byte value that the caller then assigns to a SIZE-byte lhs (with the caller choosing the lhs). .DEFEREED_INIT itself doesn't read or write memory and so can be const, which in turn allows alias analysis to be more precise. If we want to handle the VLA case using pointers instead then I think that needs to be a different IFN. If we did handle the VLA case using pointers (not expressing an opinion on that), then it would be the caller's job to allocate the VLA and work out the address of the VLA; this isn't something that .DEFERRED_INIT would work out on the caller's behalf. The address of the VLA should therefore be an argument to the new IFN, rather than something that the IFN returns. Thanks, Richard