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
Description: Binary data
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
