[cfe-dev] checkers: decoupling ValueManager from GRStateManager

[Ted Kremenek]

Hi Olaf,

The point of SValuator is to build about symbolic expression values.  It is designed to have a virtual interface so that we have the flexibility to change how much symbolic reasoning we want to handle, and give us a transparent way to "upgrade" the symbolic analysis by vending a virtual interface to symbolic expression construction.

ValueManager was added as a handy class so that checkers had a uniform place to construct certain kinds of SVals, as well as have a common place where the lifetime of certain SVal objects were managed.  This could be incorporated directly into SValuator (and maybe it should), but the idea was to keep ValueManager simple (and non-virtual) while keeping the virtual (extensible) parts of symbolic expression construction in SValuator.

I'm fundamentally opposed to several of the changes you are suggesting (more comments inline), but I do agree with your overall motivation.  I think a good first step might be to better define the roles of ValueManager and SValuator.  ValueManager should be used for basic value construction and management, and SValuator for more advanced things.  In particular, methods in ValueManager that currently rely on SValuator could probably just be moved to SValuator (e.g., convertToArrayIndex()).  That would resolve most of the issues that you are seeing.

For some historical context, libChecker previously was libAnalysis, but the two were split apart because libAnalysis is now used by the frontend for some compiler warnings.  We didn't want the compiler itself to have to depend on including the entire static analyzer core, so libAnalysis is now intended to include the common analysis components that would be used by the frontend and the static analyzer.  With this in mind, would the goal be to move ValueManager to libAnalysis?  I'm curious about what pieces you'd like to reuse from ValueManager with other dataflow analyses?  Currently ValueManager uses the SymbolManager as well as the MemRegion abstraction.  Both of these are fairly tied to the path-sensitive engine, and likely need to stay in libChecker.

On Oct 25, 2010, at 9:01 AM, Olaf Krzikalla wrote:

> Hi @clang,
> 
> I'm trying to decouple ValueManager and some other classes from the GRState machinery. The goal is to make these classes reusable in a dataflow analysis framework. I've attached a patch with the sketch of a first step (it compiles fine, but shall serve as a reference only).
> The most problematic thing is the placement of SValuator. Currently ValueManager holds it but I moved it to GRStateManager. Thus ValueManager can exist independently from GRStateManage and even StoreManager. In turn the SValuator now needs a StoreManager in addition to the ValueManager for construction.
> But neither SValuator nor the classes derived from StoreManager need the GRStateManager anymore. Unfortunately in this first step a lot of GRState* pointers still appear in the interfaces of StoreManager and SValuator. However apparently some comments suggest that this will be changed anyway (e.g. StoreManager::getSizeInElements, StoreManager::CastResult was unused).
> I had to made one functional change tough. I moved the body of SimpleSValuator::EvalCastNL/EvalCastL to ValueManager::castNonLoc/castLoc thus making these functions not longer configureable.
> The questions are:
> 1. Is it OK to essentially make SimpleSValuator::EvalCastNL/EvalCastLnon-virtual?

Absolutely not.  These needs to stay virtual (see my second comment).

> 2. What is the point of the SValuator class at all? Can/should it be removed? The interface and implementation can become a part of StoreManager.

SValuator should not be removed, and it should not become a part of StoreManager.  They are orthogonal concepts.

StoreManager is used for managing bindings between locations and values.  SValuator is used for building SVal expressions.  These are intentionally decoupled tasks so that one can engineer a new StoreManager (or SValuator) without worrying about implementing both tasks.  They have virtual interfaces so that we can incrementally (and transparently) slide in new implementations of these interfaces that provide improved analysis precision, but through well-defined and thin APIs.

> 3. Is it possible to remove the GRState* from the SValuator and the StoreManager interfaces alltogether?

We've been gradually moving in that direction, and it may be possible at this point.  That said, I think should be treated as an orthogonal problem from decoupling ValueManager from these classes.  Both StoreManager and SValuator play key roles in the path-sensitive engine.  If we can simplify them to not use GRState*, that's great, but afterwards we shouldn't be prohibited from adding this back into their interfaces in the future should the need arise.

> 4. In which direction is the checkers framework currently developed?

I need a little more context to understand this (fairly open-ended) question.  What do you mean by the "checkers framework?"  Are you referring to the Checker library?