<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Mon, Dec 1, 2014 at 11:04 AM, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span class="">On Mon, Dec 1, 2014 at 10:33 AM, Aaron Ballman <span dir="ltr"><<a href="mailto:aaron@aaronballman.com" target="_blank">aaron@aaronballman.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span>On Wed, Nov 26, 2014 at 9:09 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>> wrote:<br>
> On Tue, Nov 25, 2014 at 6:51 AM, Aaron Ballman <<a href="mailto:aaron@aaronballman.com" target="_blank">aaron@aaronballman.com</a>><br>
> wrote:<br>
>><br>
>> The attached patch adds a -Wunused-value warning when an expression<br>
>> with side effects is used in an unevaluated expression context, such<br>
>> as sizeof(), or decltype(). It mostly reuses the logic from<br>
>> Expr::HasSideEffects, except with a flag that treats certain<br>
>> expressions as not having side effects -- mostly<br>
>> instantiation-dependent contexts, and function call-like operations.<br>
>><br>
>> int f();<br>
>> int j = 0;<br>
>><br>
>> (void)sizeof(f()); // Ok<br>
>> (void)sizeof(++j); // Warn<br>
>> (void)sizeof(int[++j]); // Ok<br>
>><br>
>> I've added support for: sizeof, typeid, _Generic, _Alignof, noexcept,<br>
>> and decltype.<br>
><br>
><br>
> It is a very common idiom to use arbitrary expressions in sizeof, decltype,<br>
> or noexcept expressions; in the former two cases, for SFINAE, and in the<br>
> last case to compute an exception specification for an arbitrary function.<br>
> In all these cases, expressions with side-effects seem completely<br>
> reasonable. How does your patch behave in those cases?<br>
<br>
</span>I believe it behaves sensibly, but am happy to get counter-examples<br>
where I can tweak something.</blockquote><div><br></div></span><div>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.</div><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Basically, if the side-effect involves<br>
something function-like (including overloaded operators), we ignore it<br>
as being side-effecting. I've found that this cuts the signal-to-noise<br>
ratio down considerably. I think this also matches the intent of<br>
unevaluated contexts -- when the compiler needs a declaration, but not<br>
a definition, it's safe to ignore side-effects. Eg)<br>
<br>
struct S {<br>
  S operator++(int);<br>
  S(int i);<br>
  S();<br>
<br>
  int& f();<br>
  S g();<br>
};<br>
<br>
void f() {<br>
  S s;<br>
  int i = 0;<br>
  (void)noexcept(s++); // Ok, because of operator++()<br>
  (void)noexcept(i++); // Not Ok, because this doesn't involve a function call<br>
  (void)noexcept(i = 5); // Not Ok, because this doesn't involve a function call<br>
  (void)noexcept(s = 5); // Ok, because of copy constructor call<br>
<br>
  (void)sizeof(s.f()); // Ok, because of f()<br>
  (void)sizeof(s.f() = 5); // Not Ok, because f() returns an int&, and<br>
this assignment could reasonably be side-effecting<br>
  (void)noexcept(s.g() = 5); // Ok, because of copy constructor call<br>
<span>}<br></span></blockquote><div><br></div></span><div>Here's a SFINAE idiom for detecting incrementability:</div><div><br></div><div>  template<typename T> static one &is_incrementable_helper(decltype(declval<T>()++) *p);<br></div></div></div></div></blockquote><div><br></div><div>Um, you'll need some remove_reference magic in here, but you get the general idea...</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div></div><div>  template<typename T> static two &is_incrementable_helper(...);</div><div><br></div><div>  template<typename T> using is_incrementable =</div><div>    std::integer_constant<bool, sizeof(is_incrementable_helper<T>(nullptr)) == sizeof(one)>;</div><div><br></div><div>I think your warning would warn on uses of 'is_incrementable<int>::value'?</div><div><br></div><div>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++: <a href="http://www.boost.org/doc/libs/1_55_0/boost/detail/is_incrementable.hpp" target="_blank">http://www.boost.org/doc/libs/1_55_0/boost/detail/is_incrementable.hpp</a></div><div><br></div><div>One way to avoid these cases would be to always suppress this warning if it occurs while instantiating a template.</div><span class=""><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span>
><br>
> Does this actually catch bugs in practice?<br>
<br>
</span>I believe so. There are two CERT advisories against it, which is why I<br>
am implementing the functionality.</blockquote><div><br></div></span><div>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?)</div><div><br></div><div>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?</div><div><div class="h5"><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">There are other analysis tools<br>
which also implement similar functionality, so we're not breaking new<br>
ground.<br>
<br>
<a href="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" target="_blank">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</a><br>
<a href="https://www.securecoding.cert.org/confluence/display/cplusplus/EXP32-CPP.+Do+not+rely+on+side+effects+in+unevaluated+operands" target="_blank">https://www.securecoding.cert.org/confluence/display/cplusplus/EXP32-CPP.+Do+not+rely+on+side+effects+in+unevaluated+operands</a><br>
<span><br>
><br>
> +  /// call, volatile variable read, or throwing an exception. If<br>
> ForUnevalDiag<br>
> +  /// is true, this call treats certain expressions as not having side<br>
> effects<br>
> +  /// when they otherwise would (such as function call-like expressions,<br>
> where<br>
> +  /// the intent isn't on the side effects of the call, but the signature;<br>
> or<br>
> +  /// instantiation-dependent expressions).<br>
> +  bool HasSideEffects(const ASTContext &Ctx, bool ForUnevalDiag = false)<br>
> const;<br>
><br>
> This new flag seems very special-case. I think what you're really trying to<br>
> express here is what value the function should return if it doesn't know.<br>
> (That is, it's choosing between determining "might possibly have side<br>
> effects" and "definitely has side effects".)<br>
<br>
</span>I've renamed the flag to something more sensible.<br>
<br>
As for use within macros, my biggest concern is that several of these<br>
unevaluated contexts, at least in C, are almost exclusively used<br>
within a macro. For instance, you rarely see a use of _Generic that<br>
isn't in a macro (see tgmath.h for an example:<br>
<a href="http://web.mit.edu/freebsd/head/include/tgmath.h" target="_blank">http://web.mit.edu/freebsd/head/include/tgmath.h</a>), and when you<br>
#include <stdalign.h>, it gives you the prettier "alignof" instead of<br>
"_Alignof" via a macro (to be fair, _Alignof on an expression is an<br>
extension, but it's still one I wish to support). If we disabled<br>
support of this within macros, it could reduce the effectiveness. Then<br>
again, regarding _Generic:<br>
<br>
void baz(int);<br>
<br>
#define foo(x) _Generic(x, default : baz)(x)<br>
#define bar(x) _Generic(x, default : baz)<br>
<br>
int x = 0;<br>
bar(x++)(x); // Could usefully warn, but highly unlikely for people to<br>
write code like this<br>
foo(x++); // Should not warn, more likely usage pattern<br>
<br>
Since it's easier to ease restrictions later, I've implemented the<br>
macro support as well -- if the expression is within a macro and we<br>
are only interested in definite side effects, we do not emit a<br>
diagnostic.<br>
<br>
Thanks!<br>
<span><font color="#888888"><br>
~Aaron<br>
</font></span></blockquote></div></div></div><br></div></div>
</blockquote></div><br></div></div>