Hi Kaelyn, 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.
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: 7578 CheckArrayAccess(lhs.get()); 7579 CheckArrayAccess(rhs.get()); 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? Thanks so much, Ted On Jul 27, 2011, at 4:18 PM, Kaelyn Uhrain wrote: > Ted & Eric, > > 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 > http://codereview.appspot.com/4675068 and attached a diff of the changes from > what I'd committed and later rolled back. > > Cheers, > Kaelyn > > On Mon, Jul 25, 2011 at 6:17 PM, Kaelyn Uhrain <[email protected]> wrote: > Just realized my patch missed the new unit test file. d'oh! > > > On Sat, Jul 23, 2011 at 11:35 AM, Ted Kremenek <[email protected]> wrote: > This all sounds good to me. I think the patch can go in as is. > > On Jul 21, 2011, at 4:04 PM, Kaelyn Uhrain wrote: > >> Hi Ted, >> >> 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". >> >> Cheers, >> Kaelyn >> >> On Thu, Jul 21, 2011 at 1:18 PM, Ted Kremenek <[email protected]> wrote: >> Hi Kaelyn, >> >> 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. >> >> 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. >> >> On Jul 20, 2011, at 11:39 AM, Kaelyn Uhrain wrote: >> >>> 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.: >>> >>> void foo(int n) { >>> char x[5]; >>> if (n > 0) bar(x + 6 - n); >>> } >>> >>> Cheers, >>> Kaelyn >>> >>> On Tue, Jul 19, 2011 at 11:44 AM, Kaelyn Uhrain <[email protected]> wrote: >>> 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: >>> >>> - 16.5% increase in warnings from before my patch (originally reported a >>> 24% increase) >>> - 47% of those new warnings being about pointer arithmetic >>> - 6.7% of all of the warnings emitted with my patch applied are concerning >>> pointer arithmetic. (originally reported 8.3%) >>> - The new pointer arithmetic warnings represent a 7.7% increase in warnings >>> from before my patch, not 10%. >>> >>> I'll send another email once I have a feel for how noisy the pointer >>> arithmetic warnings are. >>> >>> Cheers, >>> Kaelyn >>> >>> >>> On Mon, Jul 18, 2011 at 5:54 PM, Kaelyn Uhrain <[email protected]> wrote: >>> Ted, >>> >>> 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.: >>> >>> char *foo[5]; >>> foo[77]; // -Warray-bounds already found >>> &foo[77]; // -Warray-bounds currently misses >>> *foo[77]; // -Warray-bounds currently misses >>> >>> 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. >>> >>> 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. >>> >>> Cheers, >>> Kaelyn >>> >>> >>> On Mon, Jul 18, 2011 at 5:36 PM, Ted Kremenek <[email protected]> wrote: >>> Hi Kaelyn, >>> >>> 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. >>> >>> Ted >>> >>> On Jul 18, 2011, at 5:18 PM, Kaelyn Uhrain wrote: >>> >>>> Ted, >>>> >>>> On Thu, Jul 14, 2011 at 5:04 PM, Ted Kremenek <[email protected]> wrote: >>>> >>>> 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". >>>> >>>> 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). >>>> >>>> Cheers, >>>> Kaelyn >>> >>> >>> >>> >>> <array-bounds-enhancement3.diff> >> >> >> <array-bounds-enhancement4.diff> > > > > <array-bounds-enhancement-changes.diff>_______________________________________________ > cfe-commits mailing list > [email protected] > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
