Ping
On Thu, Dec 4, 2014 at 10:35 AM, Aaron Ballman <[email protected]> wrote: > On Wed, Dec 3, 2014 at 2:40 PM, Richard Smith <[email protected]> wrote: >> -bool Expr::HasSideEffects(const ASTContext &Ctx) const { >> +bool Expr::HasSideEffects(const ASTContext &Ctx, >> + bool OnlyDefiniteEffects) const { >> >> I would reverse the sense of this flag, so that a 'true' input and a 'true' >> output mean the same thing. >> >> + bool DefiniteEffectsResult = !OnlyDefiniteEffects; >> >> 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. >> >> + // These depend on whether we are determining side effects for the >> purposes >> + // of unevaluated expression operands, like sizeof(). For instance, a >> + // function call expression is assumed to be acceptable because the >> user is >> + // interested in the results of the call, not expecting side effects >> from >> + // the call, as in: sizeof(f()); >> >> 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. >> >> + return DefiniteEffectsResult; >> >> 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. >> >> + case UnresolvedLookupExprClass: >> + // This should *only* be reachable when determining side effects for >> the >> + // purposes of unevaluated expression operands for decltype(). This can >> + // happen in situations where the decltype expression looks up an >> overloaded >> + // function, or when the function is an uninstantiated template. >> + assert(OnlyDefiniteEffects); >> + return false; >> >> 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. >> >> + // so side effects are likely result in unintended consequences. >> >> Grammar error here; missing word? Also, "are likely" seems to be overstating >> the case. >> >> + else if (!PotentiallyEvaluated && E->HasSideEffects(Context, true)) { >> >> 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. >> >> On Tue, Dec 2, 2014 at 6:46 AM, Aaron Ballman <[email protected]> >> wrote: >>> >>> On Mon, Dec 1, 2014 at 4:39 PM, Aaron Ballman <[email protected]> >>> wrote: >>> > On Mon, Dec 1, 2014 at 2:04 PM, Richard Smith <[email protected]> >>> > wrote: >>> >> On Mon, Dec 1, 2014 at 10:33 AM, Aaron Ballman <[email protected]> >>> >> wrote: >>> >>> >>> >>> On Wed, Nov 26, 2014 at 9:09 PM, Richard Smith <[email protected]> >>> >>> wrote: >>> >>> > On Tue, Nov 25, 2014 at 6:51 AM, Aaron Ballman >>> >>> > <[email protected]> >>> >>> > 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. >>> > >>> > Will those build with a clang built with MSVC on Windows? I didn't >>> > think we were there quite yet (and it's all I have access to ATM). >>> > >>> >> >>> >>> >>> >>> 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'? >>> > >>> > It compiles without warning, actually. The use of T within decltype is >>> > instantiation-dependent, which my patch treats as only being possibly >>> > side-effecting, instead of definitely. >>> > >>> >> 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 >>> > >>> > This also compiles without warning. >>> > >>> >> >>> >> One way to avoid these cases would be to always suppress this warning >>> >> if it >>> >> occurs while instantiating a template. >>> > >>> > if (isInstantiationDependent()) >>> > return DefiniteEffectsResult; >> >> >> That will affect the behavior when parsing the template, not when >> instantiating it. During / after template instantiation, your expression >> will not be instantiation dependent. >> >> 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. >> >>> > ;-) >>> > >>> >> >>> >>> > >>> >>> > 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?) >>> > >>> > They can definitely be theoretical, but many are based on actual >>> > events. We try to ensure the rules are based on plausible, real world >>> > scenarios that could result in a security vulnerability, and this >>> > definitely met our criteria (twice). A quick browse of CVE did not >>> > bring up major known vulns from this. >> >> >> 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. > > Attached is an updated patch based on this feedback, and further > feedback from IRC. Of special note, this patch now handled typeids > which are evaluated as a separate warning case (since it's equally > likely that the user would expect the expression operand of typeid to > be unevaluated when it actually turns out to be evaluated). > > I reran my bootstrapping tests against Clang, and got no false > positives or true positives. > > Regarding the true positive rate -- I would expect that to be > relatively low for established code bases. People generally find this > sort of bug because they expect the side effect to happen but it > doesn't. However, I think it is a valid warning to justify the cost; > we're not the only vendor who implements such a diagnostic (ECLAIR, > LDRA, PC-Lint, and PRQA all implement it as well from a quick check). > I could see a case for moving this to the static analyzer, but it's > also not path-sensitive, and really doesn't gain us much in terms of > code maintenance. > > If you, or rtrieu, have some other large code bases to test this > against, I would greatly appreciate it. > > Thanks! > > ~Aaron _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
