Attached are the most recent version of the two patches. (By the way, is there a website set up for conducting code reviews for clang? If so, maybe in the future that would be more convenient and easier to use that instead of many emails).<br>
<br>Jordan, I think you were looking at an older patch. Some of your comments are probably already resolved. However, just in case...<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><div>+</div><div>+ if ( offset.isValid()</div><div>+ && !offset.hasSymbolicOffset()</div><div>+ && offset.getOffset() != 0) {</div><div>+ os << "; the memory location passed is an offset of an allocated location";</div>
<div>+ }</div><div>+</div></div></blockquote><blockquote style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex" class="gmail_quote"><div><br></div></blockquote><blockquote style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex" class="gmail_quote">
<div>...this test is not correct; it will fire for free(&local[1]).<br></div></blockquote><div> </div>I do not agree. My understanding is that local stack variables would fail the following test in ReportBadFree:<br><br>
<div style="margin-left:40px">const MemRegion *MR = ArgVal.getAsRegion();<br> if (MR) {<br></div><br>which protects the code you mention above. In FreeMemAux, before ReportBadFree is called the symbol is confirmed to be known malloc'ed memory (which did not appear in earlier patches). Just to be sure, I do have a unit test which tests just this:<br>
<br><div style="margin-left:40px">void testFreeNonMallocPointerWithOffset() {<br> char c;<br> char * r = &c;<br> r = r + 10;<br> free(r-10); // expected-warning {{Argument to free() is the address of the local variable 'c', which is not memory allocated by malloc()}}<br>
}<br></div><br><br><blockquote style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex" class="gmail_quote"><font size="4">Given that the message isn't great either, and that
you've already established what kind of value is being freed, I think
you should just make a new helper function to report the error,
ReportOffsetFree or something. Then you can make a nicer error message,
like "Argument to free() is an offset of an allocation". (I don't think
this message is perfect either -- it doesn't mention malloc(), "an
offset" isn't really the correct term -- but at least it's more
concise.)</font><br></blockquote><br>The reason I added to ReportBadFree instead of making another function was I believed that most of the code would be identical. Additionally, ReportBadFree also reports if the free is performed on malloc'ed data or not, which I believed would be useful in a message. If you feel that the message should be more clear, I can add another function, ReportOffsetFree, for it.<br>
<br><font size="4"><div><br></div></font><blockquote style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex" class="gmail_quote"><font size="4"><div><div>+void freeOffsetPointerPassedToFunction()</div>
<div>+{</div><div>+ int *p = malloc(sizeof(int)*2);</div><div>+ p[1] = 0;</div><div>+ p += 1;</div><div>+ myfooint(*p); // not passing the pointer, only a value pointed by pointer</div><div>+
free(p); // expected-warning {{Argument to free() is not memory
allocated by malloc(); the memory location passed is an offset of an
allocated location}}</div><div>+}</div></div></font><br><font size="4"><div>A better test would be to pass 'p' as a <i>const</i> pointer, which means the contents of the region get invalidated but 'p' itself will not.</div>
</font></blockquote><br>I am not clear on what you mean. Pass 'p' as a const pointer to free? The const will be ignored in that case. Or maybe you mean myfooint? That accepts an integer and not a pointer. Maybe you were referring to something different, maybe passing 'p' to a function that takes a const pointer. I have added another test case that tests for this, called testOffsetPassedAsConst.<br>
<br><br><blockquote style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex" class="gmail_quote"><span style="font-size:15px"><div><div><div>- if (Call && doesNotFreeMemory(Call, State))</div>
<div>+ if ((Kind == PSK_DirectEscapeOnCall ||</div><div>+ Kind == PSK_IndirectEscapeOnCall) &&</div><div>+ doesNotFreeMemory(Call, State)) {</div></div></div></span><br><span style="font-size:15px"><div>
This
is not correct. Before, this branch was only taken if the Kind
is PSK_DirectEscapeOnCall. Indirect escapes can still possibly free
memory (although it's unlikely).</div></span><br><span style="font-size:15px"><div><div>- if (Call && guaranteedNotToCloseFile(*Call))</div><div>+ if ((Kind == PSK_DirectEscapeOnCall ||</div><div>+ Kind == PSK_IndirectEscapeOnCall) &&</div>
<div>+ guaranteedNotToCloseFile(*Call)) {</div></div></span><br><span style="font-size:15px"><div>Ditto.</div></span></blockquote><br>Anna mentioned that the behavior change is intended for the malloc checker. However, I did not consider the behavior change for the simple stream checker. In the attached patch, I have changed the condition to only check for direct escape.<br>
<br>- Branden<br><br><br><div class="gmail_quote">On Fri, Jan 18, 2013 at 10:07 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 class="h5"><div>On Jan 18, 2013, at 7:02 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"><span style="font-size:14px">(see previous e-mail, "Re: Add PointerEscapeKind to checkPointerEscape callback")</span><div style="font-size:14px"><br></div><div style="font-size:14px">
I'd definitely like to see this improvement to MallocChecker! Let's see...</div><div style="font-size:14px"><br></div><div style="font-size:14px"><div>+</div><div>+ const MemRegion *Rbase = R->getBaseRegion();</div>
<div> </div><div>- const SymbolicRegion *SR = dyn_cast<SymbolicRegion>(R);</div><div>+ const SymbolicRegion *SR = dyn_cast<SymbolicRegion>(Rbase);</div></div><div style="font-size:14px"><br></div><div style="font-size:14px">
I would just collapse these into one line. More importantly, I think you should rename SR and possibly Sym to indicate that they're the <i>base</i> region/symbol, which isn't the same as the region actually passed to free.</div>
<div style="font-size:14px"><br></div><div style="font-size:14px"><br></div><div><div><font size="4">+ // Check if the memory location being freed is the actual location</font></div><div><font size="4">+ // allocated, or an offset.</font></div>
<div><font size="4">+ RegionOffset offset = R->getAsOffset();</font></div><div><font size="4"><br></font></div><div><font size="4">LLVM style guide is Germanic; all nouns are capitalized, even local variables.</font></div>
<div><font size="4"><br></font></div><div><font size="4"><br></font></div><div><font size="4">+ if ( RS && RS->isAllocated() &&</font></div><div><font size="4"><br></font></div><div><font size="4">Conventionally we have no space after the open paren for an 'if'.</font></div>
<div><font size="4"><br></font></div><div><font size="4">+ offset.isValid() &&</font></div><div><font size="4">+ offset.hasSymbolicOffset() == false &&</font></div><div><font size="4"><br></font></div>
<div><font size="4">Conventionally written "!offset.hasSymbolicOffset()"</font></div><div><font size="4"><br></font></div><div><font size="4"><br></font></div><div><font size="4"><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>+ RegionOffset offset = MR->getAsOffset();</div><div><br></div><div>Capital letter, again, but...</div>
<div><br></div><div><br></div><div><div>+</div><div>+ if ( offset.isValid()</div><div>+ && !offset.hasSymbolicOffset()</div><div>+ && offset.getOffset() != 0) {</div><div>+ os << "; the memory location passed is an offset of an allocated location";</div>
<div>+ }</div><div>+</div></div><div><br></div><div>...this test is not correct; it will fire for free(&local[1]). Given that the message isn't great either, and that you've already established what kind of value is being freed, I think you should just make a new helper function to report the error, ReportOffsetFree or something. Then you can make a nicer error message, like "Argument to free() is an offset of an allocation". (I don't think this message is perfect either -- it doesn't mention malloc(), "an offset" isn't really the correct term -- but at least it's more concise.)</div>
<div><br></div><div>Also, please put the && at the end of the line, not the start.</div><div><br></div><div>And while you're in there...</div><div><br></div><div> while (const ElementRegion *ER = dyn_cast<ElementRegion>(MR))<br>
MR = ER->getSuperRegion();<br><br></div><div>...this should probably be replaced by either MR->getBaseRegion(), so that we strip off struct field layers as well, or MR->StripCasts(), so that we don't say that '&x[1]' is the address of 'x'.</div>
<div><br></div></font></div></div></div></blockquote><div><br></div></div></div><div>In one of the later patches Branden does replace the loop with MR = MR->getBaseRegion(). I think he submitted it separately.</div><div>
<br></div>Branden, can you send us just the final 2 patches?</div><div><br></div><div>Thanks!</div><span class="HOEnZb"><font color="#888888"><div>Anna.</div></font></span><div class="im"><div><br><blockquote type="cite">
<div style="word-wrap:break-word"><div><div><font size="4"><div>Okay, I guess you're <i>not</i> in there anymore. I'll file a bug.</div><div><br></div><div><br></div><div><div>+void freeOffsetPointerPassedToFunction()</div>
<div>+{</div><div>+ int *p = malloc(sizeof(int)*2);</div><div>+ p[1] = 0;</div><div>+ p += 1;</div><div>+ myfooint(*p); // not passing the pointer, only a value pointed by pointer</div><div>+ free(p); // expected-warning {{Argument to free() is not memory allocated by malloc(); the memory location passed is an offset of an allocated location}}</div>
<div>+}</div></div><div><br></div><div>A better test would be to pass 'p' as a <i>const</i> pointer, which means the contents of the region get invalidated but 'p' itself will not.</div><div><br></div><div>
<br></div><div>Okay, I think that's it! Thanks, Branden.</div><div>Jordan</div><div><br></div></font></div><div style="font-size:14px"><br></div></div></div></blockquote></div><br></div></div></blockquote></div><br>