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);
+
----------------
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.
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
