rjmccall added inline comments.
================ Comment at: lib/Sema/SemaExpr.cpp:16218 + checkNonTrivialCUnion(E->getType(), E->getExprLoc(), + Sema::NTCUC_LValueToRValueVolatile); + ---------------- 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. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63753/new/ https://reviews.llvm.org/D63753 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits