<html><head></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><br><div><div>On Nov 19, 2010, at 10:38 AM, Zhanyong Wan (λx.x x) wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><span class="Apple-style-span" style="border-collapse: separate; font-family: Helvetica; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-border-horizontal-spacing: 0px; -webkit-border-vertical-spacing: 0px; -webkit-text-decorations-in-effect: none; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; font-size: medium; ">Hi, Ted,<br><br>I made a new patch based on our discussion last night. I've uploaded<br>it to<span class="Apple-converted-space"> </span><a href="http://codereview.appspot.com/2920041/">http://codereview.appspot.com/2920041/</a><span class="Apple-converted-space"> </span>(the raw patch can be<br>found at<span class="Apple-converted-space"> </span><a href="http://codereview.appspot.com/download/issue2920041_2001.diff">http://codereview.appspot.com/download/issue2920041_2001.diff</a>).<br><br>Things to note:<br><br>1. In addition to trying to fix PR8419, I also added comments that<br>helped me understand how the code I'm touching works.<br><br>2. The meat of the fix is in CFG.cpp. I'm still not sure if this is a<br>principled fix, but all tests pass, so I hope it's at least an<br>improvement.<br><br>Would you please take a look? Thanks!</span></blockquote></div><br><div>Hi Zhanyong,</div><div><br></div><div>I think there is goodness here. I'd prefer this be separated up into separate commits. For example, purely documentation comments that are unrelated to the functionality changes should go into a separate commit.</div><div><br></div><div>Comments inline.</div><div><br></div><div><blockquote type="cite"><div><font class="Apple-style-span" color="#000000">Index: test/Analysis/misc-ps-region-store.cpp</font></div><div><font class="Apple-style-span" color="#000000">===================================================================</font></div><div><font class="Apple-style-span" color="#000000">--- test/Analysis/misc-ps-region-store.cpp</font><span class="Apple-tab-span" style="white-space:pre"><font class="Apple-style-span" color="#000000"> </font></span><font class="Apple-style-span" color="#000000">(revision 119815)</font></div><div><font class="Apple-style-span" color="#000000">+++ test/Analysis/misc-ps-region-store.cpp</font><span class="Apple-tab-span" style="white-space:pre"><font class="Apple-style-span" color="#000000"> </font></span><font class="Apple-style-span" color="#000000">(working copy)</font></div><div><font class="Apple-style-span" color="#000000">@@ -159,6 +159,25 @@</font></div><div><font class="Apple-style-span" color="#000000"> for (; ; x++) { }</font></div><div><font class="Apple-style-span" color="#000000"> }</font></div><div><font class="Apple-style-span" color="#000000"> </font></div><div><font class="Apple-style-span" color="#000000">+// PR8419 -- this used to crash.</font></div><div><font class="Apple-style-span" color="#000000">+</font></div><div><font class="Apple-style-span" color="#000000">+class String8419 {</font></div><div><font class="Apple-style-span" color="#000000">+ public:</font></div><div><font class="Apple-style-span" color="#000000">+ char& get(int n);</font></div><div><font class="Apple-style-span" color="#000000">+ char& operator[](int n);</font></div><div><font class="Apple-style-span" color="#000000">+};</font></div><div><font class="Apple-style-span" color="#000000">+</font></div><div><font class="Apple-style-span" color="#000000">+char& get8419();</font></div><div><font class="Apple-style-span" color="#000000">+</font></div><div><font class="Apple-style-span" color="#000000">+void Test8419() {</font></div><div><font class="Apple-style-span" color="#000000">+ String8419 s;</font></div><div><font class="Apple-style-span" color="#000000">+ ++(s.get(0));</font></div><div><font class="Apple-style-span" color="#000000">+ get8419()++; // used to crash</font></div><div><font class="Apple-style-span" color="#000000">+ ++s[0]; // used to crash</font></div><div><font class="Apple-style-span" color="#000000">+ s[0] &= 1; // used to crash</font></div><div><font class="Apple-style-span" color="#000000">+ s[0]++; // used to crash</font></div><div><font class="Apple-style-span" color="#000000">+}</font></div><div><font class="Apple-style-span" color="#000000">+</font></div><div><font class="Apple-style-span" color="#000000"> // PR8426 -- this used to crash.</font></div></blockquote><div><br></div><div><br></div><div>Test case is reasonable. This this also pass with the basic store? If so, this should just go in misc-ps.m.</div><br><blockquote type="cite"><div><font class="Apple-style-span" color="#000000"> </font></div><div><font class="Apple-style-span" color="#000000"> void Use(void* to);</font></div><div><font class="Apple-style-span" color="#000000">Index: include/clang/Checker/PathSensitive/Store.h</font></div><div><font class="Apple-style-span" color="#000000">===================================================================</font></div><div><font class="Apple-style-span" color="#000000">--- include/clang/Checker/PathSensitive/Store.h</font><span class="Apple-tab-span" style="white-space:pre"><font class="Apple-style-span" color="#000000"> </font></span><font class="Apple-style-span" color="#000000">(revision 119815)</font></div><div><font class="Apple-style-span" color="#000000">+++ include/clang/Checker/PathSensitive/Store.h</font><span class="Apple-tab-span" style="white-space:pre"><font class="Apple-style-span" color="#000000"> </font></span><font class="Apple-style-span" color="#000000">(working copy)</font></div><div><font class="Apple-style-span" color="#000000">@@ -22,6 +22,9 @@</font></div><div><font class="Apple-style-span" color="#000000"> </font></div><div><font class="Apple-style-span" color="#000000"> namespace clang {</font></div><div><font class="Apple-style-span" color="#000000"> </font></div><div><font class="Apple-style-span" color="#000000">+/// Store - This opaque type encapsulates an immutable map. Different</font></div><div><font class="Apple-style-span" color="#000000">+/// subclasses of StoreManager may choose to put different types of</font></div><div><font class="Apple-style-span" color="#000000">+/// keys and values in a Store.</font></div><div><font class="Apple-style-span" color="#000000"> typedef const void* Store;</font></div></blockquote><div><br></div><div>Instead of "immutable map", I would say "an immutable mapping from locations to values." The second sentence is incorrect. The entire purpose of the store is to map from locations to values; not arbitrary keys and values. Different StoreManagers may represent this mapping as they choose. At a high-level, the store represents the symbolic memory model.</div><div><br></div><br><blockquote type="cite"><div><font class="Apple-style-span" color="#000000"> </font></div><div><font class="Apple-style-span" color="#000000"> class GRState;</font></div><div><font class="Apple-style-span" color="#000000">Index: include/clang/Checker/PathSensitive/GRExprEngine.h</font></div><div><font class="Apple-style-span" color="#000000">===================================================================</font></div><div><font class="Apple-style-span" color="#000000">--- include/clang/Checker/PathSensitive/GRExprEngine.h</font><span class="Apple-tab-span" style="white-space:pre"><font class="Apple-style-span" color="#000000"> </font></span><font class="Apple-style-span" color="#000000">(revision 119815)</font></div><div><font class="Apple-style-span" color="#000000">+++ include/clang/Checker/PathSensitive/GRExprEngine.h</font><span class="Apple-tab-span" style="white-space:pre"><font class="Apple-style-span" color="#000000"> </font></span><font class="Apple-style-span" color="#000000">(working copy)</font></div><div><font class="Apple-style-span" color="#000000">@@ -513,6 +513,10 @@</font></div><div><font class="Apple-style-span" color="#000000"> public:</font></div><div><font class="Apple-style-span" color="#000000"> // FIXME: 'tag' should be removed, and a LocationContext should be used</font></div><div><font class="Apple-style-span" color="#000000"> // instead.</font></div><div><font class="Apple-style-span" color="#000000">+ // FIXME: Comment on the meaning of the arguments, when 'St' may not</font></div><div><font class="Apple-style-span" color="#000000">+ // be the same as Pred->state, and when 'location' may not be the</font></div><div><font class="Apple-style-span" color="#000000">+ // same as state->getLValue(Ex).</font></div><div><font class="Apple-style-span" color="#000000">+ /// Simulate a read of the result of Ex.</font></div><div><font class="Apple-style-span" color="#000000"> void EvalLoad(ExplodedNodeSet& Dst, const Expr* Ex, ExplodedNode* Pred,</font></div><div><font class="Apple-style-span" color="#000000"> const GRState* St, SVal location, const void *tag = 0,</font></div><div><font class="Apple-style-span" color="#000000"> QualType LoadTy = QualType());</font></div><div><font class="Apple-style-span" color="#000000">@@ -522,7 +526,7 @@</font></div><div><font class="Apple-style-span" color="#000000"> void EvalStore(ExplodedNodeSet& Dst, const Expr* AssignE, const Expr* StoreE,</font></div><div><font class="Apple-style-span" color="#000000"> ExplodedNode* Pred, const GRState* St, SVal TargetLV, SVal Val,</font></div><div><font class="Apple-style-span" color="#000000"> const void *tag = 0);</font></div><div><font class="Apple-style-span" color="#000000">-private: </font></div><div><font class="Apple-style-span" color="#000000">+private:</font></div><div><font class="Apple-style-span" color="#000000"> void EvalLoadCommon(ExplodedNodeSet& Dst, const Expr* Ex, ExplodedNode* Pred,</font></div><div><font class="Apple-style-span" color="#000000"> const GRState* St, SVal location, const void *tag,</font></div><div><font class="Apple-style-span" color="#000000"> QualType LoadTy);</font></div></blockquote><div><br></div><div>Comment seems reasonable.</div><div><br></div><br><blockquote type="cite"><div><font class="Apple-style-span" color="#000000">Index: include/clang/Checker/PathSensitive/Environment.h</font></div><div><font class="Apple-style-span" color="#000000">===================================================================</font></div><div><font class="Apple-style-span" color="#000000">--- include/clang/Checker/PathSensitive/Environment.h</font><span class="Apple-tab-span" style="white-space:pre"><font class="Apple-style-span" color="#000000"> </font></span><font class="Apple-style-span" color="#000000">(revision 119815)</font></div><div><font class="Apple-style-span" color="#000000">+++ include/clang/Checker/PathSensitive/Environment.h</font><span class="Apple-tab-span" style="white-space:pre"><font class="Apple-style-span" color="#000000"> </font></span><font class="Apple-style-span" color="#000000">(working copy)</font></div><div><font class="Apple-style-span" color="#000000">@@ -27,7 +27,12 @@</font></div><div><font class="Apple-style-span" color="#000000"> class ValueManager;</font></div><div><font class="Apple-style-span" color="#000000"> class LiveVariables;</font></div><div><font class="Apple-style-span" color="#000000"> </font></div><div><font class="Apple-style-span" color="#000000">+// FIXME: The name clang::Environment is too generic. Find a more</font></div><div><font class="Apple-style-span" color="#000000">+// descriptive name (e.g. GREnvironment?).</font></div><div></div></blockquote><div><br></div><div>Instead of a FIXME, why not just do this change?</div><br><blockquote type="cite"><div><font class="Apple-style-span" color="#000000"> </font></div><div><font class="Apple-style-span" color="#000000">+/// Environment - An immutable map from Stmts to their current</font></div><div><font class="Apple-style-span" color="#000000">+/// symbolic values (SVals).</font></div><div><font class="Apple-style-span" color="#000000">+///</font></div></blockquote><div><br></div><div>Comment seems reasonable. This is basically the dual of 'store.' Instead of mapping from locations to values, it maps from expressions to values. This essentially captures the current semantics of an expression as it was just evaluated by the analyzer engine.</div><br><blockquote type="cite"><div><font class="Apple-style-span" color="#000000"> class Environment {</font></div><div><font class="Apple-style-span" color="#000000"> private:</font></div><div><font class="Apple-style-span" color="#000000"> friend class EnvironmentManager;</font></div><div><font class="Apple-style-span" color="#000000">@@ -51,6 +56,9 @@</font></div><div><font class="Apple-style-span" color="#000000"> return X ? *X : UnknownVal();</font></div><div><font class="Apple-style-span" color="#000000"> }</font></div><div><font class="Apple-style-span" color="#000000"> </font></div><div><font class="Apple-style-span" color="#000000">+ /// Like LookupExpr, but doesn't require Ex to be in the map (in</font></div><div><font class="Apple-style-span" color="#000000">+ /// which case the return value will either be UnknownVal() or</font></div><div><font class="Apple-style-span" color="#000000">+ /// created by the ValueManager).</font></div><div><font class="Apple-style-span" color="#000000"> SVal GetSVal(const Stmt* Ex, ValueManager& ValMgr) const;</font></div></blockquote><div><br></div><div>This is a high-level, public API; this level of detail in the comment seems a little weird. The fact that values are lazily generated is an implementation detail. I'd rather the comment just say that this fetches the current binding of the expression in the Environment. The fact that the binding is implicitly or explicitly represented in the Environment is an implementation detail.</div><div><br></div><div>Also, LookupExpr is a private detail of the class. Comparing GetSVal to LookupExpr is a little weird when it comes to documenting a method used by clients.</div><br><blockquote type="cite"><div><font class="Apple-style-span" color="#000000"> </font></div><div><font class="Apple-style-span" color="#000000"> /// Profile - Profile the contents of an Environment object for use</font></div><div><font class="Apple-style-span" color="#000000">Index: include/clang/Checker/PathSensitive/GRState.h</font></div><div><font class="Apple-style-span" color="#000000">===================================================================</font></div><div><font class="Apple-style-span" color="#000000">--- include/clang/Checker/PathSensitive/GRState.h</font><span class="Apple-tab-span" style="white-space:pre"><font class="Apple-style-span" color="#000000"> </font></span><font class="Apple-style-span" color="#000000">(revision 119815)</font></div><div><font class="Apple-style-span" color="#000000">+++ include/clang/Checker/PathSensitive/GRState.h</font><span class="Apple-tab-span" style="white-space:pre"><font class="Apple-style-span" color="#000000"> </font></span><font class="Apple-style-span" color="#000000">(working copy)</font></div><div><font class="Apple-style-span" color="#000000">@@ -1,4 +1,4 @@</font></div><div><font class="Apple-style-span" color="#000000">-//== GRState*h - Path-Sens. "State" for tracking valuues -----*- C++ -*--==//</font></div><div><font class="Apple-style-span" color="#000000">+//== GRState.h - Path-sensitive "State" for tracking values -----*- C++ -*--==//</font></div></blockquote><div><br></div><div>Good cleanup.</div><br><blockquote type="cite"><div><font class="Apple-style-span" color="#000000"> //</font></div><div><font class="Apple-style-span" color="#000000"> // The LLVM Compiler Infrastructure</font></div><div><font class="Apple-style-span" color="#000000"> //</font></div><div><font class="Apple-style-span" color="#000000">@@ -7,7 +7,7 @@</font></div><div><font class="Apple-style-span" color="#000000"> //</font></div><div><font class="Apple-style-span" color="#000000"> //===----------------------------------------------------------------------===//</font></div><div><font class="Apple-style-span" color="#000000"> //</font></div><div><font class="Apple-style-span" color="#000000">-// This file defines SymbolRef, ExprBindKey, and GRState*</font></div><div><font class="Apple-style-span" color="#000000">+// This file defines SymbolRef, ExprBindKey, and GRState*.</font></div><div><font class="Apple-style-span" color="#000000"> //</font></div><div><font class="Apple-style-span" color="#000000"> //===----------------------------------------------------------------------===//</font></div><div><font class="Apple-style-span" color="#000000"> </font></div><div><font class="Apple-style-span" color="#000000">@@ -53,15 +53,17 @@</font></div><div><font class="Apple-style-span" color="#000000"> };</font></div><div><font class="Apple-style-span" color="#000000"> </font></div><div><font class="Apple-style-span" color="#000000"> //===----------------------------------------------------------------------===//</font></div><div><font class="Apple-style-span" color="#000000">-// GRState- An ImmutableMap type Stmt*/Decl*/Symbols to SVals.</font></div><div><font class="Apple-style-span" color="#000000">+// GRState- An ImmutableMap from Stmt*/Decl*/Symbols to SVals.</font></div></blockquote><div><br></div><div>This comment is actually out-of-date, since it isn't an ImmutableMap object anymore.</div><br><blockquote type="cite"><div><font class="Apple-style-span" color="#000000"> //===----------------------------------------------------------------------===//</font></div><div><font class="Apple-style-span" color="#000000"> </font></div><div><font class="Apple-style-span" color="#000000"> class GRStateManager;</font></div><div><font class="Apple-style-span" color="#000000"> </font></div><div><font class="Apple-style-span" color="#000000">-/// GRState - This class encapsulates the actual data values for</font></div><div><font class="Apple-style-span" color="#000000">-/// for a "state" in our symbolic value tracking. It is intended to be</font></div><div><font class="Apple-style-span" color="#000000">-/// used as a functional object; that is once it is created and made</font></div><div><font class="Apple-style-span" color="#000000">-/// "persistent" in a FoldingSet its values will never change.</font></div><div><font class="Apple-style-span" color="#000000">+/// GRState - This class encapsulates the symbolic values of all Stmts</font></div><div><font class="Apple-style-span" color="#000000">+/// in the program and the constraints on the values at a particular</font></div><div><font class="Apple-style-span" color="#000000">+/// moment.</font></div></blockquote><div><br></div><div>I would probably bullet it out. The GRState represents:</div><div><br></div><div>(1) Mapping from locations to values (Store)</div><div>(2) Mapping from expressions to values (Environment)</div><div>(3) Constraints on symbolic values</div><div><br></div><div>Together these represent the "abstract state" of a program.</div><br><blockquote type="cite"><div><font class="Apple-style-span" color="#000000"> A client of this class may also embed arbitrary data in</font></div><div><font class="Apple-style-span" color="#000000">+/// it via a GenericDataMap. GRState is intended to be used as a</font></div><div><font class="Apple-style-span" color="#000000">+/// functional object; that is, once it is created and made</font></div><div><font class="Apple-style-span" color="#000000">+/// "persistent" in a FoldingSet, its values will never change.</font></div><div><font class="Apple-style-span" color="#000000"> class GRState : public llvm::FoldingSetNode {</font></div><div><font class="Apple-style-span" color="#000000"> public:</font></div><div><font class="Apple-style-span" color="#000000"> typedef llvm::ImmutableSet<llvm::APSInt*> IntSetTy;</font></div><div><font class="Apple-style-span" color="#000000">@@ -73,9 +75,9 @@</font></div><div><font class="Apple-style-span" color="#000000"> friend class GRStateManager;</font></div><div><font class="Apple-style-span" color="#000000"> </font></div><div><font class="Apple-style-span" color="#000000"> GRStateManager *StateMgr;</font></div><div><font class="Apple-style-span" color="#000000">- Environment Env;</font></div><div><font class="Apple-style-span" color="#000000">+ Environment Env; // Maps a Stmt to its current SVal.</font></div><div><font class="Apple-style-span" color="#000000"> Store St;</font></div><div><font class="Apple-style-span" color="#000000">- GenericDataMap GDM;</font></div><div><font class="Apple-style-span" color="#000000">+ GenericDataMap GDM; // custom data stored by a client of this class</font></div></blockquote><div><br></div><div>Please keep comments consistent. Start with capital letters; end with with periods.</div><br><blockquote type="cite"><div><font class="Apple-style-span" color="#000000"> </font></div><div><font class="Apple-style-span" color="#000000"> /// makeWithStore - Return a GRState with the same values as the current</font></div><div><font class="Apple-style-span" color="#000000"> /// state with the exception of using the specified Store.</font></div><div><font class="Apple-style-span" color="#000000">@@ -120,8 +122,9 @@</font></div><div><font class="Apple-style-span" color="#000000"> </font></div><div><font class="Apple-style-span" color="#000000"> void setGDM(GenericDataMap gdm) { GDM = gdm; }</font></div><div><font class="Apple-style-span" color="#000000"> </font></div><div><font class="Apple-style-span" color="#000000">- /// Profile - Profile the contents of a GRState object for use</font></div><div><font class="Apple-style-span" color="#000000">- /// in a FoldingSet.</font></div><div><font class="Apple-style-span" color="#000000">+ /// Profile - Profile the contents of a GRState object for use in a</font></div><div><font class="Apple-style-span" color="#000000">+ /// FoldingSet. Two GRState objects are considered equal if they</font></div><div><font class="Apple-style-span" color="#000000">+ /// have the same Environment, Store, and GenericDataMap.</font></div></blockquote><div><br></div><div>Looks great.</div><br><blockquote type="cite"><div><font class="Apple-style-span" color="#000000"> static void Profile(llvm::FoldingSetNodeID& ID, const GRState* V) {</font></div><div><font class="Apple-style-span" color="#000000"> V->Env.Profile(ID);</font></div><div><font class="Apple-style-span" color="#000000"> ID.AddPointer(V->St);</font></div><div><font class="Apple-style-span" color="#000000">@@ -163,19 +166,14 @@</font></div><div><font class="Apple-style-span" color="#000000"> // (3) A binary value "Assumption" that indicates whether the constraint is</font></div><div><font class="Apple-style-span" color="#000000"> // assumed to be true or false.</font></div><div><font class="Apple-style-span" color="#000000"> //</font></div><div><font class="Apple-style-span" color="#000000">- // The output of "Assume" are two values:</font></div><div><font class="Apple-style-span" color="#000000">+ // The output of "Assume*" is a new GRState object with the added constraints.</font></div><div><font class="Apple-style-span" color="#000000">+ // If no new state is feasible, NULL is returned.</font></div></blockquote><div><br></div>Looks great.</div><div><br><blockquote type="cite"><div><font class="Apple-style-span" color="#000000"> //</font></div><div><font class="Apple-style-span" color="#000000">- // (a) "isFeasible" is set to true or false to indicate whether or not</font></div><div><font class="Apple-style-span" color="#000000">- // the assumption is feasible.</font></div><div><font class="Apple-style-span" color="#000000">- //</font></div><div><font class="Apple-style-span" color="#000000">- // (b) A new GRState object with the added constraints.</font></div><div><font class="Apple-style-span" color="#000000">- //</font></div><div><font class="Apple-style-span" color="#000000">- // FIXME: (a) should probably disappear since it is redundant with (b).</font></div><div><font class="Apple-style-span" color="#000000">- // (i.e., (b) could just be set to NULL).</font></div><div><font class="Apple-style-span" color="#000000">- //</font></div><div><font class="Apple-style-span" color="#000000"> </font></div><div><font class="Apple-style-span" color="#000000"> const GRState *Assume(DefinedOrUnknownSVal cond, bool assumption) const;</font></div><div><font class="Apple-style-span" color="#000000">- </font></div><div><font class="Apple-style-span" color="#000000">+</font></div><div><font class="Apple-style-span" color="#000000">+ // FIXME: The type of this method doesn't match the comment above.</font></div><div><font class="Apple-style-span" color="#000000">+ // Comment on what this really does.</font></div></blockquote><div><br></div><div>Here's what this method does. The above version of Assume takes an 'assumption' parameter. This one doesn't. This method assumes both "true" and "false" for 'cond', and returns both corresponding states. It's shorthand for doing 'Assume' twice.</div><br><blockquote type="cite"><div><font class="Apple-style-span" color="#000000"> std::pair<const GRState*, const GRState*></font></div><div><font class="Apple-style-span" color="#000000"> Assume(DefinedOrUnknownSVal cond) const;</font></div><div><font class="Apple-style-span" color="#000000"> </font></div><div><font class="Apple-style-span" color="#000000">@@ -194,9 +192,7 @@</font></div><div><font class="Apple-style-span" color="#000000"> //==---------------------------------------------------------------------==//</font></div><div><font class="Apple-style-span" color="#000000"> </font></div><div><font class="Apple-style-span" color="#000000"> /// BindCompoundLiteral - Return the state that has the bindings currently</font></div><div><font class="Apple-style-span" color="#000000">- /// in 'state' plus the bindings for the CompoundLiteral. 'R' is the region</font></div><div><font class="Apple-style-span" color="#000000">- /// for the compound literal and 'BegInit' and 'EndInit' represent an</font></div><div><font class="Apple-style-span" color="#000000">- /// array of initializer values.</font></div><div><font class="Apple-style-span" color="#000000">+ /// in this state plus the bindings for the CompoundLiteral.</font></div></blockquote><div><br></div>Looks good.</div><div><br><blockquote type="cite"><div><font class="Apple-style-span" color="#000000"> const GRState *bindCompoundLiteral(const CompoundLiteralExpr* CL,</font></div><div><font class="Apple-style-span" color="#000000"> const LocationContext *LC,</font></div><div><font class="Apple-style-span" color="#000000"> SVal V) const;</font></div><div><font class="Apple-style-span" color="#000000">Index: include/clang/Checker/PathSensitive/SVals.h</font></div><div><font class="Apple-style-span" color="#000000">===================================================================</font></div><div><font class="Apple-style-span" color="#000000">--- include/clang/Checker/PathSensitive/SVals.h</font><span class="Apple-tab-span" style="white-space:pre"><font class="Apple-style-span" color="#000000"> </font></span><font class="Apple-style-span" color="#000000">(revision 119815)</font></div><div><font class="Apple-style-span" color="#000000">+++ include/clang/Checker/PathSensitive/SVals.h</font><span class="Apple-tab-span" style="white-space:pre"><font class="Apple-style-span" color="#000000"> </font></span><font class="Apple-style-span" color="#000000">(working copy)</font></div><div><font class="Apple-style-span" color="#000000">@@ -39,13 +39,26 @@</font></div><div><font class="Apple-style-span" color="#000000"> class GRStateManager;</font></div><div><font class="Apple-style-span" color="#000000"> class ValueManager;</font></div><div><font class="Apple-style-span" color="#000000"> </font></div><div><font class="Apple-style-span" color="#000000">+/// SVal - This represents a symbolic expression, which can be either</font></div><div><font class="Apple-style-span" color="#000000">+/// an L-value or an R-value.</font></div><div><font class="Apple-style-span" color="#000000">+///</font></div></blockquote><div><br></div>Looks good.</div><div><br><blockquote type="cite"><div><font class="Apple-style-span" color="#000000"> class SVal {</font></div><div><font class="Apple-style-span" color="#000000"> public:</font></div><div><font class="Apple-style-span" color="#000000">- enum BaseKind { UndefinedKind, UnknownKind, LocKind, NonLocKind };</font></div><div><font class="Apple-style-span" color="#000000">+ enum BaseKind {</font></div><div><font class="Apple-style-span" color="#000000">+ // The enumerators must be representable using 2 bits.</font></div><div><font class="Apple-style-span" color="#000000">+ UndefinedKind = 0, // for subclass UndefinedVal (an uninitialized value)</font></div><div><font class="Apple-style-span" color="#000000">+ UnknownKind = 1, // for subclass UnknownVal (a void value)</font></div><div><font class="Apple-style-span" color="#000000">+ LocKind = 2, // for subclass Loc (an L-value)</font></div><div><font class="Apple-style-span" color="#000000">+ NonLocKind = 3 // for subclass NonLoc (an R-value that's not</font></div><div><font class="Apple-style-span" color="#000000">+ // an L-value)</font></div><div><font class="Apple-style-span" color="#000000">+ };</font></div><div><font class="Apple-style-span" color="#000000"> enum { BaseBits = 2, BaseMask = 0x3 };</font></div><div><font class="Apple-style-span" color="#000000"> </font></div><div><font class="Apple-style-span" color="#000000"> protected:</font></div><div><font class="Apple-style-span" color="#000000"> const void* Data;</font></div><div><font class="Apple-style-span" color="#000000">+</font></div><div><font class="Apple-style-span" color="#000000">+ /// The lowest 2 bits are a BaseKind (0 -- 3).</font></div><div><font class="Apple-style-span" color="#000000">+ /// The higher bits are an unsigned "kind" value.</font></div><div><font class="Apple-style-span" color="#000000"> unsigned Kind;</font></div><div><font class="Apple-style-span" color="#000000"> </font></div><div><font class="Apple-style-span" color="#000000"> protected:</font></div><div><font class="Apple-style-span" color="#000000">@@ -200,7 +213,7 @@</font></div><div><font class="Apple-style-span" color="#000000"> return V->getBaseKind() == UnknownKind;</font></div><div><font class="Apple-style-span" color="#000000"> }</font></div><div><font class="Apple-style-span" color="#000000"> };</font></div><div><font class="Apple-style-span" color="#000000">- </font></div><div><font class="Apple-style-span" color="#000000">+</font></div><div><font class="Apple-style-span" color="#000000"> class DefinedSVal : public DefinedOrUnknownSVal {</font></div><div><font class="Apple-style-span" color="#000000"> private:</font></div><div><font class="Apple-style-span" color="#000000"> // Do not implement. We want calling these methods to be a compiler</font></div><div><font class="Apple-style-span" color="#000000">Index: lib/Analysis/CFG.cpp</font></div><div><font class="Apple-style-span" color="#000000">===================================================================</font></div><div><font class="Apple-style-span" color="#000000">--- lib/Analysis/CFG.cpp</font><span class="Apple-tab-span" style="white-space:pre"><font class="Apple-style-span" color="#000000"> </font></span><font class="Apple-style-span" color="#000000">(revision 119815)</font></div><div><font class="Apple-style-span" color="#000000">+++ lib/Analysis/CFG.cpp</font><span class="Apple-tab-span" style="white-space:pre"><font class="Apple-style-span" color="#000000"> </font></span><font class="Apple-style-span" color="#000000">(working copy)</font></div><div><font class="Apple-style-span" color="#000000">@@ -900,12 +900,29 @@</font></div><div><font class="Apple-style-span" color="#000000"> return VisitChildren(S);</font></div><div><font class="Apple-style-span" color="#000000"> }</font></div><div><font class="Apple-style-span" color="#000000"> </font></div><div><font class="Apple-style-span" color="#000000">+// Returns true if a CFGBuilder should treat 'S' as an L-value when</font></div><div><font class="Apple-style-span" color="#000000">+// visiting it.</font></div><div><font class="Apple-style-span" color="#000000">+static bool ShouldTreatAsLValue(Stmt* S) {</font></div><div><font class="Apple-style-span" color="#000000">+ // A call to a reference-returning function is an L-value.</font></div><div><font class="Apple-style-span" color="#000000">+ if (CallExpr* CallEx = dyn_cast<CallExpr>(S))</font></div><div><font class="Apple-style-span" color="#000000">+ return CallEx->getCallReturnType()->isReferenceType();</font></div><div><font class="Apple-style-span" color="#000000">+</font></div><div><font class="Apple-style-span" color="#000000">+ // Otherwise, we assume it's not an L-value.</font></div><div><font class="Apple-style-span" color="#000000">+ // FIXME: document why this assumption is OK, or fix this in a</font></div><div><font class="Apple-style-span" color="#000000">+ // principled way if it's not.</font></div><div><font class="Apple-style-span" color="#000000">+ return false;</font></div><div><font class="Apple-style-span" color="#000000">+}</font></div><div><font class="Apple-style-span" color="#000000">+</font></div><div><font class="Apple-style-span" color="#000000"> /// VisitChildren - Visit the children of a Stmt.</font></div><div><font class="Apple-style-span" color="#000000"> CFGBlock *CFGBuilder::VisitChildren(Stmt* Terminator) {</font></div><div><font class="Apple-style-span" color="#000000"> CFGBlock *B = Block;</font></div><div><font class="Apple-style-span" color="#000000"> for (Stmt::child_iterator I = Terminator->child_begin(),</font></div><div><font class="Apple-style-span" color="#000000"> E = Terminator->child_end(); I != E; ++I) {</font></div><div><font class="Apple-style-span" color="#000000">- if (*I) B = Visit(*I);</font></div><div><font class="Apple-style-span" color="#000000">+ if (*I) {</font></div><div><font class="Apple-style-span" color="#000000">+ B = Visit(*I, ShouldTreatAsLValue(*I) ?</font></div><div><font class="Apple-style-span" color="#000000">+ AddStmtChoice::AsLValueNotAlwaysAdd :</font></div><div><font class="Apple-style-span" color="#000000">+ AddStmtChoice::NotAlwaysAdd);</font></div><div><font class="Apple-style-span" color="#000000">+ }</font></div><div><font class="Apple-style-span" color="#000000"> }</font></div><div><font class="Apple-style-span" color="#000000"> return B;</font></div><div><font class="Apple-style-span" color="#000000"> }</font></div></blockquote><div><br></div><div>I don't this is right. I'll comment in a separate email.</div><div><br></div><br><blockquote type="cite"><div><font class="Apple-style-span" color="#000000">Index: lib/Checker/Environment.cpp</font></div><div><font class="Apple-style-span" color="#000000">===================================================================</font></div><div><font class="Apple-style-span" color="#000000">--- lib/Checker/Environment.cpp</font><span class="Apple-tab-span" style="white-space:pre"><font class="Apple-style-span" color="#000000"> </font></span><font class="Apple-style-span" color="#000000">(revision 119815)</font></div><div><font class="Apple-style-span" color="#000000">+++ lib/Checker/Environment.cpp</font><span class="Apple-tab-span" style="white-space:pre"><font class="Apple-style-span" color="#000000"> </font></span><font class="Apple-style-span" color="#000000">(working copy)</font></div><div><font class="Apple-style-span" color="#000000">@@ -101,7 +101,8 @@</font></div><div><font class="Apple-style-span" color="#000000"> Environment EnvironmentManager::bindExprAndLocation(Environment Env,</font></div><div><font class="Apple-style-span" color="#000000"> const Stmt *S,</font></div><div><font class="Apple-style-span" color="#000000"> SVal location, SVal V) {</font></div><div><font class="Apple-style-span" color="#000000">- return Environment(F.Add(F.Add(Env.ExprBindings, MakeLocation(S), V), S, V));</font></div><div><font class="Apple-style-span" color="#000000">+ return Environment(F.Add(F.Add(Env.ExprBindings, MakeLocation(S), location),</font></div><div><font class="Apple-style-span" color="#000000">+ S, V));</font></div><div><font class="Apple-style-span" color="#000000"> }</font></div></blockquote><div><br></div><div>This is a good bug fix, and should go into a patch of its own.</div><br><blockquote type="cite"><div><font class="Apple-style-span" color="#000000"> </font></div><div><font class="Apple-style-span" color="#000000"> namespace {</font></div><div><font class="Apple-style-span" color="#000000">Index: lib/Checker/GRExprEngine.cpp</font></div><div><font class="Apple-style-span" color="#000000">===================================================================</font></div><div><font class="Apple-style-span" color="#000000">--- lib/Checker/GRExprEngine.cpp</font><span class="Apple-tab-span" style="white-space:pre"><font class="Apple-style-span" color="#000000"> </font></span><font class="Apple-style-span" color="#000000">(revision 119815)</font></div><div><font class="Apple-style-span" color="#000000">+++ lib/Checker/GRExprEngine.cpp</font><span class="Apple-tab-span" style="white-space:pre"><font class="Apple-style-span" color="#000000"> </font></span><font class="Apple-style-span" color="#000000">(working copy)</font></div><div><font class="Apple-style-span" color="#000000">@@ -909,7 +909,7 @@</font></div><div><font class="Apple-style-span" color="#000000"> break;</font></div><div><font class="Apple-style-span" color="#000000"> }</font></div><div><font class="Apple-style-span" color="#000000"> </font></div><div><font class="Apple-style-span" color="#000000">- case Stmt::CXXMemberCallExprClass: {</font></div><div><font class="Apple-style-span" color="#000000">+ case Stmt::CXXMemberCallExprClass: {</font></div><div><font class="Apple-style-span" color="#000000"> const CXXMemberCallExpr *MCE = cast<CXXMemberCallExpr>(S);</font></div><div><font class="Apple-style-span" color="#000000"> VisitCXXMemberCallExpr(MCE, Pred, Dst);</font></div><div><font class="Apple-style-span" color="#000000"> break;</font></div><div><font class="Apple-style-span" color="#000000">@@ -1944,10 +1944,11 @@</font></div><div><font class="Apple-style-span" color="#000000"> EvalBind(Dst, StoreE, *NI, GetState(*NI), location, Val);</font></div><div><font class="Apple-style-span" color="#000000"> }</font></div></blockquote><div><br></div><div>What changed here?</div><br><blockquote type="cite"><div><font class="Apple-style-span" color="#000000"> </font></div><div><font class="Apple-style-span" color="#000000">-void GRExprEngine::EvalLoad(ExplodedNodeSet& Dst, const Expr *Ex, </font></div><div><font class="Apple-style-span" color="#000000">+void GRExprEngine::EvalLoad(ExplodedNodeSet& Dst, const Expr *Ex,</font></div><div><font class="Apple-style-span" color="#000000"> ExplodedNode* Pred,</font></div><div><font class="Apple-style-span" color="#000000"> const GRState* state, SVal location,</font></div><div><font class="Apple-style-span" color="#000000"> const void *tag, QualType LoadTy) {</font></div><div><font class="Apple-style-span" color="#000000">+ assert(!isa<NonLoc>(location) && "location cannot be a NonLoc.");</font></div></blockquote><div><br></div><div>Is it possible to change 'location' to a Loc, instead of having this assertion?</div><div><br></div><br><blockquote type="cite"><div><font class="Apple-style-span" color="#000000"> </font></div><div><font class="Apple-style-span" color="#000000"> // Are we loading from a region? This actually results in two loads; one</font></div><div><font class="Apple-style-span" color="#000000"> // to fetch the address of the referenced value and one to fetch the</font></div><div><font class="Apple-style-span" color="#000000">@@ -3154,7 +3155,7 @@</font></div><div><font class="Apple-style-span" color="#000000"> assert (U->isIncrementDecrementOp());</font></div><div><font class="Apple-style-span" color="#000000"> ExplodedNodeSet Tmp;</font></div><div><font class="Apple-style-span" color="#000000"> const Expr* Ex = U->getSubExpr()->IgnoreParens();</font></div><div><font class="Apple-style-span" color="#000000">- VisitLValue(Ex, Pred, Tmp);</font></div><div><font class="Apple-style-span" color="#000000">+ VisitLValue(Ex, Pred, Tmp); // This modifies Tmp.</font></div></blockquote><div><br></div><div>Do you think this comment is necessary? This happens all over the place within the engine.</div><br><blockquote type="cite"><div><font class="Apple-style-span" color="#000000"> </font></div><div><font class="Apple-style-span" color="#000000"> for (ExplodedNodeSet::iterator I = Tmp.begin(), E = Tmp.end(); I!=E; ++I) {</font></div><div><font class="Apple-style-span" color="#000000"> </font></div><div><font class="Apple-style-span" color="#000000">@@ -3163,7 +3164,7 @@</font></div><div><font class="Apple-style-span" color="#000000"> </font></div><div><font class="Apple-style-span" color="#000000"> // Perform a load.</font></div><div><font class="Apple-style-span" color="#000000"> ExplodedNodeSet Tmp2;</font></div><div><font class="Apple-style-span" color="#000000">- EvalLoad(Tmp2, Ex, *I, state, V1);</font></div><div><font class="Apple-style-span" color="#000000">+ EvalLoad(Tmp2, Ex, *I, state, V1); // This modifies Tmp2.</font></div><div></div></blockquote><div><br></div>Do you think this comment is necessary? This happens all over the place within the engine.</div></body></html>