[cfe-commits] r125640 - in /cfe/trunk: include/clang/Basic/DiagnosticSemaKinds.td include/clang/Sema/Sema.h lib/Sema/SemaChecking.cpp lib/Sema/SemaExpr.cpp test/Analysis/out-of-bounds.c test/Sema/array-bounds.c

[Ted Kremenek]

On Feb 17, 2011, at 3:08 AM, Chandler Carruth wrote:

> This breaks the Chrome build, where we have some code like this:
> 
> template <bool extendArray>
> void myFunc() {
>    int arr[3 + (extendArray ? 1 : 0)];
> 
>    if (extendArray)
>        arr[3] = 42;
> }
> 
> You can always explicitly specialize myFunc for true and false. I think it would make the resulting code strictly more clean. I'm kinda glad the warning fires on this type of construct.

While I agree with that assessment, I'm not certain why we are getting a ConstantArrayType whose size is 3 if the only instantiation of myFunc is for extendArray = false.  I'd like to know *why* the warning is firing here.

>  
> void f() {
>    myFunc<false>();
> }
> 
> (The real code is here:
> http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/filters/FEConvolveMatrix.cpp#L245)
> 
> In fact, it seems to warn also in a case like this:
> 
> void f() {
>    int arr[42];
>    if (0)
>        arr[100] = 5;
> }
> 
> Similarly, why write the code this way? Now, we could perhaps suppress the warning when the entire subscript expression comes from a macro expansion (the only conceivable way I see for this to be strongly intentional code), but I'd like to understand how often it happens in code.

W.r.t. macros, I have the opposite opinion.  Macros seem like a great place where these kind of bugs take root.

Aside from whether or not the array access can trigger (due to path reachability), this is meant to be a 100% accurate warning.  That is, *if* the access were to occur, it would definitely be an out-of-bounds access.  I see no reason not to warning for such code.  They are just time bombs waiting to go off.

>  
> Would it be possible to make the warning a bit more conservative?
> 
> The examples you gave are rather hard, they would involve proper control flow analysis. That doesn't belong in warnings.

I'm fine with throwing (limited) flow-analysis at warnings, but only when its needed.  We already construct CFGs for some of our warnings, and the CFGBuilder already understands the case of 'if (0)' and pruning those basic blocks from the CFG.  We could easily suppress these cases here with negligible compile-time penalty in the common case (i.e., only do the flow analysis when the warning triggers).  I'm just not convinced that we should.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110217/be0601b7/attachment.html>