From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ed1-x535.google.com (mail-ed1-x535.google.com [IPv6:2a00:1450:4864:20::535]) by sourceware.org (Postfix) with ESMTPS id 5533A38A941E for ; Tue, 11 Jan 2022 13:53:15 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 5533A38A941E Received: by mail-ed1-x535.google.com with SMTP id z22so22404243edd.12 for ; Tue, 11 Jan 2022 05:53:15 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=OIfgPnpBnF971HAwk3puQwdsKvy9rBLdu2tm6W0MNvo=; b=EMvXkLJHv8rpRiPNeoMC7hr66QApaPW+SlQG1jGjO+GhYz62AftRJPaYiipeiSlefF oVtye00otWZNVIlW23XFPvd7EXqbUhUdXUrm77JruD8fWT0p27rmnSqvOX4DTupKHq+o +CANdP2toI3MmcRRthfELIP9P3vlZdY5w/b6ZeEsCTxFVaKq3a3PHeYFEAaZZ6sV/6a8 20vuI52A4DuCqDRtPQ62N0QkLemAv/QNlzN6Na6/x3+I61YDV/Z6Hn0GpSevnxoT5UcR xhaRLxwCHozInYJgp9+pilcskPC1NYHIvzXjbFWD4Yh4UcqFuB93OROZpflU8/oVCbJc /d8w== X-Gm-Message-State: AOAM5306c5lRdpPvSrpGNZfh8EfjJyfjNkv2uWFPkJDfUSB4IQ6kfB+C QVpz3nk4tJ1B+sH8+raLHZaAH0y7dhBsFZDCIHc= X-Google-Smtp-Source: ABdhPJy+9QivrA6//HIbTBIgn9KZz6iSESaH3HLRbcPecY31N/Sdv2HIg1gg8c5OMpLw9uHpshxT+nMkISE/udgC9Pc= X-Received: by 2002:a05:6402:31f0:: with SMTP id dy16mr4379241edb.364.1641909193429; Tue, 11 Jan 2022 05:53:13 -0800 (PST) MIME-Version: 1.0 References: <9006256F-76C9-4C27-A7C6-1817C9A8CE1B@oracle.com> In-Reply-To: <9006256F-76C9-4C27-A7C6-1817C9A8CE1B@oracle.com> From: Richard Biener Date: Tue, 11 Jan 2022 14:53:02 +0100 Message-ID: Subject: Re: [Patch][V3][Patch 1/2]Change the 3rd parameter of function .DEFERRED_INIT from IS_VLA to decl name To: Qing Zhao Cc: Martin Sebor , gcc Patches Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, KAM_SHORT, RCVD_IN_DNSWL_NONE, 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: Tue, 11 Jan 2022 13:53:17 -0000 On Tue, Jan 11, 2022 at 12:58 AM Qing Zhao wrote: > > Hi, Richard, > > I splited the previous patch for =E2=80=9CEnable -Wuninitialized + -ftriv= ial-auto-var-init for address taken variables=E2=80=9D into two separate pa= tches. > This is the first one > > This first patch is to fix (or work around ) PR103720, therefore it=E2= =80=99s an important change, and need to be go into GCC12. > At the same time, this patch is the preparation for the second patch that= will actually enable -Wuninitialized + -ftrivial-auto-var-init for address= taken variables. > > The reason I separate the previous patch into two is: most of the previou= s concern was on the second part of the patch (the change in tree-ssa-unini= t.c), I don=E2=80=99t > want those concern prevent this first patch from being approved into GCC1= 2. > > > In this part, I addressed your comments in gimplify.c : > > =3D=3D=3D=3D=3D > tree decl_name > + =3D build_string_literal (IDENTIFIER_LENGTH (DECL_NAME (decl)) + 1, > + IDENTIFIER_POINTER (DECL_NAME (decl))); > > you need to deal with DECL_NAME being NULL. > =3D=3D=3D=3D=3D > > Please also see the detailed description below for the problem and soluti= on of this patch. > > This first patch has been bootstrapped and regressing tested on both X86 = and aarch64. > > Okay for GCC12? + + char *decl_name_anonymous =3D xasprintf ("D.%u", DECL_UID (decl)); + const char *decl_name_str =3D DECL_NAME (decl) + ? IDENTIFIER_POINTER (DECL_NAME (decl)) + : decl_name_anonymous; + tree decl_name + =3D build_string_literal (strlen (decl_name_str) + 1, + decl_name_str); please avoid the xasprintf in the case DECL_NAME is not NULL, I'd be happy with sth like tree decl_name; if (DECL_NAME (decl)) decl_name =3D build_string_literal (...); else { char *decl_name_anon =3D xasprintf (...); decl_name =3D build_string_literal (...); free (decl_name_anon); } otherwise the patch is OK to commit (just do the above change and re-test / push). Thanks, Richard. > Thanks. > > Qing. > > > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D > > Change the 3rd parameter of function .DEFERRED_INIT from > IS_VLA to decl name. > > Currently, the 3rd parameter of function .DEFERRED_INIT is IS_VLA, which = is > not needed at all; > > In this patch, we change the 3rd parameter from IS_VLA to the name of the= var > decl for the following purposes: > > 1. Fix (or work around) PR103720: > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=3D103720 > > As confirmed in PR103720, with the current definition of .DEFERRED_INIT, > > Dom transformed: > c$a$0_6 =3D .DEFERRED_INIT (8, 2, 0); > _1 =3D .DEFERRED_INIT (8, 2, 0); > > into: > c$a$0_6 =3D .DEFERRED_INIT (8, 2, 0); > _1 =3D c$a$0_6; > > which is incorrectly done due to Dom treating the two calls to const func= tion > .DEFERRED_INIT as the same call since all actual parameters are the same. > > The same issue has been exposed in PR102608 due to a different optimizati= on VN, > the fix for PR102608 is to specially handle call to .DEFERRED_INIT in VN = to > exclude it from CSE. > > To fix PR103720, we could do the same as the fix to PR102608 to specially > handle call to .DEFERRED_INIT in Dom to exclude it from being optimized. > > However, in addition to Dom and VN, there should be other optimizations t= hat > have the same issue as PR103720 or PR102608 (As I built Linux kernel with > -ftrivial-auto-var-init=3Dzero -Werror, I noticed a bunch of bugos warnin= gs). > > Other than identifying all the optimizations and specially handling call = to > .DEFERRED_INIT in all these optimizations, changing the 3rd parameter of = the > function .DEFERRED_INIT from IS_VLA to the name string of the var decl mi= ght > be a better workaround (or a fix). After this change, since the 3rd actua= l > parameter is the name string of the variable, different calls for differe= nt > variables will have different name strings as the 3rd actual, As a result= , the > optimization that previously treated the different calls to .DEFERRED_INI= T as > the same will be prevented. > > 2. Prepare for enabling -Wuninitialized + -ftrivail-auto-var-init for add= ress > taken variables. > > As discussion in the following thread: > > https://gcc.gnu.org/pipermail/gcc-patches/2021-August/577431.html > > With the current implemenation of -ftrivial-auto-var-init and uninitializ= ed > warning analysis, the uninitialized warning for an address taken auto var= iable > might be missed since the variable is completely eliminated by optimizati= on and > replaced with a temporary variable in all the uses. > > In order to improve such situation, changing the 3rd parameter of the fun= ction > .DEFERRED_INIT to the name string of the variable will provide necessary > information to uninitialized warning analysis to make the missing warning > possible. > > gcc/ChangeLog: > > 2022-01-10 qing zhao > > * gimplify.c (gimple_add_init_for_auto_var): Delete the 3rd argum= ent. > Change the 3rd argument of function .DEFERRED_INIT to the name of= the > decl. > (gimplify_decl_expr): Delete the 3rd argument when call > gimple_add_init_for_auto_var. > * internal-fn.c (expand_DEFERRED_INIT): Update comments to reflec= t > the 3rd argument change of function .DEFERRED_INIT. > * tree-cfg.c (verify_gimple_call): Update comments and verificati= on > to reflect the 3rd argument change of function .DEFERRED_INIT. > * tree-sra.c (generate_subtree_deferred_init): Delete the 3rd arg= ument. > (sra_modify_deferred_init): Change the 3rd argument of function > .DEFERRED_INIT to the name of the decl. > > gcc/testsuite/ChangeLog: > > 2022-01-10 qing zhao > > * c-c++-common/auto-init-1.c: Adjust testcase to reflect the 3rd > argument change of function .DEFERRED_INIT. > * c-c++-common/auto-init-10.c: Likewise. > * c-c++-common/auto-init-11.c: Likewise. > * c-c++-common/auto-init-12.c: Likewise. > * c-c++-common/auto-init-13.c: Likewise. > * c-c++-common/auto-init-14.c: Likewise. > * c-c++-common/auto-init-15.c: Likewise. > * c-c++-common/auto-init-16.c: Likewise. > * c-c++-common/auto-init-2.c: Likewise. > * c-c++-common/auto-init-3.c: Likewise. > * c-c++-common/auto-init-4.c: Likewise. > * c-c++-common/auto-init-5.c: Likewise. > * c-c++-common/auto-init-6.c: Likewise. > * c-c++-common/auto-init-7.c: Likewise. > * c-c++-common/auto-init-8.c: Likewise. > * c-c++-common/auto-init-9.c: Likewise. > * c-c++-common/auto-init-esra.c: Likewise. > * c-c++-common/auto-init-padding-1.c: Likewise. > * gcc.target/aarch64/auto-init-2.c: Likewise. > > ******The complete patch is attached: >