[cfe-commits] [PATCH] getLValueElement

[Zhongxing Xu]

On Wed, Oct 22, 2008 at 10:50 PM, Ted Kremenek <kremenek at apple.com> wrote:

>
> On Oct 22, 2008, at 7:35 AM, Zhongxing Xu wrote:
>
>  This implements the getLValueElement function for RegionStore. It only
>> handle integer indices currently.
>>
>
> Okay.
>
>
>> The base region is assumed an ElementRegion because we expect VisitCast()
>> to evaluate the base expr's
>> value to be loc::MemRegionVal(ElementRegion). This is consistent with the
>> C standard, which says
>> expression of type array of T is cast to pointer to T.
>>
>
> Is the idea for VisitCast to return the ElementRegion at index 0?


Yes.


>  Where would this logic actually be performed?


In GRExprEngine::VisitCast().

 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.


>
>
> +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.


>
>
> +    return UnknownVal();
> +
> +  case loc::MemRegionKind: {
> +    // We expect BaseR is an ElementRegion, not a base VarRegion.
> +    const MemRegion* BaseR = cast<loc::MemRegionVal>(BaseL).getRegion();
> +
> +    const ElementRegion* ElemR = cast<ElementRegion>(BaseR);
> +
> +    SVal Idx = ElemR->getIndex();
> +
> +    nonloc::ConcreteInt *CI1, *CI2;
> +
> +    // Only handle integer indices for now.
> +    if ((CI1 = dyn_cast<nonloc::ConcreteInt>(&Idx)) &&
> +        (CI2 = dyn_cast<nonloc::ConcreteInt>(&Offset))) {
> +      SVal NewIdx = CI1->EvalBinOp(StateMgr.getBasicVals(),
> BinaryOperator::Add,
> +                                   *CI2);
> +      return loc::MemRegionVal(MRMgr.getElementRegion(NewIdx,
> +
>  ElemR->getSuperRegion()));
> +    }
> +    break;
>
> This looks good assuming that the MemRegion for the base is an
> ElementRegion.
>
> 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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20081022/c3743998/attachment.html>