> On Jan 11, 2022, at 7:53 AM, Richard Biener <richard.guent...@gmail.com>
> wrote:
>
> 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 for the comment, will fix this and commit the patch.
Qing
>
> 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:
