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

[Zhongxing Xu]

On Fri, Nov 14, 2008 at 8:51 AM, Ted Kremenek <kremenek at apple.com> wrote:

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

The standard does not specify any information about this conversion. The
compiler interprets E1[E2] as *(E1+E2). The sign does not affect the way
that the machine does 'add' (or 'sub'). (The sign only affects some
operations, c.f. LLVM instructions)

This should not affect us as long as the bits of APSInt is the same as the
target pointer. But APSInt operators require the operands have the same
signs. So I choose to let them be both signed.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20081114/2323492a/attachment.html>