Re: [patch for gcc12 stage1][version 2] add -ftrivial-auto-var-init and variable attribute "uninitialized" to gcc

[Richard Sandiford <richard.sandiford@arm.com>]

Finally getting to this now that the GCC 11 rush is over.  Sorry for
the slow response.

I've tried to review most of the code below, but skipped the testsuite
parts in the interests of time.  I'll probably have more comments in
future rounds, just wanted to get the ball rolling.

This is realy Richi's area more than mine though, so please take this
with a grain of salt.

Qing Zhao <qing.zhao@oracle.com> writes:
> 2.  initialize all paddings to zero when -ftrivial-auto-var-init is present.
> In expr.c (store_constructor):
>
>         Clear the whole structure when
>         -ftrivial-auto-var-init and the structure has paddings.
>
> In gimplify.c (gimplify_init_constructor):
>
>         Clear the whole structure when
>         -ftrivial-auto-var-init and the structure has paddings.

Just to check: are we sure we want to use zero as the padding fill
value even for -ftrivial-auto-var-init=pattern?  Or should it be
0xAA instead, to match the integer fill pattern?

I can see the arguments both ways, just thought it was worth asking.

> […]
> @@ -1589,6 +1592,24 @@ handle_retain_attribute (tree *pnode, tree name, tree ARG_UNUSED (args),
>    return NULL_TREE;
>  }
>  
> +/* Handle a "uninitialized" attribute; arguments as in

This occurs in existing code too, but s/a/an/.

> +   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))
> +    DECL_UNINITIALIZED (*node) = 1;
> +  else
> +    {
> +      warning (OPT_Wattributes, "%qE attribute ignored", name);
> +      *no_add_attrs = true;
> +    }
> +
> +  return NULL_TREE;
> +}
> +
>  /* Handle a "externally_visible" attribute; arguments as in
>     struct attribute_spec.handler.  */

> […]
> @@ -11689,6 +11689,34 @@ Perform basic block vectorization on trees. This flag is enabled by default at
>  @option{-O3} and by @option{-ftree-vectorize}, @option{-fprofile-use},
>  and @option{-fauto-profile}.
>  
> +@item -ftrivial-auto-var-init=@var{choice}
> +@opindex ftrivial-auto-var-init
> +Initialize automatic variables with either a pattern or with zeroes to increase
> +program security by preventing uninitialized memory disclosure and use.
> +
> +The three values of @var{choice} are:
> +
> +@itemize @bullet
> +@item
> +@samp{uninitialized} doesn't initialize any automatic variables.
> +This is C and C++'s default.
> +
> +@item
> +@samp{pattern} Initialize automatic variables with values which will likely
> +transform logic bugs into crashes down the line, are easily recognized in a
> +crash dump and without being values that programmers can rely on for useful
> +program semantics.
> +The values used for pattern initialization might be changed in the future.
> +
> +@item
> +@samp{zero} Initialize automatic variables with zeroes.
> +@end itemize
> +
> +The default is @samp{uninitialized}.
> +
> +You can control this behavior for a specific variable by using the variable
> +attribute @code{uninitialized} (@pxref{Variable Attributes}).
> +

I think it's important to say here that GCC still considers the
variables to be uninitialised and still considers reading them to
be undefined behaviour.  The option is simply trying to improve the
security and predictability of the program in the presence of these
uninitialised variables.

I think it would also be worth saying that options like -Wuninitialized
still try to warn about uninitialised variables, although using
-ftrivial-auto-var-init may change which warnings are generated.

(The above comments are just a summary, not suitable for direct
inclusion. :-))

