On Mon, Dec 1, 2014 at 4:39 PM, Aaron Ballman <[email protected]> wrote: > On Mon, Dec 1, 2014 at 2:04 PM, 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. > > Will those build with a clang built with MSVC on Windows? I didn't > think we were there quite yet (and it's all I have access to ATM). > >> >>> >>> 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); >> 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'? > > It compiles without warning, actually. The use of T within decltype is > instantiation-dependent, which my patch treats as only being possibly > side-effecting, instead of definitely. > >> 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 > > This also compiles without warning. > >> >> One way to avoid these cases would be to always suppress this warning if it >> occurs while instantiating a template. > > if (isInstantiationDependent()) > return DefiniteEffectsResult; > > ;-) > >> >>> > >>> > 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?) > > They can definitely be theoretical, but many are based on actual > events. We try to ensure the rules are based on plausible, real world > scenarios that could result in a security vulnerability, and this > definitely met our criteria (twice). A quick browse of CVE did not > bring up major known vulns from this. > >> 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? > > I will test to see (it may take me a while, bootstrapping on Windows > does not happen easily). Alternatively, this could go under a flag > aside from -Wunused-value, in case people want to handle this > separately, if that would assuage fears?
I applied my patch and bootstrapped Clang, and it did not yield any additional diagnostics (false positives or bugs). ~Aaron _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
