[cfe-commits] r59238 - /cfe/trunk/lib/Analysis/RegionStore.cpp

[Ted Kremenek]

On Nov 13, 2008, at 3:27 PM, Zhongxing Xu <xuzhongxing at gmail.com> wrote:

>
>
> On Thu, Nov 13, 2008 at 11:03 PM, Ted Kremenek <kremenek at apple.com>  
> wrote:
>
> On Nov 13, 2008, at 1:15 AM, Zhongxing Xu wrote:
>
>> Author: zhongxingxu
>> Date: Thu Nov 13 03:15:14 2008
>> New Revision: 59238
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=59238&view=rev
>> Log:
>> Array index might be unsigned. We have to generate a temporary  
>> signed value for
>> it to be evaluated by APSInt::operators.
>>
>> Modified:
>>    cfe/trunk/lib/Analysis/RegionStore.cpp
>>
>> Modified: cfe/trunk/lib/Analysis/RegionStore.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Analysis/RegionStore.cpp?rev=59238&r1=59237&r2=59238&view=diff
>>
>> === 
>> === 
>> === 
>> =====================================================================
>> --- cfe/trunk/lib/Analysis/RegionStore.cpp (original)
>> +++ cfe/trunk/lib/Analysis/RegionStore.cpp Thu Nov 13 03:15:14 2008
>> @@ -197,6 +197,18 @@
>>   // Only handle integer indices for now.
>>   if ((CI1 = dyn_cast<nonloc::ConcreteInt>(&Idx)) &&
>>       (CI2 = dyn_cast<nonloc::ConcreteInt>(&Offset))) {
>> +
>> +    // Temporary SVal to hold a potential signed APSInt.
>> +    SVal SignedInt;
>> +
>> +    // Index might be unsigned. We have to convert it to signed.
>> +    if (CI2->getValue().isUnsigned()) {
>> +      llvm::APSInt SI = CI2->getValue();
>> +      SI.setIsSigned(true);
>> +      SignedInt = nonloc::ConcreteInt(getBasicVals().getValue(SI));
>> +      CI2 = cast<nonloc::ConcreteInt>(&SignedInt);
>> +    }
>> +
>
> Hi Zhongxing,
>
> I'm not saying this isn't needed, but is there a particular  
> justification for the signed -> unsigned conversion?  (I actually  
> don't know by just looking at your patch).  It seems like things  
> like this should go in GRExprEngine, not the Store.
>
> Consistent signedness is required by llvm::APSInt. So we should find  
> a place to handle this. May be we should ensure all ConcreteInt be  
> signed in GRExprEngine?

One question I have is whether or not this conversion is specified in  
the C standard.  I would like to have a clean way of handling pointer  
arithmetic and integer overflow (which affects array indexing) and  
that probably should be handled in GRExprEngine.  GRExprEngine of  
course could delegate specific micro-operations to Store/ 
ConstraintManager/etc as we already do now for various things.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20081113/a164e2e1/attachment.html>