<div dir="ltr"><div>@@ -3009,7 +3026,7 @@</div><div> const CastExpr *CE = cast<CastExpr>(this);</div><div> if (CE->getCastKind() == CK_LValueToRValue &&</div><div> CE->getSubExpr()->getType().isVolatileQualified())</div><div>- return true;</div><div>+ return IncludePossibleEffects;</div><div> break;</div><div> }</div><div><br></div><div>A volatile load is always a side-effect.</div><div> </div><div>@@ -3022,7 +3039,7 @@</div><div> case CXXTemporaryObjectExprClass: {</div><div> const CXXConstructExpr *CE = cast<CXXConstructExpr>(this);</div><div> if (!CE->getConstructor()->isTrivial())</div><div>- return true;</div><div>+ return IncludePossibleEffects;</div><div> // A trivial constructor does not add any side-effects of its own. Just look</div><div> // at its arguments.</div><div> break;</div><div><br></div><div>This should be</div><div><br></div><div> if (IncludePossibleEffects)</div><div> return true;</div><div><div><br></div><div>There are a bunch of cases under</div><div><br></div><div> // FIXME: Classify these cases better.</div></div><div><br></div><div>These are all "don't know" cases, and so should say something like:</div><div><br></div><div> if (IncludePossibleEffects)</div><div> return true;</div><div> break;</div><div><br></div><div>rather than just</div><div><br></div><div> return true;</div><div><br></div><div>(You can then put ObjCMessageExprClass and ObjCPropertyRefExprClass back where they were.)</div><div><br></div><div>Otherwise, the patch LGTM; we can decide on whether this makes sense as an on-by-default warning once we get more experience with its false positive rate.</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Dec 15, 2014 at 6:57 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:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Ping<br>
<div class="HOEnZb"><div class="h5"><br>
On Thu, Dec 4, 2014 at 10:35 AM, Aaron Ballman <<a href="mailto:aaron@aaronballman.com">aaron@aaronballman.com</a>> wrote:<br>
> On Wed, Dec 3, 2014 at 2:40 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>> wrote:<br>
>> -bool Expr::HasSideEffects(const ASTContext &Ctx) const {<br>
>> +bool Expr::HasSideEffects(const ASTContext &Ctx,<br>
>> + bool OnlyDefiniteEffects) const {<br>
>><br>
>> I would reverse the sense of this flag, so that a 'true' input and a 'true'<br>
>> output mean the same thing.<br>
>><br>
>> + bool DefiniteEffectsResult = !OnlyDefiniteEffects;<br>
>><br>
>> This seems like a bad name, since it's the result you return when you're not<br>
>> sure whether there's a side-effect or not. UnknownResult or similar would be<br>
>> better.<br>
>><br>
>> + // These depend on whether we are determining side effects for the<br>
>> purposes<br>
>> + // of unevaluated expression operands, like sizeof(). For instance, a<br>
>> + // function call expression is assumed to be acceptable because the<br>
>> user is<br>
>> + // interested in the results of the call, not expecting side effects<br>
>> from<br>
>> + // the call, as in: sizeof(f());<br>
>><br>
>> This comment is too specific to the user of this function rather than<br>
>> talking about this function's actual purpose. Just "// We don't know whether<br>
>> a call has side effects." would be fine.<br>
>><br>
>> + return DefiniteEffectsResult;<br>
>><br>
>> We shouldn't return at this point if OnlyDefiniteEffects is true; we should<br>
>> break out of the switch so that we go on to analyze the operands.<br>
>><br>
>> + case UnresolvedLookupExprClass:<br>
>> + // This should *only* be reachable when determining side effects for<br>
>> the<br>
>> + // purposes of unevaluated expression operands for decltype(). This can<br>
>> + // happen in situations where the decltype expression looks up an<br>
>> overloaded<br>
>> + // function, or when the function is an uninstantiated template.<br>
>> + assert(OnlyDefiniteEffects);<br>
>> + return false;<br>
>><br>
>> Just return UnknownResult here? We don't know whether this expression would<br>
>> have side-effects. If this function is supposed to now be usable on<br>
>> unresolved expressions, we should make it handle them in all cases, not just<br>
>> most cases.<br>
>><br>
>> + // so side effects are likely result in unintended consequences.<br>
>><br>
>> Grammar error here; missing word? Also, "are likely" seems to be overstating<br>
>> the case.<br>
>><br>
>> + else if (!PotentiallyEvaluated && E->HasSideEffects(Context, true)) {<br>
>><br>
>> I think we should still warn in the potentially-evaluated case; that's<br>
>> probably more likely to be a bug, because people probably expect the operand<br>
>> of typeid to be unevaluated at least as often as they expect it to be<br>
>> evaluated.<br>
>><br>
>> On Tue, Dec 2, 2014 at 6:46 AM, Aaron Ballman <<a href="mailto:aaron@aaronballman.com">aaron@aaronballman.com</a>><br>
>> wrote:<br>
>>><br>
>>> On Mon, Dec 1, 2014 at 4:39 PM, Aaron Ballman <<a href="mailto:aaron@aaronballman.com">aaron@aaronballman.com</a>><br>
>>> wrote:<br>
>>> > On Mon, Dec 1, 2014 at 2:04 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>><br>
>>> > wrote:<br>
>>> >> On Mon, Dec 1, 2014 at 10:33 AM, Aaron Ballman <<a href="mailto:aaron@aaronballman.com">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">richard@metafoo.co.uk</a>><br>
>>> >>> wrote:<br>
>>> >>> > On Tue, Nov 25, 2014 at 6:51 AM, Aaron Ballman<br>
>>> >>> > <<a href="mailto:aaron@aaronballman.com">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,<br>
>>> >>> >> 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<br>
>>> >>> >> 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,<br>
>>> >>> >> 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<br>
>>> >>> > 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<br>
>>> >>> 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,<br>
>>> >> sizeof(is_incrementable_helper<T>(nullptr))<br>
>>> >> == sizeof(one)>;<br>
>>> >><br>
>>> >> I think your warning would warn on uses of<br>
>>> >> '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<br>
>>> >> 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<br>
>>> >> if it<br>
>>> >> occurs while instantiating a template.<br>
>>> ><br>
>>> > if (isInstantiationDependent())<br>
>>> > return DefiniteEffectsResult;<br>
>><br>
>><br>
>> That will affect the behavior when parsing the template, not when<br>
>> instantiating it. During / after template instantiation, your expression<br>
>> will not be instantiation dependent.<br>
>><br>
>> It looks like you're not seeing issues for decltype because you put the<br>
>> check in ActOnDecltypeExpression rather than BuildDecltypeType (so it's only<br>
>> called from the parser and not from template instantiation). I would expect<br>
>> you'll get warnings during template instantiation for all the other forms of<br>
>> unevaluated operand.<br>
>><br>
>>> > ;-)<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<br>
>>> >> 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>
>><br>
>><br>
>> If the true positive rate is effectively zero, it is wasteful to spend<br>
>> compile time checking for this (and to incur the maintenance cost of the<br>
>> extra code). We should run this over some larger codebases before going<br>
>> ahead with it.<br>
><br>
> Attached is an updated patch based on this feedback, and further<br>
> feedback from IRC. Of special note, this patch now handled typeids<br>
> which are evaluated as a separate warning case (since it's equally<br>
> likely that the user would expect the expression operand of typeid to<br>
> be unevaluated when it actually turns out to be evaluated).<br>
><br>
> I reran my bootstrapping tests against Clang, and got no false<br>
> positives or true positives.<br>
><br>
> Regarding the true positive rate -- I would expect that to be<br>
> relatively low for established code bases. People generally find this<br>
> sort of bug because they expect the side effect to happen but it<br>
> doesn't. However, I think it is a valid warning to justify the cost;<br>
> we're not the only vendor who implements such a diagnostic (ECLAIR,<br>
> LDRA, PC-Lint, and PRQA all implement it as well from a quick check).<br>
> I could see a case for moving this to the static analyzer, but it's<br>
> also not path-sensitive, and really doesn't gain us much in terms of<br>
> code maintenance.<br>
><br>
> If you, or rtrieu, have some other large code bases to test this<br>
> against, I would greatly appreciate it.<br>
><br>
> Thanks!<br>
><br>
> ~Aaron<br>
</div></div></blockquote></div><br></div>