Add the way to extract SVals of arguments used in a call for a given StackFrameCtx

[k-wisniewski]

Summary

his patch adds getArgsSVal method to ProgramState that allows the user to obtain SVals of argumetns used in a call that created the given StackFrameCtx. The problem with arguments SVals being overwritten has been fixed according to @NoQ 's suggestion from the previous revision of this patch (https://reviews.llvm.org/D26760)

[zaks.anna]

Hi!

Looks like this this is used by the Infinite recursion checker. Specifically, the checker not only needs to get Smalls for arguments of the current CallEvent, but it also looks for arguments of other calls on the stack. The checker walks the LocationContext and uses this new API to look up the arguments passed to the calls on the stack.

The two concerns I have with this patch is that it is extending the overloaded ProgramState API and also that this does not fully cover all the call types we have in C/ObjC/C++. The good news is that the CallEvent API was created specifically to encapsulate this complexity and it already contains the needed API to look up the SVal of the argument. Have you considered creating CallEvent for the calls on the stack and using that existing API to find the Smalls of the arguments.

Here is some documentation from CallEvent.h:
`
/ CallEvents are created through the factory methods of CallEventManager.
/
/ CallEvents should always be cheap to create and destroy. In order for
/ CallEventManager to be able to re-use CallEvent-sized memory blocks,
/ subclasses of CallEvent may not add any data members to the base class.
/ Use the "Data" and "Location" fields instead.

`

It looks like this code from BugReporterVisitors.cpp is doing something similar:

` // Don't automatically suppress a report if one of the arguments is
 // known to be a null pointer. Instead, start tracking /that/ null
 // value back to its origin.
 ProgramStateManager &StateMgr = BRC.getStateManager();
 CallEventManager &CallMgr = StateMgr.getCallEventManager();

 ProgramStateRef State = N->getState();
 CallEventRef<> Call = CallMgr.getCaller(StackFrame, State);
 for (unsigned I = 0, E = Call->getNumArgs(); I != E; ++I) {
   Optional<Loc> ArgV = Call->getArgSVal(I).getAs<Loc>();
   if (!ArgV)
     continue;

`

I hope this approach works and you could just add that code to your checker. (If not, we can investigate other options.)

[NoQ]

Hmm, i'm thinking of just the opposite - refactor the CallEvent methods to use the respective new ProgramState methods.

This way we'd avoid touching the CallEvent allocator for a simple Environment lookup, while still avoiding code duplication.

[zaks.anna]

Hmm, i'm thinking of just the opposite - refactor the CallEvent methods to use the respective new ProgramState methods.

This way we'd avoid touching the CallEvent allocator for a simple Environment lookup, while still avoiding code duplication.

I see what you are suggesting, but I still have the concerns I've mentioned in my first comment. The whole purpose of CallEvent is to allow an easy and lightweight way of dealing with all the different types of calls, which is exactly what we are doing in this checker. CallEvent is used for this exact purpose elsewhere throughout the analyzer. What do you mean by "touching the CallEvent allocator" and why would you want to discourage that?

CallEventManager::getCaller contains logic to dispatch to different CallEvent types depending on what call we are dealing with. The argument lookup will depend on type of the call. The method added in this PR does not deal with all call types, but only with CallExpr, so you'd need to duplicate the code from CallEvent construction to here.

As I've mentioned before, I do not think that Call dispatch logic and API for looking up arguments SVals should belong to ProgramState as it is a higher abstraction and should not reason about LocationContexts or Calls (which are part of Program Location and AST respectively). In addition, using CallEven to look up argument values is much simpler to understand for API users.

[zaks.anna]

Artem just pointed out that I have "Smalls" instead of "SVals" in my first comment.

[NoQ]

Chatted about this a bit more. Because CallEvent is an API designed mostly for convenience, and ProgramState is a core API that needs to stay as clean and separate from other entities and easy to understand as possible, it would be the best to

