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

[Richard Biener <rguenther@suse.de>]

On Tue, 10 Aug 2021, Qing Zhao wrote:

> 
> 
> > On Aug 10, 2021, at 2:36 AM, Richard Biener <rguenther@suse.de> wrote:
> > 
> > On Mon, 9 Aug 2021, Qing Zhao wrote:
> > 
> >> Hi, Richard,
> >> 
> >> Thanks a lot for you review.
> >> 
> >> Although these comments are not made on the latest patch (7th version) :-), all the comments are valid since the parts you commented
> >> are not changed in the 7th version.
> >> 
> >> 
> >>> On Aug 9, 2021, at 9:09 AM, Richard Biener <rguenther@suse.de> wrote:
> >>> 
> >>> 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).
> >> 
> >> Right, I should warn when the attribute is placed for globals or static variables. 
> >> I will try TREE_STATIC/DECL_EXTERNAL to see whether it’s work or not.
> >> 
> >>> 
> >>> +  /* 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
> >> 
> >> Sure, will move the creation of  function node of BUILT_IN_CLEAR_PADDING for Fortran etc. to 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?).
> >> 
> >> Yes, I added an extra argument “for_auto_init” for “BUILT_IN_CLEAR_PADDING”, this argument is added to suppress errors emitted during folding
> >> BUILT_IN_CLEAR_PADDING for flexible array member . Such errors should Not be emitted when “BUILT_IN_CLEAR_PADDING” is called with compiler automatic initialization.
> >> Please see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101586, comment #6 from Jakub Jelinek.
> >> 
> >>> I suggest
> >>> to re-name is_vla/for_auto_init to something more descriptive.
> >> 
> >> Okay, I will. 
> >>> 
> >>> +   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.
> >> 
> >> Okay, will make this more clear in the comments.
> >> 
> >>> 
> >>> +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.
> >> 
> >> The routine “build_deferred_init” is ONLY called during gimplification phase by the routine “gimple_add_init_for_auto_var", at this place,
> >> Is_vla should be determined by the caller to check the size of the DECL. If it’s a vla, the “maybe_with_size_expr” will be applied for
> >> DECL to make it to a WITH_SIZE_EXPR.  So, the assertion is purely to make sure this at gimplification phase.
> >> 
> >> Yes, the size of the VLA decl might become a constant later due to constant propagation, etc.  but during the gimplification phase, the assertion should be true.
> >>> 
> >>> +         /* 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.
> >> 
> >> Thanks for the suggestion, I will do this.
> >> 
> >> 
> >>> 
> >>> 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.
> >> 
> >> Yes, you are right, this case was not covered right now, and this should be covered.
> >> 
> >> Looks like that I need to move the “gimple_add_padding_init_for_auto_var” inside the routine “gimplify_init_constructor” to
> >> Cover all the cases. 
> >> 
> >>> 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?
> >> 
> >> You are right, I might need to restrict it Only to INIT_EXPR. 
> >> Will update.
> >> 
> >>> 
> >>> +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?
> >> 
> >> The LHS of the call to .DEFERRED_INIT is the DECL itself whatever it’s a VLA or not. 
> >> 
> >> In order to create a memset call, we need the Address of this DECL as the first argument. 
> >> If the DECL is not a VLA, we just simply apply “build_fold_addr_expr” on this DECL to get its address,
> >> However, for VLA, during gimplification phase “gimplify_vla_decl”, we have already created a temporary
> >> address variable for this DECL, and recorded this address variable with “DECL_VALUE_EXPR(DECL), 
> >> We should use this already created address variable  for VLAs. 
> > 
> > So the issue is that the LHS of the .DEFERRED_INIT call is not properly
> > gimplified.  We should not have such decl there but I see we do not
> > have IL verification that covers this.
> 
> Don’t quite understand here:  do you mean all the LHS of .DEFERRED_INIT call are not properly gimplified, or
> Only the LHS of .DEFERRED_INIT call for VLA are not properly gimplified?

Especially in the VLA case but likely also in general (though unlikely
since usually the receiver of initializations are simple enough).  I'd
expect the VLA case end up as

 *ptr_to_decl = .DEFERRED_INIT (...);

where *ptr_to_decl is the DECL_VALUE_EXPR of the decl.

> What do you mean by “such” decl? A decl whole “DECL_VALUE_EXPR(DECL)” is valid?

A 'decl' that has a DECL_VALUE_EXPR should not appear in the IL, it should
always be refered to as its DECL_VALUE_EXPR.

Richard.

> Qing
> > 
> > The gimplifier usually does this in gimplify_var_or_parm_decl,
> > but you can of course substitute DECL_VALUE_EXPR yourself if the
> > decl was already gimplified (was it?)
> > 
> >> 
> >>> 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.
> >> 
> >> Don’t quite understand here. you mean that all the “DECL_VALUE_EXPRs” have been resolved at the phase RTL expansion,
> >> So I cannot use this to get the address variable of the VLA?
> >> 
> >> (However, my unit testing cases for VLAs are all looks fine).
> >> 
> >>> 
> >>> +  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.
> >> 
> >> Okay, I will restrict the pattern initializer computation when really needed. 
> >> 
> >>> 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.
> >> 
> >> Thanks for the suggestion. 
> >> 
> >> Will try this. 
> >> 
> >>> 
> >>> +  /* 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?
> >> 
> >> You mean this issue is only there when the decl is the first argument (the old design for .DEFERRED_INIT).
> >> With the new design, this issue is not there anymore?
> > 
> > I think so, yes - the change should no longer be needed.
> > 
> > Ricahrd.
> > 
> >>> 
> >>> +  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.
> >> 
> >> Okay, will update this.
> >> 
> >>> 
> >>> 
> >>> Thanks and sorry for the delay in reviewing this (again).
> >> 
> >> Thanks again for your detailed review and suggestions.
> >> 
> >> I will update the patch accordingly and send the updated patch soon.
> >> 
> >> Qing
> >>> 
> >>> Richard.
> >>> 
> >>> 
> >>>> Thanks
> >>>> 
> >> 
> >> 
> > 
> > -- 
> > Richard Biener <rguenther@suse.de>
> > SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
> > Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)