<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; "><br><div><div>On Oct 18, 2013, at 3:05 , Richard <<a href="mailto:tarka.t.otter@gmail.com">tarka.t.otter@gmail.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>On 14 Oct 2013, at 18:29, Jordan Rose <<a href="mailto:jordan_rose@apple.com">jordan_rose@apple.com</a>> wrote:</div><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>Ah...yes, you were right the first time. It's not that the true and false state are the same, per se...it's that the two <i>sinks</i> have the same state that causes the analyzer to think they're the same. There are a few ways around this:</div><div><br></div><div>1. Ignore cases where the state doesn't change. DereferenceChecker doesn't do this explicitly, but it does skip non-Locs, which will mostly have the same effect.</div><div><br></div><div>2. Add an explicit tag. A SimpleProgramPointTag can be declared as a static variable to provide a tag that's different from the default checker tag.</div><div><br></div><div>Either one of these would work. #1 saves a bit of work when we have no information, but it's not impossible that we'll eventually end up having a case where an unconstrained UnknownVal should be considered a problem.</div><div><br></div><div>Jordan</div><br></div></blockquote><br></div><div>I went with the Tag solution, nice and simple. So, attached is the current patch with passing test, any further suggestions?</div><div><br></div><div>Ta,</div><div>Richard</div></div></blockquote><br></div><div>Looking good! I do still have a few reservations, though. The main one is this:</div><div><br></div><div><div>+ if (const SymIntExpr *SIExpr = dyn_cast<SymIntExpr>(*I)) {</div><div>+ // A symbol is being compared to an int value. The Assumption value</div><div>+ // is adjusted so that it is checking the symbol exists.</div><div>+ BinaryOperatorKind op = SIExpr->getOpcode();</div><div>+ bool rhs = SIExpr->getRHS() != 0;</div><div>+ if ((op == BO_EQ && rhs == false) || (op == BO_NE && rhs == true))</div><div>+ Assumption = !Assumption;</div><div>+</div><div>+ continue;</div><div>+ }</div><div><br></div><div>I'm not actually sure which case this handles...is it after a weak function pointer has been converted to bool? Because it also matches this case:</div><div><br></div><div>NSAppKitVersionNumber > NSAppKitVersionNumber10_7</div><div><br></div><div>At the very least, you should be checking that the opcode is one of the operators you expect.</div><div><br></div><div>Besides that, here are a storm of little notes:</div><div><br></div><div><div>+ if (ExplodedNode *N = C.generateSink(StNull, 0, &Tag)) {</div><div>+ ImplicitNullDerefEvent Event = { L, false, N, &C.getBugReporter() };</div><div>+ dispatchEvent(Event);</div><div>+ }</div></div><div><br></div><div>Extra indentation here.</div><div><br></div><div><div><br></div><div>+ if (!(isa<SimpleCall>(&Call) || isa<ObjCMethodCall>(&Call)))</div></div><div><br></div><div>isa<> should work on references as well as pointers.</div><div><br></div><div><br></div><div><div>+ // If we have an objc method, check for the classes introduced version.</div><div>+ if (const ObjCMethodDecl *MD = dyn_cast<ObjCMethodDecl>(D)) {</div><div>+ VersionTuple ClassIntroduced;</div><div>+ const DeclContext *DC = MD->getDeclContext();</div><div>+ if (const ObjCProtocolDecl *PD = dyn_cast<ObjCProtocolDecl>(DC))</div><div>+ ClassIntroduced = versionIntroduced(PD);</div><div>+ else</div><div>+ ClassIntroduced = versionIntroduced(MD->getClassInterface());</div><div>+</div><div>+ Introduced = std::max(Introduced, ClassIntroduced);</div><div>+ }</div></div><div><br></div><div>I think you only need to do this if the method itself doesn't have an introduced version...it's probably safe to assume that headers are sane and won't have a method introduced before the class that contains it.</div><div><br></div><div>Also, typos/formatting: "ObjC" (or full "Objective-C"), and "class's".</div><div><br></div><div>Nice handling of protocols/categories/implementation/interface.</div><div><br></div><div><br></div><div><div>+ // Walk through the symbols children looking for a version check.</div></div><div><br></div><div><div>Typo: "symbol's"</div></div><div><br></div><div><br></div><div><div>+ // If this is a weak function, it is not OK to derefence if availability</div><div>+ // has not been checked for.</div></div><div><br></div><div>Typo: "dereference"</div><div><br></div><div><br></div><div><div>+ os << "The method is introduced in ";</div><div>+ os << Introduced;</div><div>+ os << ", later than the deployment target ";</div><div>+ os << deploymentTarget(State);</div></div><div><div>+</div><div>+ BugReport *R = new BugReport(*BT, os.str(), N);</div><div>+ BR->emitReport(R);</div></div><div><br></div><div>I'm not so happy with the message. For one thing, this applies to functions as well as methods, and may eventually apply to enumerations, their enumerators, and/or global variables. For another, it doesn't quite say what's wrong. How about <i>"Calling</i> method introduced in X.Y (deployment target is X.W)"?</div><div><br></div><div>It's also too bad that there's no highlight range here, but I guess the location is accurate enough.</div><div><br></div><div><br></div><div><div>+const Decl* UnavailableMethodChecker::weakDeclForSymbol(SymbolRef Sym) const {</div></div><div><br></div><div>Asterisk on the right, please. </div><div><br></div><div><br></div><div><div>+const Decl*</div><div>+UnavailableMethodChecker::weakDeclForRegion(const MemRegion *R) const {</div></div><div><br></div><div>...and here too.</div><div><br></div><div><br></div><div><div>+ }</div><div>+</div><div>+ else if (const SymbolConjured *SConj = dyn_cast<SymbolConjured>(*I)) {</div></div><div><br></div><div>Please keep these on one line. If it looks too crowded, you can add a blank line before the close brace.</div><div><br></div><div><div><br></div><div>+ // This is an objective C message, first get the class availability.</div></div><div><br></div><div>Formatting: "Objective-C"</div><div><br></div><div><div><br></div><div>+ std::string SelStr = ME->getSelector().getAsString();</div><div>+ if (SelStr == "respondsToSelector:" ||</div><div>+ SelStr == "instancesRespondToSelector:") {</div></div><div><br></div><div>getAsString() is expensive because it constructs a new string on the heap. Better to check that it is a unary selector, then grab the first selector piece and check that.</div><div><br></div><div><br></div><div><div>+ const ObjCSelectorExpr *SelExpr = cast<ObjCSelectorExpr>(ME->getArg(0));</div></div><div><br></div><div>This could fail if the user passes a SEL variable.</div><div><br></div><div><br></div><div><div>+ // We are only checking for SymInt and Symbol comparisons</div><div>+ else</div><div>+ break;</div></div><div><br></div><div>I'm not sure you want to break...perhaps just ignore these?</div><div><br></div><div><br></div><div>Thanks for working on this; sorry for drawing it out even more!</div><div>Jordan</div></div></body></html>