[cfe-dev] fix for Clang PR 8419

[Ted Kremenek]

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.

Second, I don't think this is the droid you are looking for.  The method 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).  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 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.

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20101106/98930252/attachment.html>