>  @item -fvect-cost-model=@var{model}
>  @opindex fvect-cost-model
>  Alter the cost model used for vectorization.  The @var{model} argument
> […]
> diff --git a/gcc/gimplify.c b/gcc/gimplify.c
> index 6da6698..fafd2e9 100644
> --- a/gcc/gimplify.c
> +++ b/gcc/gimplify.c
> @@ -1716,6 +1716,116 @@ gimplify_vla_decl (tree decl, gimple_seq *seq_p)
>  
>    gimplify_and_add (t, seq_p);
>  
> +  /* Add a call to memset or calls to memcpy to initialize this vla
> +     when the user requested.  */
> +  if (!DECL_ARTIFICIAL (decl)
> +      && VAR_P (decl)
> +      && !DECL_EXTERNAL (decl)
> +      && !TREE_STATIC (decl)
> +      && !DECL_UNINITIALIZED (decl))
> +    switch (flag_trivial_auto_var_init)
> +      {
> +      case AUTO_INIT_UNINITIALIZED:
> +	break;
> +      case AUTO_INIT_ZERO:
> +	{
> +	  /* Generate a call to memset to initialize this vla.  */
> +	  gcall *gs;
> +	  t = builtin_decl_implicit (BUILT_IN_MEMSET);
> +	  gs = gimple_build_call (t, 3, addr, integer_zero_node,
> +				  DECL_SIZE_UNIT (decl));
> +	  gimple_call_set_memset_for_uninit (gs, true);
> +	  gimplify_seq_add_stmt (seq_p, gs);
> +	}
> +	break;
> +      case AUTO_INIT_PATTERN:
> +	{
> +	  /* Generate the following sequence to initialize this vla:
> +	     if (DECL_SIZE_UNIT (decl) > 0) goto label_true;
> +					    else goto label_cont;
> +	     label_true:
> +	      {
> +		element_type = TREE_TYPE (TREE_TYPE (decl));
> +		size_of_element = DECL_SIZE_UNIT (element_type);
> +		init_node = build_pattern_cst (element_type);
> +		cur = addr;
> +		offset = DECL_SIZE_UNIT (decl) - size_of_element;
> +		end = addr + offset;
> +
> +		label_loop:
> +		  {
> +		    memcpy (cur, &init_node, size_of_element);
> +		    cur  += size_of_element;
> +		    if (cur <= end) goto label_loop;
> +				    else goto label_cont;
> +		  }
> +	      }
> +	      label_cont:
> +	  */
> +
> +	  tree size_of_element, element_type;
> +	  tree cur, end, offset;
> +	  tree init_node, addrof_init_node;
> +	  tree t;
> +
> +	  tree label_true = create_artificial_label (UNKNOWN_LOCATION);
> +	  tree label_cont = create_artificial_label (UNKNOWN_LOCATION);
> +	  tree label_loop = create_artificial_label (UNKNOWN_LOCATION);
> +
> +	  element_type = TREE_TYPE (TREE_TYPE (decl));
> +
> +	  gcond *cond_stmt = gimple_build_cond (GT_EXPR, DECL_SIZE_UNIT (decl),
> +						build_zero_cst (sizetype),
> +						label_true,
> +						label_cont);
> +	  gimplify_seq_add_stmt (seq_p, cond_stmt);
> +	  gimplify_seq_add_stmt (seq_p, gimple_build_label (label_true));
> +
> +	  size_of_element = create_tmp_var (sizetype,
> +					    ".size_of_element");
> +
> +	  init_node = create_tmp_var (element_type, ".init_node");
> +	  mark_addressable (init_node);
> +	  addrof_init_node = build_fold_addr_expr_loc (UNKNOWN_LOCATION,
> +						       init_node);
> +
> +	  gimplify_assign (size_of_element, TYPE_SIZE_UNIT (element_type),
> +			   seq_p);
> +	  gimplify_assign (init_node, build_pattern_cst (element_type), seq_p);

Rather than building a separate init_node here, would it work to assign
to element 0 instead?  Then the loop could start at element 1.

What about nested VLAs, like:

  void g(void *);
  void f(int a)
  {
    int x[a][a];
    g(x);
  }

?

> +
> +	  cur = create_tmp_var (ptr_type, ".cur_addr");
> +	  end = create_tmp_var (ptr_type, ".end_addr");
> +	  offset = create_tmp_var (sizetype, ".offset");
> +
> +	  gimplify_assign (cur, addr, seq_p);
> +	  gimplify_seq_add_stmt (seq_p,
> +				 gimple_build_assign (offset, MINUS_EXPR,
> +						      DECL_SIZE_UNIT (decl),
> +						      size_of_element));
> +	  gimplify_seq_add_stmt (seq_p,
> +				 gimple_build_assign (end, POINTER_PLUS_EXPR,
> +						      addr, offset));
> +
> +	  gimplify_seq_add_stmt (seq_p, gimple_build_label (label_loop));
> +
> +	  t = builtin_decl_implicit (BUILT_IN_MEMCPY);
> +	  gimplify_seq_add_stmt (seq_p,
> +				 gimple_build_call (t, 3, cur,
> +						    addrof_init_node,
> +						    size_of_element));
> +	  gimplify_seq_add_stmt (seq_p,
> +				 gimple_build_assign (cur, POINTER_PLUS_EXPR,
> +						      cur, size_of_element));
> +	  cond_stmt = gimple_build_cond (LE_EXPR, cur, end, label_loop,
> +					 label_cont);
> +	  gimplify_seq_add_stmt (seq_p, cond_stmt);
> +	  gimplify_seq_add_stmt (seq_p, gimple_build_label (label_cont));
> +	}
> +	break;
> +      default:
> +	gcc_assert (0);
> +      }
> +
>    /* Record the dynamic allocation associated with DECL if requested.  */
>    if (flag_callgraph_info & CALLGRAPH_INFO_DYNAMIC_ALLOC)
>      record_dynamic_alloc (decl);
> @@ -1738,6 +1848,65 @@ force_labels_r (tree *tp, int *walk_subtrees, void *data ATTRIBUTE_UNUSED)
>    return NULL_TREE;
>  }
>  
> +
> +/* Build a call to internal const function DEFERRED_INIT:
> +   1st argument: DECL;
> +   2nd argument: INIT_TYPE;
> +
> +   as DEFERRED_INIT (DECL, INIT_TYPE)
> +
> +   DEFERRED_INIT is defined as:
> +   DEF_INTERNAL_FN(DEFERRED_INIT, ECF_CONST | ECF_LEAF | ECF_NOTHROW, NULL).  */

