This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] Be more careful about C++17 copy elision.
ClosedPublic

Authored by NoQ on Mar 23 2018, 4:46 PM.

Details

Summary

Just wanted to play safer in a couple of places.

In D44597 i said:

If the object has trivial destructor then CXXBindTemporaryExpr is not there, and the AST is pretty much indistinguishable from the simple case so we re-use SimpleVariableConstructionContext and SimpleReturnedValueConstructionContext. I'm not entirely sure that this is indeed the same case, but for the purposes of the analyzer, which is the primary user of construction contexts, this seems fine because when the object has trivial destructor it is not scary to inline the constructor.

Well, this is actually an easy question. There certainly are cases where the AST is quite distinguishable, eg. ternary conditional operator. However, because construction contexts are implemented as a whitelist of possible ASTs that we support, it's easy to see that the ternary operator is in fact the only case. The patch suppresses construction contexts in this case for now, because the example from http://lists.llvm.org/pipermail/cfe-dev/2018-March/057357.html made me nervous (even though this particular example works well when there are no destructors involved, so this is a speculative play-safe thingy without an actual analyzer-based test).

Also raise the improperly-constructed flag for the return value construction context into a non-temporary. This also has no effect on the analyzer because we'd inline the constructor anyway. Additionally i added a test case to demonstrate how this is broken.

Diff Detail

Repository
rL LLVM

Event Timeline

NoQ created this revision.Mar 23 2018, 4:46 PM
NoQ updated this revision to Diff 140167.Mar 28 2018, 5:48 PM

Conditional operators, like call-expressions in D44273, may also be glvalues of record type instead of simply being reference-typed.

NoQ updated this revision to Diff 140168.Mar 28 2018, 5:49 PM

Fix a comment in tests.

LGTM provided comments are answered. Field rename would be appreciated, if possible.

lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
215 ↗(On Diff #140168)

Is this field false by default? Also, double negation is harder to read (e.g. not modelled properly = false vs. modelled property = true), but I guess that should have been said earlier.

This revision is now accepted and ready to land.Mar 30 2018, 10:48 AM
NoQ added inline comments.Mar 30 2018, 11:55 AM
lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
215 ↗(On Diff #140168)

Yeah, flags within EvalCallOptions are all "red" flags to warn evalCall() that something is fishy about the call. They're all off by default. I'm not sure what's the best way to change that. I don't want each branch to set like 4 different flags, so they should be non-"red" by default.

This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.