[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 a comment.

Thanks, just a few minor comment requests now.



================
Comment at: include/clang/AST/DeclBase.h:1453
+    /// copy.
+    uint64_t HasNonTrivialToPrimitiveCopyCUnion : 1;
+
----------------
Please include in these comments that these imply the associated basic non-triviality predicates.


================
Comment at: include/clang/AST/Type.h:1133
+  /// Check if this is or contains a non-trivial C struct/union type.
+  bool hasNonTrivialPrimitiveCStruct() const;
 
----------------
rjmccall wrote:
> rjmccall wrote:
> > ahatanak wrote:
> > > rjmccall wrote:
> > > > You only want these checks to trigger on unions with non-trivial members (or structs containing them), right?  How about something like `hasNonTrivialPrimitiveCUnionMember()`?  Or maybe make it more descriptive for the use sites, like `isPrimitiveCRestrictedType()`?
> > > > 
> > > > Also, it would be nice if the fast path of this could be inlined so that clients usually didn't had to make a call at all.  You can write the `getBaseElementTypeUnsafe()->getAs<RecordType>()` part in an `inline` implementation at the bottom this file.
> > > Since we don't keep track of whether a struct or union is or contains unions with non-trivial members, we'll have to use the visitors to detect such structs or unions or, to do it faster, add a bit to `RecordDeclBits` that indicates the presence of non-trivial unions. I guess it's okay to add another bit to `RecordDeclBits`?
> > It looks like there's plenty of space in `RecordDeclBits`, yeah.
> This comment seems like the right place to explain what makes a union non-trivial in C (that it contains a member which is non-trivial for *any* of the reasons that a type might be non-trivial).
Okay, if we're tracking these separately, please put separate comments on each.  Also, please mention in each comment that this implies the associated basic non-triviality predicate.


================
Comment at: lib/Sema/SemaDecl.cpp:12053
+                            NTCUC_UninitAutoVar);
     }
+
----------------
ahatanak wrote:
> rjmccall wrote:
> > ahatanak wrote:
> > > rjmccall wrote:
> > > > Please add a comment explaining why this is specific to local variables.
> > > I was trying to explain why this should be specific to local variables and realized that it's not clear to me whether it should be.
> > > 
> > > Suppose there is a union with two fields that are both non-trivial:
> > > 
> > > ```
> > > union U {
> > >   Type A a;
> > >   Type B a;
> > > };
> > > 
> > > U global;
> > > ```
> > > 
> > > In this case, is value-initialization (which is essentially default-initialization plus a bunch of zero-initialization as per our previous discussion) used to initialize `global`? If so, should we reject the code since it requires default-initialization? It should be fine if we can assume default-initialization means zero-initialization for non-trivial types in C, but what if `TypeA` or `TypeB` requires initializing to a non-zero value?
> > Yeah, the default-initialization dimension of this problem is interesting.  The C++ rule makes sense for C++ because default initialization of a C++ class requires an actual, arbitrary-side-effects constructor call, which of course you can't reasonably do implicitly for a union member.  As discussed previously, non-trivial C types can presumably always be default-initialized with a constant bit pattern.  That means that, as long as we can do any initialization work at all, then it's in principle not a problem as long as the bit pattern is the same for all the union members requiring non-trivial initialization (and in particular if there's only one such member).  So it's just like you say, we *could* just initialize such unions conservatively as long as two different members don't require inconsistent patterns, which in practice they currently never do.  That's all true independent of storage duration — if we can write that pattern into a global, we can write into a local.  The only caveat is that a semantic need for non-trivial default initialization almost certainly means that there's a semantic need for non-trivial destruction as well, which of course can't be done on a local union (but isn't a problem for a global because we just don't destroy them).
> > 
> > On the other hand, on a language level it's much simpler to just say that we can't default-initialize a union of any storage duration if it has a non-trivial member, and then the language rule doesn't depend on bit-level representations.  If there's interest, we can look into weakening that rule later by saying that e.g. it's possible to default-initialize a union with at most one non-trivial member.
> > 
> > Apropos, do we consider unions with non-trivial members to be non-trivial members for the purposes of enclosing unions?  Seems like we should.  Probably the most sensible way to handle that is to also flag the union as being non-trivial in a dimension if it has a member that's non-trivial in that dimension (which might also let you fast-path some of the checking you need to do).  Essentially, we'd consider the case where copying is impossible to be a subset of the case where copying is non-trivial.
> Yes, this patch does consider unions with non-trivial members to be non-trivial members for the purposes of enclosing unions.
> 
> I've made changes that make clang diagnose global variables that are or have C union types that are non-trivial to default-initialize. This disallows declaring global C union variables that have ObjC ARC pointer fields, but we can relax this later if users want them.
Well, presumably you're only diagnosing them if they're actually default-initialized.  Users have an easy workaround if they actually want to declare a global union containing a `__strong` reference: they can just initialize the member they actually want to initialize.


Repository:
  rC Clang

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

https://reviews.llvm.org/D63753