[cfe-dev] fix for Clang PR 8419

[Zhanyong Wan (λx.x x)]

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?

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

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

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?

> by use for the construction of PathDiagnostics (when we walk the
> ExplodedGraph).  You can see this because bindExprAndLocation() is called in
> only one place:
> void GRExprEngine::EvalLoadCommon(...) {
>     ...
>     }
>     else {
>       if (LoadTy.isNull())
>         LoadTy = Ex->getType();
>       SVal V = state->getSVal(cast<Loc>(location), LoadTy);
>       MakeNode(Dst, Ex, *NI, state->bindExprAndLocation(Ex, location, V),
>                ProgramPoint::PostLoadKind, tag);
>     }
>   }
> }
> You're absolutely right that you found a bug in bindExprAndLocation().  When
> I added it I could quite figure out why I couldn't get the changes I wanted
> to work in BugReporter, and I ran out of time and decided to revisit it
> later.  This may indeed be the problem I was encountering.
> Concerning ++s[0], the code that simulates pre/post-increment in
> GRExprEngine needs to be modified to understand reference types.  It assumes
> now that the result of ++s[0] is an r-value, which is the result of doing a
> load from s[0] after it's value has been incremented.

OK, I'll take a look there.  Thanks!

> On Nov 4, 2010, at 5:28 PM, Zhanyong Wan (λx.x x) wrote:
>
> Hi Ted,
>
> I'm working on fixing PR8419, and would like to check with you if I'm
> on the right track.
>
> As I found out, the analyzer crashes on
>
>  ++s[0];
>
> as it expects s[0] to be a Loc (which it should be), but instead sees a
> NonLoc.
>
> Upon reading the analyzer code, I see two things that don't seem right:
>
> 1. EnvironmentManager::bindExprAndLocation() throws away the
> 'location' argument.
>
> 2. GRExprEngine.cpp calls state->getSVal(Ex) to get the location of
> Ex, but the implementation of getSVal(Ex) doesn't use the right key
> (should be something like MakeLocation(Ex) to look up the expression.
>
> Please see http://codereview.appspot.com/2920041/ for a very early
> draft.  It's not yet cleaned up and some changes aren't strictly
> necessary -- I'll clean up after the discussion.
>
> I'm sure it's not quite right, as I'm still figuring out how the
> analyzer works.  (I wish there are more comments in the code. ;)
>
> Could you let me know if the direction of the patch is correct?  What
> problems do you see in it?
>
> Also, what do you think is the best way to ramp up on the analyzer code
> base?
>
> Thanks,
> --
> Zhanyong
>
>



-- 
Zhanyong