<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; "><br><div><div>On Jan 28, 2013, at 7:31 PM, Branden Archer <<a href="mailto:b.m.archer4@gmail.com">b.m.archer4@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><blockquote style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex" class="gmail_quote">I'm still not sure why this enum needs any explicit integer values (the "= 1").<br></blockquote>
<br>No reason really. Easy enough to remove.<br><br><blockquote style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex" class="gmail_quote"><div style="text-align:left">No need for the comment - we are no longer getting the super region.<br>
</div></blockquote><div> </div><div>Oops. <br><br><blockquote style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex" class="gmail_quote">Also, please divide by ASTContext's getCharWidth(), not 8.<br>
</blockquote><br>Is getCharWidth() guaranteed to always be 8? If not, the result will be incorrect. RegionOffset.getOffset() returns a number in bits, and if the char width on the system is more than 8 bits then the printed result will not make sense.<br>
<br><blockquote style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex" class="gmail_quote">"Argument to free() is offset by # bytes from the start of memory allocated by malloc()"<br>
</blockquote><div><br>I like it!<br><div><br><blockquote style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex" class="gmail_quote">We need to add a test case to the first patch that exercises the new behavior.<br>
</blockquote><br>I am not sure what sort of test case would be appropriate. I have added two test cases, one for the malloc checker and the other for the simple stream checker, for the case where the reference of interest is passed to a function that accepts it as a const. Is this maybe what you had in mind?<br>
<br></div></div></div></blockquote><div>The test case needs to exercise this change:</div><div><div>- SymbolsIndirectlyInvalidated, /*CallEvent*/ 0);</div><div>+ SymbolsIndirectlyInvalidated, Call, PSK_IndirectEscapeOnCall);</div><div><br></div><div>In conjunction with this :</div><div><div>- if (Call && guaranteedNotToCloseFile(*Call))</div><div>+ if ((Kind == PSK_DirectEscapeOnCall ||</div><div>+ Kind == PSK_IndirectEscapeOnCall) &&</div><div>+ guaranteedNotToCloseFile(*Call)) {</div><div> return State;</div><div>+ }</div><div><br></div><div>Basically, you need to pass a pointer which we are tracking to a function call indirectly (ex: as a field in a struct..). You should pass it to a function which is known not to free memory or close stream. Finally, you leak that resource/pointer.</div><div><br></div><div>Previously, we would have a false negative - no leak would be reported. Now, we should be catching the leak.</div><div><br></div><div>Anna.</div></div></div></div><div><br><blockquote type="cite"><div><div><div>- Branden<br></div><br></div></div><br><div class="gmail_quote">On Fri, Jan 25, 2013 at 10:44 PM, Anna Zaks <span dir="ltr"><<a href="mailto:ganna@apple.com" target="_blank">ganna@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><br><div><div><div>On Jan 25, 2013, at 6:31 PM, Jordan Rose <<a href="mailto:jordan_rose@apple.com" target="_blank">jordan_rose@apple.com</a>> wrote:</div>
<br><blockquote type="cite"><div style="word-wrap:break-word"><div>Almost there!</div><div><br></div><div><div>+/// \brief Describes the different reasons a pointer escapes</div><div>+/// during analysis.</div><div>+enum PointerEscapeKind {</div>
<div>+ /// A pointer escapes due to binding its value to a location</div><div>+ /// that the analyzer cannot track.</div><div>+ PSK_EscapeOnBind = 1,</div><div>+</div><div>+ /// The pointer has been passed to a function call directly.</div>
<div>+ PSK_DirectEscapeOnCall,</div><div>+</div><div>+ /// The pointer has been passed to a function indirectly.</div><div>+ /// For example, the pointer is accessible through an</div><div>+ /// argument to a function.</div>
<div>+ PSK_IndirectEscapeOnCall,</div><div>+</div><div>+ /// The reason for pointer escape is unknown. For example, </div><div>+ /// a region containing this pointer is invalidated.</div><div>+ PSK_EscapeOther</div><div>
+};</div></div><div><br></div><div>I'm still not sure why this enum needs any explicit integer values (the "= 1").</div><div><br></div><div><br></div><div><div>- (RS->isReleased() ? "Attempt to free released memory" : </div>
<div>+ (RsBase->isReleased() ? "Attempt to free released memory" :</div><div> "Attempt to free non-owned memory"), N);</div></div><div><br></div><div>I was going to say that the following line needs to be lined up as well, but really I think this is an odd case where the prevailing style is to put the : on the next line and line it up with the ?.</div>
<div><br></div><div><br></div><div>+ Offset.hasSymbolicOffset() == false &&</div><div><br></div><div>Should just be !Offset.hasSymbolicOffset()</div><div><br></div><div><br></div><div><div><div>+void MallocChecker::ReportOffsetFree(CheckerContext &C, SVal ArgVal,</div>
<div>+ SourceRange range) const {</div></div><div><br></div><div>Capital "range", please!</div><div><br></div><div><br></div><div></div><div>+ const MemRegion *MR = ArgVal.getAsRegion();</div>
<div>+ assert((MR) && "Only MemRegion based symbols can have offset free errors");</div></div><div><br></div><div>Good assertion; conventionally no parens around a single assertion condition.</div><div>
<br></div><div><br></div><div><div>+ // Get the offset into the memory region before</div><div>+ // getting the super region, as retrieving the</div><div>+ // super region will ignore the offset.</div></div><div>
<br></div><div>The comment's (still?) wrapped very aggressively here; you can unwind it to 80 columns.</div><div><br></div></div></blockquote><div><br></div></div><div>No need for the comment - we are no longer getting the super region.</div>
<div><div><br></div><blockquote type="cite"><div style="word-wrap:break-word"><div><br></div><div><div>+ os << "Argument to free() was not allocated by malloc(), but instead is " <<</div>
<div>+ "a pointer offset by " << Offset.getOffset()/8 <<</div><div>+ " byte(s) from the start of allocated memory";</div></div><div><br></div><div>Another case of operator inconsistency in the LLVM standards. We tend to break before << and then line them up with the previous <<.</div>
<div><br></div><div>Also, please divide by ASTContext's getCharWidth(), not 8.</div><div><br></div><div><blockquote type="cite">The message emitted from ReportOffsetFree is a little different from the one that was emitted in ReportBadFree. Take a look and see if you agree with its phrasing and the information it is providing.<br>
</blockquote><br></div><div>I like the increased information! I think you can go even further, though, and just directly say what's wrong, rather than leading with "was not allocated by malloc()".</div><div>
<br></div><div>"Argument to free() is offset by # bytes from the start of allocated memory"</div></div></blockquote><div><br></div></div>+1</div><div><br></div><div>Or, if we want to mention "malloc".</div>
<div><br></div><div>"Argument to free() is offset by # bytes from the start of memory allocated by malloc()"</div><div><br><blockquote type="cite"><div style="word-wrap:break-word"><div><br></div>
<div>(Also, we can totally get "byte"/"bytes" correct!)</div><div><br></div><div><br></div><div><blockquote type="cite">However, no expected-warning comment resulted in a failure. Is there a special format for the expected-warning comment that I am missing, or is the comparison that is taking place not quite right?<br>
</blockquote><br></div><div>You were very close! expected-warning deliberately matches a <i>substring</i> of the actual warning emitted, so that you can choose how sensitive it is about the particular phrasing of the warning. Since this is the first test for the offset warning, you probably want to use the full string.</div>
<div><br></div><br><div><blockquote type="cite"><div> I sort of agree with part of both of the possibilities. I would rather the assert be its own contained unit, but I do not like inverting the logic in the assert, as it may make it a little harder to parse. How about this?<br>
<br><span style="font-family:courier new,monospace"> assert((Call != NULL ||<br> (Kind != PSK_DirectEscapeOnCall &&<br> Kind != PSK_IndirectEscapeOnCall)) &&<br> "Call must not be NULL when escaping on call");</span><br>
</div><br>I will probably be called on spacing rules though, as I tried to indent pass the ( only if within the same set of (). That is, the Kinds should line up, but the Call and "Call should not, as the first Call is within an extra ().<br>
</blockquote><div><br></div><div>That's probably the best we're going to get (and the best indentation), and Anna agrees with you on preferring to keep everything inside the assert. Let's go with this.</div><div>
<br></div><div>After this I think it'll be ready to commit! Anna?</div></div></div></blockquote><br></div><div>We need to add a test case to the first patch that exercises the new behavior.</div><div><br></div>
<div>
Otherwise, looks good.</div><div>Thanks Branden!</div><div><br></div><div><blockquote type="cite"><div style="word-wrap:break-word"><div><br></div><div>Jordan</div></div></blockquote></div><br></div></blockquote>
</div><br>
<span><02-update-malloc-checker.patch></span><span><01-modify-checkPointerEscape.patch></span></blockquote></div><br></body></html>