[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: include/clang/AST/Type.h:1133
+  /// Check if this is or contains a non-trivial C struct/union type.
+  bool hasNonTrivialPrimitiveCStruct() const;
 
----------------
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.


================
Comment at: lib/Sema/SemaExpr.cpp:16218
+    checkNonTrivialCUnion(E->getType(), E->getExprLoc(),
+                          Sema::NTCUC_LValueToRValueVolatile);
+
----------------
ahatanak wrote:
> ahatanak wrote:
> > rjmccall wrote:
> > > ahatanak wrote:
> > > > rjmccall wrote:
> > > > > ahatanak wrote:
> > > > > > rjmccall wrote:
> > > > > > > ahatanak wrote:
> > > > > > > > rjmccall wrote:
> > > > > > > > > It looks like you're generally warning about this based on the specific context that forced an lvalue-to-rvalue conversion.  I'm not sure `volatile` is special except that we actually perform the load even in unused-value contexts.  Is the assumption that you've exhaustively covered all the other contexts of lvalue-to-rvalue conversions whose values will actually be used?  That seems questionable to me.
> > > > > > > > Yes, that was my assumption. All the other contexts where lvalue-to-rvalue conversion is performed and the result is used are already covered by other calls sites of `checkNonTrivialCUnion`, which informs the users that the struct/union is being used in an invalid context.
> > > > > > > > 
> > > > > > > > Do you have a case in mind that I didn't think of where a lvalue-to-rvalue conversion requires a non-trivial initialization/destruction/copying of a union but clang fails to emit any diagnostics?
> > > > > > > > 
> > > > > > > > Also I realized that lvalue-to-rvalue conversion of volatile types doesn't always require non-trivial destruction, so I think `CheckDestruct` shouldn't be set in this case.
> > > > > > > > 
> > > > > > > > ```
> > > > > > > > void foo(U0 *a, volatile U0 *b) {
> > > > > > > >   // this doesn't require destruction.
> > > > > > > >   // this is perfectly valid if U0 is non-trivial to destruct but trivial to copy.
> > > > > > > >   *a = *b;  
> > > > > > > > }
> > > > > > > > ```
> > > > > > > > 
> > > > > > > > For the same reason, I think `CheckDestruct` shouldn't be set for function returns (but it should be set for function parameters since they are destructed by the callee).
> > > > > > > There are a *lot* of places that trigger lvalue-to-rvalue conversion.  Many of them aren't legal with structs (in C), but I'm worried about approving a pattern with the potential to be wrong by default just because we didn't think about some weird case.  As an example, casts can trigger lvalue-to-rvalue conversion; I think the only casts allowed with structs are the identity cast and the cast to `void`, but those are indeed allowed.  Now, a cast to `void` means the value is ignored, so we can elide a non-volatile load in the operand, and an identity cast isn't terminal; if the idea is that we're checking all the *terminal* uses of a struct r-value, then we're in much more restricted territory (and don't need to worry about things like commas and conditional operators that can propagate values out).  But this still worries me.
> > > > > > > 
> > > > > > > I'm not sure we need to be super-pedantic about destructibility vs. copyability in some of this checking.  It's certain possible in C++, but I can't imagine what sort of *primitive* type would be trivially copyable but not trivially destructible.  (The reverse isn't true: something like a relative pointer would be non-trivially copyable but still trivially destructible.)
> > > > > > > 
> > > > > > > Is there any way to make this check cheaper so that we can immediately avoid any further work for types that are obviously copyable/destructible?  All the restricted types are (possibly arrays of) record types, right?
> > > > > > I'm not sure I fully understand what you are saying, but by "cheaper", do you mean fewer and simpler rules for allowing or disallowing non-trivial unions even when that might result in rejecting unions used in contexts in which non-trivial initialization/destruction/copying is not required? If so, we can probably diagnose any lvalue-to-rvalue conversions regardless of whether the source is volatile if the type is either non-trivial to copy or destruct.
> > > > > Sorry, that point was separate from the discussion of `volatile` and lvalue-to-rvalue conversions.  I mean that you're changing a lot of core paths in Sema, and it would be nice if we could very quickly decide based on the type that no restrictions apply instead of having to make a function call, a switch, and a bunch of other calls in order to realize that e.g. `void*` never needs additional checking.  Currently you have a `!CPlusPlus` check in front of all the `checkNonTrivialCUnion` calls; I would like something that reliably avoids doing this work for the vast majority of types that are not restricted, even in C.
> > > > Instead of checking `!CPlusPlus`, we can call `isNonTrivialPrimitiveCType` (which is deleted in this patch) to do a cheaper check of whether the type requires any non-trivial default-initialize/destruct/copy functions. The function still uses the copy visitor, so if we want to make the check even cheaper, we can add a flag to `RecordDecl` that indicates whether it contains a non-trivial union.
> > > I think it would be sufficient for the fast path to just check for a C record type (i.e. `!isa<CXXRecordDecl>(RT->getDecl())`).  But it doesn't seem unreasonable to record non-copyable/destructible/defaultable types on the `RecordDecl` if we have the bits.
> > I don't think we can call `!isa<CXXRecordDecl>` if the type is an array? I created a new function `hasNonTrivialPrimitiveCStruct` instead that checks whether the type is a non-trivial C struct or union.
> Actually, it's not wrong. It only excludes C++ records, so the function call will be made for all the other types including C structs/unions or arrays of them that are trivial.
I was skipping a few steps, but yes, you'd need to look through array types (which you can do "unsafely" because you don't need to care about top-level qualification), then check if the base element type was a `RecordType`.


Repository:
  rC Clang

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

https://reviews.llvm.org/D63753