[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/SemaExpr.cpp:16218
+    checkNonTrivialCUnion(E->getType(), E->getExprLoc(),
+                          Sema::NTCUC_LValueToRValueVolatile);
+
----------------
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?


Repository:
  rC Clang

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

https://reviews.llvm.org/D63753