This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] Enable constructor support in evalCall event
ClosedPublic

Authored by vrnithinkumar on Jun 20 2020, 4:43 AM.

Details

Summary

Adding CXX constructor calls support with evalCall event

  • Made changes to pass EvalCallOptions via runCheckersForEvalCall into defaultEvalCall.
  • Added tests to verify the order.
  • Updated the AnalysisOrderChecker to support evalCall for testing

Diff Detail

Event Timeline

vrnithinkumar created this revision.Jun 20 2020, 4:43 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 20 2020, 4:43 AM
vrnithinkumar retitled this revision from Enabling ctr in evalCall event to [analyzer] Enabling ctr in evalCall event.Jun 20 2020, 4:48 AM
vrnithinkumar edited the summary of this revision. (Show Details)
vrnithinkumar marked an inline comment as done.Jun 20 2020, 4:52 AM
vrnithinkumar added inline comments.
clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
100 ↗(On Diff #272251)

Moved the struct EvalCallOptions outside ExprEngine class.
Since CheckerManager.h using forward declaration of ExprEngine and inner type EvalCallOptions is needed for the runCheckersForEvalCall function declaration.

NoQ accepted this revision.Jun 20 2020, 8:57 AM

Fantastic, thank you!

clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
586 ↗(On Diff #272251)

If you want you can make it an optional argument of runCheckersForEvalCall(), like it's done in defaultEvalCall().

clang/test/Analysis/cxxctr-evalcall-analysis-order.cpp
34

No NEwLiNE aT EnD Of FiLE

clang/test/Analysis/new-ctor-conservative.cpp
30 ↗(On Diff #272251)

Thx!

This revision is now accepted and ready to land.Jun 20 2020, 8:57 AM
vrnithinkumar marked an inline comment as done.

Addressing review comment adding miised new line

vrnithinkumar marked 2 inline comments as done.Jun 20 2020, 4:02 PM
vrnithinkumar added inline comments.
clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
586 ↗(On Diff #272251)

I tried to make it as default argument for runCheckersForEvalCall() but struct EvalCallOptions is forward declared in CheckerManager.h.

clang/test/Analysis/cxxctr-evalcall-analysis-order.cpp
34

added new line

vrnithinkumar marked an inline comment as done.

Fixing wrongly deleted the old commit via arc

NoQ added a comment.Jun 21 2020, 2:07 AM

Hmm, given that you're using ARC, you're probably intending to turn the review title/summary into a commit message. Let's make it prettier then:

  • Commit messages are typically written in imperative mood, eg. "Enabling" -> "Enable", "Adding"/"Added" -> "Add".
  • Abbreviating "constructor" to "ctr" was really unnecessary :)
clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
586 ↗(On Diff #272251)

Oh well! You probably still don't need a separate local variable, can you try EvalCallOptions() or even {}?

Szelethus accepted this revision.Jun 21 2020, 4:40 AM
Szelethus added inline comments.
clang/lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp
130 ↗(On Diff #272276)

This seems a bit cryptic, how about {argno:?

Fixing test failures

vrnithinkumar marked 5 inline comments as done.Jun 24 2020, 11:58 AM
vrnithinkumar added inline comments.
clang/lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp
130 ↗(On Diff #272276)

For more clarity, added "{argno:".

clang/lib/StaticAnalyzer/Core/CheckerManager.cpp
669 ↗(On Diff #273117)

This is for fixing the unit test failures.
Previously assert to check the type inside cast<CallExpr> was causing the unit test failures.
The reason was CXXConstructExpr was not inherited from CallExpr and the cast was causing the assert failure with isa check.

I am not sure removing the cast is the best solution.
And the TODO comment, I did not understood properly.

Alternative approach was to cast it to CXXConstructExpr if it is not CallExpr but not sure whether runCheckersForEvalCall should aware of CXXConstructExpr.

clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
586 ↗(On Diff #272251)

Updated to use EvalCallOptions()

vrnithinkumar marked 2 inline comments as done.Jun 24 2020, 12:28 PM
vrnithinkumar retitled this revision from [analyzer] Enabling ctr in evalCall event to [analyzer] Enable constructor support in evalCall event.

clang-format fix

vrnithinkumar marked 2 inline comments as done.Jun 24 2020, 2:27 PM
NoQ accepted this revision.Jun 24 2020, 4:42 PM

Thanks! I'll commit.

clang/lib/StaticAnalyzer/Core/CheckerManager.cpp
669 ↗(On Diff #273117)

Wait, was it that easy?

Yeah, looks like the cast is indeed unnecessary. Somebody probably meant something when they put it here but they didn't bless us with a comment and i don't see any immediate reasons why would this code require a CallExpr specifically.

The TODO above is about supporting the situation when there is no expression *at all*. Like, CXXConstructExpr is still *something*, but in some cases the call isn't a result of evaluating any expression at all. One of the most common examples of such calls is destructor calls. Say, automatic destructors of local variables get triggered at the end of scope rather than at an expression. We'll have to get back to it when we evalCall() a destructor :)

NoQ added a comment.Jun 24 2020, 9:11 PM

@vrnithinkumar what's your preferred Full Name <email> for llvm's git?

In D82256#2113233, @NoQ wrote:

@vrnithinkumar what's your preferred Full Name <email> for llvm's git?

Nithin Vadukkumchery Rajendrakumar <vrnithinkumar@gmail.com>

This revision was automatically updated to reflect the committed changes.