Re: [patch][version 6] add -ftrivial-auto-var-init and variable attribute "uninitialized" to gcc

[Richard Biener <rguenther@suse.de>]

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  <qing.zhao@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.
>         (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  <qing.zhao@oracle.com>
> 
>         * c-attribs.c (handle_uninitialized_attribute): New function.
>         (c_common_attribute_table): Add "uninitialized" attribute.
> 
> gcc/testsuite/ChangeLog:
> 
> 
> 2021-07-26  qing zhao  <qing.zhao@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-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 <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)