[PATCH] Warning on unevaluated expression contexts with side effects

Mon Dec 1 11:06:10 PST 2014

On Mon, Dec 1, 2014 at 11:04 AM, 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.
>
>
>> 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
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20141201/4c7e9f19/attachment.html>