ahatanak marked an inline comment as done.
ahatanak added inline comments.
================
Comment at: lib/Sema/SemaExpr.cpp:16218
+ checkNonTrivialCUnion(E->getType(), E->getExprLoc(),
+ Sema::NTCUC_LValueToRValueVolatile);
+
----------------
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.
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
