<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div class="gmail_quote">-bool Expr::HasSideEffects(const ASTContext &Ctx) const {</div><div class="gmail_quote">+bool Expr::HasSideEffects(const ASTContext &Ctx,</div><div class="gmail_quote">+ bool OnlyDefiniteEffects) const {</div><div class="gmail_quote"><br></div><div class="gmail_quote">I would reverse the sense of this flag, so that a 'true' input and a 'true' output mean the same thing.</div></div><div class="gmail_quote"><br></div><div class="gmail_quote">+ bool DefiniteEffectsResult = !OnlyDefiniteEffects;<br></div><div class="gmail_quote"><br></div><div class="gmail_quote">This seems like a bad name, since it's the result you return when you're not sure whether there's a side-effect or not. UnknownResult or similar would be better.</div><div class="gmail_quote"><br></div><div class="gmail_quote"><div class="gmail_quote">+ // These depend on whether we are determining side effects for the purposes</div><div class="gmail_quote">+ // of unevaluated expression operands, like sizeof(). For instance, a</div><div class="gmail_quote">+ // function call expression is assumed to be acceptable because the user is</div><div class="gmail_quote">+ // interested in the results of the call, not expecting side effects from</div><div class="gmail_quote">+ // the call, as in: sizeof(f());</div><div><br></div><div>This comment is too specific to the user of this function rather than talking about this function's actual purpose. Just "// We don't know whether a call has side effects." would be fine.</div><div><br></div><div>+ return DefiniteEffectsResult;<br></div><div><br></div><div>We shouldn't return at this point if OnlyDefiniteEffects is true; we should break out of the switch so that we go on to analyze the operands.</div><div><br></div><div><div>+ case UnresolvedLookupExprClass:</div><div>+ // This should *only* be reachable when determining side effects for the</div><div>+ // purposes of unevaluated expression operands for decltype(). This can</div><div>+ // happen in situations where the decltype expression looks up an overloaded</div><div>+ // function, or when the function is an uninstantiated template.</div><div>+ assert(OnlyDefiniteEffects);</div><div>+ return false;</div></div><div><br></div><div>Just return UnknownResult here? We don't know whether this expression would have side-effects. If this function is supposed to now be usable on unresolved expressions, we should make it handle them in all cases, not just most cases.</div><div><br></div><div>+ // so side effects are likely result in unintended consequences.<br></div><div><br></div><div>Grammar error here; missing word? Also, "are likely" seems to be overstating the case.</div></div><div class="gmail_quote"><br></div><div class="gmail_quote">+ else if (!PotentiallyEvaluated && E->HasSideEffects(Context, true)) {<br></div><div class="gmail_quote"><br></div><div class="gmail_quote">I think we should still warn in the potentially-evaluated case; that's probably more likely to be a bug, because people probably expect the operand of typeid to be unevaluated at least as often as they expect it to be evaluated.</div><div class="gmail_quote"><br></div><div class="gmail_quote">On Tue, Dec 2, 2014 at 6:46 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"><div><div>On Mon, Dec 1, 2014 at 4:39 PM, Aaron Ballman <<a href="mailto:aaron@aaronballman.com" target="_blank">aaron@aaronballman.com</a>> wrote:<br>
> On Mon, Dec 1, 2014 at 2:04 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>> wrote:<br>
>> On Mon, Dec 1, 2014 at 10:33 AM, Aaron Ballman <<a href="mailto:aaron@aaronballman.com" target="_blank">aaron@aaronballman.com</a>><br>
>> wrote:<br>
>>><br>
>>> 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>><br>
>>> 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,<br>
>>> > 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<br>
>>> > function.<br>
>>> > In all these cases, expressions with side-effects seem completely<br>
>>> > reasonable. How does your patch behave in those cases?<br>
>>><br>
>>> I believe it behaves sensibly, but am happy to get counter-examples<br>
>>> where I can tweak something.<br>
>><br>
>><br>
>> You could try building boost and eigen with -Werror=<this warning>. If<br>
>> they're both fine with it, then I'm happy to assume it's OK =) But see<br>
>> below.<br>
><br>
> Will those build with a clang built with MSVC on Windows? I didn't<br>
> think we were there quite yet (and it's all I have access to ATM).<br>
><br>
>><br>
>>><br>
>>> 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<br>
>>> call<br>
>>> (void)noexcept(i = 5); // Not Ok, because this doesn't involve a<br>
>>> 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>
>>> }<br>
>><br>
>><br>
>> Here's a SFINAE idiom for detecting incrementability:<br>
>><br>
>> template<typename T> static one<br>
>> &is_incrementable_helper(decltype(declval<T>()++) *p);<br>
>> template<typename T> static two &is_incrementable_helper(...);<br>
>><br>
>> template<typename T> using is_incrementable =<br>
>> std::integer_constant<bool, sizeof(is_incrementable_helper<T>(nullptr))<br>
>> == sizeof(one)>;<br>
>><br>
>> I think your warning would warn on uses of 'is_incrementable<int>::value'?<br>
><br>
> It compiles without warning, actually. The use of T within decltype is<br>
> instantiation-dependent, which my patch treats as only being possibly<br>
> side-effecting, instead of definitely.<br>
><br>
>> Here's some code in boost that uses a different approach, but that will<br>
>> probably also elicit a warning when applied to a type that uses the builtin<br>
>> operator++:<br>
>> <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><br>
><br>
> This also compiles without warning.<br>
><br>
>><br>
>> One way to avoid these cases would be to always suppress this warning if it<br>
>> occurs while instantiating a template.<br>
><br>
> if (isInstantiationDependent())<br>
> return DefiniteEffectsResult;<br></div></div></blockquote><div><br></div><div>That will affect the behavior when parsing the template, not when instantiating it. During / after template instantiation, your expression will not be instantiation dependent.</div><div><br></div><div>It looks like you're not seeing issues for decltype because you put the check in ActOnDecltypeExpression rather than BuildDecltypeType (so it's only called from the parser and not from template instantiation). I would expect you'll get warnings during template instantiation for all the other forms of unevaluated operand.</div><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"><div><div>
> ;-)<br>
><br>
>><br>
>>> ><br>
>>> > Does this actually catch bugs in practice?<br>
>>><br>
>>> I believe so. There are two CERT advisories against it, which is why I<br>
>>> am implementing the functionality.<br>
>><br>
>><br>
>> Does that actually imply that this is a real-world problem? (The CERT<br>
>> advisory (for C++) says "Severity: Low, Likelihood: Unlikely". Are CERT<br>
>> advisories always based on actual events, or can they be just theoretical?)<br>
><br>
> They can definitely be theoretical, but many are based on actual<br>
> events. We try to ensure the rules are based on plausible, real world<br>
> scenarios that could result in a security vulnerability, and this<br>
> definitely met our criteria (twice). A quick browse of CVE did not<br>
> bring up major known vulns from this.<br></div></div></blockquote><div><br></div><div>If the true positive rate is effectively zero, it is wasteful to spend compile time checking for this (and to incur the maintenance cost of the extra code). We should run this over some larger codebases before going ahead with it.</div><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"><div><div>
>> Have you run this warning across any codebases of significant size and found<br>
>> bugs or false positives? For instance, does it issue any warnings in a<br>
>> bootstrap build of Clang?<br>
><br>
> I will test to see (it may take me a while, bootstrapping on Windows<br>
> does not happen easily). Alternatively, this could go under a flag<br>
> aside from -Wunused-value, in case people want to handle this<br>
> separately, if that would assuage fears?<br>
<br>
</div></div>I applied my patch and bootstrapped Clang, and it did not yield any<br>
additional diagnostics (false positives or bugs).<br>
<span><font color="#888888"><br>
~Aaron<br>
</font></span></blockquote></div><br></div></div>