<html><head><meta http-equiv="Content-Type" content="text/html charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; ">Hey Ted,<div><br><div><div>On 17 Jan 2013, at 19:16, Ted Kremenek <<a href="mailto:kremenek@apple.com">kremenek@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><meta http-equiv="Content-Type" content="text/html charset=us-ascii"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div>Hi Richard,</div><div><br></div><div>Sorry for joining this so late. I have mixed thoughts about this direction, but I think we should give it a try and see how well it works out. Thanks so much for working in this! I have a few specific comments on the patch itself:</div><div><br></div><div><blockquote type="cite"><div>Index: include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h</div><div>===================================================================</div><div>--- include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h<span class="Apple-tab-span" style="white-space:pre"> </span>(revision 172612)</div><div>+++ include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h<span class="Apple-tab-span" style="white-space:pre"> </span>(working copy)</div><div>@@ -532,9 +532,10 @@</div><div> /// FunctionTextRegion - A region that represents code texts of function.</div><div> class FunctionTextRegion : public CodeTextRegion {</div><div> const NamedDecl *FD;</div><div>+ const SymbolExtent *WeakSym;</div><div> public:</div><div> FunctionTextRegion(const NamedDecl *fd, const MemRegion* sreg)</div><div>- : CodeTextRegion(sreg, FunctionTextRegionKind), FD(fd) {</div><div>+ : CodeTextRegion(sreg, FunctionTextRegionKind), FD(fd), WeakSym(NULL) {</div><div> assert(isa<ObjCMethodDecl>(fd) || isa<FunctionDecl>(fd));</div><div> }</div><div> </div><div>@@ -555,7 +556,17 @@</div><div> const NamedDecl *getDecl() const {</div><div> return FD;</div><div> }</div><div>- </div><div>+</div><div>+ const SymbolExtent *getWeakSymbol() const {</div><div>+ return WeakSym;</div><div>+ }</div></blockquote><div><br></div><div>Please include a doxygen comment for this method, since this is new functionality that needs some explanation.</div><br></div></div></blockquote><div><br></div><div>Done.</div><br><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><blockquote type="cite"><div>+</div><div>+ void setWeakSymbol(const SymbolExtent *S) {</div><div>+ // Only weak functions can have symbols.</div><div>+ assert(cast<ValueDecl>(FD)->isWeak());</div><div>+ WeakSym = S;</div><div>+ }</div></blockquote><div><br></div><div>MemRegions need to be immutable, so the setWeakSymbol() is not going to work. Can we just have the symbol associated with the FunctionTextRegion when it gets created?</div></div></blockquote><div><br></div><div>I would like to do this, but I don't see how it is possible easily. The SymbolExtent requires the FunctionTextRegion in its constructor, and I don't see a nice way to get hold of the SymbolManager in the FunctionTextRegions constructor to create it. Any suggestions here?</div><br><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><br><blockquote type="cite"><div>+</div><div> virtual void dumpToStream(raw_ostream &os) const;</div><div> </div><div> void Profile(llvm::FoldingSetNodeID& ID) const;</div><div>Index: lib/StaticAnalyzer/Core/SVals.cpp</div><div>===================================================================</div><div>--- lib/StaticAnalyzer/Core/SVals.cpp<span class="Apple-tab-span" style="white-space:pre"> </span>(revision 172612)</div><div>+++ lib/StaticAnalyzer/Core/SVals.cpp<span class="Apple-tab-span" style="white-space:pre"> </span>(working copy)</div><div>@@ -73,6 +73,9 @@</div><div> const MemRegion *R = X->stripCasts();</div><div> if (const SymbolicRegion *SymR = dyn_cast<SymbolicRegion>(R))</div><div> return SymR->getSymbol();</div><div>+</div><div>+ else if (const FunctionTextRegion *FnR = dyn_cast<FunctionTextRegion>(R))</div><div>+ return FnR->getWeakSymbol();</div><div> }</div></blockquote><div><br></div><div>Minor not: please no empty line between if the 'if' and the 'else if'. Also please include a comment here, within the else if clause, describing what this is doing.</div><br></div></blockquote><div><br></div><div>Done.</div><br><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><blockquote type="cite"><div> return 0;</div><div> }</div><div>Index: lib/StaticAnalyzer/Core/SimpleConstraintManager.h</div><div>===================================================================</div><div>--- lib/StaticAnalyzer/Core/SimpleConstraintManager.h<span class="Apple-tab-span" style="white-space:pre"> </span>(revision 172612)</div><div>+++ lib/StaticAnalyzer/Core/SimpleConstraintManager.h<span class="Apple-tab-span" style="white-space:pre"> </span>(working copy)</div><div>@@ -96,6 +96,10 @@</div><div> ProgramStateRef assumeAuxForSymbol(ProgramStateRef State,</div><div> SymbolRef Sym,</div><div> bool Assumption);</div><div>+</div><div>+ ProgramStateRef assumeLocSymbol(ProgramStateRef State,</div><div>+ SymbolRef S,</div><div>+ bool Assumption);</div><div> };</div></blockquote><div><br></div><div>Please include a doxygen comment. Gradually over time we want to get all of these methods doxygenified, and new code should definitely include comments.</div><br></div></blockquote><div><br></div><div>Done, not too much to say about this one.</div><br><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><blockquote type="cite"><div> </div><div> } // end GR namespace</div><div>Index: lib/StaticAnalyzer/Core/SymbolManager.cpp</div><div>===================================================================</div><div>--- lib/StaticAnalyzer/Core/SymbolManager.cpp<span class="Apple-tab-span" style="white-space:pre"> </span>(revision 172612)</div><div>+++ lib/StaticAnalyzer/Core/SymbolManager.cpp<span class="Apple-tab-span" style="white-space:pre"> </span>(working copy)</div><div>@@ -334,6 +334,14 @@</div><div> </div><div> QualType SymbolExtent::getType() const {</div><div> ASTContext &Ctx = R->getMemRegionManager()->getContext();</div><div>+</div><div>+ // If this symbol is wrapping a weak function region</div><div>+ // then the type should be a function pointer.</div><div>+ if (const FunctionTextRegion *FTR = dyn_cast<FunctionTextRegion>(R)) </div><div>+ if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(FTR->getDecl()))</div><div>+ if (FD->isWeak())</div><div>+ return Ctx.getPointerType(FD->getType());</div><div>+</div><div> return Ctx.getSizeType();</div><div> }</div><div> </div><div>Index: lib/StaticAnalyzer/Core/SValBuilder.cpp</div><div>===================================================================</div><div>--- lib/StaticAnalyzer/Core/SValBuilder.cpp<span class="Apple-tab-span" style="white-space:pre"> </span>(revision 172612)</div><div>+++ lib/StaticAnalyzer/Core/SValBuilder.cpp<span class="Apple-tab-span" style="white-space:pre"> </span>(working copy)</div><div>@@ -190,7 +190,17 @@</div><div> }</div><div> </div><div> DefinedSVal SValBuilder::getFunctionPointer(const FunctionDecl *func) {</div><div>- return loc::MemRegionVal(MemMgr.getFunctionTextRegion(func));</div><div>+ const FunctionTextRegion *FTR = MemMgr.getFunctionTextRegion(func);</div><div>+</div><div>+ // If this is a weak function it needs an associated symbol</div><div>+ // for binding a possible NULL value to.</div><div>+ if (func->isWeak()) {</div><div>+ const SymbolExtent *S = SymMgr.getExtentSymbol(FTR);</div><div>+ FunctionTextRegion *F = const_cast<FunctionTextRegion *>(FTR);</div><div>+ F->setWeakSymbol(S);</div><div>+ }</div></blockquote><div><br></div><div>I don't think we need 'setWeakSymbol()'. We can just associate the symbol with the region when it gets created. SValBuilder shouldn't' be tampering with the definition of a MemRegion after it has been created.</div><br></div></blockquote><div><br></div><div>As above.</div><br><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><blockquote type="cite"><div>+</div><div>+ return loc::MemRegionVal(FTR);</div><div> }</div><div> </div></blockquote><div><br></div><br><blockquote type="cite"><div> DefinedSVal SValBuilder::getBlockPointer(const BlockDecl *block,</div><div>Index: lib/StaticAnalyzer/Core/SimpleConstraintManager.cpp</div><div>===================================================================</div><div>--- lib/StaticAnalyzer/Core/SimpleConstraintManager.cpp<span class="Apple-tab-span" style="white-space:pre"> </span>(revision 172612)</div><div>+++ lib/StaticAnalyzer/Core/SimpleConstraintManager.cpp<span class="Apple-tab-span" style="white-space:pre"> </span>(working copy)</div><div>@@ -72,6 +72,16 @@</div><div> return state;</div><div> }</div><div> </div><div>+ProgramStateRef SimpleConstraintManager::assumeLocSymbol(ProgramStateRef State,</div><div>+ SymbolRef S,</div><div>+ bool Assumption) {</div><div>+ const llvm::APSInt &Zero = getBasicVals().getZeroWithPtrWidth();</div><div>+ if (Assumption)</div><div>+ return assumeSymNE(State, S, Zero, Zero);</div><div>+ else</div><div>+ return assumeSymEQ(State, S, Zero, Zero);</div><div>+}</div></blockquote><blockquote type="cite"><div>+</div><div> ProgramStateRef SimpleConstraintManager::assumeAux(ProgramStateRef state,</div><div> Loc Cond, bool Assumption) {</div><div> switch (Cond.getSubKind()) {</div><div>@@ -81,19 +91,20 @@</div><div> </div><div> case loc::MemRegionKind: {</div><div> // FIXME: Should this go into the storemanager?</div><div>-</div><div> const MemRegion *R = cast<loc::MemRegionVal>(Cond).getRegion();</div><div> const SubRegion *SubR = dyn_cast<SubRegion>(R);</div><div>-</div><div>+</div><div> while (SubR) {</div><div> // FIXME: now we only find the first symbolic region.</div><div>- if (const SymbolicRegion *SymR = dyn_cast<SymbolicRegion>(SubR)) {</div><div>- const llvm::APSInt &zero = getBasicVals().getZeroWithPtrWidth();</div><div>- if (Assumption)</div><div>- return assumeSymNE(state, SymR->getSymbol(), zero, zero);</div><div>- else</div><div>- return assumeSymEQ(state, SymR->getSymbol(), zero, zero);</div><div>- }</div><div>+ if (const SymbolicRegion *SymR = dyn_cast<SymbolicRegion>(SubR))</div><div>+ return assumeLocSymbol(state, SymR->getSymbol(), Assumption);</div><div>+</div></blockquote><div><br></div><div>Please no empty line.</div><br><blockquote type="cite">+ // If this region is modelling a weak function</blockquote><div><br></div><div>"modelling" is has only one "l".</div><br></div></blockquote><div><br></div><div>Done.</div><br><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><blockquote type="cite"><div>+ // then we want to assume based on its associated symbol.</div><div>+ else if (const FunctionTextRegion *FTR = dyn_cast<FunctionTextRegion>(SubR))</div><div>+ if (const SymbolExtent *S = FTR->getWeakSymbol())</div><div>+ return assumeLocSymbol(state, S, Assumption);</div><div>+</div><div> SubR = dyn_cast<SubRegion>(SubR->getSuperRegion());</div><div> }</div><div> </div></blockquote><br></div><div>The patch also need some kind of test case to exercise this functionality, although I'm not certain exactly what yet you can test. We don't want to add code that isn't tested in some way.</div><div><br></div><div>Thanks for working on this!</div><div><br></div><div>Ted</div></div></blockquote><div><br></div><div><br></div><div>I have included a test case with the patch, it tests for values being assumed for weak functions, and checks that global function behaviour remains the same. Sufficient?</div><div><br></div></div></div></body></html>