[PATCH] D63753: [Sema] Instead of rejecting C unions with non-trivial fields, detect attempts to destruct/initialize/copy them.

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: lib/Sema/SemaDecl.cpp:11085
+    if (isa<ImplicitValueInitExpr>(I))
+      continue;
+    if (auto E = dyn_cast<InitListExpr>(I))
----------------
ahatanak wrote:
> rjmccall wrote:
> > Why is this okay?  Don't we need to check default-initialization for these?
> I didn't consider an `ImplicitValueInitExpr` to be a default-initialization since IRGen currently emits a memcpy to initialize the field instead of calling a synthesized default-initialization function as it does when a local variable that's a non-trivial C struct is declared without an initializer.
> 
> ```
> typedef struct {
>   id f0, f1;
> } S0 ;
> 
> typedef struct {
>   S0 s0;
>   int f2;
> } S;
> 
> void test(void) {
>   // emits a memcpy of a global constant.
>   S s = { .f2 = 1 };
> }
> ```
> 
> It's not clear to me whether this is a default-initialization, but if it is, we should check default-initialization here and IRGen should be fixed to emit a call to the default-initialization function.
C does not use the terms "default-initialization" and "value-initialization", but the the initialization performed for static/thread storage duration objects generally matches what C++ would call value-initialization.  These rules are laid out in section 6.7.9 of the standard, which also includes the following in its description of the rules for list-initializations of aggregates (regardless of their storage duration):

> p19: ...all subobjects that are not initialized explicitly shall be initialized implicitly the same as objects that have static storage duration.

So `s.s0` must be initialized as if a static object (in C++ terms, value-initialized), not left with an indeterminate value like an object of automatic storage duration without an initializer would be (in C++ terms, default-initialized).

I assume that the default-initialization function for a non-trivial C struct only initializes the fields that have non-trivial default-initialization.  That's not good enough for value-initialization, which is required to recursively value-initialize *all* of the fields (and even zero-initialize any padding).  Note that that's the *only* difference between these two kinds of initialization.  Furthermore, if you think about what default-initialization actually means for all our leaf non-trivial C types, it's just assigning a predictable bit-pattern (e.g. a null pointer for ARC's `__strong` and `__weak` references), not doing anything that requires executing tailored code at runtime.  So here's what that tells us:

- We can't call the default-initialization function because it might leave trivial fields uninitialized (in general, although `struct S0` in your example doesn't have any, so effectively it does a full value-initialization).

- `memcpy` from a global constant is always a valid implementation of value-initialization for non-trivial C types.  It's also a valid implementation for default-implementation; it just might do more than it really needs to.  (In the case of `struct S0`, it would be *more efficient* to use `memset`, but `memcpy` is still *valid*.)

  Now, maybe someday we'll add a non-trivial C type that really requires non-trivial code to initialize, but I doubt we'd want to, because we'd have to forbid e.g. declaring global variables of that type, and we wouldn't have a simple answer for telling people how to properly initialize a dynamically-allocated object.  Even if we do, we can wait until then to generalize.

- Regardless, yes, this is a value-initialization and so it should check whether we can value-initialize the type.  Since value-initialization just means default-initialization plus a bunch of zero-initialization (which we can assume we can always do), that just means checking whether we can default-initialize the type.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63753/new/

https://reviews.llvm.org/D63753