Page MenuHomePhabricator

[analyzer] Don't communicate evaluation failures through memregion hierarchy.
ClosedPublic

Authored by NoQ on Jan 23 2018, 6:36 PM.

Details

Summary
  • If we are unable to determine a good target region for a C++ constructor call, we come up with a temporary region and construct into that.
  • If we suddenly notice that our constructor is part of an C++ array construction (operator new[] or an array-typed local variable), we come up with the element-zero region and construct into that, same for destructors.

Both are bad not only because we fail to model stuff correctly, but also because we use region structure to communicate the fact that we failed to ExprEngine::evalCall, which would later decide if we should inline the call or not by looking at the region structure. The former is particularly bad because this prevents us from supporting the actual construction into temporaries - even if we produce a correct temporary region, we won't inline the constructor because temporary region is an (incorrect) indication of failure.

In order to unblock the progress in this area, this patch adds an auxiliary flag for evalCall's clients to communicate potential problems with the call. Then evalCall would decide if it should still inline the call. Apparently, the current decision process is non-trivial in this case: for instance, we may still inline the constructor even if the target region is incorrect aka temporary, when the class's destructor is trivial. In any case, it's good to have all the when-to-inline heuristics in one place.

For now ExprEngine::evalCall itself doesn't have a flag - only ExprEngine::defaultEvalCall does. Because for now it's only relevant to constructor/destructor calls, but checker-side evalCall only works for simple call expressions - that's a separate issue. Ideally the flag should go there as well, and defaultEvalCall would only be called from ExprEngine::evalCall.

Functional change here is accidental - by communicating array destructor situation properly, we're able to fix an old FIXME.

Diff Detail

Repository
rL LLVM

Event Timeline

NoQ created this revision.Jan 23 2018, 6:36 PM
include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
570 ↗(On Diff #131184)

Those are not really flags. Perhaps options?

572 ↗(On Diff #131184)

OK my C++ knowledge is weak here.
What happens if you don't initialize those at the callsite and then read? Wouldn't be safer to set them both to false in the declaration?

661 ↗(On Diff #131184)

Which flag?

lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
92 ↗(On Diff #131184)

Doxygen comment for out-parameter? E.g. it's non-obvious that it only sets the flag to true and does not touch it otherwise.

94 ↗(On Diff #131184)

Perhaps IsArray will also do?

142 ↗(On Diff #131184)

Or I suppose you could do makeZeroElementRegion(..., &Flags.IsArrayConstructorOrDestructor) to express the intent

171 ↗(On Diff #131184)

Same as previous comment, I suppose you could write into IsArrayConstructorOrDestructor directly.

451 ↗(On Diff #131184)

Again perhaps write into Flags.Is... directly?

NoQ updated this revision to Diff 131321.Jan 24 2018, 11:18 AM
NoQ marked 8 inline comments as done.

Address comments.

include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
572 ↗(On Diff #131184)

Yeah, i guess i'd add a constructor. Unfortunately inline initialization for bitfields is not available until C++20. So i hope we'd be able to get rid of this construct before we switch to C++20 =)

661 ↗(On Diff #131184)

The respective flag. To be exact, the IntentionallyLongTemporaryFlagNameThatNobodyWouldEverBotherToReadCorrectly one :)

Fxd thx :)

dcoughlin accepted this revision.Jan 31 2018, 6:15 PM

This looks good to me. However, I do think you should take George's suggestion to have makeZeroElementRegion() have a boolean out parameter rather than a EvalCallOptions out parameter. This avoids unnecessarily coupling of makeZeroElementRegion() and EvalallOptions. You will need to make the boolean fields not bitfields (since we can't take a bitfield's address in C++). But EvalCallOptions is only on the stack (and we can pass it by const reference) so I don't think the increased size is something we should worry about.

This revision is now accepted and ready to land.Jan 31 2018, 6:15 PM
NoQ updated this revision to Diff 132450.Feb 1 2018, 12:16 PM

Switched to plain bools and in-class initializers.

This revision was automatically updated to reflect the committed changes.
NoQ added a comment.Feb 6 2018, 3:17 PM

Functional change here is accidental - by communicating array destructor situation properly, we're able to fix an old FIXME.

Minor temporary insanity. This test was "fixed" because in mayInlineCall() for destructors i started to look for the flag that i never set for destructors :/