[cfe-dev] fix for Clang PR 8419

[Ted Kremenek]

On Nov 8, 2010, at 5:05 PM, Zhanyong Wan (λx.x x) wrote:

> Thanks for the comments, Ted!
> 
> On Sat, Nov 6, 2010 at 11:18 AM, Ted Kremenek <kremenek at apple.com> wrote:
>> Hi Zhangyong,
>> First, I appreciate the added comments in the patch.  Please make those
>> doxygen comments.  For comments that don't involve added functionality, I'm
>> fine with committing those as is.
> 
> Will do.  The comments reflect *my* understanding of the code base,
> and thus may not be correct.  Have you reviewed them for accuracy?

I quickly scanned them, but I will re-review them.  From what I can tell they looked fine.

> 
>> Second, I don't think this is the droid you are looking for.  The method
> 
> I'm not surprised at all. ;-)  Due to a lack of understanding on how
> the analyzer works at a high level, I'm still trying to put the pieces
> together in the dark.

Thanks for digging in.

> 
>> bindExprAndLocation() was added so that the diagnostics engine could recover
>> some of the original information from GRState before getSVal() folds
>> symbols.  This is a very recent addition, but the extra data isn't needed
>> for evaluation (so the rest of the changes you suggest I don't think are
>> correct).
> 
> Who's supposed to consume the extra data?  Is the consumer still to be added?

This data is meant to be used by the BugReporterVisitors, which are used by BugReporter to add annotations to the path diagnostics.  This includes add messages such as "null pointer assigned here" or "assuming pointer is null."  The extra data is used by the BugReporterVisitors to garner additional information about what happened during a transition.

> 
>>  This line, however, is wrong:
>>   return Environment(F.Add(F.Add(Env.ExprBindings, MakeLocation(S), V), S,
>> V));
>> As you point out, it should be:
>>   return Environment(F.Add(F.Add(Env.ExprBindings, MakeLocation(S),
>> location), S, V));
>> In general, the way EvalLoad works is that some expressions are known to
>> evaluate to lvalues and other to rvalues.  The Environment doesn't care
>> whether an Expr* binds to an lvalue or rvalue; all of those are kept in the
>> same mapping.  However, EvalLoad simulates an lvalue-to-rvalue conversion,
>> and replaces the previous binding of the Expr* with the new value.  The
>> purpose of bindExprAndLocation() is to preserve the original location value
> 
> Is there a difference between a location value and an L-value?  If
> yes, it's not clear to me what the difference is.
>  If no, what do we
> introduce the term "location value" when L-value is already in wide
> use?

There is a difference; it's about how the values are used.

In the static analyzer, a "location" (SVals in namespace clang::loc) represents a pointer value.  Pointer values, however, can be manipulated, just like any other scalar value.  For example:

   int **p;
   ...
  *p = q;

In this example, the expression 'q' evaluates to a pointer value.  That value is an rvalue.  During analysis, this value can either be UnknownVal, UndefinedVal, or some subclass of Loc (i.e., a location).  In comparison, the expression on the left-side, '*p', evaluates to an lvalue (in this case, the location of memory pointed-to by 'p').  The result of the assignment is that the rvalue on the right is bound to the location of the lvalue on the left.  By definition, and lvalue is a location, but an rvalue can also be a location since pointers can be used as first-class values.

Digging deeper, the expression 'q' is evaluated in two stages.  First, the lvalue of the variable 'q' is computed.  Then, because the entire expression is meant to be an rvalue, there is an lvalue-to-rvalue conversion, which results in a load.  This is what EvalLoad does.


> 
> In
> 
>  return Environment(F.Add(F.Add(Env.ExprBindings, MakeLocation(S),
>    location), S, V));
> 
> what's the relation between 'location' and 'V'?  Is the latter the
> result of the lvalue-to-rvalue conversion from the former?

Going back to the example for the expression 'q', the 'location' is the lvalue and 'V' is the rvalue after the lvalue-to-rvalue conversion.  In this case, the lvalue is the location of 'q' and the rvalue is the loaded contents of 'q'.