From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out2.suse.de (smtp-out2.suse.de [195.135.220.29]) by sourceware.org (Postfix) with ESMTPS id 80BCB385740E for ; Mon, 9 Aug 2021 14:09:39 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 80BCB385740E Received: from relay2.suse.de (relay2.suse.de [149.44.160.134]) by smtp-out2.suse.de (Postfix) with ESMTP id 3268F1FDE7; Mon, 9 Aug 2021 14:09:38 +0000 (UTC) Received: from murzim.suse.de (murzim.suse.de [10.160.4.192]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by relay2.suse.de (Postfix) with ESMTPS id 1D081A3B8A; Mon, 9 Aug 2021 14:09:38 +0000 (UTC) Date: Mon, 9 Aug 2021 16:09:38 +0200 (CEST) From: Richard Biener To: Qing Zhao cc: Martin Jambor , Jakub Jelinek , Kees Cook , Nick Alcock via Gcc-patches , Richard Biener Subject: Re: [patch][version 6] add -ftrivial-auto-var-init and variable attribute "uninitialized" to gcc In-Reply-To: <52E29277-1403-4755-901A-528116C43FB8@oracle.com> Message-ID: References: <52E29277-1403-4755-901A-528116C43FB8@oracle.com> User-Agent: Alpine 2.21 (LSU 202 2017-01-01) MIME-Version: 1.0 X-Spam-Status: No, score=-4.8 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, KAM_SHORT, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 8BIT X-Content-Filtered-By: Mailman/MimeDel 2.1.29 X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 09 Aug 2021 14:09:42 -0000 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 > > * 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 > > * c-attribs.c (handle_uninitialized_attribute): New function. > (c_common_attribute_table): Add "uninitialized" attribute. > > gcc/testsuite/ChangeLog: > > > 2021-07-26 qing zhao > > * 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 SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)