[PATCH] Warning on unevaluated expression contexts with side effects

[Aaron Ballman]

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. 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
}

>
> 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. 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: UnevaluatedExpr_v2.patch
Type: application/octet-stream
Size: 18412 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20141201/4778910d/attachment.obj>