<html><head></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; ">Hi Kaelyn,<div><br></div><div>Sorry for the delay in responding. I'm trying to understand the differences proposed between your last patch and the new one. When I look at the "delta from patch set" from 2 to 3, I see changes that appear unrelated to this feature, which I find confusing.</div><div><br></div><div>What would help me is if I understood more of the high-level motivation of the changes. What I see is calls to "CheckArrayAccess" moved to various places, for example when checking the comma operator:</div><div><br></div><div> 7578 CheckArrayAccess(lhs.get());<br> 7579 CheckArrayAccess(rhs.get());</div><div><br></div><div>This feels a little kludgy. I'm not saying it is wrong; I just don't understand the motivation. Could you provide some insight behind these changes other than they got the particular test cases working?</div><div><br></div><div>Thanks so much,</div><div>Ted</div><div><br></div><div><div><div>On Jul 27, 2011, at 4:18 PM, Kaelyn Uhrain wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite">Ted & Eric,<br><br>After having to revert my array bounds enhancements (which had gone in as r136046) due to false positives in cases like &array[array_size], I've reworked the array bounds checking a bit and think I have it working correctly now. I've uploaded the full set of changes including my reverted changes to <br>
<a href="http://codereview.appspot.com/4675068">http://codereview.appspot.com/4675068</a> and attached a diff of the changes from what I'd committed and later rolled back.<br><br>Cheers,<br>Kaelyn<br><br><div class="gmail_quote">
On Mon, Jul 25, 2011 at 6:17 PM, Kaelyn Uhrain <span dir="ltr"><<a href="mailto:rikka@google.com">rikka@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
Just realized my patch missed the new unit test file. d'oh!<div><div></div><div class="h5"><br><br><div class="gmail_quote">On Sat, Jul 23, 2011 at 11:35 AM, Ted Kremenek <span dir="ltr"><<a href="mailto:kremenek@apple.com" target="_blank">kremenek@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"><div style="word-wrap: break-word;">This all sounds good to me. I think the patch can go in as is.<br>
<div><br><div><div>
<div></div><div><div>On Jul 21, 2011, at 4:04 PM, Kaelyn Uhrain wrote:</div><br></div></div><blockquote type="cite"><div><div></div><div>Hi Ted,<br><br>I've attached a new version of my patch that moves the pointer arithmetic portion of the -Warray-bounds improvements into a separate flag, -Warray-bounds-pointer-arithmetic. Since the remaining noise in the pointer arithmetic check is more a case of undefined behavior that does the right thing with most (all?) compilers than a case of being true false positives, the plan is to have -Warray-bounds-pointer-arithmetic initially be disabled by default and in later patches to enhance the warning by e.g. providing a fixit note to add parens to exprs like "ptr + sizeof("foo") - 1" so they become "ptr + (sizeof("foo")) - 1".<br>
<br>Cheers,<br>Kaelyn<br><br><div class="gmail_quote">On Thu, Jul 21, 2011 at 1:18 PM, Ted Kremenek <span dir="ltr"><<a href="mailto:kremenek@apple.com" target="_blank">kremenek@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<div style="word-wrap: break-word;"><div>Hi Kaelyn,</div><div><br></div>While I think there will be some contention here, I think this is still too high of a false positive rate for this warning to be on by default in the compiler. Users simply aren't going to tolerate it. I think it is reasonable for a codebase to adopt a strict policy for this warning, but I don't think a 17% false positive rate (or possibly higher) is acceptable for a default warning for all users of Clang.<div>
<br></div><div>Unless you believe you think there are additional heuristics to drop the false positive rate down below 5-10%, I think I'm fine with proceeding as putting this in as an opt-in warning, and then refine from there.<br>
<div><br><div><div><div></div><div><div>On Jul 20, 2011, at 11:39 AM, Kaelyn Uhrain wrote:</div><br></div></div><blockquote type="cite"><div><div></div><div>I've attached an updated version of my patch that better handles cases where pointer arithmetic is done after casting a constant-size array to a pointer for a smaller base type (e.g. casting an int array to char*). Of the pointer arithmetic warnings, about 24% could be considered false positives; however, the actual number of false positives is quite small and 2/3 of them stem from the use of a single macro--if you count those as a single warning & false positive, the rate drops to 17%. Of the false positives most are from semi-questionable pointer arithmetic where a constant greater than the length of the array/pointer is being added to the pointer and some int > 1 being subtracted from it, e.g.:<br>
<br>void foo(int n) {<br> char x[5];<br> if (n > 0) bar(x + 6 - n);<br>}<br><br>Cheers,<br>Kaelyn<br><br><div class="gmail_quote">On Tue, Jul 19, 2011 at 11:44 AM, Kaelyn Uhrain <span dir="ltr"><<a href="mailto:rikka@google.com" target="_blank">rikka@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">I'm still looking at the pointer arithmetic warnings to determine which are false positives, but I realized I screwed up my previous stats a bit as I forgot to account for duplicated/repeated warnings (e.g. the same header included in multiple compilation units and so generating the same warnings multiple times). For unique warnings, the stats are:<br>
<br>- 16.5% increase in warnings from before my patch (originally reported a 24% increase)<br>- 47% of those new warnings being about pointer arithmetic<br>- 6.7% of all of the warnings emitted with my patch applied are concerning pointer arithmetic. (originally reported 8.3%)<br>
- The new pointer arithmetic warnings represent a 7.7% increase in warnings from before my patch, not 10%.<br><br>I'll send another email once I have a feel for how noisy the pointer arithmetic warnings are.<br><br>Cheers,<br>
<font color="#888888">
Kaelyn</font><div><div></div><div><br><br><div class="gmail_quote">On Mon, Jul 18, 2011 at 5:54 PM, Kaelyn Uhrain <span dir="ltr"><<a href="mailto:rikka@google.com" target="_blank">rikka@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
Ted,<br><br>You're welcome. I'll try to figure out what fraction of the pointer arithmetic warnings are false positives (requires a bit of manual digging on my part to determine if the code is indeed buggy or if it is valid / intended). For the overall 24% increase in warnings, keep in mind that over half of that is the existing bounds checking now being applied to cases where it wasn't before, i.e.:<br>
<br>char *foo[5];<br>foo[77]; // -Warray-bounds already found<br>&foo[77]; // -Warray-bounds currently misses<br>*foo[77]; // -Warray-bounds currently misses<br><br>The function that did the bounds checking would never catch the last two cases because it would see a UnaryOperator (in the above cases for the '&' and the '*') and skip the expression instead of looking inside the UnaryOperator expression for the array subscripting.<br>
<br>The new pointer arithmetic bounds checking only represents a 10% increase in warnings--and IMHO that is the only part where the number of false positives introduced might be an issue.<br><br>Cheers,<br><font color="#888888">Kaelyn</font><div>
<div></div><div><br><br><div class="gmail_quote">
On Mon, Jul 18, 2011 at 5:36 PM, Ted Kremenek <span dir="ltr"><<a href="mailto:kremenek@apple.com" target="_blank">kremenek@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<div style="word-wrap: break-word;">Hi Kaelyn,<div><br></div><div>Thanks for the statistics. What would be good to know is what fraction of these are false positives (i.e., are these all real bugs)? A small random sample might be helpful. A 24% increase in warnings is fairly substantial, and we don't want to do that unless there is a real benefit.</div>
<div><br></div><font color="#888888"><div>Ted</div></font><div><div></div><div><div><br><div><div>On Jul 18, 2011, at 5:18 PM, Kaelyn Uhrain wrote:</div><br><blockquote type="cite"><div class="gmail_quote">Ted,<br>
<br>On Thu, Jul 14, 2011 at 5:04 PM, Ted Kremenek <span dir="ltr"><<a href="mailto:kremenek@apple.com" target="_blank">kremenek@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<div style="word-wrap: break-word;"><br><div>The only other issue: should this be controlled under a separate warning flag, at least initially so we can experiment with this new warning and see how noisy it is? E.g. "-Warray-bounds-pointer-arithmetic".</div>
</div></blockquote></div><br>I've tested the -Warray-bounds changes against the Google codebase and my patch increases the number of warnings from -Warray-bounds by 24%. Of the new warnings, 57.33% are for array indexes that most likely weren't picked up before because of unary operators like & or * (11.1% of all the warnings now emitted), and the remaining 42.67% are from out-of-bounds pointer arithmetic (8.3% of all the warnings from -Warray-bounds).<br>
<br>Cheers,<br>Kaelyn<br>
</blockquote></div><br></div></div></div></div>
</blockquote></div><br>
</div></div></blockquote></div><br>
</div></div></blockquote></div><br>
</div></div><span><array-bounds-enhancement3.diff></span></blockquote></div><br></div></div></div></blockquote></div><br>
</div></div><span><array-bounds-enhancement4.diff></span></blockquote></div><br></div></div>
</blockquote></div><br>
</div></div></blockquote></div><br>
<span><array-bounds-enhancement-changes.diff></span>_______________________________________________<br>cfe-commits mailing list<br><a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits<br></blockquote></div><br></div></body></html>