I don't think it's necessary to repeat the definition here.

> +
> +static gimple *
> +build_deferred_init (tree decl,
> +		     enum auto_init_type init_type)
> +{
> +  tree init_type_node
> +    = build_int_cst (integer_type_node, (int) init_type);
> +   return gimple_build_call_internal (IFN_DEFERRED_INIT, 2,
> +				      decl, init_type_node);

Nit: indentation is off in the return statement.

> +}
> +
> […]
> @@ -1831,6 +2000,17 @@ gimplify_decl_expr (tree *stmt_p, gimple_seq *seq_p)
>  	       as they may contain a label address.  */
>  	    walk_tree (&init, force_labels_r, NULL, NULL);
>  	}
> +      /* When there is no explicit initializer, if the user requested,
> +	 We should insert an artifical initializer for this automatic
> +	 variable for non vla variables.  */

I think we should explain why we can skip VLAs here.

> +      else if (flag_trivial_auto_var_init > AUTO_INIT_UNINITIALIZED
> +	       && !DECL_UNINITIALIZED (decl)
> +	       && !TREE_STATIC (decl)
> +	       && !is_vla)
> +	gimple_add_init_for_auto_var (decl,
> +				      flag_trivial_auto_var_init,
> +				      flag_auto_init_approach,
> +				      seq_p);
>      }
>  
>    return GS_ALL_DONE;
> […]
> diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
> index 7e3aae5..da02be2 100644
> --- a/gcc/tree-cfg.c
> +++ b/gcc/tree-cfg.c
> @@ -3471,6 +3471,9 @@ verify_gimple_call (gcall *stmt)
>  	}
>      }
>  
> +  if (gimple_call_internal_p (stmt, IFN_DEFERRED_INIT))
> +    return false;
> +

Why is this needed?  I think a comment would be helpful.

