On Tue, Jan 11, 2022 at 12:58 AM Qing Zhao <qing.z...@oracle.com> wrote:
>
> Hi, Richard,
>
> I splited the previous patch for “Enable -Wuninitialized +
> -ftrivial-auto-var-init for address taken variables” into two separate
> patches.
> This is the first one
>
> This first patch is to fix (or work around ) PR103720, therefore it’s 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 previous
> concern was on the second part of the patch (the change in
> tree-ssa-uninit.c), I don’t
> want those concern prevent this first patch from being approved into GCC12.
>
>
> In this part, I addressed your comments in gimplify.c :
>
> =====
> tree decl_name
> + = build_string_literal (IDENTIFIER_LENGTH (DECL_NAME (decl)) + 1,
> + IDENTIFIER_POINTER (DECL_NAME (decl)));
>
> you need to deal with DECL_NAME being NULL.
> =====
>
> Please also see the detailed description below for the problem and solution
> of this patch.
>
> This first patch has been bootstrapped and regressing tested on both X86 and
> aarch64.
>
> Okay for GCC12?
+
+ char *decl_name_anonymous = xasprintf ("D.%u", DECL_UID (decl));
+ const char *decl_name_str = DECL_NAME (decl)
+ ? IDENTIFIER_POINTER (DECL_NAME (decl))
+ : decl_name_anonymous;
+ tree decl_name
+ = 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 = build_string_literal (...);
else
{
char *decl_name_anon = xasprintf (...);
decl_name = 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.
>
>
> =================================
>
> 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=103720
>
> As confirmed in PR103720, with the current definition of .DEFERRED_INIT,
>
> Dom transformed:
> c$a$0_6 = .DEFERRED_INIT (8, 2, 0);
> _1 = .DEFERRED_INIT (8, 2, 0);
>
> into:
> c$a$0_6 = .DEFERRED_INIT (8, 2, 0);
> _1 = c$a$0_6;
>
> which is incorrectly done due to Dom treating the two calls to const function
> .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 optimization
> 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 that
> have the same issue as PR103720 or PR102608 (As I built Linux kernel with
> -ftrivial-auto-var-init=zero -Werror, I noticed a bunch of bugos warnings).
>
> 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 might
> be a better workaround (or a fix). After this change, since the 3rd actual
> parameter is the name string of the variable, different calls for different
> variables will have different name strings as the 3rd actual, As a result, the
> optimization that previously treated the different calls to .DEFERRED_INIT as
> the same will be prevented.
>
> 2. Prepare for enabling -Wuninitialized + -ftrivail-auto-var-init for address
> 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 uninitialized
> warning analysis, the uninitialized warning for an address taken auto variable
> might be missed since the variable is completely eliminated by optimization
> and
> replaced with a temporary variable in all the uses.
>
> In order to improve such situation, changing the 3rd parameter of the function
> .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 <qing.z...@oracle.com>
>
> * gimplify.c (gimple_add_init_for_auto_var): Delete the 3rd argument.
> 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 reflect
> the 3rd argument change of function .DEFERRED_INIT.
> * tree-cfg.c (verify_gimple_call): Update comments and verification
> to reflect the 3rd argument change of function .DEFERRED_INIT.
> * tree-sra.c (generate_subtree_deferred_init): Delete the 3rd
> argument.
> (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 <qing.z...@oracle.com>
>
> * 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:
>
