[PATCH] Warning on unevaluated expression contexts with side effects

[Richard Smith]

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.


> 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'?

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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20141201/f61ef46c/attachment.html>