Page MenuHomePhabricator

[Analyzer] [NFC] Parameter Regions
ClosedPublic

Authored by baloghadamsoftware on May 25 2020, 8:31 AM.

Details

Summary

Currently, parameters of functions without their definition present cannot be represented as regions because it would be difficult to ensure that the same declaration is used in every case. To overcome this, we split VarRegion to two subclasses: NonParamVarRegion and ParamVarRegion. The latter does not store the Decl of the parameter variable. Instead it stores the index of the parameter which enables retrieving the actual Decl every time using the function declaration of the stack frame. To achieve this we also removed storing of Decl from DeclRegion and made getDecl() pure virtual. The individual Decls are stored in the appropriate subclasses, such as FieldRegion, ObjCIvarRegion and the newly introduced NonParamVarRegion.

Diff Detail

Event Timeline

This is an alternative approach to D79704. The advantage is that we must not duplicate code everywhere where VarRegion (and sometimes also DeclRegion) is used. The drawback is that after we implement handling of parameters of functions without Decl (paramters of unknown functions passed by pointer) the getDecl() method may return nullptr. We must handle this case everywhere in the code where VarRegion or DeclRegion is used and parameters of such functions may appear. (We should do this in the original apprach as well, but there it is more difficult to forget such a place because ParamRegion is used explicitly. Furthermore, where handling of ParamRegion is completely forgotten we usually do not crash, but if we forget to check the result of getDecl() for nullptr we get null pointer dereferences.)

NoQ added a comment.May 26 2020, 8:54 AM

Nice, looks like you managed to reuse most of the code. I still feel like we should ditch DeclRegion entirely (forcing its ~5 current users consider its specific variants separately) but i don't insist.

clang/include/clang/StaticAnalyzer/Checkers/SValExplainer.h
233–234

There's a standard function for that. I never remember what it's called, something something numeric something plural something suffix, but it's there.

clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
539–540 ↗(On Diff #266028)

Why did you relax this cast<> to dyn_cast<>?

clang/lib/StaticAnalyzer/Core/MemRegion.cpp
181

It will eventually have to. Maybe `"...not implemented yet"?

Updated according to the comments.

Wrong diff uploaded previously, now fixed.

baloghadamsoftware marked 4 inline comments as done.May 26 2020, 12:41 PM
In D80522#2055040, @NoQ wrote:

Nice, looks like you managed to reuse most of the code. I still feel like we should ditch DeclRegion entirely (forcing its ~5 current users consider its specific variants separately) but i don't insist.

Thank you for you comments. I will check tomorrow whether we could remove DeclRegion without too much code duplication.

clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
539–540 ↗(On Diff #266028)

I started from the original approach and forgot to reset this file.

balazske added inline comments.May 27 2020, 4:02 AM
clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
917

This assert should not be duplicated. It has the place in VarRegion or NonParamVarRegion, but not both.

976

typo here: "paremters"

978

"paremeters"

979

NonParamVarRegions ?

baloghadamsoftware marked an inline comment as done.

Updated according to the comments of @balazske.

baloghadamsoftware marked 5 inline comments as done.May 28 2020, 6:31 AM
baloghadamsoftware added inline comments.
clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
917

Copy-paste...

baloghadamsoftware marked an inline comment as done.May 28 2020, 6:35 AM

I will check tomorrow whether we could remove DeclRegion without too much code duplication.

It turns out that DeclRegion is used in 7 different checkers plus in BugReporterVisitors. One typical use case is to get a name for the region (by getting the Decl and trying to cast it to NamedDecl), but there are also others as well. For now I suggest to keep it to avoid code duplication.

NoQ accepted this revision.Jun 1 2020, 4:53 AM

Looks great, thanks! I like how it turned out, i guess let's prefer this to D79704.

clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
921

What's the point of overriding one pure virtual method with another pure virtual method?

984

*unknown* function pointer!

This revision is now accepted and ready to land.Jun 1 2020, 4:53 AM
baloghadamsoftware retitled this revision from [Analyzer] [NFC] Parameter Regions -- Alternative Approach to [Analyzer] [NFC] Parameter Regions.Jun 1 2020, 10:41 PM
baloghadamsoftware marked 2 inline comments as done.Jun 3 2020, 4:01 AM
baloghadamsoftware added inline comments.
clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
921

To constrain the return value from Decl to VarDecl. Without this we have change all the callers to cast the return value from this function to VarDecl. It would be ugly.

NoQ added inline comments.Jun 3 2020, 4:35 AM
clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
921

Aha, covariance! Nice!

NoQ added inline comments.Jun 8 2020, 5:13 AM
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
404–409

I guess we should document the landmine discussed in D80366#inline-747804.

This revision was automatically updated to reflect the committed changes.

This introduced a compiler warning:

/Users/teemperor/1llvm/llvm-project/clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:1102:23: warning: 'getDecl' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]
  const ObjCIvarDecl *getDecl() const;
                      ^
/Users/teemperor/1llvm/llvm-project/clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:898:28: note: overridden virtual function is here
  virtual const ValueDecl *getDecl() const = 0;
                           ^

This introduced a compiler warning:

/Users/teemperor/1llvm/llvm-project/clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:1102:23: warning: 'getDecl' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]
  const ObjCIvarDecl *getDecl() const;
                      ^
/Users/teemperor/1llvm/llvm-project/clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:898:28: note: overridden virtual function is here
  virtual const ValueDecl *getDecl() const = 0;
                           ^

Thx! Fixed now!