<html><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><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><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></body></html>