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

[Qing Zhao <qing.zhao@oracle.com>]

Hi, Richard,

Thanks a lot for your review.

> On Apr 23, 2021, at 2:05 PM, Richard Sandiford <richard.sandiford@arm.com> wrote:
> 
> 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.

For this question, I think Kees had provided the background information on it.
Yes, this is basically following Clang’s current implementation in order to match C spec.

> 
>> […]
>> @@ -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/.
Okay, will fix it.
> 
>> +   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. :-))

Agreed.

Will update the documentation per your suggestions.

> 
>> @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.
I think that should be fine, can save a little time. 
> 
> What about nested VLAs, like:
> 
>  void g(void *);
>  void f(int a)
>  {
>    int x[a][a];
>    g(x);
>  }
> 
Will check on this. 
> ?
> 
>> +
>> +	  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.

Okay, will delete it.

> 
>> +
>> +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.
Okay.

> 
>> +}
>> +
>> […]
>> @@ -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.

VLA is handled in another place already, it should be initialized with calls to memset/memcpy.

> 
>> +      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.
Will add the comments.
> 
>>   /* ???  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.

Okay. 

> 
>>  /* 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”
Okay.

> 
>> +   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?

For call to DEFFERED_INIT, yes, this is guaranteed.

>  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.
I can definitely add an assertion to make sure this.

> 
> 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.
Will double check on 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 :-)

This is the part I am not very sure, so I added the FIXME in order to get more review and suggestion
 to make sure it. -:)
> 
> I guess the thing we need to decide here is whether -ftrivial-auto-var-init
> should affect debug-only constructs too.

Where can I get more details on Debug-only constructs ?

>  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.

What’s DRHS? Where can I get more info on it?

> 
>> +	}
>> +      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
Okay.
> 
>> +
>> +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.
OKay, will try to understand Debug-only construct details. 
> 
>> +
>> +  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.

So, you mean the above change will not have any impact at all?

> 
>> /* 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?

I should add comments when I added this part of the code.
I remembered that this was to fix a missed warning for complex expression in the regression test.
But I forgot the details, I will check on this, and add more detailed comments in the code.

> 
>>   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.
Okay.
> 
>>   /* 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.

Yes.
> 
> I think that deserves a comment though.

Will add comments here.
> 
>> 	  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.

Okay, will check on the details and add more comments.
> 
> Nit: too many blank links at the end.

Will fix this.

> 
>>   /* 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.

Agreed, will change the name more specifically. 
> 
>> +{
>> +  /* 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

Okay.
> 
>> +	 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.

Good point, yes, it might be better to handle boolean separately.

> 
> 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.

Okay.

> 
> 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 :-)

The pattern initialization part is much more tricky, I am hoping to get more help and
Discussion here to make sure each case.


> 
>> +    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.

Okay.

> 
> 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...;

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

Is there a good testing case in gcc test suite about NULLPTR_TYPE that I can take a look?

> 
>> +	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.

Okay.
> 
>> +      }
>> +
>> +    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/

Okay.
> 
>> +	       field of the record, and no need to initialize.  */
> 
> Why doesn't it need to be initialized though?

My understanding is, the compiler will not allocate memory for the latest field of the record if its a VLA, it’s the user’s responsibility to allocate 
Memory for it.  Therefore, compiler doesn’t  need to initialize it.
> 
>> +	    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/

Okay.
> 
>> +	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.

Okay, will make sure this.

> 
>> +	  }
>> +	  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/

Okay.
> 
>> +	gcc_assert (0);
> 
> Should be gcc_unreachable () instead.
Okay.

> 
>> +      }
>> +    default:
>> +      if (!AGGREGATE_TYPE_P (type))
>> +	return fold_convert (type, build_pattern_cst (unsigned_type_node));
>> +      else
>> +	gcc_assert (0);
> 
> Same here.
Okay.
> 
>> @@ -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?

Not sure, where can I get more details on __builtin_clear_padding? I can study a little bit more on this to make sure this.
> 
>> +{
>> +  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.

Okay.

Thanks a lot again for your review.

Qing
> 
> Thanks,
> Richard