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.

jfb added a comment.Jan 8 2020, 2:21 PM

@vitalybuka could we move this patch forward?

Can patch description be made a bit more verbose?

However with -ftrivial-auto-var-init=pattern those undefs became 0xAA pattern and break some code.

Break how?

Does this have to be an unilateral change,
likely penalizing non--ftrivial-auto-var-init= cases,
i.e. [why] can't it be only done for when -ftrivial-auto-var-init= is enabled?

Does this have to be an unilateral change,
likely penalizing non--ftrivial-auto-var-init= cases,
i.e. [why] can't it be only done for when -ftrivial-auto-var-init= is enabled?

We left off near that conclusion (https://reviews.llvm.org/D68115#1686887); however, -ftrivial-auto-var-init= merely increases the chance that code expecting the zeroing to occur would break. A separate option to control zeroing for union padding would help in cases where the zeroing does not happen for reasons other than -ftrivial-auto-var-init.

Does this have to be an unilateral change,
likely penalizing non--ftrivial-auto-var-init= cases,
i.e. [why] can't it be only done for when -ftrivial-auto-var-init= is enabled?

We left off near that conclusion (https://reviews.llvm.org/D68115#1686887);

Would be great if @rsmith / @aaron.ballman could comment on that

however, -ftrivial-auto-var-init= merely increases the chance that code expecting the zeroing to occur would break. A separate option to control zeroing for union padding would help in cases where the zeroing does not happen for reasons other than -ftrivial-auto-var-init.

Does this have to be an unilateral change,
likely penalizing non--ftrivial-auto-var-init= cases,
i.e. [why] can't it be only done for when -ftrivial-auto-var-init= is enabled?

We left off near that conclusion (https://reviews.llvm.org/D68115#1686887);

Would be great if @rsmith / @aaron.ballman could comment on that

I don't have super strong opinions on it, but I think a separate feature for zeroing union padding is what gives users the most flexibility.

A separate option to control zeroing for union padding would help in cases where the zeroing does not happen for reasons other than -ftrivial-auto-var-init.

Agreed.

Does this have to be an unilateral change,
likely penalizing non--ftrivial-auto-var-init= cases,
i.e. [why] can't it be only done for when -ftrivial-auto-var-init= is enabled?

We left off near that conclusion (https://reviews.llvm.org/D68115#1686887);

Would be great if @rsmith / @aaron.ballman could comment on that

I don't have super strong opinions on it, but I think a separate feature for zeroing union padding is what gives users the most flexibility.

A separate option to control zeroing for union padding would help in cases where the zeroing does not happen for reasons other than -ftrivial-auto-var-init.

Agreed.

I would be happy to finish this patch if we agree on something.

So if I understand this the proposal is to have something like -fzero-union-padding which is off by default.
When it's OFF compiler will continue to do whatever it does now.
When it's ON it will set zeroes into padding with or without -ftrivial-auto-var-init.
Is this correct?

So if I understand this the proposal is to have something like -fzero-union-padding which is off by default.
When it's OFF compiler will continue to do whatever it does now.
When it's ON it will set zeroes into padding with or without -ftrivial-auto-var-init.
Is this correct?

That's my understanding and I would find such an option useful.

I would be happy to finish this patch if we agree on something.

So if I understand this the proposal is to have something like -fzero-union-padding which is off by default.
When it's OFF compiler will continue to do whatever it does now.
When it's ON it will set zeroes into padding with or without -ftrivial-auto-var-init.
Is this correct?

In general, I believe so, yes. To be clear, it only sets zeros into union padding, not *all* padding. I do not have an opinion on whether we want it to be -fzero-union-padding as opposed to -finit-union-padding that honors the pattern from -ftrivial-auto-init=pattern and defaults to zero if no pattern is specified.

vitalybuka added a comment.EditedJan 14 2020, 1:54 PM

I would be happy to finish this patch if we agree on something.

So if I understand this the proposal is to have something like -fzero-union-padding which is off by default.
When it's OFF compiler will continue to do whatever it does now.
When it's ON it will set zeroes into padding with or without -ftrivial-auto-var-init.
Is this correct?

In general, I believe so, yes. To be clear, it only sets zeros into union padding, not *all* padding. I do not have an opinion on whether we want it to be -fzero-union-padding as opposed to -finit-union-padding that honors the pattern from -ftrivial-auto-init=pattern and defaults to zero if no pattern is specified.

They whole point of the patch was to avoid breaking code by -ftrivial-auto-init=pattern with "MyUnion my_union = {}". So to fix that only -fzero-union-padding behavior helpful.
-ftrivial-auto-init=pattern as-is already inits union padding with patterns.

I would be happy to finish this patch if we agree on something.

So if I understand this the proposal is to have something like -fzero-union-padding which is off by default.
When it's OFF compiler will continue to do whatever it does now.
When it's ON it will set zeroes into padding with or without -ftrivial-auto-var-init.
Is this correct?

In general, I believe so, yes. To be clear, it only sets zeros into union padding, not *all* padding. I do not have an opinion on whether we want it to be -fzero-union-padding as opposed to -finit-union-padding that honors the pattern from -ftrivial-auto-init=pattern and defaults to zero if no pattern is specified.

They whole point of the patch was to avoid breaking code by -ftrivial-auto-init=pattern with "MyUnion my_union = {}". So to fix that only -fzero-union-padding behavior helpful.
-ftrivial-auto-init=pattern as-is already inits union padding with patterns.

Ah, okay, good to know!

jfb added a comment.Mar 27 2020, 11:40 AM

What's the verdict then?

In D68115#1946612, @jfb wrote:

What's the verdict then?

It sounds like we are looking for -fzero-union-padding. That's been where the discussion has left off twice for months.

It sounds like we are looking for -fzero-union-padding. That's been where the discussion has left off twice for months.

I believe the state of Clang prior to this patch is actually wrong. We reach this code regardless of the kind of initialization used for the union, and some forms of initialization require zeroing of padding whereas some do not. If we want to model this precisely, we'll need to track on APValue whether the padding bits of the union constant are zeroed or not. For example, given:

union U { char c; int n; };
U u = U(); // value-initialization
U v = U{}; // aggregate initialization

Clang emits { i8 0, [3 x i8] undef } for both u and v. That's correct for v, but incorrect for u: it should emit { i8 0, [3 x i8] zeroinitializer } or perhaps simply zeroinitializer, because value-initialization invokes zero-initialization, which zeroes the padding.

Fixing this properly is likely not very hard, but it would require more changes than are present in this patch. (This patch is conservatively correct, but initializes more than we need to initialize.)

We also need to track in APValue whether padding bits are zeroed in order to correctly support bit_cast from structs with padding. Per discussion in committee, the intended behavior for bit_cast is:

A bit in the value representation of the result is indeterminate if does not correspond to a bit in the value representation of from or corresponds to a bit of an object that is not within its lifetime or has an indeterminate value ([basic.indet]). For each bit in the value representation of the result that is indeterminate, the smallest object containing that bit has an indeterminate value; the behavior is undefined unless that object is of unsigned ordinary character type or std::byte type. The result does not otherwise contain any indeterminate values.

So in particular:

struct A { char c; int n; };
constexpr long n = bit_cast<long>(A()); // ok, 0
constexpr long m = bit_cast<long>(A{}); // ill-formed, indeterminate value due to uninitialized padding between c and n

So I would propose we take the following path:

  1. Extend Clang's constant evaluator and APValue to track, for a struct or union value, whether all padding bits are zeroed. (Should always be true for a value with no padding bits.)
  2. Land this patch behind a flag to zero all padding bits for unions, ideally extended to cover struct padding as well as union padding.
  3. After doing (1), extend __builtin_bit_cast support to properly handle padding bits.
  4. After doing (1) and (2), extend constant aggregate emission to always zero padding when required by the language standard. (If you want, make the flag be three-way: never zero, zero as required by language standard, always zero, maybe: -fzero-padding=never / -fzero-padding=std, -fzero-padding=always.)

Note that (1) and (2) are independent, so I don't think we need to block this patch (2) on the implementation of (1), but we should be aware that we're not done here until we do steps (3) and (4).

  1. After doing (1), extend __builtin_bit_cast support to properly handle padding bits.
  2. After doing (1) and (2), extend constant aggregate emission to always zero padding when required by the language standard. (If you want, make the flag be three-way: never zero, zero as required by language standard, always zero, maybe: -fzero-padding=never / -fzero-padding=std, -fzero-padding=always.)

Just to make sure I understand correctly. There is no proposal to consider non-standard zeroing behaviour in constant expression evaluation; right?

jfb added a comment.Mar 27 2020, 2:27 PM

It sounds like we are looking for -fzero-union-padding. That's been where the discussion has left off twice for months.

I believe the state of Clang prior to this patch is actually wrong.

That's my understanding as well. I'd like it if this patch (or a follow-up) got us back to standard behavior. In either case, I'd like the proposed behavior to bee on-by-default when doing initialization of stack variables.

rsmith added a comment.Apr 5 2020, 3:47 PM
In D68115#1946990, @jfb wrote:

It sounds like we are looking for -fzero-union-padding. That's been where the discussion has left off twice for months.

I believe the state of Clang prior to this patch is actually wrong.

That's my understanding as well. I'd like it if this patch (or a follow-up) got us back to standard behavior. In either case, I'd like the proposed behavior to bee on-by-default when doing initialization of stack variables.

Your preference is noted. However, I think the majority opinion expressed on this review at this point favors not guaranteeing zero-initialization except where required by the relevant standard. That'd also be consistent with our stance on trivial auto variable initialization in general.

I'm not yet sure about whether we want separate controls for this and for -ftrivial-auto-init, or whether from a user's perspective there's really only one question: should bits left uninitialized be undef, guaranteed zero, or guaranteed to be filled with a pattern -- independent of whether they're padding bits? (And related, do we actually want control over zeroing union padding in all cases or only for trivial automatic variables? And do we want control over zeroing or pattern-filling objects allocated with new with trivial initialization?)

jfb added a comment.Apr 5 2020, 5:08 PM
In D68115#1946990, @jfb wrote:

It sounds like we are looking for -fzero-union-padding. That's been where the discussion has left off twice for months.

I believe the state of Clang prior to this patch is actually wrong.

That's my understanding as well. I'd like it if this patch (or a follow-up) got us back to standard behavior. In either case, I'd like the proposed behavior to bee on-by-default when doing initialization of stack variables.

Your preference is noted. However, I think the majority opinion expressed on this review at this point favors not guaranteeing zero-initialization except where required by the relevant standard. That'd also be consistent with our stance on trivial auto variable initialization in general.

Sorry, I didn't express myself well: if trivial auto-var init is on, then I want initialization of union padding to also be on. I think this is exactly compatible with what you want, as is says nothing of the behavior when trivial auto-var init is not on.

I'm not yet sure about whether we want separate controls for this and for -ftrivial-auto-init, or whether from a user's perspective there's really only one question: should bits left uninitialized be undef, guaranteed zero, or guaranteed to be filled with a pattern -- independent of whether they're padding bits? (And related, do we actually want control over zeroing union padding in all cases or only for trivial automatic variables? And do we want control over zeroing or pattern-filling objects allocated with new with trivial initialization?)

Trivial auto-var init should always initialize all stack padding, and should not also initialize heap padding. There should be separate controls for heap padding, in part because this interacts with the allocator (whereas stack initialization does not).

rsmith added a comment.Apr 5 2020, 6:09 PM
In D68115#1962863, @jfb wrote:

I think the majority opinion expressed on this review at this point favors not guaranteeing zero-initialization except where required by the relevant standard. That'd also be consistent with our stance on trivial auto variable initialization in general.

Sorry, I didn't express myself well: if trivial auto-var init is on, then I want initialization of union padding to also be on. I think this is exactly compatible with what you want, as is says nothing of the behavior when trivial auto-var init is not on.

I'm not yet sure about whether we want separate controls for this and for -ftrivial-auto-init, or whether from a user's perspective there's really only one question: should bits left uninitialized be undef, guaranteed zero, or guaranteed to be filled with a pattern -- independent of whether they're padding bits? (And related, do we actually want control over zeroing union padding in all cases or only for trivial automatic variables? And do we want control over zeroing or pattern-filling objects allocated with new with trivial initialization?)

Trivial auto-var init should always initialize all stack padding, and should not also initialize heap padding. There should be separate controls for heap padding, in part because this interacts with the allocator (whereas stack initialization does not).

That sounds reasonable to me. So the behavior we're looking for is:

  • If -ftrivial-auto-init is off, then we guarantee to zero padding when the language spec requires it, and otherwise provide no such guarantee.
  • If -ftrivial-auto-init=zeroes then we guarantee to zero padding within structs and unions with automatic storage duration, when that padding would otherwise be left uninitialized.
  • If -ftrivial-auto-init=pattern then we guarantee to pattern-fill padding within structs and unions with automatic storage duration, when that padding would otherwise be left uninitialized (and will provide the zeroes required by the language rule when that is the required behavior).

[One possible tweak: for the pattern case, should we guarantee that the uninitialized padding will be pattern-filled? It would be simpler if we guaranteed it to be *either* zero- or pattern-filled; that way we can provide a conservatively-correct approximation by zero-filling whenever we're unsure.]

And we do not initially provide any guarantees as to what happens to padding within objects of other storage durations beyond what the language spec requires. (We might at some point in the future, but that would be behind a separate flag from -ftrivial-auto-init.) I'd be happy with that approach. Does that address everyone's concerns?

If so, how do we make progress on this? @vitalybuka, are you interested in doing the work to track whether padding should be zeroed in APValue?

jfb added a comment.Apr 5 2020, 8:48 PM

That sounds reasonable to me. So the behavior we're looking for is:

  • If -ftrivial-auto-init is off, then we guarantee to zero padding when the language spec requires it, and otherwise provide no such guarantee.
  • If -ftrivial-auto-init=zeroes then we guarantee to zero padding within structs and unions with automatic storage duration, when that padding would otherwise be left uninitialized.
  • If -ftrivial-auto-init=pattern then we guarantee to pattern-fill padding within structs and unions with automatic storage duration, when that padding would otherwise be left uninitialized (and will provide the zeroes required by the language rule when that is the required behavior).

That's exactly what I'd like, yes!

[One possible tweak: for the pattern case, should we guarantee that the uninitialized padding will be pattern-filled? It would be simpler if we guaranteed it to be *either* zero- or pattern-filled; that way we can provide a conservatively-correct approximation by zero-filling whenever we're unsure.]

Not guaranteeing a specific value for "pattern" remains my preferred choice. Where feasible, I'd rather we generate the most-repeated pattern so it's cheaper to synthesize.

And we do not initially provide any guarantees as to what happens to padding within objects of other storage durations beyond what the language spec requires. (We might at some point in the future, but that would be behind a separate flag from -ftrivial-auto-init.) I'd be happy with that approach. Does that address everyone's concerns?

Yup!

In D68115#1962863, @jfb wrote:

I think the majority opinion expressed on this review at this point favors not guaranteeing zero-initialization except where required by the relevant standard. That'd also be consistent with our stance on trivial auto variable initialization in general.

That's my view regarding the default behaviour with or without -ftrivial-auto-var-init=pattern, but I believe that there are cases where -ftrivial-auto-var-init=pattern is known to cause trouble for user code (due to the code having what is strictly uninitialized union padding).

[ ... ]

I'm not yet sure about whether we want separate controls for this and for -ftrivial-auto-init, or whether from a user's perspective there's really only one question: should bits left uninitialized be undef, guaranteed zero, or guaranteed to be filled with a pattern -- independent of whether they're padding bits? (And related, do we actually want control over zeroing union padding in all cases or only for trivial automatic variables? And do we want control over zeroing or pattern-filling objects allocated with new with trivial initialization?)

I have found that users are less convinced that uninitialized union padding is something to fix in user code than for cases where a variable is missing an initializer and nonetheless accessed without a prior assignment.

And we do not initially provide any guarantees as to what happens to padding within objects of other storage durations beyond what the language spec requires. (We might at some point in the future, but that would be behind a separate flag from -ftrivial-auto-init.) I'd be happy with that approach. Does that address everyone's concerns?

There's a chance that "point in the future" is now (and a separate flag was proposed).

jfb added a comment.May 15 2020, 8:20 PM

To get this unblocked a bit, I implemented a diagnostic: https://reviews.llvm.org/D80055

As an outsider: In Swift, reading an uninitialized variable is a compile-time error. Clang is not powerful enough to do this analysis. Aren't you really looking for the Clang Intermediate Language (CIL) ?

jfb added a comment.May 17 2020, 11:34 AM

As an outsider: In Swift, reading an uninitialized variable is a compile-time error. Clang is not powerful enough to do this analysis. Aren't you really looking for the Clang Intermediate Language (CIL) ?

I have an entire talk about this: https://www.youtube.com/watch?v=I-XUHPimq3o

I watched the talk, but I still prefer compile-time errors over runtime crashes.