[cfe-commits] PATCH: Enhance array bounds checking

[Kaelyn Uhrain]

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 <rikka at google.com> wrote:

> Just realized my patch missed the new unit test file. d'oh!
>
>
> On Sat, Jul 23, 2011 at 11:35 AM, Ted Kremenek <kremenek at apple.com> 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 <kremenek at apple.com> 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 <rikka at google.com>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 <rikka at google.com>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 <kremenek at apple.com>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 <kremenek at apple.com>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>
>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110727/d5b2e7ba/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: array-bounds-enhancement-changes.diff
Type: application/octet-stream
Size: 10648 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110727/d5b2e7ba/attachment.obj>