[cfe-dev] checkers: decoupling ValueManager from GRStateManager

[Olaf Krzikalla]

Hi Ted,

 > 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.
OK. So SValuator relies on ValueManager, but (IMHO from here on) 
actually should not need StoreManager. The construction of even fancy 
SVals should not depend on the way they are bound to Regions.
Then the StoreManager classes can be build using an particular SValuator
for the creation of SVals. In an ideal world even SValuator and the 
StoreManager base class don't need to use each other (as they are 
orthogonal).
However, currently all three classes/concepts are somehow cross-linked.
Separating ValueManager seems to be the easiest task by moving 
convertToArrayIndex to SValuator. But you have to expose 
ArrayIndexWidth and ArrayIndexTy from ValueManager (two get-functions 
are enough, but I don't like growing interfaces). I'm not sure if these 
two values are always the same. If yes, then I could imagine to move 
these functions to ASTContext. These two data members seem a little bit 
odd in ValueManager.
Once this is done you get the problem that even the StoreManager base 
class needs to use the SValuator. StoreManager::CastRetrievedVal could 
be moved to SValuator (it creates a SVal, so it might belong to 
SValuator), but currently I have no idea how to deal with 
StoreManager::getLValueElement (which also creates an SVal, but is 
virtual).
On the other hand the use of StoreManager should be removed from 
SValuator. StoreManager::CastRegion (and in turn 
StoreManager::MakeElementRegion) can become part of SValuator.
However, StoreManager::ArrayToPointer looks somewhat strange. Wouldn't 
it be possible to just move the implementation of RegionStoreManager to 
SValuator, thus doing always the "right" thing (Basic- and 
FlatStoreManager implement this function as a no-op).
Eventually we could end that first step with a layering: ValueManager, 
SValuator, StoreManager.
OK, after these random thoughts some more specific comments:


> The point of SValuator is to build about symbolic expression values.
In understand that but I'm not convinced that a virtual interface is 
necessary. For me SValuator seemed to be an class which encapsulate some 
TODO things. Are there two different SValuator implementations thinkable 
(other than a basic vs. an enhanced one)?

> I'm curious about what pieces you'd like to reuse from ValueManager with other dataflow analyses?
Short answer: as much as possible. Ideally also SValuator and 
BasicStoreManager - thats why I want to remove/replace GRState from 
their interfaces. You might remember of our conversation here several 
month ago. Back then I ended up with a solution which was sufficient at 
that time. Now it is time to make things more complete. I have attached 
my current interface header file. As you can see, ValueFlowCollector 
could become simpler by just using ValueManager. And the use of 
SValuator and BasicStoreManager could simplify my implementation 
further. If I would take the code as it currently is, I would need a 
GRStateManager (and this would open a can of worms).

>> 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?
Yes, I mean the Checker library. Based on your other statements I 
conclude that separating things is apparently indeed one of the things 
currently in development. So the more concrete question is: shall I wait 
just another month or shall I actively work on the decoupling issue?


Best regards
Olaf Krzikalla
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: ValueFlowAnalysis.h
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20101026/4c5ad8f8/attachment.h>