[cfe-commits] [PATCH] getLValueElement

[Ted Kremenek]

On Oct 22, 2008, at 8:04 AM, Zhongxing Xu wrote:
>
> Is the idea for VisitCast to return the ElementRegion at index 0?
>
> Yes.
>
>  Where would this logic actually be performed?
>
> In GRExprEngine::VisitCast().

I should have been more specific.  I know that's when the logic  
happens, not just where the actual code lives.

>
>
>  VisitCast (in GRExprEngine) should have no notion of ElementRegion,  
> as that is something specific to a given Store implementation.  Do  
> we need to add another Store method that GRExprEngine::VisitCast  
> calls back to?
>
> This is where I plan to add a new TransferFunction subclass  
> corresponding to RegionStore as GRSimpleVals to BasicStore. And in  
> that TransferFunction, EvalCast will do what is required to convert  
> the VarRegion to ElementRegion at index 0.

Sounds reasonable, but my overall feeling is that the whole transfer  
function mechanism needs to evolve, but that's not something we need  
to solve immediately.  Ideally, it would be nice if RegionStore could  
just be used by GRSimpleVals.  For example, the casting logic in  
GRSimpleVals seems fairly universal, and casts involving locations  
should probably just be forwarded on to the respective Store  
implementation.

For example, having the method:

   virtual SVal ArrayToPointer(SVal Array) { return Array; }

in StoreManager would be sufficient, and just have VisitCast call that  
(RegionStore of course would override this).  That way we get a clean  
separation of concerns.

What I've noticed over time is the "assumption creep" that accrues in  
different pieces of libAnalysis, where one component will assume  
something about the implementation of another.  These assumptions  
usually lead to redundant code or making things less flexible then  
they should be.  Fixing these later usually ends up being a real pain.

> +SVal RegionStoreManager::getLValueElement(const GRState* St,
> +                                          SVal Base, SVal Offset) {
> +  if (Base.isUnknownOrUndef())
> +    return Base;
> +
> +  Loc BaseL = cast<Loc>(Base);
> +
> +  switch (BaseL.getSubKind()) {
> +  default:
> +    assert("Other cases are not handled yet.");
>
> This assertion will always be true.  Did you mean assert(false &&  
> "...")?
>
> No, I just don't want the program to crash when things happen, but  
> also put reminder there.

Then it's not really an assertion.  Just use a comment.

>
> Incidentally, do you need a switch statement, or do we plan on  
> adding support for bases other than MemRegion?
>
> I don't know whether bases other than MemRegion will happen in  
> practice. So I leave space for that. If we are sure no other cases,  
> we can remove the switch.

I'd prefer to keep the code cleaner, and change it to a switch  
statement later.