Hi, Richard,
Thanks a lot for your review.
> On Sep 6, 2021, at 5:16 AM, Richard Biener <rguent...@suse.de> wrote:
>
> On Sat, 21 Aug 2021, Qing Zhao wrote:
>
>> Hi,
>>
>> This is the 8th version of the patch for the new security feature for GCC.
>> I have tested it with bootstrap on both x86 and aarch64, regression testing
>> on both x86 and aarch64.
>> Also tested it with the kernel testing case provided by Kees.
>> Also compile CPU2017 (running is ongoing), without any issue.
>>
>> Please take a look at this patch and let me know any issues.
>
> + /* If this DECL is a VLA, a temporary address variable for it has been
> + created, the replacement for DECL is recorded in DECL_VALUE_EXPR
> (decl),
> + we should use it as the LHS of the call. */
> +
> + tree lhs_call
> + = is_vla ? DECL_VALUE_EXPR (decl) : decl;
> + gimplify_assign (lhs_call, call, seq_p);
>
> you shouldn't need to replace the lhs with DECL_VALUE_EXPR of it
> here, gimplify_assign should take care of that.
Okay, I see.
I will change the above sequence simply to the following:
- /* If this DECL is a VLA, a temporary address variable for it has been
- created, the replacement for DECL is recorded in DECL_VALUE_EXPR (decl),
- we should use it as the LHS of the call. */
-
- tree lhs_call
- = is_vla ? DECL_VALUE_EXPR (decl) : decl;
gimplify_assign (lhs_call, call, seq_p);
}
Let me know if this change is not right.
> +/* Return true if the DECL need to be automaticly initialized by the
> + compiler. */
> +static bool
> +is_var_need_auto_init (tree decl)
> +{
> + if (auto_var_p (decl)
> + && (opt_for_fn (current_function_decl, flag_auto_var_init)
>
> maybe I said otherwise at some point but you can test 'flag_auto_var_init'
> directly when not in an IPA pass, no need to use 'opt_for_fn'
Okay, I changed all the usage of “opt_for_fn” back to “flag_auto_var_init”
since the new transformation is not in an IPA pass:
is_var_need_auto_init (tree decl)
{
if (auto_var_p (decl)
- && (opt_for_fn (current_function_decl, flag_auto_var_init)
- > AUTO_INIT_UNINITIALIZED)
+ && (flag_auto_var_init > AUTO_INIT_UNINITIALIZED)
&& (!lookup_attribute ("uninitialized", DECL_ATTRIBUTES (decl))))
return true;
return false;
@@ -1941,8 +1934,7 @@ gimplify_decl_expr (tree *stmt_p, gimple_seq *seq_p)
else if (is_var_need_auto_init (decl))
{
gimple_add_init_for_auto_var (decl,
- opt_for_fn (current_function_decl,
- flag_auto_var_init),
+ flag_auto_var_init,
is_vla,
seq_p);
/* The expanding of a call to the above .DEFERRED_INIT will apply
@@ -1953,8 +1945,7 @@ gimplify_decl_expr (tree *stmt_p, gimple_seq *seq_p)
In order to make the paddings as zeroes for pattern init, We
should add a call to __builtin_clear_padding to clear the
paddings to zero in compatiple with CLANG. */
- if (opt_for_fn (current_function_decl, flag_auto_var_init)
- == AUTO_INIT_PATTERN)
+ if (flag_auto_var_init == AUTO_INIT_PATTERN)
gimple_add_padding_init_for_auto_var (decl, is_vla, seq_p);
}
}
(END)
>
> + > AUTO_INIT_UNINITIALIZED)
> + && (!lookup_attribute ("uninitialized", DECL_ATTRIBUTES (decl))))
> + return true;
> + return false;
>
>
> diff --git a/gcc/tree.c b/gcc/tree.c
> index e923e67b6942..23d7b17774ce 100644
> --- a/gcc/tree.c
> +++ b/gcc/tree.c
> @@ -9508,6 +9508,22 @@ build_common_builtin_nodes (void)
> tree tmp, ftype;
> int ecf_flags;
>
> + /* If user requests automatic variables initialization, the builtin
> + BUILT_IN_CLEAR_PADDING is needed. */
> + if (flag_auto_var_init > AUTO_INIT_UNINITIALIZED
> + && !builtin_decl_explicit_p (BUILT_IN_CLEAR_PADDING))
>
> I think this is prone to fail with LTO and auto-var-init setting
> different in different TUs. Just build the builtin unconditionally
> when it's not available.
So, change the above to:
[opc@qinzhao-ol8u3-x86 gcc]$ git diff tree.c
diff --git a/gcc/tree.c b/gcc/tree.c
index d014fda16cd1..43624059223b 100644
--- a/gcc/tree.c
+++ b/gcc/tree.c
@@ -9510,10 +9510,7 @@ build_common_builtin_nodes (void)
tree tmp, ftype;
int ecf_flags;
- /* If user requests automatic variables initialization, the builtin
- BUILT_IN_CLEAR_PADDING is needed. */
- if (flag_auto_var_init > AUTO_INIT_UNINITIALIZED
- && !builtin_decl_explicit_p (BUILT_IN_CLEAR_PADDING))
+ if (!builtin_decl_explicit_p (BUILT_IN_CLEAR_PADDING))
{
ftype = build_function_type_list (void_type_node,
ptr_type_node,
>
> + {
> + ftype = build_function_type_list (void_type_node,
> + ptr_type_node,
> + ptr_type_node,
> + integer_type_node,
> + NULL_TREE);
> + local_define_builtin ("__builtin_clear_padding", ftype,
> + BUILT_IN_CLEAR_PADDING,
> + "__builtin_clear_padding",
> + ECF_LEAF | ECF_NOTHROW);
> + }
>
>
> With these changes the patch looks OK to me, so please go ahead
> after fixing the above.
Thanks again for your patience and time to review this patch.
I will check in the patch after I finished testing.
Thanks.
Qing
>
> Thanks,
> Ricahrd.
>
>> thanks.
>>
>> Qing
>>
>> In this version:
>>
>> *****Fix all issues raised by Richard B on 8/9: (compared with version 7)
>>
>> 1. in "handle_uninitialized_attribute", exclude globals with additional
>> checking and warning message. Update corresponding testing cases for this
>> change c-c++-common/auto-init-9/10.c
>> 2. add more clarification on the new argument "for_auto_init" for
>> "BUILT_IN_CLEAR_PADDING" in the comments part.
>> 3. use auto_var_p to replace "VAR_P && !DECL_EXTERNAL && !TREE_STATIC";
>> add a new helper routine "is_var_need_auto_init" to check whether a
>> variable need to be initialized.
>> 4. move the "gimple_add_padding_init_for_auto_var" inside the routine
>> "gimplify_init_constructor";
>> 5. restrict gimple_add_padding_init_for_auto_var only for INIT_EXPR;
>> 6. in "gimplify.c" phase, generate a GENERIC call to DEFERRED_INIT +
>> gimplfiy_assign instead of a gimple call to fully gimplify the call to the
>> .DEFERRED_INIT to avoid some potential issues.
>> 7. in "internal-fn.c" phase, use helper routines "lang_hooks.types,
>> type_for_mode", "can_native_interpret_type_p", "native_interpret_expr" to
>> simplify the implementation for pattern generation.
>> 8. move "generating tree node for __BUILTIN_CLEAR_PADDING" in tree.c;
>> 9. cleanup tree-cfg.c.
>> 10. testing cases updates.
>> for address taken variable -Wuninitialized warnings, add "xfail" for C
>> testing cases; deleting C++ testing cases since "xfail" does not work.
>> (FIXME, add them back if something like "xfail" works for C++.)
>>
>> ******Known issue:
>>
>> 1. -Wuninitialized warnings migh not be issued for address taken auto
>> variables.
>> please see https://gcc.gnu.org/pipermail/gcc-patches/2021-August/577431.html
>> for details.
>>
>> Plan: add a separate patch to improve -Wuninitialized on address taken
>> auto variables.
>>
>> ******NOTE:
>>
>> 1. Changes in tree-sra.c have been reviewed and approved by Martin Jambor.
>> 2. the code changes should be very clean now;
>> 3. The testing cases might need more review, I need comments and
>> suggestions on testing cases part. (Not feel very comfortable especially on
>> the testing cases for checking pattern initializations).
>>
>> ******Please see version 7 at:
>> https://gcc.gnu.org/pipermail/gcc-patches/2021-July/576341.html
>>
>> ******ChangeLog is:
>> gcc/ChangeLog:
>>
>> 2021-08-21 qing zhao <qing.z...@oracle.com>
>>
>> * builtins.c (expand_builtin_memset): Make external visible.
>> * builtins.h (expand_builtin_memset): Declare extern.
>> * common.opt (ftrivial-auto-var-init=): New option.
>> * doc/extend.texi: Document the uninitialized attribute.
>> * doc/invoke.texi: Document -ftrivial-auto-var-init.
>> * flag-types.h (enum auto_init_type): New enumerated type
>> auto_init_type.
>> * gimple-fold.c (clear_padding_type): Add one new parameter.
>> (clear_padding_union): Likewise.
>> (clear_padding_emit_loop): Likewise.
>> (clear_type_padding_in_mask): Likewise.
>> (gimple_fold_builtin_clear_padding): Handle this new parameter.
>> * gimplify.c (gimple_add_init_for_auto_var): New function.
>> (gimple_add_padding_init_for_auto_var): New function.
>> (is_var_need_auto_init): New function.
>> (gimplify_decl_expr): Add initialization to automatic variables per
>> users' requests.
>> (gimplify_call_expr): Add one new parameter for call to
>> __builtin_clear_padding.
>> (gimplify_init_constructor): Add padding initialization in the end.
>> * internal-fn.c (INIT_PATTERN_VALUE): New macro.
>> (expand_DEFERRED_INIT): New function.
>> * internal-fn.def (DEFERRED_INIT): New internal function.
>> * tree-cfg.c (verify_gimple_call): Verify calls to .DEFERRED_INIT.
>> * tree-sra.c (generate_subtree_deferred_init): New function.
>> (scan_function): Avoid setting cannot_scalarize_away_bitmap for
>> calls to .DEFERRED_INIT.
>> (sra_modify_deferred_init): New function.
>> (sra_modify_function_body): Handle calls to DEFERRED_INIT specially.
>> * tree-ssa-structalias.c (find_func_aliases_for_call): Likewise.
>> * tree-ssa-uninit.c (warn_uninit): Handle calls to DEFERRED_INIT
>> specially.
>> (check_defs): Likewise.
>> (warn_uninitialized_vars): Likewise.
>> * tree-ssa.c (ssa_undefined_value_p): Likewise.
>> * tree.c (build_common_builtin_nodes): Build tree node for
>> BUILT_IN_CLEAR_PADDING when needed.
>>
>> gcc/c-family/ChangeLog:
>>
>> 2021-08-21 qing zhao <qing.z...@oracle.com>
>>
>> * c-attribs.c (handle_uninitialized_attribute): New function.
>> (c_common_attribute_table): Add "uninitialized" attribute.
>>
>> gcc/testsuite/ChangeLog:
>>
>>
>> 2021-08-20 qing zhao <qing.z...@oracle.com>
>>
>> * c-c++-common/auto-init-1.c: New test.
>> * c-c++-common/auto-init-10.c: New test.
>> * c-c++-common/auto-init-11.c: New test.
>> * c-c++-common/auto-init-12.c: New test.
>> * c-c++-common/auto-init-13.c: New test.
>> * c-c++-common/auto-init-14.c: New test.
>> * c-c++-common/auto-init-15.c: New test.
>> * c-c++-common/auto-init-16.c: New test.
>> * c-c++-common/auto-init-2.c: New test.
>> * c-c++-common/auto-init-3.c: New test.
>> * c-c++-common/auto-init-4.c: New test.
>> * c-c++-common/auto-init-5.c: New test.
>> * c-c++-common/auto-init-6.c: New test.
>> * c-c++-common/auto-init-7.c: New test.
>> * c-c++-common/auto-init-8.c: New test.
>> * c-c++-common/auto-init-9.c: New test.
>> * c-c++-common/auto-init-esra.c: New test.
>> * c-c++-common/auto-init-padding-1.c: New test.
>> * c-c++-common/auto-init-padding-2.c: New test.
>> * c-c++-common/auto-init-padding-3.c: New test.
>> * g++.dg/auto-init-uninit-pred-1_a.C: New test.
>> * g++.dg/auto-init-uninit-pred-2_a.C: New test.
>> * g++.dg/auto-init-uninit-pred-3_a.C: New test.
>> * g++.dg/auto-init-uninit-pred-4.C: New test.
>> * gcc.dg/auto-init-sra-1.c: New test.
>> * gcc.dg/auto-init-sra-2.c: New test.
>> * gcc.dg/auto-init-uninit-1.c: New test.
>> * gcc.dg/auto-init-uninit-12.c: New test.
>> * gcc.dg/auto-init-uninit-13.c: New test.
>> * gcc.dg/auto-init-uninit-14.c: New test.
>> * gcc.dg/auto-init-uninit-15.c: New test.
>> * gcc.dg/auto-init-uninit-16.c: New test.
>> * gcc.dg/auto-init-uninit-17.c: New test.
>> * gcc.dg/auto-init-uninit-18.c: New test.
>> * gcc.dg/auto-init-uninit-19.c: New test.
>> * gcc.dg/auto-init-uninit-2.c: New test.
>> * gcc.dg/auto-init-uninit-20.c: New test.
>> * gcc.dg/auto-init-uninit-21.c: New test.
>> * gcc.dg/auto-init-uninit-22.c: New test.
>> * gcc.dg/auto-init-uninit-23.c: New test.
>> * gcc.dg/auto-init-uninit-24.c: New test.
>> * gcc.dg/auto-init-uninit-25.c: New test.
>> * gcc.dg/auto-init-uninit-26.c: New test.
>> * gcc.dg/auto-init-uninit-3.c: New test.
>> * gcc.dg/auto-init-uninit-34.c: New test.
>> * gcc.dg/auto-init-uninit-36.c: New test.
>> * gcc.dg/auto-init-uninit-37.c: New test.
>> * gcc.dg/auto-init-uninit-4.c: New test.
>> * gcc.dg/auto-init-uninit-5.c: New test.
>> * gcc.dg/auto-init-uninit-6.c: New test.
>> * gcc.dg/auto-init-uninit-8.c: New test.
>> * gcc.dg/auto-init-uninit-9.c: New test.
>> * gcc.dg/auto-init-uninit-A.c: New test.
>> * gcc.dg/auto-init-uninit-B.c: New test.
>> * gcc.dg/auto-init-uninit-C.c: New test.
>> * gcc.dg/auto-init-uninit-H.c: New test.
>> * gcc.dg/auto-init-uninit-I.c: New test.
>> * gcc.target/aarch64/auto-init-1.c: New test.
>> * gcc.target/aarch64/auto-init-2.c: New test.
>> * gcc.target/aarch64/auto-init-3.c: New test.
>> * gcc.target/aarch64/auto-init-4.c: New test.
>> * gcc.target/aarch64/auto-init-5.c: New test.
>> * gcc.target/aarch64/auto-init-6.c: New test.
>> * gcc.target/aarch64/auto-init-7.c: New test.
>> * gcc.target/aarch64/auto-init-8.c: New test.
>> * gcc.target/aarch64/auto-init-padding-1.c: New test.
>> * gcc.target/aarch64/auto-init-padding-10.c: New test.
>> * gcc.target/aarch64/auto-init-padding-11.c: New test.
>> * gcc.target/aarch64/auto-init-padding-12.c: New test.
>> * gcc.target/aarch64/auto-init-padding-2.c: New test.
>> * gcc.target/aarch64/auto-init-padding-3.c: New test.
>> * gcc.target/aarch64/auto-init-padding-4.c: New test.
>> * gcc.target/aarch64/auto-init-padding-5.c: New test.
>> * gcc.target/aarch64/auto-init-padding-6.c: New test.
>> * gcc.target/aarch64/auto-init-padding-7.c: New test.
>> * gcc.target/aarch64/auto-init-padding-8.c: New test.
>> * gcc.target/aarch64/auto-init-padding-9.c: New test.
>> * gcc.target/i386/auto-init-1.c: New test.
>> * gcc.target/i386/auto-init-2.c: New test.
>> * gcc.target/i386/auto-init-21.c: New test.
>> * gcc.target/i386/auto-init-22.c: New test.
>> * gcc.target/i386/auto-init-23.c: New test.
>> * gcc.target/i386/auto-init-24.c: New test.
>> * gcc.target/i386/auto-init-3.c: New test.
>> * gcc.target/i386/auto-init-4.c: New test.
>> * gcc.target/i386/auto-init-5.c: New test.
>> * gcc.target/i386/auto-init-6.c: New test.
>> * gcc.target/i386/auto-init-7.c: New test.
>> * gcc.target/i386/auto-init-8.c: New test.
>> * gcc.target/i386/auto-init-padding-1.c: New test.
>> * gcc.target/i386/auto-init-padding-10.c: New test.
>> * gcc.target/i386/auto-init-padding-11.c: New test.
>> * gcc.target/i386/auto-init-padding-12.c: New test.
>> * gcc.target/i386/auto-init-padding-2.c: New test.
>> * gcc.target/i386/auto-init-padding-3.c: New test.
>> * gcc.target/i386/auto-init-padding-4.c: New test.
>> * gcc.target/i386/auto-init-padding-5.c: New test.
>> * gcc.target/i386/auto-init-padding-6.c: New test.
>> * gcc.target/i386/auto-init-padding-7.c: New test.
>> * gcc.target/i386/auto-init-padding-8.c: New test.
>> * gcc.target/i386/auto-init-padding-9.c: New test.
>>
>> ****** The patch against the 7th patch:
>>
>>
>> ******The complete 8th patch:
>>
>>
>
> --
> Richard Biener <rguent...@suse.de>
> SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
> Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)
