[PATCH] Move some code into GRSimpleVals

Clang Frontend
1

To make GRExprEngine unaware of the specific implementation of GRStates and Stores, this patch moves some code from GRExprEngine into GRSimpleVals. This is an initial refactoring. It moves some code in GRExprEngine::getInitialState() into GRSimpleVals::getInitialDeclState().

Zhongxing Xu

grsimplevals.patch (4.08 KB)

2

Hi Zhongxing,

This is an excellent idea, but I don't think putting this logic in GRSimpleVals is the right place. This is really a property of the particular implementation of Store, since that handles the abstraction of "memory". Specifically, this logic seems tied to BasicStore and BasicStoreManager respectively.

I think the most logical place would be to put it in BasicStoreManager's implementation of "getInitialStore()". We can either pass LiveVariables& to getInitialStore(), or pass GRStateManager& to getInitialStore() (and migrate the LiveVariables& instance variable from GRExprEngine to GRStateManager).

Ted

3

This is an excellent idea, but I don’t think putting this logic in GRSimpleVals is the right place. This is really a property of the particular implementation of Store, since that handles the abstraction of “memory”. Specifically, this logic seems tied to BasicStore and BasicStoreManager respectively.

I think the most logical place would be to put it in BasicStoreManager’s implementation of “getInitialStore()”. We can either pass LiveVariables& to getInitialStore(), or pass GRStateManager& to getInitialStore() (and migrate the LiveVariables& instance variable from GRExprEngine to GRStateManager).

Ted

Done.

This patch extends BasicStoreManager::getInitialStore() to include code that symbolicates input variables.
It has two simplifications from the original code:

  1. Local variables are not handled, since they are handled in GRExprEngine::VisitDeclStmt().
  2. Removes redundant handle of ImplicitParamDecl, since it is a subclass of VarDecl.

getInitialStore.patch (7.21 KB)

4

Hi Zhongxing,

This patch looks really awesome. The only problem I see is that VisitDeclStmt doesn't handle the fact that a local variable is uninitialized before VisitDeclStmt is called (this poses a problem because the Expr* for the initializer is visited before the DeclStmt). That's why local variables are initialized to UndefinedVal. Observe that the test case Analysis/uninit-ps-rdar6145427.m fails because of this change (run 'make test').

I think the solution is to leave in:

       RVal X = (VD->hasGlobalStorage() || isa<ParmVarDecl>(VD) ||
                 isa<ImplicitParamDecl>(VD))
              ? RVal::GetSymbolValue(SymMgr, VD)
              : UndefinedVal();

instead of:

         // Only handle globals and parameters here. Local variables are handled
         // in VisitDeclStmt().
         if (VD->hasGlobalStorage() || isa<ParmVarDecl>(VD) ||
             isa<ImplicitParamDecl>(VD)) {
           RVal X = RVal::GetSymbolValue(StateMgr.getSymbolManager(), VD);
           St = SetRVal(St, lval::DeclVal(VD), X);
         }

You're right that the following is dead code and should be removed:

     } else if (ImplicitParamDecl *IPD = dyn_cast<ImplicitParamDecl>(SD)) {
         RVal X = RVal::GetSymbolValue(SymMgr, IPD);
       StateMgr.SetRVal(StateImpl, lval::DeclVal(IPD), X);
     }

Ted

5

This patch looks really awesome. The only problem I see is that VisitDeclStmt doesn’t handle the fact that a local variable is uninitialized before VisitDeclStmt is called (this poses a problem because the Expr* for the initializer is visited before the DeclStmt). That’s why local variables are initialized to UndefinedVal. Observe that the test case Analysis/uninit-ps-rdar6145427.m fails because of this change (run ‘make test’).

I think the solution is to leave in:

RVal X = (VD->hasGlobalStorage() || isa(VD) ||
isa(VD))
? RVal::GetSymbolValue(SymMgr, VD)
: UndefinedVal();

instead of:

// Only handle globals and parameters here. Local variables are handled
// in VisitDeclStmt().
if (VD->hasGlobalStorage() || isa(VD) ||
isa(VD)) {
RVal X = RVal::GetSymbolValue(StateMgr.getSymbolManager(), VD);
St = SetRVal(St, lval::DeclVal(VD), X);
}
Ted

New patch is attached.

getInitialStore2.patch (7.25 KB)