Hi,
On Sun, Jul 25 2021, Qing Zhao via Gcc-patches wrote:
> Hi,
>
> This is the 5th 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 and run CPU2017, without any issue.
>
> Please take a look and let me know your comments and suggestions.
>
> thanks.
>
> Qing
>
> ******Compare 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.
sorry if I did not make myself clear in my last email, but the deferred
init calls should not result into setting any bits in
cannot_scalarize_away_bitmap in the SRA pass, otherwise you'll get
different optimization with and without -ftrivial-auto-var-init.
So you either need to change build_access_from_expr like I described in
my email or add the following to your patch, which is probably slightly
mor efficient (but it has been only very mildly tested).
diff --git a/gcc/tree-sra.c b/gcc/tree-sra.c
index d1280e5f884..602b0fb3a2d 100644
--- a/gcc/tree-sra.c
+++ b/gcc/tree-sra.c
@@ -1395,7 +1395,12 @@ scan_function (void)
t = gimple_call_lhs (stmt);
if (t && !disqualify_if_bad_bb_terminating_stmt (stmt, t, NULL))
- ret |= build_access_from_expr (t, stmt, true);
+ {
+ if (gimple_call_internal_p (stmt, IFN_DEFERRED_INIT))
+ ret |= !!build_access_from_expr_1 (t, stmt, true);
+ else
+ ret |= build_access_from_expr (t, stmt, true);
+ }
break;
case GIMPLE_ASM:
And unfortunately I just spotted another potential problem in:
> +static enum assignment_mod_result
> +sra_modify_deferred_init (gimple *stmt, gimple_stmt_iterator *gsi)
> +{
> + tree lhs = gimple_call_lhs (stmt);
> + tree init_type = gimple_call_arg (stmt, 1);
> + tree is_vla = gimple_call_arg (stmt, 2);
> +
> + struct access *lhs_access = get_access_for_expr (lhs);
> + if (!lhs_access)
> + return SRA_AM_NONE;
> +
> + location_t loc = gimple_location (stmt);
> +
> + if (lhs_access->grp_to_be_replaced)
> + {
> + tree lhs_repl = get_access_replacement (lhs_access);
> + gimple_call_set_lhs (stmt, lhs_repl);
> + tree arg0_repl = TYPE_SIZE_UNIT (TREE_TYPE (lhs_repl));
> + gimple_call_set_arg (stmt, 0, arg0_repl);
> + sra_stats.deferred_init++;
I think you want to add:
gcc_assert (!lhs_access->first_child);
return SRA_AM_MODIFIED;
here, otherwise you risk that (in a case of a single-field structure)
the call you have just modified here...
> + }
> +
> + if (lhs_access->first_child)
> + generate_subtree_deferred_init (lhs_access->first_child,
> + init_type, is_vla, gsi, loc);
> + if (lhs_access->grp_covered)
> + {
> + unlink_stmt_vdef (stmt);
> + gsi_remove (gsi, true);
...will be deleted here.
> + release_defs (stmt);
> + return SRA_AM_REMOVED;
> + }
> +
> + return SRA_AM_MODIFIED;
> +}
Sorry again for spotting this late and perhaps mis-communicating about
the cannot_scalarize_away_bitmap issue earlier. Apart from these two
things, I believe the tree-sra.c bits are fin.
Martin
> 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 4 at:
> https://gcc.gnu.org/pipermail/gcc-patches/2021-July/574642.html
>
> ******ChangeLog is:
> gcc/ChangeLog:
>
> 2021-07-23 qing zhao <qing.z...@oracle.com>
>
> * builtins.c (expand_builtin_memset): Make external visible.
> * builtins.h (expand_builtin_memset): Declare extern.
> * common.opt (ftrivial-auto-var-init=): New option.
> * doc/extend.texi: Document the uninitialized attribute.
> * doc/invoke.texi: Document -ftrivial-auto-var-init.
> * flag-types.h (enum auto_init_type): New enumerated type
> auto_init_type.
> * gimple-fold.c (clear_padding_type): Add one new parameter.
> (clear_padding_union): Likewise.
> (clear_padding_emit_loop): Likewise.
> (clear_type_padding_in_mask): Likewise.
> (gimple_fold_builtin_clear_padding): Handle this new parameter.
> * gimplify.c (gimple_add_init_for_auto_var): New function.
> (maybe_with_size_expr): Forword declaration.
> (build_deferred_init): New function.
> (gimple_add_padding_init_for_auto_var): New function.
> (gimplify_decl_expr): Add initialization to automatic variables per
> users' requests.
> (gimplify_call_expr): Add one new parameter for call to
> __builtin_clear_padding.
> (gimplify_modify_expr_rhs): Add padding initialization before
> gimplify_init_constructor.
> * internal-fn.c (INIT_PATTERN_VALUE): New macro.
> (expand_DEFERRED_INIT): New function.
> * internal-fn.def (DEFERRED_INIT): New internal function.
> * tree-cfg.c (verify_gimple_call): Verify calls to .DEFERRED_INIT.
> * tree-sra.c (generate_subtree_deferred_init): New function.
> (sra_modify_deferred_init): Likewise.
> (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-23 qing zhao <qing.z...@oracle.com>
>
> * c-attribs.c (handle_uninitialized_attribute): New function.
> (c_common_attribute_table): Add "uninitialized" attribute.
>
