<html><head><meta http-equiv="Content-Type" content="text/html charset=iso-8859-1"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div>A few more comments; sorry for the delay.</div><div><br></div><div>Please don't forget the LLVM naming conventions: capital letters for member variables, parameters, and local variables.</div><div><br></div><div><div>+ // If parameter is declared as pointer to const in function declaration,</div><div>+ // then check if corresponding argument in function call is</div><div>+ // pointing to undefined symbol value (uninitialized memory).</div><div>+ StringRef Message;</div><div>+</div><div>+ if (ParamDecl->getType()->isPointerType()) {</div><div>+ Message = "Function call argument is a pointer to uninitialized value";</div><div>+ } else if (ParamDecl->getType()->isReferenceType()) {</div><div>+ Message = "Function call argument is an uninitialized value";</div><div>+ } else</div><div>+ return false;</div></div><div><br></div><div>Should we be checking parameters declared as arrays as well? Those aren't <i>quite</i> the same (ideally we'd check <i>all</i> the elements, but that's expensive).</div><div><br></div><div><br></div><div><div>+ if (!ParamDecl->getType().getTypePtr()->getPointeeType().isConstQualified())</div><div>+ return false;</div><div>+</div></div><div><br></div><div>Again, the getTypePtr() is unnecessary because of QualType's operator->.</div><div><br></div><div><br></div><div><blockquote type="cite"><div style="font-family: Tahoma; font-size: 10pt; direction: ltr;"><span style="font-size: 10pt; font-family: Tahoma, sans-serif;">I also noticed that it didn't catch the uninitialized new, only uninitialized malloc. There is already an existing test for this in test file new.cpp under // Incorrectly-modelled behavior , it states that it should detect uninitialized, but still marked as //no warning. Is this ToDo or a bug?.<br></span></div></blockquote><br></div><div>You're right, we should handle this, specifically in VisitCXXNewExpr (or in a core checker). I think we were waiting for the 'operator new' implementation to land first, but it seems like that's sort of stopped making forward progress right now. In any case, though, it's not a problem with this patch, and those examples will start working. Let's not worry about it right now.</div><div><br></div><div>Jordan</div><div><br></div><br><div><div>On Mar 7, 2014, at 7:28 , Per Viberg <<a href="mailto:Per.Viberg@evidente.se">Per.Viberg@evidente.se</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div style="font-family: Tahoma; font-size: 10pt; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; direction: ltr;"><div><div style="margin-top: 0px; margin-bottom: 0px;"><span lang="EN-US"><br class="Apple-interchange-newline"> </span></div><span style="font-size: 10pt; font-family: Tahoma, sans-serif;">Hi Jordan,<br><br>I've fixed all comments as suggested.<span class="Apple-converted-space"> </span><br><br>The print out tracks back only 1 assignment back for pointers, not all the way to the source (see test case f6 & f5 in test file .c ). For references it tracks back until either the source or the first assignment from function return(see test cases f6_1 & f6_2 in test file .cpp). It's a bit limited, but at least it doesn't seem to print out something misleading. I noticed, it prints out some warnings also as notes, is there a way around this?.<br><br>I also noticed that it didn't catch the uninitialized new, only uninitialized malloc. There is already an existing test for this in test file new.cpp under // Incorrectly-modelled behavior , it states that it should detect uninitialized, but still marked as //no warning. Is this ToDo or a bug?.<br><br><br></span><div style="font-size: 13px; font-family: Tahoma;"><div style="margin-top: 0px; margin-bottom: 0px;"><span lang="EN-US" style="font-size: 8pt; font-family: Arial, sans-serif; color: gray;">.......................................................................................................................</span><span lang="EN-US" style="font-size: 8pt; font-family: Arial, sans-serif;"><br>Per Viberg<span class="Apple-converted-space"> </span></span><span lang="EN-US" style="font-size: 8pt; font-family: Arial, sans-serif; color: gray;">Senior Engineer</span><span lang="EN-US" style="font-size: 8.5pt; font-family: Arial, sans-serif; color: gray;"><br>Evidente ES East</span><span lang="EN-US" style="font-size: 8pt; font-family: Arial, sans-serif; color: gray;"><span class="Apple-converted-space"> </span>AB Warfvinges väg 34 SE-112 51 Stockholm Sweden</span><span lang="EN-US" style="font-size: 10pt; font-family: Tahoma, sans-serif;"></span></div><div style="margin-top: 0px; margin-bottom: 0px;"><span lang="EN-GB" style="font-size: 8pt; font-family: Arial, sans-serif; color: gray;">Phone: +46 (0)8 402 79 00<br>Mobile: +46 (0)70 912 42 52<br>E-mail: <span class="Apple-converted-space"> </span><a href="mailto:Per.Viberg@evidente.se" target="_blank"><font color="#0000ff">Per.Viberg@evidente.se</font></a><span class="Apple-converted-space"> </span></span><span lang="EN-GB" style="font-size: 8pt; font-family: Arial, sans-serif;"><br><br><a href="http://www.evidente.se/" target="_blank"><font color="#0000ff">www.evidente.se</font></a></span></div><div style="margin-top: 0px; margin-bottom: 0px;"><span lang="EN-GB" style="font-size: 6pt; font-family: Arial, sans-serif;">This e-mail, which might contain confidential information, is addressed to the above stated person/company. If you are not the correct addressee, employee or in any other way the person concerned, please notify the sender immediately. At the same time, please delete this e-mail and destroy any prints. Thank You.</span></div></div></div><div style="font-family: 'Times New Roman'; font-size: 16px;"><hr tabindex="-1"><div id="divRpF351064" style="direction: ltr;"><font face="Tahoma" size="2"><b>Från:</b><span class="Apple-converted-space"> </span>Jordan Rose [<a href="mailto:jordan_rose@apple.com">jordan_rose@apple.com</a>]<br><b>Skickat:</b><span class="Apple-converted-space"> </span>den 4 mars 2014 03:53<br><b>Till:</b><span class="Apple-converted-space"> </span>Per Viberg<br><b>Cc:</b><span class="Apple-converted-space"> </span><a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a>; Anders Rönnholm; Daniel Marjamäki<br><b>Ämne:</b><span class="Apple-converted-space"> </span>Re: improving detection of uninitialized arguments (the CallAndMessageChecker)<br></font><br></div><div></div><div><div>Hi, Per. Thanks again for working on this. I don't think we can rename the existing core.CallAndMessage, as much as we might want to. Names that aren't in "alpha" are API-ish, and we try not to break them without a strong reason.</div><div><br></div><div>We've also been leaving "Checker" off of the fully-qualified names (e.g. CallAndMessageUnInitRefArg instead of CallAndMessageUnInitRefArgChecker).</div><div><br></div><div><div>+ ChecksFilter filter;</div><div>+ CallAndMessageChecker()</div><div>+ : filter() {}</div></div><div><br></div><div>Since ChecksFilter has a non-trivial constructor, you can just leave out the explicit constructor for CallAndMessageChecker and the right thing will happen.</div><div><br></div><div>+ OwningPtr<BugType> &BT, const ParmVarDecl* ParamDecl) const;</div><div><br></div><div>Two LLVM style nitpicks: please attach the * to the variable name, and please wrap to the next line to stay within 80 columns.</div><div><br></div><div>+ if (filter.check_CallAndMessageUnInitRefArgChecker) {</div><div><br></div><div>Suggestion: factor this whole section out into a helper function.</div><div><br></div><div><br></div><div><div>+ //ParamIsPointer</div><div>+ //ParamIsReference</div><div>+ if (ParamDecl->getType().getTypePtr()->isPointerType()) {</div><div>+ ParamIsPointerToConst = ParamDecl->getType().getTypePtr()</div><div>+ ->getPointeeType().isConstQualified();</div><div>+ Message = "Function call argument is a pointer to uninitialized value";</div><div>+ } else if (ParamDecl->getType().getTypePtr()->isReferenceType()) {</div><div>+ ParamIsReferenceToConst = ParamDecl->getType().getTypePtr()</div><div>+ ->getPointeeType().isConstQualified();</div><div>+ Message = "Function call argument is an uninitialized value";</div><div>+ }</div></div><div><br></div><div>A few things here:</div><div>- No need for these comments.</div><div>- QualType has an operator->, so you can drop the .getTypePtr()s without any change.</div><div>- It's probably very slightly nicer to factor out the common code (checking for const qualification on the pointee). I would do something like this:</div><div><br></div><div>if (ParamDecl->getType()->isPointerType())</div><div><span class="Apple-tab-span" style="white-space: pre;"></span>Message = "..."</div><div>// same for references</div><div><br></div><div>if (!Message)</div><div><span class="Apple-tab-span" style="white-space: pre;"></span>return;</div><div>if (!ParamDecl->getType()->getPointeeType().isConstQualified())</div><div><span class="Apple-tab-span" style="white-space: pre;"></span>return;</div><div><br></div><div><br></div><div>+ LazyInit_BT("Uninitialized argument value", BT);</div><div><br></div><div>This is duplicated below. It's a little unfortunate to have to keep the strings in sync. Maybe we can factor that out somehow? (Or maybe it's better to use different BugTypes. What do you think?)</div><div><br></div><div><br></div><div><div>+ if (argEx)</div><div>+ bugreporter::trackNullOrUndefValue(N, argEx, *R);</div></div><div><br></div><div>We definitely need tests for this. Are we actually going to track back to when this pointer first start pointing to an undefined region? If not, what path notes<span class="Apple-converted-space"> </span><i>are</i> we showing here? Does this work for both references and pointers? (You can see how to test path notes in test/Analysis/inlining/path-notes.c.)</div><div><br></div><div>If things aren't working perfectly here, it probably doesn't block the patch, but we should still know where we are today and what still needs to be done. And we don't want to print something that's straight-up incorrect.</div><div><br></div><div>For the test cases, how about some pointers to memory returned by malloc() as well? Also, please add "core" to the set of checkers run in each case...it's a more realistic testing environment to have the core checkers enabled.</div><div><br></div><div>Thank you!</div><div>Jordan</div><div><br></div><br><div><div>On Feb 26, 2014, at 5:29 , Per Viberg <<a href="mailto:Per.Viberg@evidente.se" target="_blank">Per.Viberg@evidente.se</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div><div style="direction: ltr; font-family: Tahoma; font-size: 10pt;"><br>Hi,<br><br>I've extended the check that warns for uninitialized arguments to:<br>- warn for pointer arguments that point to uninitialized local (stack) variables. <span class="Apple-converted-space"> </span><br>- warn for function calls with uninitialized arguments, when the corresponding function parameter is a const reference.<span class="Apple-converted-space"> </span><br><br><b><font face="Courier New">void doStuff(const int *p);<br>void doStuffRef(const int& c);<br><br>void f(void) {<br> int x;<br> doStuff(&x); // warning<br>}<br></font></b><b><font face="Courier New">void g(void) {<br> int x;<br> doStuffRef(x); //</font></b><b><font face="Courier New"><b><font face="Courier New"><b><font face="Courier New"><span class="Apple-converted-space"> </span>warning</font></b><br></font></b> <span class="Apple-converted-space"> </span></font></b><br><br>I've also parameterize the CallAndMessageChecker in a similar way as CheckSecuritySyntaxOnly.<br><br>that is:<br>// only check if arguments are uninitialized (the old functionality only)<br>clang -cc1 -analyze -analyzer-checker=core.callandmessage uninit-const.cpp<br><br>// with extended functionality, i.e. "check if const pointer arguments are uninitialized" (also performs the old check).<br>clang -cc1 -analyze -analyzer-checker=alpha.core.CallAndMessageUnInitRefArgChecker uninit-const.cpp<br><b><font face="Courier New"><br></font></b><div><br>/Per<br><br><div style="font-size: 13px; font-family: Tahoma;"><div style="margin-top: 0px; margin-bottom: 0px;"><span lang="EN-US" style="font-size: 8pt; font-family: Arial, sans-serif; color: gray;">.......................................................................................................................</span><span lang="EN-US" style="font-size: 8pt; font-family: Arial, sans-serif;"><br>Per Viberg<span class="Apple-converted-space"> </span></span><span lang="EN-US" style="font-size: 8pt; font-family: Arial, sans-serif; color: gray;">Senior Engineer</span><span lang="EN-US" style="font-size: 8.5pt; font-family: Arial, sans-serif; color: gray;"><br>Evidente ES East</span><span lang="EN-US" style="font-size: 8pt; font-family: Arial, sans-serif; color: gray;"><span class="Apple-converted-space"> </span>AB Warfvinges väg 34 SE-112 51 Stockholm Sweden</span><span lang="EN-US" style="font-size: 10pt; font-family: Tahoma, sans-serif;"></span></div><div style="margin-top: 0px; margin-bottom: 0px;"><span lang="EN-GB" style="font-size: 8pt; font-family: Arial, sans-serif; color: gray;">Phone: +46 (0)8 402 79 00<br>Mobile: +46 (0)70 912 42 52<br>E-mail: <span class="Apple-converted-space"> </span><a href="mailto:Per.Viberg@evidente.se" target="_blank"><font color="#0000ff">Per.Viberg@evidente.se</font></a><span class="Apple-converted-space"> </span></span><span lang="EN-GB" style="font-size: 8pt; font-family: Arial, sans-serif;"><br><br><a href="http://www.evidente.se/" target="_blank"><font color="#0000ff">www.evidente.se</font></a></span></div><div style="margin-top: 0px; margin-bottom: 0px;"><span lang="EN-GB" style="font-size: 6pt; font-family: Arial, sans-serif;">This e-mail, which might contain confidential information, is addressed to the above stated person/company. If you are not the correct addressee, employee or in any other way the person concerned, please notify the sender immediately. At the same time, please delete this e-mail and destroy any prints. Thank You.</span></div></div></div></div></div><span><refToUninitConst_rev1.diff></span>_______________________________________________<br>cfe-commits mailing list<br><a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br><a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br></blockquote></div><br></div></div></div><span><refToUninitConst_rev1_3.diff></span></blockquote></div><br></body></html>