>    /* ???  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
> diff --git a/gcc/tree-core.h b/gcc/tree-core.h
> index d2e6c89..fb42c41 100644
> --- a/gcc/tree-core.h
> +++ b/gcc/tree-core.h
> @@ -1815,6 +1815,7 @@ struct GTY(()) tree_decl_with_vis {
>   unsigned in_text_section : 1;
>   unsigned in_constant_pool : 1;
>   unsigned dllimport_flag : 1;
> + unsigned uninitialized : 1;
>   /* Don't belong to VAR_DECL exclusively.  */
>   unsigned weak_flag : 1;
>  
> @@ -1836,7 +1837,7 @@ struct GTY(()) tree_decl_with_vis {
>   unsigned final : 1;
>   /* Belong to FUNCTION_DECL exclusively.  */
>   unsigned regdecl_flag : 1;
> - /* 14 unused bits. */
> + /* 13 unused bits.  */

It doesn't look like DECL_UNINITIALIZED is used in compile-time-sensitive
code, so it might be better to keep "uninitialized" as a normal attribute
and use lookup_attribute where necessary.

>   /* 32 more unused on 64 bit HW. */
>  };
>  
> diff --git a/gcc/tree-sra.c b/gcc/tree-sra.c
> index d177f1b..fe439f7 100644
> --- a/gcc/tree-sra.c
> +++ b/gcc/tree-sra.c
> @@ -384,6 +384,13 @@ static struct
>  
>    /* Numbber of components created when splitting aggregate parameters.  */
>    int param_reductions_created;
> +
> +  /* Number of deferred_init calls that are modified.  */
> +  int deferred_init;
> +
> +  /* Number of deferred_init calls that are created by
> +     generate_subtree_deferred_init.  */
> +  int subtree_deferred_init;
>  } sra_stats;
>  
>  static void
> @@ -4070,6 +4077,105 @@ get_repl_default_def_ssa_name (struct access *racc, tree reg_type)
>    return get_or_create_ssa_default_def (cfun, racc->replacement_decl);
>  }
>  
> +
> +/* Generate statements to call .DEFERRED_INIT to initialize scalar replacements
> +   of accesses within a subtree ACCESS, all its children, siblings and their

IMO reads more easily with “;” rather than “,” before “all”

> +   children are to be processed.  TOP_OFFSET is the offset  of the processed
> +   subtree which has to be subtracted from offsets of individual accesses to
> +   get corresponding offsets for AGG.  GSI is a statement iterator used to place
> +   the new statements.  */
> +static void
> +generate_subtree_deferred_init (struct access *access, tree agg,
> +				enum auto_init_type init_type,
> +				HOST_WIDE_INT top_offset,
> +				gimple_stmt_iterator *gsi,
> +				location_t loc)
> +{
> +  do
> +    {
> +      if (access->grp_to_be_replaced)
> +	{
> +	  tree repl = get_access_replacement (access);
> +	  tree init_type_node
> +	    = build_int_cst (integer_type_node, (int) init_type);
> +	  gimple *call = gimple_build_call_internal (IFN_DEFERRED_INIT, 2,
> +						     repl, init_type_node);
> +	  gimple_call_set_lhs (call, repl);

AFAICT “access” is specifically for the lhs of the original call.
So there seems to be an implicit assumption here that the lhs of the
original call is the same as the first argument of the original call.
Is that guaranteed/required?  If so, I think it's something that
tree-cfg.c should check.  It might also be worth having an assertion
in sra_modify_deferred_init.

If the argument and lhs can be different then I think we need to
handle the access patterns for them both.

Or is the idea that any instance of:

  LHS = .DEFERRED_INIT (RHS, INIT_TYPE)

can be replaced with:

  LHS = .DEFERRED_INIT (LHS, INIT_TYPE)

without affecting semantics?  If so, I think that would be worth
a comment.

> +	  gsi_insert_before (gsi, call, GSI_SAME_STMT);
> +	  update_stmt (call);

It seems odd that we need to call update_stmt for a new stmt, but I see
other code in the file also does this.

> +	  gimple_set_location (call, loc);
> +
> +	  sra_stats.subtree_deferred_init++;
> +	}
> +      else if (access->grp_to_be_debug_replaced)
> +	{
> +	  /* FIXME, this part might have some issue.  */
> +	  tree drhs = build_debug_ref_for_model (loc, agg,
> +						 access->offset - top_offset,
> +						 access);
> +	  gdebug *ds = gimple_build_debug_bind (get_access_replacement (access),
> +						drhs, gsi_stmt (*gsi));
> +	  gsi_insert_before (gsi, ds, GSI_SAME_STMT);

Would be good to fix the FIXME :-)

I guess the thing we need to decide here is whether -ftrivial-auto-var-init
should affect debug-only constructs too.  If it doesn't, exmaining removed
components in a debugger might show uninitialised values in cases where
the user was expecting initialised ones.  There would be no security
concern, but it might be surprising.

I think in principle the DRHS can contain a call to DEFERRED_INIT.
Doing that would probably require further handling elsewhere though.

> +	}
> +      if (access->first_child)
> +	generate_subtree_deferred_init (access->first_child, agg, init_type,
> +					top_offset, gsi, loc);
> +
> +      access = access ->next_sibling;
> +    }
> +  while (access);
> +}
> +
> +/* For a call to .DEFERRED_INIT:
> +   var = .DEFERRED_INIT (var, init_type);
> +   examine the LHS variable VAR and replace it with a scalar replacement if
> +   there is one, also replace the RHS call to a call to .DEFERRED_INIT of
> +   the corresponding scalar relacement variable.  Examine the subtree and
> +   do the scalar replacements in the subtree too.  STMT is the call, GSI is
> +   the statment iterator to place newly created statements.  */

