Page MenuHomePhabricator

Zero initialize padding in unions
Needs ReviewPublic

Authored by vitalybuka on Sep 26 2019, 6:36 PM.

Details

Summary

Existing implementation puts undef into padding bits which almost always
compiled into zeroes. However with -ftrivial-auto-var-init=pattern those undefs
became 0xAA pattern and break some code. We need to zero initialized them.

C++

11.6 Initializers
6 To zero-initialize an object or reference of type T means:
(6.3) — if T is a (possibly cv-qualified) union type, its padding bits (6.7) are initialized to zero bits

8 To value-initialize an object of type T means:
(8.2) — if T is a (possibly cv-qualified) class type without a user-provided or deleted default constructor, then
the object is zero-initialized and the semantic constraints for default-initialization are checked, and if T
has a non-trivial default constructor, the object is default-initialized;

11.6.1 Aggregates

3 When an aggregate is initialized by an initializer list as specified in 11.6.4, the elements of the initializer list
are taken as initializers for the elements of the aggregate, in order. Each element is *copy-initialized* from the
corresponding initializer-clause. If the initializer-clause is an expression and a narrowing conversion (11.6.4)

8 If there are fewer initializer-clauses in the list than there are elements in a non-union aggregate, then each
element not explicitly initialized is initialized as follows:
If the aggregate is a union and the initializer list is empty, then
(8.4) — if any variant member has a default member initializer, that member is initialized from its default
member initializer;
(8.5) — otherwise, the first member of the union (if any) is copy-initialized from an empty initializer list.

11.6.4 List-initialization
(3.3) — Otherwise, if T is an aggregate, aggregate initialization is performed
(3.4) — Otherwise, if the initializer list has no elements and T is a class type with a default constructor, the
object is value-initialized.
...
(3.10) — Otherwise, if the initializer list has no elements, the object is value-initialized

Looks like C does not require, as union is not aggregate, but a lot of code already relies on this behavior.

6.7.9 Initialization
10. If an object that has automatic storage duration is not initialized explicitly, its value is indeterminate.
If an object that has static or thread storage duration is not initialized explicitly, then:
— if it is a union, the first named member is initialized (recursively) according to these rules, and
any padding is initialized to zero bits;
21. If there are fewer initializers in a brace-enclosed list than there are elements or members of an
aggregate, or fewer characters in a string literal used to initialize an array of known size than there
are elements in the array, the remainder of the aggregate shall be initialized implicitly the same as
objects that have static storage duration.

Event Timeline

vitalybuka created this revision.Sep 26 2019, 6:36 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 26 2019, 6:36 PM
vitalybuka edited the summary of this revision. (Show Details)Sep 26 2019, 6:37 PM
vitalybuka edited the summary of this revision. (Show Details)
vitalybuka marked an inline comment as done.Sep 26 2019, 7:04 PM
vitalybuka added inline comments.
clang/test/CodeGenCXX/designated-init.cpp
68–69

"1->0" here is suspicions

vitalybuka marked an inline comment as done.Sep 26 2019, 7:27 PM
vitalybuka added inline comments.
clang/test/CodeGenCXX/designated-init.cpp
68–69

interesting that I can't compile it with GCC as C++

I can compile the following as C with GCC and C/C++ with Clang
struct WithOverwritePaddingWithBitfield overwrite_padding = {{1}, .a.bitfield = 3};
but even without the patch it was 0 in the last byte

remove unused var

hubert.reinterpretcast added inline comments.
clang/test/CodeGen/init.c
197

This is C++ aggregate initialization and not value-initialization. The wording you quoted from the C++ standard is for zero-initialization, which might be part of value initialization, but you have not shown that aggregate initialization of a union involves zero-initialization of that union.

vitalybuka edited the summary of this revision. (Show Details)Sep 27 2019, 3:18 PM
vitalybuka marked an inline comment as done.Sep 27 2019, 3:34 PM
vitalybuka added inline comments.
clang/test/CodeGen/init.c
197

