[cfe-commits] PATCH: Enhance array bounds checking

Mon Aug 1 14:18:36 PDT 2011

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 <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>
> 
> 
> 
> <array-bounds-enhancement-changes.diff>_______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110801/8497b278/attachment.html>