[cfe-dev] -Warray-bounds seems over-zealous on Clang

[Peter Geoghegan]

On 15 July 2011 11:24, Nicola Gigante <nicola.gigante at gmail.com> wrote:
>
> Il giorno 13/lug/2011, alle ore 22.21, John McCall ha scritto:
>
> On Jul 13, 2011, at 1:10 PM, Nicola Gigante wrote:
>
> Il giorno 13/lug/2011, alle ore 17.49, Ted Kremenek ha scritto:
>
> On Jul 13, 2011, at 8:05 AM, David Blaikie wrote:
>
> So, options that seem to be being discussed include:
>
> 1) suppress this warning in all cases where the array is of length 1 and the
> last element in a struct
>   1.1) refinement: only when the length is specified explicitly and not via
> macro expansion, etc. (as John suggested)
>   1.2) refinement: under c99 recommend a fixup to use flexible arrays
> 2) split the warning in two, the second being the cases suppressed by the
> above option (probably less interesting if 1.1 is implemented)
>
>
> (1) and (2) aren't mutually exclusive.  (2) is still useful when the
> heuristics implied by 1.1 and 1.2 aren't good enough.
>
> I've tried to write such a patch, as it seemed simple, but I'm stuck because
> I don't know enough
> about internal clang's APIs yet.
> Until now, I've come up with the simple patch attached, that disables the
> warning if
> the array is declared inside a record type and the size is one.
> Questions:
> - From the NamedDecl* object representing the array declaration, how do I
> know if it's declared last in the struct?
>
> It should be a FieldDecl, and its parent should be a RecordDecl.  I don't
> think there's a cleaner solution than just iterating through the fields of
> the record and complaining if it's not the last one.
>
> - From the NamedDecl*, how do I know if the size of the member declaration
> comes from a macro expansion?
>
> The exception should apply to:
>   - a field of a struct, where
>   - the field's TypeLoc is a (possibly parenthesized) ConstantArrayTypeLoc
> and
>   - the size expression in that TypeLoc is a (possibly parenthesized)
> IntegerLiteral and
>   - the source location of that literal is not a macro ID.
>
> Hello.
> I've attached the patch. It seems to work as intended. The only thing
> missing is to split up
> the warning. How to do it?
> Also what do you mean by "possibly parenthesized" ConstantArrayTypeLoc and
> IntegerLiteral?

While I thank you for your efforts, I don't see why we have to
explicitly use C89 to avoid the warning, since we're only avoiding the
warning under circumstances exactly consistent with the use of the
idiom.

What's valid in C89 is valid in C99, with very few exceptions. Just
because this is arguably bad style in C99 does not mean that we should
see a warning - warnings are supposed to prevent downright errors that
are still valid code. No one would seriously suggest that we should
have warnings when C style casts are used in C++ on non-POD types,
even though they're extremely hazardous there. Precedent matters.

Consider that in PostgreSQL, a large C application with almost 1000 C
files, we only see this warning 4 or 5 times. The sorts of errors we
hope to protect people from with this warning are very unlikely to be
statically detectable.

-- 
Peter Geoghegan       http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training and Services