[PATCH] Warning on unevaluated expression contexts with side effects

[Aaron Ballman]

On Mon, Dec 1, 2014 at 4:39 PM, Aaron Ballman <aaron at aaronballman.com> wrote:
> On Mon, Dec 1, 2014 at 2:04 PM, Richard Smith <richard at metafoo.co.uk> wrote:
>> On Mon, Dec 1, 2014 at 10:33 AM, Aaron Ballman <aaron at aaronballman.com>
>> wrote:
>>>
>>> On Wed, Nov 26, 2014 at 9:09 PM, Richard Smith <richard at metafoo.co.uk>
>>> wrote:
>>> > On Tue, Nov 25, 2014 at 6:51 AM, Aaron Ballman <aaron at aaronballman.com>
>>> > 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