[cfe-dev] checkers: decoupling ValueManager from GRStateManager

[Ted Kremenek]

On Oct 27, 2010, at 9:45 AM, Olaf Krzikalla wrote:

> Am 26.10.2010 18:52, schrieb Ted Kremenek:
>> I think it might be easier for me to see where this is going if I had an
>> idea of what you *don't* want to use.
> Maybe that's indeed the right question. To start again: my only goal is to know if two expressions refer to the same memory location (with former assignments taken into consideration). The memory location is represented by a MemRegion and my reasoning engine is already based on SVals.

Right.  So the "data" pieces you need are MemRegions, SVals, symbols (which are used by the first two).  You also need all the "builder" pieces for these objects.

> However I reason over basic blocks only (only parts of a function) and I want to control which statements are analyzed.

Sure.  You basically want to substitute GRExprEngine with something else.

> Of course my reasoning holds an internal state too, which holds the binding of SVals to MemRegions. That said, I have no problem with GRState as it is. Indeed, this and StoreManager provides some functionality which I currently have implemented by my own (but like to get rid off).

Okay.  Makes sense.

> A fundamental difference is that I actually have only one state (which changes or gets extended when a new assignment is evaluated). Thus I don't need all the ExplodedNode stuff of GRExprEngine.

Makes sense.

> Another difference is the overall purpose of my analysis: it is not for bug reporting, but for code optimization. Thus I don't need the diagnostic part of AnalysisManager.

Okay, that's a good point.  AnalysisManager can possibly be refactored into parts that care just about "analysis" and those about bug-finding.

> Unfortunately it is currently not possible to use only some parts of the GR framework. If you stick on GRState, you need GRStateManager, which in turn needs GRSubEngine, which eventually needs an AnalysisManager, which I don't want to use in its current form.

Makes sense.

First, I'd like to double GRStateManager from GRSubEngine.  That coupling seems wrong anyway, and I'll look into breaking that dependency.

Concerning AnalysisManager, I think it's realistic for me to look into separating that into pieces needed just for raw analysis, and those for bug finding.  I need to think about this a bit more.

> In addition, I initially thought that I can control the generation of ExplodedNodes in GRExprEngine so, that there is always only one successor node representing the actual state. But that attempt failed too. Thus I have mentally abandoned the GR* framework.

Makes sense.  It's not the right tool for your task.

> However I still have hope that I can use other parts like the building of SVals from binary expressions as it is done in SValuator.
> Maybe I should just left the triumvirate of ValueManager, SValuator and StoreManager to your disposal and just introduce an abstraction layer for GRState. A very quick check shows, that the only functions of GRState used by StoreManager and SValuator are getStore, getSymVal and getSVal. Introducing an ABC as base class of GRState
> could decouple the three classes (and their subclasses) from the GREngine (and ConstraintManager too). Of course I could go even one step higher and try to decouple GRStateManager from GRSubEngine (or change GRSubEngine).
> The bottom line is that I actually have no idea where I can or shall make the cut between the actual GR framework and the underlying helper classes. You claim that it is all tightly coupled.

I think a reasonable starting point is to try and sever GRStateManager from GRSubEngine.

A follow-on simplification may be to eliminate ValueManager entirely, since that functionality probably just belongs in SValuator (and you voiced interest in re-using SValuator yourself).

> 
> > Currently GRState* is passed to SValuator because
>> SimpleSValuator uses methods from ConstraintManager (which it can
>> reference via a GRState object).
> I wouldn't go so far to say that SValuator uses ConstraintManager. It just uses 'any' method to retrieve the integer value of a symbol.

The integer value of a symbol is defined by the constraints on that value.  That's why ConstraintManager is involved.

That said, we can possibly factor this better, lifting the assumption out of SValuator that it is talking to ConstraintManager.

> 
>> In particular,
>> SimpleSValuator uses ConstraintManager::getSymVal(), which is a virtual
>> method whose result depends on the current GRState. Thus I'm not really
>> certain this dependency can be severed unless we sever the dependency
>> between SValuator and ConstraintManager.
> But I don't see why it is bad to sever this dependency?

I think the dependency can be severed only if we replace it with something more generic.  For example, GRExprEngine passes an object to SValuator (or provides one when SValuator is created) that gives the SValuator a little more context on how to perform these queries.  This would be a layer of indirection, allowing the SValuator to talk to the ConstraintManager, but not assume that is what it is doing.  In this way, GRExprEngine can continue to use SValuator in a way more optimal for it and other clients (such as your code) can use SValuator without it depending on ConstraintManager.

I'll need to think about the right APIs and abstraction here, as it is an important refactoring.

> > Nobody else aside
>> from you has expressed interest in reusing these components in a
>> different context, so you will probably need to drive this work.
> Several month ago someone asked about taint analysis, subject was "Help with Taint analysis".

That's a fair point, but that conversation never when to this level of detail about the use of specific pieces of the libChecker library.

> 
>> since it seems you are interested in using both SValuator and the
>> StoreManager
> Indeed.
> 
> > (which inherently rely on a bunch of other stuff).
> And that's what I want to change.

Makes sense.

I can't promise you an immediate turnaround here.  What I'd like to do is to stage this refactoring, so I can see what makes sense.  There's a bunch of cleanups I want to do the analyzer core anyway, especially since implementing C++ support appears to be getting some critical momentum.