[cfe-commits] r57240 - /cfe/trunk/lib/Analysis/GRExprEngine.cpp

[Zhongxing Xu]

On Thu, Oct 9, 2008 at 4:01 AM, Ted Kremenek <kremenek at apple.com> wrote:

>
> On Oct 7, 2008, at 7:43 PM, Zhongxing Xu wrote:
>
>  New patch:
>>
>> Index: lib/Analysis/GRExprEngine.cpp
>> ===================================================================
>> --- lib/Analysis/GRExprEngine.cpp       (revision 57240)
>> +++ lib/Analysis/GRExprEngine.cpp       (working copy)
>> @@ -1132,10 +1132,12 @@
>>           case 5:
>>             if (!memcmp(s, "panic", 5)) Builder->BuildSinks = true;
>>             else if (!memcmp(s, "error", 5)) {
>> -              Expr* Arg = *CE->arg_begin();
>> -              if (IntegerLiteral* IL = dyn_cast<IntegerLiteral>(Arg))
>> -                if (IL->getValue() != 0)
>> +              if (CE->getNumArgs() > 0) {
>> +                RVal X = GetRVal(St, *CE->arg_begin());
>> +                nonlval::ConcreteInt* CI =
>> dyn_cast<nonlval::ConcreteInt>(&X);
>> +                if (CI && CI->getValue() != 0)
>>                   Builder->BuildSinks = true;
>> +              }
>>             }
>>             break;
>>
>
> Looks great!  Please apply!
>
> Two possible (optional) amendments.
>
> If you expect that "error" takes a exact number of arguments and has a
> particular signature (e.g., the first parameter must be a typedef of a
> specific name), you may wish to match against those specific properties,
> since "error" may be a function that is implemented differently in different
> code bases.  Incidentally, what code base are you analyzing?


I was scanning GNU tar 1.20: increment.c. And omit error() caused a false
alarm of null dereference.

SYNOPSIS
       #include <error.h>

       void error(int status, int errnum, const char *format, ...);



>
>
> A second suggestion is that you could possibly use the "Assume" logic to
> check if the argument could be '1', and when it could be then make the node
> a sink.  This allows the path pruning to also work in the case of symbolic
> values, e.g:
>
>  int foo(int code) {
>   error(code);
>   // more stuff that executes if code != 1
>  }
>
> The idea would be to split the state, just as we do with dereferences
> (i.e., where we assume that the pointer could be null and then also assume
> that the pointer is null).  It makes the code a little more complicated, but
> it does make it handle many more cases, and causes the analysis engine to
> reason about all the constraints that it has and not just prune the path
> when the RVal evaluates to the constant '1'.
>

I'll leave a FIXME for this. I shall fix some Visit* logic in
GRExprEngine.cpp before extending the RegionStoreManager.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20081009/70893fd1/attachment.html>