On Tue, 27 Jul 2021, Qing Zhao wrote:
> Hi,
>
> This is the 6th 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 compile CPU2017 (running is ongoing), without any issue. (With the fix
> to bug https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101586).
>
> Please take a look and let me know any issue.
+/* Handle an "uninitialized" attribute; arguments as in
+ struct attribute_spec.handler. */
+
+static tree
+handle_uninitialized_attribute (tree *node, tree name, tree ARG_UNUSED
(args),
+ int ARG_UNUSED (flags), bool
*no_add_attrs)
+{
+ if (!VAR_P (*node))
+ {
+ warning (OPT_Wattributes, "%qE attribute ignored", name);
+ *no_add_attrs = true;
+ }
you are documenting this attribute for automatic variables but
here you allow placement on globals as well (not sure if at this
point TREE_STATIC / DECL_EXTERNAL are set correctly).
+ /* for languages that do not support BUILT_IN_CLEAR_PADDING, create the
+ function node for padding initialization. */
+ if (!fn)
+ {
+ tree ftype = build_function_type_list (void_type_node,
+ ptr_type_node,
the "appropriate" place to do this would be
tree.c:build_common_builtin_nodes
You seem to marshall the is_vla argument as for_auto_init when
expanding/folding the builtin and there it's used to suppress
diagnostics (and make covered pieces not initialized?). I suggest
to re-name is_vla/for_auto_init to something more descriptive.
+ gimple_fold_builtin_clear_padding. If FOR_AUTO_INIT,
+ not emit some of the error messages since doing that
+ might confuse the end user. */
doesn't explain to me whether errors still might be raised or
what the actual behavior is.
+static gimple *
+build_deferred_init (tree decl,
+ enum auto_init_type init_type,
+ bool is_vla)
+{
+ gcc_assert ((is_vla && TREE_CODE (decl) == WITH_SIZE_EXPR)
+ || (!is_vla && TREE_CODE (decl) != WITH_SIZE_EXPR));
so the is_vla parameter looks redundant (and the assert dangerous?).
Either the caller knows it deals with a VLA, then that should be
passed through - constant sizes can also later appear during
optimization after all - or is_vla should be determined here
based on whether the size at gimplification time is constant.
+ /* If the user requests to initialize automatic variables, we
+ should initialize paddings inside the variable. Add a call to
+ __BUILTIN_CLEAR_PADDING (&object, 0, for_auto_init = true) to
+ initialize paddings of object always to zero regardless of
+ INIT_TYPE. */
+ if (opt_for_fn (current_function_decl, flag_auto_var_init)
+ > AUTO_INIT_UNINITIALIZED
+ && VAR_P (object)
+ && !DECL_EXTERNAL (object)
+ && !TREE_STATIC (object))
+ gimple_add_padding_init_for_auto_var (object, false, pre_p);
+ return ret;
I think you want to use either auto_var_p (object) or
auto_var_in_fn_p (object, current_function_decl). Don't you also
want to check for the 'uninitialized' attribute here? I suggest
to abstract the check on whether 'object' should be subject
to autoinit to a helper function.
There's another path above this calling gimplify_init_constructor
for the case of
const struct S x = { ... };
struct S y = x;
where it will try to init 'y' from the CTOR directly, it seems you
do not cover this case. I also think that the above place applies
to all aggregate assignment statements, not only to INIT_EXPRs?
So don't you want to restrict clear-padding emit here?
+static void
+expand_DEFERRED_INIT (internal_fn, gcall *stmt)
+{
+ tree var = gimple_call_lhs (stmt);
+ tree size_of_var = gimple_call_arg (stmt, 0);
+ tree vlaaddr = NULL_TREE;
+ tree var_type = TREE_TYPE (var);
+ bool is_vla = (bool) TREE_INT_CST_LOW (gimple_call_arg (stmt, 2));
+ enum auto_init_type init_type
+ = (enum auto_init_type) TREE_INT_CST_LOW (gimple_call_arg (stmt, 1));
+
+ gcc_assert (init_type > AUTO_INIT_UNINITIALIZED);
+
+ /* if this variable is a VLA, get its SIZE and ADDR first. */
+ if (is_vla)
+ {
+ /* The temporary address variable for this vla should have been
+ created during gimplification phase. Refer to gimplify_vla_decl
+ for details. */
+ tree var_decl = (TREE_CODE (var) == SSA_NAME) ?
+ SSA_NAME_VAR (var) : var;
+ gcc_assert (DECL_HAS_VALUE_EXPR_P (var_decl));
+ gcc_assert (TREE_CODE (DECL_VALUE_EXPR (var_decl)) ==
INDIRECT_REF);
+ /* Get the address of this vla variable. */
+ vlaaddr = TREE_OPERAND (DECL_VALUE_EXPR (var_decl), 0);
err - isn't the address of the decl represented by the LHS
regardless whether this is a VLA or not? Looking at DECL_VALUE_EXPR
looks quite fragile since that's not sth data dependence honors.
It looks you only partly gimplify the build init here? All
DECL_VALUE_EXPRs should have been resolved.
+ if (is_vla || (!use_register_for_decl (var)))
...
+ else
+ {
+ /* If this variable is in a register, use expand_assignment might
+ generate better code. */
you compute the patter initializer even when not needing it,
that's wasteful. It's also quite ugly, IMHO you should
use can_native_interpret_type_p (var_type) and native_interpret
a char [] array initialized to the pattern and if
!can_native_interpret_type_p () go the memset route.
+ /* We will not verify the arguments for the calls to .DEFERRED_INIT.
+ Such call is not a real call, just a placeholder for a later
+ initialization during expand phase.
+ This is mainly to avoid assertion failure for the following
+ case:
+
+ uni_var = .DEFERRED_INIT (var_size, INIT_TYPE, is_vla);
+ foo (&uni_var);
+
+ in the above, the uninitialized auto variable "uni_var" is
+ addressable, therefore should not be in registers, resulting
+ the assertion failure in the following argument verification. */
+ if (gimple_call_internal_p (stmt, IFN_DEFERRED_INIT))
+ return false;
+
/* ??? The C frontend passes unpromoted arguments in case it
didn't see a function declaration before the call. So for now
leave the call arguments mostly unverified. Once we gimplify
unit-at-a-time we have a chance to fix this. */
- for (i = 0; i < gimple_call_num_args (stmt); ++i)
isn't that from the time there was a decl argument to .DEFERRED_INIT?
+ if (gimple_call_internal_p (stmt, IFN_DEFERRED_INIT))
+ {
+ tree size_of_arg0 = gimple_call_arg (stmt, 0);
+ tree size_of_lhs = TYPE_SIZE_UNIT (TREE_TYPE (lhs));
+ tree is_vla_node = gimple_call_arg (stmt, 2);
+ bool is_vla = (bool) TREE_INT_CST_LOW (is_vla_node);
+
+ if (TREE_CODE (lhs) == SSA_NAME)
+ lhs = SSA_NAME_VAR (lhs);
+
'lhs' is not looked at after this, no need to look at SSA_NAME_VAR.
Thanks and sorry for the delay in reviewing this (again).
Richard.
> Thanks
>
> Qing
>
> ******Compared with the 5th version, the changes are:
>
> 1. Fix two issues raised by Martin Jambor in tree-sra.c:
>
> A. Inside "scan_function", Do not set cannot_scalarize_away_bitmap for a
> call to DEFERRED_INIT.
> B. Fix a potential issue for single-field structure.
>
> 2. Add two testing cases based on gcc/testsuite/gcc.dg/tree-ssa/sra-12.c to
> verity SRA total scalarization will not be confused by auto initializatoin.
>
> ******the 5th version compared with the 4th version, the following are the
> major changes:
>
> 1. delete the code for handling "grp_to_be_debug_replaced" since they are not
> needed per Martin Jambor's suggestion.
> 2. for Pattern init, call __builtin_clear_padding after the call to
> .DEFERRED_INIT to initialize the paddings to zeroes;
> 3. for partially or fully initialized auto variables, call
> __builtin_clear_padding before the real initialization to initialize
> the paddings to zeroes.
> 4. Update the documentation with padding initialization to zeroes.
> 5. in order to reuse __builtin_clear_padding for auto init purpose, add one
> more dummy argument to indiciate whether it's for auto init or not,
> if for auto init, do not emit error messages to avoid confusing users.
> 6. Add new testing cases to verify padding initializations.
> 7. rename some of the old testing cases to make the file name reflecting the
> testing purpose per Kees Cook's suggestions.
>
> ******Please see version 5 at:
> https://gcc.gnu.org/pipermail/gcc-patches/2021-July/575977.html
>
> ******ChangeLog is:
>
> gcc/ChangeLog:
>
> 2021-07-26 qing zhao <[email protected]>
>
> * 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.
> (maybe_with_size_expr): Forword declaration.
> (build_deferred_init): New function.
> (gimple_add_padding_init_for_auto_var): 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_modify_expr_rhs): Add padding initialization before
> gimplify_init_constructor.
> * 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.
>
> gcc/c-family/ChangeLog:
>
> 2021-07-26 qing zhao <[email protected]>
>
> * c-attribs.c (handle_uninitialized_attribute): New function.
> (c_common_attribute_table): Add "uninitialized" attribute.
>
> gcc/testsuite/ChangeLog:
>
>
> 2021-07-26 qing zhao <[email protected]>
>
> * 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-1_b.C: New test.
> * g++.dg/auto-init-uninit-pred-2_a.C: New test.
> * g++.dg/auto-init-uninit-pred-2_b.C: New test.
> * g++.dg/auto-init-uninit-pred-3_a.C: New test.
> * g++.dg/auto-init-uninit-pred-3_b.C: New test.
> * g++.dg/auto-init-uninit-pred-4.C: New test.
> * g++.dg/auto-init-uninit-pred-loop-1_a.cc: New test.
> * g++.dg/auto-init-uninit-pred-loop-1_b.cc: New test.
> * g++.dg/auto-init-uninit-pred-loop-1_c.cc: New test.
> * g++.dg/auto-init-uninit-pred-loop_1.cc: 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-11.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 complete 6th version of the patch is:
>
>
>
--
Richard Biener <[email protected]>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)