1. construct a CallEvent in the checker (which is cheap) and query that,
2. and also allow constructing CallEvents for the top frame (with StackFrameContext::inTopFrame(), with null call site ("origin") expression), and
3. add a branch to the CallEvent::getArgSVal() method (and similar methods for obtaining implicit arguments like C++ this, ObjC self, ObjC super) to handle the top frame case.

If this sounds hard, handling this completely on the checker side is fine, because few checker are using this at the moment.

[zaks.anna]

Update: Adding support for top frame to the CallEvent is difficult, so let's just use CallEvent API in the checkers for all frames but the top one and have custom top frame handling in the Recursion checker itself. The top frame handling does not need to be complete (handle all call types) since in the worst case we will be missing bugs (no false positives due to this).

include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h

public:
   SVal getLValue(const FieldDecl *decl, SVal Base) const;
 
   /// Get the lvalue for an indirect field reference.
   SVal getLValue(const IndirectFieldDecl *decl, SVal Base) const;
 
   /// Get the lvalue for an array index.
   SVal getLValue(QualType ElementType, SVal Idx, SVal Base) const;
 
+  /// Get the symbolic value of arguments used in a call
+  /// that created the given stack frame
+  SVal getArgSVal(const StackFrameContext *SFC, const unsigned ArgIdx) const;
+
   /// Returns the SVal bound to the statement 'S' in the state's environment.
   SVal getSVal(const Stmt *S, const LocationContext *LCtx) const;
   
   SVal getSValAsScalarOrLoc(const Stmt *Ex, const LocationContext *LCtx) const;
 
   /// \brief Return the value bound to the specified location.
   /// Returns UnknownVal() if none found.
   SVal getSVal(Loc LV, QualType T = QualType()) const;

 }
 
 inline SVal ProgramState::getLValue(QualType ElementType, SVal Idx, SVal Base) const{
   if (Optional<NonLoc> N = Idx.getAs<NonLoc>())
     return getStateManager().StoreMgr->getLValueElement(ElementType, *N, Base);
   return UnknownVal();
 }
 
+inline SVal ProgramState::getArgSVal(const StackFrameContext *SFC,
+                                     const unsigned ArgIdx) const {
+  const FunctionDecl *FunctionDecl = SFC->getDecl()->getAsFunction();
+  const ObjCMethodDecl *MethodDecl =
+      dyn_cast_or_null<ObjCMethodDecl>(SFC->getDecl());
+  unsigned NumArgs = FunctionDecl
+                     ? FunctionDecl->getNumParams()
+                     : MethodDecl->getSelector().getNumArgs();
+  assert(ArgIdx < NumArgs && "Arg access out of range!");
+
+  if (SFC->inTopFrame()) {
+    // if we are in the top frame we don't have any arguments bound in
+    // the environment because the call wasn't modeled in the first place.
+    const VarDecl *ArgDecl = FunctionDecl
+                             ? FunctionDecl->parameters()[ArgIdx]
+                             : MethodDecl->parameters()[ArgIdx];
+    const Loc ArgLoc = getLValue(ArgDecl, SFC);
+    StoreManager &storeMgr = stateMgr->getStoreManager();
+    Store initialStore = storeMgr.getInitialStore(SFC).getStore();
+    return storeMgr.getBinding(initialStore, ArgLoc);
+  } else {
+    // in this case we need to ask the environment as  the arguments' memory
+    // region may have been purged as no longer needed.
+    const Stmt *callSite = SFC->getCallSite();
+    const CallExpr *callSiteExpr = dyn_cast<CallExpr>(callSite);
+    const Expr *argExpr = callSiteExpr->getArg(ArgIdx);
+    return getSVal(argExpr, SFC->getParent());
+  }
+}
+
 inline SVal ProgramState::getSVal(const Stmt *Ex,
                                   const LocationContext *LCtx) const{
   return Env.getSVal(EnvironmentEntry(Ex, LCtx),
                      *getStateManager().svalBuilder);
 }
 
 inline SVal
 ProgramState::getSValAsScalarOrLoc(const Stmt *S,