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