reading this more I don't see any evidence that either C++ or C requires padding initialization.
Reading this I expect that all function here should be equivalent https://godbolt.org/z/1O_9-e
But they are not. Clang and GCC initialized padding after the first member.

vitalybuka edited the summary of this revision. (Show Details)Sep 27 2019, 4:03 PM
vitalybuka marked an inline comment as done.Sep 27 2019, 4:06 PM
vitalybuka added inline comments.
clang/test/CodeGen/init.c
197

So if go trough "aggregates" then nothing is said about padding in union.
If we go trough "list-initialization" then value initialization should be applied, part of which is zero initialization.
If so union padding should be initialized.

clang/test/CodeGen/init.c
197

In C++14, the list in [dcl.init.list] starts with:
If T is an aggregate, aggregate initialization is performed.

The bullet in C++17 follows only a case for copying and then a case for string literals.

The bullet in the C++2a CD likewise, with an additional earlier bullet that also becomes aggregate initialization.

It is in C++11 where an empty list gets value-initialization treatment, and the next bullet goes to aggregate initialization.

The inline comment was not added to an empty list case.

vitalybuka edited the summary of this revision. (Show Details)Sep 27 2019, 4:42 PM
vitalybuka marked 2 inline comments as done.Sep 27 2019, 5:07 PM
vitalybuka added inline comments.
clang/test/CodeGen/init.c
197

So if I understand you (and what I see in C++17 and C++11) then only C++11 will essentially requires zeros via value-initialization.
And the rest (including C), requires only the first field to be initialized and the tail is undefined.

clang/test/CodeGen/init.c
197

That is my understanding. C++11 will require zeroes for U u = { }; and U u{}. The tail is undefined in other cases with automatic storage duration.

jfb added a comment.Sep 27 2019, 7:25 PM

The entire point of this feature is to add guardrails to the language. What do people expect in the real world? Is there a cost to meeting these expectations? If we put the pattern (0x00 or 0xaa) in the technically undef space, what comes out?

In D68115#1686837, @jfb wrote:

The entire point of this feature is to add guardrails to the language.

I agree, and guardrails have a tendency to scratch paint if one drives against them.

What do people expect in the real world? Is there a cost to meeting these expectations?

The patch as-is moves past the scope of the -ftrivial-auto-var-init feature. The specific case I wrote the inline comment on is an instance where the initialization strategy appears deliberate and costs less space in the compiled binary than the case where the initialization strategy is hampered by trying to initialize bytes that are defined as holding indeterminate values. Paying for this extra space should require opting into (such as by using -ftrivial-auto-var-init).

If we put the pattern (0x00 or 0xaa) in the technically undef space, what comes out?

To extend the analogy, 0x00 seems to be the bumper car version in the context of the current discussion. Applications that have issues around uninitialized bytes in unions might be workable when using 0x00 as the pattern. With a non-bumper car pattern, it would become more clear to users when they are driving against the guardrails, so they aren't instead surprised when they fall off a cliff.

In D68115#1686837, @jfb wrote:

The entire point of this feature is to add guardrails to the language. What do people expect in the real world? Is there a cost to meeting these expectations? If we put the pattern (0x00 or 0xaa) in the technically undef space, what comes out?

As a user I would prefer zeros there :)

The patch as-is moves past the scope of the -ftrivial-auto-var-init feature.

That's because I had misunderstanding of the standard. I would be happy to update the patch to enable it only for -ftrivial-auto-var-init=pattern, if we want "bumper" version. For "non-bumper" we don't need a patch at all.
I don't have strong opinion which way to go.

I would be happy to update the patch to enable it only for -ftrivial-auto-var-init=pattern, if we want "bumper" version.

It seems to be a separable feature (although it does interact with -ftrivial-auto-init=pattern). That option also provides guardrails for non-unions, and "bumper guardrails" for unions can be a useful feature without the non-union guardrails.