On Wed, Jul 14, 2021 at 02:09:50PM +0000, Qing Zhao wrote:
> Hi, Richard,
>
> > On Jul 14, 2021, at 2:14 AM, Richard Biener <richard.guent...@gmail.com>
> > wrote:
> >
> > On Wed, Jul 14, 2021 at 1:17 AM Qing Zhao <qing.z...@oracle.com> wrote:
> >>
> >> Hi, Kees,
> >>
> >> I took a look at the kernel testing case you attached in the previous
> >> email, and found the testing failed with the following case:
> >>
> >> #define INIT_STRUCT_static_all = { .one = arg->one, \
> >> .two = arg->two, \
> >> .three = arg->three, \
> >> .four = arg->four, \
> >> }
> >>
> >> i.e, when the structure type auto variable has been explicitly initialized
> >> in the source code. -ftrivial-auto-var-init in the 4th version
> >> does not initialize the paddings for such variables.
> >>
> >> But in the previous version of the patches ( 2 or 3),
> >> -ftrivial-auto-var-init initializes the paddings for such variables.
> >>
> >> I intended to remove this part of the code from the 4th version of the
> >> patch since the implementation for initializing such paddings is
> >> completely different from
> >> the initializing of the whole structure as a whole with memset in this
> >> version of the implementation.
> >>
> >> If we really need this functionality, I will add another separate patch
> >> for this additional functionality, but not with this patch.
> >>
> >> Richard, what’s your comment and suggestions on this?
> >
> > I think this can be addressed in the gimplifier by adjusting
> > gimplify_init_constructor to clear
> > the object before the initialization (if it's not done via aggregate
> > copying).
>
> I did this in the previous versions of the patch like the following:
>
> @@ -5001,6 +5185,17 @@ gimplify_init_constructor (tree *expr_p, gimple_seq
> *pre_p, gimple_seq *post_p,
> /* If a single access to the target must be ensured and all elements
> are zero, then it's optimal to clear whatever their number. */
> cleared = true;
> + else if (flag_trivial_auto_var_init > AUTO_INIT_UNINITIALIZED
> + && !TREE_STATIC (object)
> + && type_has_padding (type))
> + /* If the user requests to initialize automatic variables with
> + paddings inside the type, we should initialize the paddings too.
> + C guarantees that brace-init with fewer initializers than members
> + aggregate will initialize the rest of the aggregate as-if it were
> + static initialization. In turn static initialization guarantees
> + that pad is initialized to zero bits.
> + So, it's better to clear the whole record under such situation. */
> + cleared = true;
> else
> cleared = false;
>
> Then the paddings are also initialized to zeroes with this option. (Even for
> -ftrivial-auto-var-init=pattern).
Thanks! I've tested with the attached patch to v4 and it passes all my
tests again.
> Is the above change Okay? (With this change, when
> -ftrivial-auto-var-init=pattern, the paddings for the
> structure variables that have explicit initializer will be ZEROed, not 0xFE)
Padding zeroing in the face of pattern-init is correct (and matches what
Clang does).
-Kees
--
Kees Cook
commit 8c52b69540b064e930e4d9e2e3dc011ca002306d
Author: Kees Cook <keesc...@chromium.org>
AuthorDate: Wed Jul 14 11:17:27 2021 -0700
Commit: Kees Cook <keesc...@chromium.org>
CommitDate: Wed Jul 14 12:08:56 2021 -0700
Fix padding
Based on v2 and bddde2d2-8594-4bf6-8142-cd09b0ebb...@oracle.com
diff --git a/gcc/gimplify.c b/gcc/gimplify.c
index 4db53cda77f8..dd3da86d6663 100644
--- a/gcc/gimplify.c
+++ b/gcc/gimplify.c
@@ -5071,6 +5071,18 @@ gimplify_init_constructor (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
/* If a single access to the target must be ensured and all elements
are zero, then it's optimal to clear whatever their number. */
cleared = true;
+ else if (opt_for_fn (current_function_decl, flag_auto_var_init)
+ > AUTO_INIT_UNINITIALIZED
+ && !TREE_STATIC (object)
+ && type_has_padding (type))
+ /* If the user requests to initialize automatic variables with
+ paddings inside the type, we should initialize the paddings too.
+ C guarantees that brace-init with fewer initializers than members
+ aggregate will initialize the rest of the aggregate as-if it were
+ static initialization. In turn static initialization guarantees
+ that pad is initialized to zero bits.
+ So, it's better to clear the whole record under such situation. */
+ cleared = true;
else
cleared = false;
diff --git a/gcc/tree.c b/gcc/tree.c
index 1aa6e557a049..7889c10d639f 100644
--- a/gcc/tree.c
+++ b/gcc/tree.c
@@ -10818,6 +10818,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)
+{
+ switch (TREE_CODE (type))
+ {
+ case RECORD_TYPE:
+ {
+ unsigned HOST_WIDE_INT record_size
+ = tree_to_uhwi (TYPE_SIZE_UNIT (type));
+ unsigned HOST_WIDE_INT size_sofar = 0;
+
+ for (tree field= TYPE_FIELDS (type); field; field = DECL_CHAIN (field))
+ {
+ if (TREE_CODE (field) != FIELD_DECL)
+ continue;
+ unsigned HOST_WIDE_INT cur_off = int_byte_position (field);
+ if (size_sofar < cur_off)
+ return true;
+ size_sofar = cur_off
+ + tree_to_uhwi (TYPE_SIZE_UNIT (TREE_TYPE (field)));
+ }
+ if (size_sofar < record_size)
+ return true;
+ /* If any of the field has padding, return true. */
+ for (tree field= TYPE_FIELDS (type); field; field = DECL_CHAIN (field))
+ {
+ if ((TREE_CODE (field) != FIELD_DECL))
+ continue;
+ if (AGGREGATE_TYPE_P (TREE_TYPE (field))
+ && type_has_padding (TREE_TYPE (field)))
+ return true;
+ }
+ return false;
+ }
+ case UNION_TYPE:
+ case QUAL_UNION_TYPE:
+ {
+ tree max_field = NULL;
+ unsigned max_size = 0;
+ for (tree 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;
+ }
+ }
+ if (AGGREGATE_TYPE_P (TREE_TYPE (max_field)))
+ return type_has_padding (TREE_TYPE (max_field));
+ return false;
+ }
+ case ARRAY_TYPE:
+ {
+ if (AGGREGATE_TYPE_P (TREE_TYPE (type)))
+ return type_has_padding (TREE_TYPE (type));
+ return false;
+ }
+ default:
+ return false;
+ }
+}
+
/* Return nonzero if two operands that are suitable for PHI nodes are
necessarily equal. Specifically, both ARG0 and ARG1 must be either
SSA_NAME or invariant. Note that this is strictly an optimization.
diff --git a/gcc/tree.h b/gcc/tree.h
index 8bdf16d8b4ab..56a7947d935d 100644
--- a/gcc/tree.h
+++ b/gcc/tree.h
@@ -5168,6 +5168,7 @@ extern bool operation_can_overflow (enum tree_code);
extern bool operation_no_trapping_overflow (tree, enum tree_code);
extern tree upper_bound_in_type (tree, tree);
extern tree lower_bound_in_type (tree, tree);
+extern bool type_has_padding (tree);
extern int operand_equal_for_phi_arg_p (const_tree, const_tree);
extern tree create_artificial_label (location_t);
extern const char *get_name (tree);