typo: statement

> +
> +static enum assignment_mod_result
> +sra_modify_deferred_init (gimple *stmt, gimple_stmt_iterator *gsi)
> +{
> +  tree lhs = gimple_call_lhs (stmt);
> +  enum auto_init_type init_type
> +    = (enum auto_init_type) TREE_INT_CST_LOW (gimple_call_arg (stmt, 1));
> +  struct access *access = get_access_for_expr (lhs);
> +  if (!access)
> +    return SRA_AM_NONE;
> +  location_t loc = gimple_location (stmt);
> +
> +  if (access->grp_to_be_replaced)
> +    {
> +      tree repl = get_access_replacement (access);
> +      gimple_call_set_lhs (stmt, repl);
> +      gimple_call_set_arg (stmt, 0, repl);
> +      sra_stats.deferred_init++;
> +    }
> +  else if (access->grp_to_be_debug_replaced)
> +    {
> +      /* FIXME, this part might have some issues.  */
> +      tree drepl = get_access_replacement (access);
> +      gdebug *ds = gimple_build_debug_bind (drepl, NULL_TREE,
> +					    gsi_stmt (*gsi));
> +      gsi_insert_before (gsi, ds, GSI_SAME_STMT);
> +    }

Same comments as above.

> +
> +  if (access->first_child)
> +    generate_subtree_deferred_init (access->first_child, lhs,
> +				    init_type, access->offset,
> +				    gsi, loc);
> +  if (access->grp_covered)
> +    {
> +      unlink_stmt_vdef (stmt);
> +      gsi_remove (gsi, true);
> +      release_defs (stmt);
> +      return SRA_AM_REMOVED;
> +    }
> +
> +  return SRA_AM_MODIFIED;
> +}
> +
>  /* Examine both sides of the assignment statement pointed to by STMT, replace
>     them with a scalare replacement if there is one and generate copying of
>     replacements if scalarized aggregates have been used in the assignment.  GSI
> […]
> @@ -4863,6 +4863,29 @@ find_func_aliases_for_builtin_call (struct function *fn, gcall *t)
>    return false;
>  }
>  
> +static void
> +find_func_aliases_for_deferred_init (gcall *t)
> +{
> +  tree lhsop = gimple_call_lhs (t);
> +  enum auto_init_type init_type
> +    = (enum auto_init_type) TREE_INT_CST_LOW (gimple_call_arg (t, 1));
> +  auto_vec<ce_s, 2> lhsc;
> +  auto_vec<ce_s, 4> rhsc;
> +  struct constraint_expr temp;
> +
> +  get_constraint_for (lhsop, &lhsc);
> +  if (init_type == AUTO_INIT_ZERO && flag_delete_null_pointer_checks)
> +    temp.var = nothing_id;
> +  else
> +    temp.var = nonlocal_id;
> +  temp.type = ADDRESSOF;
> +  temp.offset = 0;
> +  rhsc.safe_push (temp);
> +
> +  process_all_all_constraints (lhsc, rhsc);
> +  return;
> +}
> +

What's the reasoning behind doing it like this?  AFAICT the result
of the call doesn't validly alias anything, regardless of the init type.
Even if there happens to be a valid decl at the address given by the
chosen fill pattern, there's no expectation that accesses to that
decl will be coherent wrt accesses via the result of the call.

>  /* Create constraints for the call T.  */
>  
>  static void
> […]
> diff --git a/gcc/tree-ssa-uninit.c b/gcc/tree-ssa-uninit.c
> index 0800f59..0c61f60 100644
> --- a/gcc/tree-ssa-uninit.c
> +++ b/gcc/tree-ssa-uninit.c
> @@ -135,6 +135,20 @@ warn_uninit (enum opt_code wc, tree t, tree expr, tree var,
>    if (is_gimple_assign (context)
>        && gimple_assign_rhs_code (context) == COMPLEX_EXPR)
>      return;
> +
> +  /* Ignore REALPART_EXPR or IMAGPART_EXPR if its operand is
> +     a call to .DEFERRED_INIT.  */
> +  if (is_gimple_assign (context)
> +      && (gimple_assign_rhs_code (context) == REALPART_EXPR
> +	  || gimple_assign_rhs_code (context) == IMAGPART_EXPR))
> +    {
> +      tree v = gimple_assign_rhs1 (context);
> +      if (TREE_CODE (TREE_OPERAND (v, 0)) == SSA_NAME
> +	  && gimple_call_internal_p (SSA_NAME_DEF_STMT (TREE_OPERAND (v, 0)),
> +				     IFN_DEFERRED_INIT))
> +	return;
> +    }
> +

Which case is this handling?  In this context I would have expected
the REALPART_EXPRs and IMAGPART_EXPRs to act like normal rvalue
accessors, i.e. they are returning one half of their argument and
discarding the other half.  Why is this different from, say, accessing
one field of a structure?

>    if (!has_undefined_value_p (t))
>      return;
>  
> @@ -209,6 +223,19 @@ check_defs (ao_ref *ref, tree vdef, void *data_)
>  {
>    check_defs_data *data = (check_defs_data *)data_;
>    gimple *def_stmt = SSA_NAME_DEF_STMT (vdef);
> +
> +  /* Ignore the vdef iff the definition statement is a call
> +     to .DEFERRED_INIT function.  */
> +  if (gimple_call_internal_p (def_stmt, IFN_DEFERRED_INIT))
> +    return false;
> +
> +  /* Ignore the vdef iff the definition statement is a call
> +     to builtin_memset function that is added for uninitialized
> +     auto variable initialization.  */
> +  if (gimple_call_builtin_p (def_stmt, BUILT_IN_MEMSET)
> +      && gimple_call_memset_for_uninit_p (def_stmt))
> +    return false;
> +

s/iff/if/, since the “only if” part doesn't apply.

>    /* If this is a clobber then if it is not a kill walk past it.  */
>    if (gimple_clobber_p (def_stmt))
>      {
> @@ -611,6 +638,9 @@ warn_uninitialized_vars (bool wmaybe_uninit)
>  	  ssa_op_iter op_iter;
>  	  tree use;
>  
> +	  if (gimple_call_internal_p (stmt, IFN_DEFERRED_INIT))
> +	    continue;
> +

I guess the reasoning here is that the call is an artificial use and so
won't give a meaningful error message.  If the result of the call is
used somewhere else then we warn there instead.

I think that deserves a comment though.

>  	  if (is_gimple_debug (stmt))
>  	    continue;
>  
> diff --git a/gcc/tree-ssa.c b/gcc/tree-ssa.c
> index cf54c89..93d6124 100644
> --- a/gcc/tree-ssa.c
> +++ b/gcc/tree-ssa.c
> @@ -1325,6 +1325,23 @@ ssa_undefined_value_p (tree t, bool partial)
>    if (gimple_nop_p (def_stmt))
>      return true;
>  
> +  /* The value is undefined iff the definition statement is a call
> +     to .DEFERRED_INIT function.  */
> +  if (gimple_call_internal_p (def_stmt, IFN_DEFERRED_INIT))
> +    return true;
> +
> +  if (partial && is_gimple_assign (def_stmt)
> +      && (gimple_assign_rhs_code (def_stmt) == REALPART_EXPR
> +	  || gimple_assign_rhs_code (def_stmt) == IMAGPART_EXPR))
> +    {
> +      tree real_imag_part = TREE_OPERAND (gimple_assign_rhs1 (def_stmt), 0);
> +      if (TREE_CODE (real_imag_part) == SSA_NAME
> +	 && gimple_call_internal_p (SSA_NAME_DEF_STMT (real_imag_part),
> +				    IFN_DEFERRED_INIT))
> +	return true;
> +    }
> +
> +

Same s/iff/if/ comment here.  Also same question as above about the
REALPART_EXPR/IMAGPART_EXPR thing.

Nit: too many blank links at the end.

>    /* Check if the complex was not only partially defined.  */
>    if (partial && is_gimple_assign (def_stmt)
>        && gimple_assign_rhs_code (def_stmt) == COMPLEX_EXPR)
> diff --git a/gcc/tree.c b/gcc/tree.c
> index 7c44c22..2c9acad 100644
> --- a/gcc/tree.c
> +++ b/gcc/tree.c
> @@ -2531,6 +2531,144 @@ build_zero_cst (tree type)
>      }
>  }
>  
> +/* Build pattern constant of type TYPE.  This is used for initializing
> +   auto variables.  */
> +
> +tree
> +build_pattern_cst (tree type)

I think this should have a more descriptive name, since the pattern
is very specific to the use case.

> +{
> +  /* The following value is a guaranteed unmappable pointer value and has a
> +     repeated byte-pattern which makes it easier to synthesize.  We use it for
> +     pointers as well as integers so that aggregates are likely to be
> +     initialized with this repeated value.  */
> +  uint64_t largevalue = 0xAAAAAAAAAAAAAAAAull;
> +  /* For 32-bit platforms it's a bit trickier because, across systems, only the
> +     zero page can reasonably be expected to be unmapped, and even then we need
> +     a very low address.  We use a smaller value, and that value sadly doesn't
> +     have a repeated byte-pattern.  We don't use it for integers.  */
> +  uint32_t smallvalue = 0x000000AA;
> +
> +  switch (TREE_CODE (type))
> +    {
> +    case INTEGER_TYPE:
> +    case ENUMERAL_TYPE:
> +    case BOOLEAN_TYPE:
> +      /* This will initialize a boolean type variable to 0 instead of 1.
> + 	 We think that initializint a boolean variable to 0 other than 1

initializing

> +	 is better even for pattern initialization.  */
> +      return build_int_cstu (type, largevalue);

I've no objection to that choice for booleans, but: booleans in some
languages (like Ada) can have multibit precision.  If we want booleans
to be zero then it would probably be better to treat them as a separate
case and just use build_zero_cst (type) for them.

Also, the above won't work correctly for 128-bit integers: it will
zero-initialize the upper half.  It would probably be better to use
wi::from_buffer to construct the integer instead.

I'm also not sure what effect this will have on enumerations, or when
the fill value is outside the TYPE_MIN_VALUE…TYPE_MAX_VALUE range.
Hopefully Richi will chime in :-)

> +    case POINTER_TYPE:
> +    case OFFSET_TYPE:
> +    case REFERENCE_TYPE:
> +    case NULLPTR_TYPE:
> +      {
> +	poly_uint64 intvalue;
> +
> +	if (POINTER_SIZE == 64)
> +	  intvalue = largevalue;
> +	else if (POINTER_SIZE == 32)
> +	  intvalue = smallvalue;
> +	else
> +	  gcc_assert (0);

GCC supports 16-bit targets, so we can't assert here.  We might as
well just go for 0xAA there too.

In fact it might be simpler to have something like:

  if (POINTER_TYPE_P (type) && TYPE_PRECISION (type) < 64)
    return build_int_cstu (type, 0xAA);

  if (INTEGRAL_TYPE_P (type) || POINTER_TYPE_P (type))
    ...the wi::from_buffer thing above...;

I'm not sure if this makes sense for NULLPTR_TYPE.

> +	return build_int_cstu (type, intvalue);
> +      }
> +    case REAL_TYPE:
> +      {
> +	REAL_VALUE_TYPE rnan;
> +
> +	/* create an quiet NAN for REAL TYPE.  */
> +	if (real_nan (&rnan, "", 1, TYPE_MODE (type)))
> +	  return build_real (type, rnan);
> +	return NULL_TREE;

It doesn't look like the callers can cope with a null return value.
Maybe get_max_float would be a good fallback instead.

> +      }
> +
> +    case FIXED_POINT_TYPE:
> +      {
> +	/* FIXME.  What should we put into a fixed point?  */
> +	FIXED_VALUE_TYPE fixed;
> +	fixed_from_string (&fixed, "0xFFFFFFFFFFFFFFFF",
> +			   SCALAR_TYPE_MODE (type));
> +	return build_fixed (type, fixed);
> +      }
> +    case VECTOR_TYPE:
> +      {
> +	tree scalar = build_pattern_cst (TREE_TYPE (type));
> +	return build_vector_from_val (type, scalar);
> +      }
> +    case COMPLEX_TYPE:
> +      {
> +	tree element = build_pattern_cst (TREE_TYPE (type));
> +	return build_complex (type, element, element);
> +      }
> +    case RECORD_TYPE:
> +      {
> +	tree field;
> +	tree field_value;
> +	vec<constructor_elt, va_gc> *v = NULL;
> +	for (field= TYPE_FIELDS (type); field; field = DECL_CHAIN (field))
> +	  {
> +	    if (TREE_CODE (field) != FIELD_DECL)
> +	      continue;
> +	    /* if the field is a variable length array, it should be the last

s/if/If/

> +	       field of the record, and no need to initialize.  */

Why doesn't it need to be initialized though?

> +	    if (TREE_CODE (TREE_TYPE (field)) == ARRAY_TYPE
> +		&& TYPE_SIZE (TREE_TYPE (field)) == NULL_TREE
> +		&& ((TYPE_DOMAIN (TREE_TYPE (field)) != NULL_TREE
> +		    && TYPE_MAX_VALUE (TYPE_DOMAIN (TREE_TYPE (field)))
> +				       == NULL_TREE)
> +		   || TYPE_DOMAIN (TREE_TYPE (field)) == NULL_TREE))
> +	      continue;
> +	    field_value = build_pattern_cst (TREE_TYPE (field));
> +	    CONSTRUCTOR_APPEND_ELT (v, field, field_value);
> +	  }
> +	return build_constructor (type, v);
> +      }
> +    case UNION_TYPE:
> +    case QUAL_UNION_TYPE:
> +      {
> +	tree field, max_field = NULL;
> +	unsigned max_size = 0;
> +	tree field_value;
> +	vec<constructor_elt, va_gc> *v = NULL;
> +	/* find the field with the largest size.  */

s/find/Find/

> +	for (field = TYPE_FIELDS (type); field; field = DECL_CHAIN (field))
> +	  {
> +	    if (TREE_CODE (field) != FIELD_DECL)
> +	      continue;
> +	    if (tree_to_uhwi (TYPE_SIZE_UNIT (TREE_TYPE (field))) >= max_size)
> +	      {
> +		max_size = tree_to_uhwi (TYPE_SIZE_UNIT (TREE_TYPE (field)));
> +		max_field = field;
> +	      }

This isn't safe.  We shouldn't use tree_to_uhwi without checking whether
tree_fits_uhwi_p.  We then need a fallback for !tree_fits_uhwi_p.

> +	  }
> +	  field_value = build_pattern_cst (TREE_TYPE (max_field));
> +	  CONSTRUCTOR_APPEND_ELT (v, max_field, field_value);
> +	return build_constructor (type, v);
> +      }
> +    case ARRAY_TYPE:
> +      {
> +	vec<constructor_elt, va_gc> *elts = NULL;
> +	tree element = build_pattern_cst (TREE_TYPE (type));
> +	tree nelts = array_type_nelts (type);
> +	if (nelts && tree_fits_uhwi_p (nelts))
> +	  {
> +	    unsigned HOST_WIDE_INT n = tree_to_uhwi (nelts) + 1;
> +	    for (unsigned int i = 0; i < n; i++)
> +	      CONSTRUCTOR_APPEND_ELT (elts, NULL_TREE, element);
> +	    return build_constructor (type, elts);
> +	  }
> +	/* variable length array should not be here.  */

s/variable/Variable/

> +	gcc_assert (0);

Should be gcc_unreachable () instead.

> +      }
> +    default:
> +      if (!AGGREGATE_TYPE_P (type))
> +	return fold_convert (type, build_pattern_cst (unsigned_type_node));
> +      else
> +	gcc_assert (0);

Same here.

> @@ -11950,6 +12088,72 @@ lower_bound_in_type (tree outer, tree inner)
>      }
>  }
>  
> +/* Returns true when the given TYPE has padding inside it.
> +   return false otherwise.  */
> +bool
> +type_has_padding (tree type)

Would it be possible to reuse __builtin_clear_padding here?

> +{
> +  switch (TREE_CODE (type))
> +    {
> +    case RECORD_TYPE:
> +      {
> +	unsigned HOST_WIDE_INT record_size
> +	  = tree_to_uhwi (TYPE_SIZE_UNIT (type));

As above, it's not safe to just to use tree_to_uhwi here without checking.
I've skipped the rest of the function because of the builtin question above.

Thanks,
Richard