[PATCH] Warning on unevaluated expression contexts with side effects
Richard Smith
richard at metafoo.co.uk
Wed Dec 17 11:49:05 PST 2014
@@ -3009,7 +3026,7 @@
const CastExpr *CE = cast<CastExpr>(this);
if (CE->getCastKind() == CK_LValueToRValue &&
CE->getSubExpr()->getType().isVolatileQualified())
- return true;
+ return IncludePossibleEffects;
break;
}
A volatile load is always a side-effect.
@@ -3022,7 +3039,7 @@
case CXXTemporaryObjectExprClass: {
const CXXConstructExpr *CE = cast<CXXConstructExpr>(this);
if (!CE->getConstructor()->isTrivial())
- return true;
+ return IncludePossibleEffects;
// A trivial constructor does not add any side-effects of its own.
Just look
// at its arguments.
break;
This should be
if (IncludePossibleEffects)
return true;
There are a bunch of cases under
// FIXME: Classify these cases better.
These are all "don't know" cases, and so should say something like:
if (IncludePossibleEffects)
return true;
break;
rather than just
return true;
(You can then put ObjCMessageExprClass and ObjCPropertyRefExprClass back
where they were.)
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.
On Mon, Dec 15, 2014 at 6:57 AM, Aaron Ballman <aaron at aaronballman.com>
wrote:
> Ping
>
> On Thu, Dec 4, 2014 at 10:35 AM, Aaron Ballman <aaron at aaronballman.com>
> wrote:
> > On Wed, Dec 3, 2014 at 2:40 PM, Richard Smith <richard at metafoo.co.uk>
> 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 <aaron at aaronballman.com>
> >> wrote:
> >>>
> >>> On Mon, Dec 1, 2014 at 4:39 PM, Aaron Ballman <aaron at aaronballman.com>
> >>> wrote:
> >>> > On Mon, Dec 1, 2014 at 2:04 PM, 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.
> >>> >
> >>> > 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20141217/64d57f5c/attachment.html>
More information about the cfe-commits
mailing list