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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits