Hi, Kees,
> On Jul 14, 2021, at 2:11 PM, Kees Cook <keesc...@chromium.org> wrote:
>
> 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).
During the discussion before the 4th version of the patch, we have agreed that
pattern-init will use 0xFE byte-repeatable patterns
to initialize all the types (this includes the paddings when the structure type
variables are not explicitly initialized). And will not match
Clang’s current behavior.
If we initialize the paddings when the structure type variables are explicitly
initialized to Zeroes, then there will be inconsistency
between values that are used to initialize structure paddings under different
situations, This looks not good to me.
If we have agreed on using 0xFE byte-repeatable patterns for pattern-init, then
all the paddings should be initialized with the same
pattern.
This is the major reason I deleted the change in “gimplify_init_constructor” in
the 4th version. And considered a different implementation
for padding initializations with explicitly initialized structure variables.
Qing
>
> -Kees
>
> --
> Kees Cook
> <padding.patch>
