<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>Okay, if it already works, that's good. I just wanted to make sure that the ParmVarDecl's type had properly decayed to a pointer already. </div><div><br></div><div>Here's one more signature to test, using a little-known C feature:</div><div><br></div><div>void doStuff_staticSizedArray(const int a[static 10]) {}</div><div><br></div><div>This means the argument must be an array of size >= 10. (Why they couldn't just repurpose "a[10]" for this I'm not quite sure.) This may or may not be represented the same way, so it's worth checking that it's also getting validated.</div><div><br></div><div>A few more problems:</div><div>- This still doesn't work for Objective-C methods. I guess I'm okay with that for now, but the right answer is to use CallEvent::parameters to get the ParmVarDecls, instead of trying to save the FunctionDecl and using that.</div><div>- You can't assume every argument has a corresponding ParmVarDecl because of variadic functions. Quite a few of the other tests in Analysis are currently crashing!</div><div><br></div><div>Jordan</div><div><br></div><br><div><div>On Mar 12, 2014, at 7:51 , 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 ocsi="0" fpstyle="1" style="font-family: Helvetica; font-size: 12px; 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; word-wrap: break-word;"><div style="direction: ltr; font-family: Tahoma; font-size: 10pt;">Hi Jordan,<br><br>" Should we be checking parameters declared as arrays as well? Those aren't<span class="Apple-converted-space"> </span><i>quite</i> the same (ideally we'd check<span class="Apple-converted-space"> </span><i>all</i> the elements, but that's expensive)."<br><br>I may be wrong here, but as I've understood it: function parameter declarations which "look like" arrays, e.g. int a[] are treated by the compiler as if they were pointers. The checker checks calls to these two functions in the same way:<span class="Apple-converted-space"> </span><br><font face="Courier New">void doStuff_pointerToConstInt(const int *u){};<br>void doStuff_arrayOfConstInt(const int a[]){};<br></font><font face="Courier New"><br></font>Currently this patch also detects uninitialized arrays as arguments:<br><font face="Courier New">void ff10(void) {<br> int a[6]; // expected-note {{'a' initialized here}}<br> doStuff_arrayOfConstInt(a); // expected-warning {{Function call argument is a pointer to uninitialized value}}<br> // expected-note@-1 {{Function call argument is a pointer to uninitialized value}}<br>}</font><br>(The warning output could perhaps be changed to<span class="Apple-converted-space"> </span><font face="Courier New">{{Function call argument is an uninitialized array}}</font><span class="Apple-converted-space"> </span>?)<br><br>Pointer to pointer however, is not yet supported (this could potentially be expensive, although very useful):<br><font face="Courier New"><br>void doStuff_pointerToPointerToConstInt(int const **u){};</font><br><br><font face="Courier New">// should detect this too?<br>void ff6(void) {<br> int t;<br> const int* u = &t;<br> const int **tppu = &u;<br> doStuff_pointerToPointerToConstInt(tppu);<br>}</font><br><br><font face="Courier New">// should detect this too?</font><br><font face="Courier New">void ff5(void) {<br> const int p1,p2,p3,p4,p5;<br> int const *tad[5] = {&p1,&p2,&p3,&p4,&p5};<br> doStuff_pointerToPointerToConstInt(tad);</font><div><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 style="font-family: 'Times New Roman'; font-size: 16px;"><hr tabindex="-1"><div id="divRpF378292" style="direction: ltr;"><font size="2" face="Tahoma"><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 11 mars 2014 17:43<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: [PATCH] improving detection of uninitialized arguments (the CallAndMessageChecker)<br></font><br></div><div></div><div><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<span class="Apple-converted-space"> </span><i>quite</i> the same (ideally we'd check<span class="Apple-converted-space"> </span><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" target="_blank">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; 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 size="2" face="Tahoma"><b>Från:</b><span class="Apple-converted-space"> </span>Jordan Rose [<a href="mailto:jordan_rose@apple.com" target="_blank">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" target="_blank">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" target="_blank">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></div></div></div></div></blockquote></div><br></body></html>