On Mon, Dec 1, 2014 at 11:04 AM, Richard Smith <[email protected]> wrote:
> On Mon, Dec 1, 2014 at 10:33 AM, Aaron Ballman <[email protected]> > wrote: > >> On Wed, Nov 26, 2014 at 9:09 PM, Richard Smith <[email protected]> >> wrote: >> > On Tue, Nov 25, 2014 at 6:51 AM, Aaron Ballman <[email protected]> >> > wrote: >> >> >> >> The attached patch adds a -Wunused-value warning when an expression >> >> with side effects is used in an unevaluated expression context, such >> >> as sizeof(), or decltype(). It mostly reuses the logic from >> >> Expr::HasSideEffects, except with a flag that treats certain >> >> expressions as not having side effects -- mostly >> >> instantiation-dependent contexts, and function call-like operations. >> >> >> >> int f(); >> >> int j = 0; >> >> >> >> (void)sizeof(f()); // Ok >> >> (void)sizeof(++j); // Warn >> >> (void)sizeof(int[++j]); // Ok >> >> >> >> I've added support for: sizeof, typeid, _Generic, _Alignof, noexcept, >> >> and decltype. >> > >> > >> > It is a very common idiom to use arbitrary expressions in sizeof, >> decltype, >> > or noexcept expressions; in the former two cases, for SFINAE, and in the >> > last case to compute an exception specification for an arbitrary >> function. >> > In all these cases, expressions with side-effects seem completely >> > reasonable. How does your patch behave in those cases? >> >> I believe it behaves sensibly, but am happy to get counter-examples >> where I can tweak something. > > > You could try building boost and eigen with -Werror=<this warning>. If > they're both fine with it, then I'm happy to assume it's OK =) But see > below. > > >> Basically, if the side-effect involves >> something function-like (including overloaded operators), we ignore it >> as being side-effecting. I've found that this cuts the signal-to-noise >> ratio down considerably. I think this also matches the intent of >> unevaluated contexts -- when the compiler needs a declaration, but not >> a definition, it's safe to ignore side-effects. Eg) >> >> struct S { >> S operator++(int); >> S(int i); >> S(); >> >> int& f(); >> S g(); >> }; >> >> void f() { >> S s; >> int i = 0; >> (void)noexcept(s++); // Ok, because of operator++() >> (void)noexcept(i++); // Not Ok, because this doesn't involve a function >> call >> (void)noexcept(i = 5); // Not Ok, because this doesn't involve a >> function call >> (void)noexcept(s = 5); // Ok, because of copy constructor call >> >> (void)sizeof(s.f()); // Ok, because of f() >> (void)sizeof(s.f() = 5); // Not Ok, because f() returns an int&, and >> this assignment could reasonably be side-effecting >> (void)noexcept(s.g() = 5); // Ok, because of copy constructor call >> } >> > > Here's a SFINAE idiom for detecting incrementability: > > template<typename T> static one > &is_incrementable_helper(decltype(declval<T>()++) *p); > Um, you'll need some remove_reference magic in here, but you get the general idea... > template<typename T> static two &is_incrementable_helper(...); > > template<typename T> using is_incrementable = > std::integer_constant<bool, > sizeof(is_incrementable_helper<T>(nullptr)) == sizeof(one)>; > > I think your warning would warn on uses of 'is_incrementable<int>::value'? > > Here's some code in boost that uses a different approach, but that will > probably also elicit a warning when applied to a type that uses the builtin > operator++: > http://www.boost.org/doc/libs/1_55_0/boost/detail/is_incrementable.hpp > > One way to avoid these cases would be to always suppress this warning if > it occurs while instantiating a template. > > > >> > Does this actually catch bugs in practice? >> >> I believe so. There are two CERT advisories against it, which is why I >> am implementing the functionality. > > > Does that actually imply that this is a real-world problem? (The CERT > advisory (for C++) says "Severity: Low, Likelihood: Unlikely". Are CERT > advisories always based on actual events, or can they be just theoretical?) > > Have you run this warning across any codebases of significant size and > found bugs or false positives? For instance, does it issue any warnings in > a bootstrap build of Clang? > > There are other analysis tools >> which also implement similar functionality, so we're not breaking new >> ground. >> >> >> https://www.securecoding.cert.org/confluence/display/seccode/EXP44-C.+Do+not+rely+on+side+effects+in+operands+to+sizeof%2C+_Alignof%2C+or+_Generic >> >> https://www.securecoding.cert.org/confluence/display/cplusplus/EXP32-CPP.+Do+not+rely+on+side+effects+in+unevaluated+operands >> >> > >> > + /// call, volatile variable read, or throwing an exception. If >> > ForUnevalDiag >> > + /// is true, this call treats certain expressions as not having side >> > effects >> > + /// when they otherwise would (such as function call-like >> expressions, >> > where >> > + /// the intent isn't on the side effects of the call, but the >> signature; >> > or >> > + /// instantiation-dependent expressions). >> > + bool HasSideEffects(const ASTContext &Ctx, bool ForUnevalDiag = >> false) >> > const; >> > >> > This new flag seems very special-case. I think what you're really >> trying to >> > express here is what value the function should return if it doesn't >> know. >> > (That is, it's choosing between determining "might possibly have side >> > effects" and "definitely has side effects".) >> >> I've renamed the flag to something more sensible. >> >> As for use within macros, my biggest concern is that several of these >> unevaluated contexts, at least in C, are almost exclusively used >> within a macro. For instance, you rarely see a use of _Generic that >> isn't in a macro (see tgmath.h for an example: >> http://web.mit.edu/freebsd/head/include/tgmath.h), and when you >> #include <stdalign.h>, it gives you the prettier "alignof" instead of >> "_Alignof" via a macro (to be fair, _Alignof on an expression is an >> extension, but it's still one I wish to support). If we disabled >> support of this within macros, it could reduce the effectiveness. Then >> again, regarding _Generic: >> >> void baz(int); >> >> #define foo(x) _Generic(x, default : baz)(x) >> #define bar(x) _Generic(x, default : baz) >> >> int x = 0; >> bar(x++)(x); // Could usefully warn, but highly unlikely for people to >> write code like this >> foo(x++); // Should not warn, more likely usage pattern >> >> Since it's easier to ease restrictions later, I've implemented the >> macro support as well -- if the expression is within a macro and we >> are only interested in definite side effects, we do not emit a >> diagnostic. >> >> Thanks! >> >> ~Aaron >> > >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
