[cfe-commits] r75356 - in /cfe/trunk: lib/Analysis/GRExprEngine.cpp test/Analysis/misc-ps.m

[Ted Kremenek]

On Jul 13, 2009, at 6:28 PM, Zhongxing Xu wrote:
>>
>> Makes sense.  I think we should add a comment about why the check for
>> 'isGenericPtr' is there.  Also, we should document that if the region
>> already has a cast type, casting it to void* doesn't remove that  
>> cast type.
>> What should the semantics be if there already is a cast type  
>> associated with
>> a region?
>
> I think it's seldom that programmer would cast it back to generic
> pointer intentionally. Often it is cast back to generic
> pointer passively. E.g.:
>
> void invalidate(void* p);
> int *p = (int*)alloca();
> void *q = (void*) p; // people don't do this.
> invalidate(p); // people often do this.

Agreed.  Should we also consider 'char *' as a case in 'isGenericPtr'?

>> This code might actually be valid if 'sizeof(struct s) <= sizeof 
>> (int)' and
>> the alignments of both 'int' and 'struct s' are compatible.   
>> Otherwise,
>> we'll get a potential buffer overflow that we won't catch.  
>> Alternatively, we
>> might not actually be doing the right thing.  I think this is  
>> something
>> worth discussing.
>
> The check is done (or should be done) in NewCastRegion(). If the cast
> is legal, then type 'struct s' is used to
> invalidate the region. I actually am not very clear about what you are
> worrying about here.

I think you're right.  I think I may have thought too hard about it  
and confused myself.



>
>> Index: lib/Analysis/GRExprEngine.cpp
>> ===================================================================
>> --- lib/Analysis/GRExprEngine.cpp (revision 75492)
>> +++ lib/Analysis/GRExprEngine.cpp (working copy)
>> @@ -1119,9 +1119,9 @@
>>      //  invalidate(y);  // 'x' now binds to a symbolic region
>>      //  int z = *y;
>>      //
>> -    if (isa<Loc>(V) && !Loc::IsLocType(Ex->getType())) {
>> -      V = EvalCast(V, Ex->getType());
>> -    }
>> +    //if (isa<Loc>(V) && !Loc::IsLocType(Ex->getType())) {
>> +    //  V = EvalCast(V, Ex->getType());
>> +    // }
>> This is the main change, but I cannot actually evaluate it yet.   
>> Using the
>> '-analyzer-viz-egraph-graphviz' option, I saw that for the  
>> following code:
>>     int *__gruep__ = ((int *)&((b)->grue));
>>     int __gruev__ = *__gruep__;
>> that all we end up doing is conjuring a new symbol for the second
>> declaration of '__gruev__' (even when using RegionStoreManager).
>
> This is expected. We invalidate *__gruep__ with a conjured symbol.

Right!  Once again I confused myself; there is a difference between  
the conjured symbols used by RegionStore and the ones used to recover  
path-sensitivity in GRExprEngine.  It would be nice if if we had a way  
to distinguish them.

Just as a test, I augmented my copy of 'testB' to contain two  
infeasible null dereferences.  With RegionStore we get no null  
dereference errors (as it should be).  BasicStore flags two:

typedef struct _BStruct { void *grue; } BStruct;
void testB_aux(void *ptr);
void testB(BStruct *b) {
   {
     int *__gruep__ = ((int *)&((b)->grue));
     int __gruev__ = *__gruep__;
     int __gruev2__ = *__gruep__;
     if (__gruev__ != __gruev2__) {
       int *p = 0;
       *p = 0xDEADBEEF;
     }

     testB_aux(__gruep__);
   }
   {
     int *__gruep__ = ((int *)&((b)->grue));
     int __gruev__ = *__gruep__;
     int __gruev2__ = *__gruep__;
     if (__gruev__ != __gruev2__) {
       int *p = 0;
       *p = 0xDEADBEEF;
     }

     if (~0 != __gruev__) {}
   }
}
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20090713/2c062111/attachment.html>