Consider the common idiom in C, of a struct with its last declared
type is an array like this:
typedef struct
{
int id;
int values[1];
} my_struct;
This will be stored in dynamically allocated memory. Memory will be
allocated for my_struct, in addition to however many additional
integers must be stored. Despite how frequently this is seen, Clang
doesn't seem to like this. In particular, it over-zealously complains
about assigning past the end of "values" when that can be statically
determined (because an int rvalue is used), when the -Warray flag is
given. GCC, on the other hand, does not. This is of particular concern
when hacking on the PostgreSQL code, that makes extensive use of this
idiom:
/home/peter/build/Release/bin/clang -O2 -Wall -Wmissing-prototypes
-Wpointer-arith -Wdeclaration-after-statement -Wendif-labels
-Wformat-security -fno-strict-aliasing -fwrapv -I../../../src/include
-D_GNU_SOURCE -c -o namespace.o namespace.c
namespace.c:1497:29: warning: array index of '1' indexes past the end
of an array (that contains 1 elements) [-Warray-bounds]
operform->oprright ==
resultList->args[1])
^ ~
../../../src/include/catalog/namespace.h:37:2: note: array 'args' declared here
Oid args[1]; /* arg types
--- VARIABLE LENGTH ARRAY */
^
namespace.c:1509:30: warning: array index of '1' indexes past the end
of an array (that contains 1 elements) [-Warray-bounds]
operform->oprright == prevResult->args[1])
^ ~
../../../src/include/catalog/namespace.h:37:2: note: array 'args' declared here
Oid args[1]; /* arg types
--- VARIABLE LENGTH ARRAY */
^
namespace.c:1540:3: warning: array index of '1' indexes past the end
of an array (that contains 1 elements) [-Warray-bounds]
newResult->args[1] = operform->oprright;
^ ~
../../../src/include/catalog/namespace.h:37:2: note: array 'args' declared here
Oid args[1]; /* arg types
--- VARIABLE LENGTH ARRAY */
^
This seems like a bug to me. What's the consensus view on this?
Consider the common idiom in C, of a struct with its last declared
type is an array like this:
typedef struct
{
int id;
int values[1];
} my_struct;
This will be stored in dynamically allocated memory. Memory will be
allocated for my_struct, in addition to however many additional
integers must be stored. Despite how frequently this is seen, Clang
doesn't seem to like this. In particular, it over-zealously complains
about assigning past the end of "values" when that can be statically
determined (because an int rvalue is used), when the -Warray flag is
given. GCC, on the other hand, does not. This is of particular concern
when hacking on the PostgreSQL code, that makes extensive use of this
idiom:
/home/peter/build/Release/bin/clang -O2 -Wall -Wmissing-prototypes
-Wpointer-arith -Wdeclaration-after-statement -Wendif-labels
-Wformat-security -fno-strict-aliasing -fwrapv -I../../../src/include
-D_GNU_SOURCE -c -o namespace.o namespace.c
namespace.c:1497:29: warning: array index of '1' indexes past the end
of an array (that contains 1 elements) [-Warray-bounds]
operform->oprright ==
resultList->args[1])
^ ~
../../../src/include/catalog/namespace.h:37:2: note: array 'args' declared here
Oid args[1]; /* arg types
--- VARIABLE LENGTH ARRAY */
^
namespace.c:1509:30: warning: array index of '1' indexes past the end
of an array (that contains 1 elements) [-Warray-bounds]
operform->oprright == prevResult->args[1])
^ ~
../../../src/include/catalog/namespace.h:37:2: note: array 'args' declared here
Oid args[1]; /* arg types
--- VARIABLE LENGTH ARRAY */
^
namespace.c:1540:3: warning: array index of '1' indexes past the end
of an array (that contains 1 elements) [-Warray-bounds]
newResult->args[1] = operform->oprright;
^ ~
../../../src/include/catalog/namespace.h:37:2: note: array 'args' declared here
Oid args[1]; /* arg types
--- VARIABLE LENGTH ARRAY */
^
This seems like a bug to me. What's the consensus view on this?
I suggest using a C99 flexible array member, e.g.,
Saying "just use C99" isn't a practical solution. This is a mature
project with a large codebase, that targets many compilers, including,
for a large number of reasons, Visual Studio - Microsoft have
absolutely no interest in pursuing C99 support.
Besides, the chances of Clang actually helpfully diagnosing a problem
with the delta between how GCC does -Warray-bounds and how Clang does
it are slim - how often are these problems statically detectable? This
is C.
Even if your position wasn't unreasonable, which it is, Clang still
gives this warning when the -c89 flag is given.
Saying "just use C99" isn't a practical solution. This is a mature
project with a large codebase, that targets many compilers, including,
for a large number of reasons, Visual Studio - Microsoft have
absolutely no interest in pursuing C99 support.
Then disable the warning. It's an option for a reason.
Besides, the chances of Clang actually helpfully diagnosing a problem
with the delta between how GCC does -Warray-bounds and how Clang does
it are slim - how often are these problems statically detectable? This
is C.
I know at least one bug found with handling of [1] sized arrays and GCC
4.5 warned about that too.
Even if your position wasn't unreasonable, which it is, Clang still
gives this warning when the -c89 flag is given.
Just because you don't agree doesn't make it unreasonable. The behavior
of the code doesn't change with the standard level -- you are still
declaring an array and overflowing it.
Saying "just use C99" isn't a practical solution. This is a mature
project with a large codebase, that targets many compilers, including,
for a large number of reasons, Visual Studio - Microsoft have
absolutely no interest in pursuing C99 support.
Flexibly array members are a common extension, as are zero-length arrays, specifically for this purpose. I'm guessing that you didn't try our suggestion before dismissing it out of hand.
Besides, the chances of Clang actually helpfully diagnosing a problem
with the delta between how GCC does -Warray-bounds and how Clang does
it are slim - how often are these problems statically detectable? This
is C.
We have empirical evidence that it *does* find bugs, otherwise we wouldn't still have the warning.
Even if your position wasn't unreasonable, which it is, Clang still
gives this warning when the -c89 flag is given.
Asking you to consider using a feature from a 12-year-old standard that was specifically designed for what you're trying to do hardly falls into the category of "unreasonable", even if you consider it to be impractical.
The *practical* solution for you is to turn off the warning with -Wno-array-bounds.
Saying "just use C99" isn't a practical solution. This is a mature
project with a large codebase, that targets many compilers, including,
for a large number of reasons, Visual Studio - Microsoft have
absolutely no interest in pursuing C99 support.
We have had the same problem in the EFI firmware open source project (edk2). The edk2 project supports Visual Studio 20XX, gcc, RVCT, clang, ...
Besides, the chances of Clang actually helpfully diagnosing a problem
with the delta between how GCC does -Warray-bounds and how Clang does
it are slim - how often are these problems statically detectable? This
is C.
We have found real bugs in chipset and driver code with clangs -Warray-bounds, that the other compilers did not find.
So, given it is C, we just add a int *value = &A.value[0], and replace A.value[n] with value[n].
Luckily for us the case of the value[1] variable length array were mostly in regards to processing data structures from specifications and we had a single C function per instance that was doing the parsing. The changes were accepted to the open source project, so we did not have to turn off -Warray-bounds.
I don't see what you expect Clang to do about this.
The warning is perfectly accurate. The array is declared as size 1,
and you're indexing past it. If you have a function like
struct S {
int a[1];
};
int get_second(struct S* s) {
return s->a[1];
}
That's very clearly an out of bounds error unless very specific
conditions are met (heap allocated with enough space after the
struct...) I feel like it would be impossible to prove this in every
case.
So that just leaves us with turning the warning off. (Which you can do
yourself with -Wno-array-bounds)
I wouldn't want this off by default if that's what you're suggesting,
since it's a silver bullet for off-by-one bugs:
void iterate() {
int a[4];
for(size_t i = 0; i <= 4; i++)
a[i] += 4;
}
Do we have empirical evidence that it finds bugs in arrays with exactly 1 element? I think we should just disable it in the case that the array has a single element. This really is a common pattern.
Besides, the chances of Clang actually helpfully diagnosing a problem
with the delta between how GCC does -Warray-bounds and how Clang does
it are slim - how often are these problems statically detectable? This
is C.
We have empirical evidence that it *does* find bugs, otherwise we wouldn't still have the warning.
Do we have empirical evidence that it finds bugs in arrays with exactly 1 element? I think we should just disable it in the case that the array has a single element. This really is a common pattern.
I tend to agree, but maybe there is a good compromise. What about changing this warning to whitelist only one-element arrays which are inside of some record type? That would still catch local arrays, global arrays, etc.
We could even then add a special (extension?) warning for one-element arrays inside of record types which are accessed passed the bounds only in modes where flexible array members are available, maybe even with a fixit note that converts the declaration.
Do we have empirical evidence that it finds bugs in arrays with exactly 1 element? I think we should just disable it in the case that the array has a single element. This really is a common pattern.
I would think it reasonable if the warning was disabled for this exact
pattern only.
Disabling -Wno-array-bounds clearly isn't a practical measure, as most
of the ways that that can trigger a warning are perfectly valid. I
wouldn't be opposed to showing the warning where the struct is known
to have been declared on the stack, but going that far probably isn't
worth the effort.
My inclination and I believe the inclination of others is that, if it
catches real bugs, which Nico has shown it has, then it's worth having
the warning. Another compromise would be to have a flag about bounds
checks warnings for size 1 arrays at the end of structs (phew). =P
If you can't use C99, are flexible arrays available as an extension in
clang, either in gnu89 or via some flag? If so, you could use ifdefs
like this:
#ifdef __GNUC__
/* or some other condition, __has_feature or what have you */ #define FLEXIBLE_ARRAY #else #define FLEXIBLE_ARRAY 1 #endif
struct Buffer {
int len;
char bytes[FLEXIBLE_ARRAY];
};
My inclination and I believe the inclination of others is that, if it
catches real bugs, which Nico has shown it has, then it's worth having
the warning.
It's going to cause spurious warnings for a large number of real-world
applications. Do you think that that will endear developers of those
applications to Clang? The fact that it may catch bugs in some
extremely unlikely scenario (recall that I only want to avoid the
warning for this very specific case) should not outweigh this. Isn't
that obvious?
Another compromise would be to have a flag about bounds
checks warnings for size 1 arrays at the end of structs (phew). =P
As I said, I would think it reasonable if the warning was disabled for
this exact pattern only, so that for example it would not occur if the
array had more than 1 element, or wasn't the last piece of data in the
struct.
If you can't use C99, are flexible arrays available as an extension in
clang, either in gnu89 or via some flag? If so, you could use ifdefs
like this:
#ifdef __GNUC__
/* or some other condition, __has_feature or what have you */ #define FLEXIBLE_ARRAY #else #define FLEXIBLE_ARRAY 1 #endif
struct Buffer {
int len;
char bytes[FLEXIBLE_ARRAY];
};
It's even self-documenting.
It's gross. Our community is obsessed with code hygiene, and that will
never fly just to get Clang to stop complaining while every single
other compiler, of which there are quite a few (PostgreSQL is supposed
to be highly portable), will not complain.
#ifdefGNUC
/* or some other condition, __has_feature or what have you */ #define FLEXIBLE_ARRAY #else #define FLEXIBLE_ARRAY 1 #endif
struct Buffer {
int len;
char bytes[FLEXIBLE_ARRAY];
};
It’s even self-documenting.
It’s gross. Our community is obsessed with code hygiene, and that will
never fly just to get Clang to stop complaining while every single
other compiler, of which there are quite a few (PostgreSQL is supposed
to be highly portable), will not complain.
What exactly do you mean by code hygiene in this regard? Why would the above be unhygienic?
I think someone mentioned the idea of adding a separate warning for this narrower case and, in the case of compiling for c99, suggesting a fixup of using a flexible array. In the case of c89 at least it’d be a separate warning that, if the pattern is used extensively in the codebase, could be disabled separately from the more general warning (which would be adjusted to ignore these cases).
This would allow for the bug that was mentioned to have been found, while allowing you to disable this warning if you don’t think the chance of such bugs coming up is worth the code hygiene cost of the above construct or the move to c99 would be worthwhile in your case.
While strictly speaking that would be a valid solution, it would also
be overly-complex considering the benefits IMHO. I'd now have a flag
to pass to Clang, but only to Clang, despite the fact that it's
supposed to be a drop-in replacement for GCC. I'd now have to
explicitly indicate that I was using C89.
Once again, the burden would be on application developers who are just
using a demonstrably mainstream idiom, all to prevent obvious bugs in
poorly written applications under very specific circumstances, that
will only occur a vanishingly small number of times, and will never be
noticed by GCC either.
I think someone mentioned the idea of adding a separate warning for this
narrower case and, in the case of compiling for c99, suggesting a fixup of
using a flexible array. In the case of c89 at least it'd be a separate
warning that, if the pattern is used extensively in the codebase, could be
disabled separately from the more general warning (which would be adjusted
to ignore these cases).
This would allow for the bug that was mentioned to have been found, while
allowing you to disable this warning if you don't think the chance of such
bugs coming up is worth the code hygiene cost of the above construct or the
move to c99 would be worthwhile in your case.
While strictly speaking that would be a valid solution, it would also
be overly-complex considering the benefits IMHO. I'd now have a flag
to pass to Clang, but only to Clang, despite the fact that it's
supposed to be a drop-in replacement for GCC. I'd now have to
explicitly indicate that I was using C89.
In the past, warnings that warns about a common pattern but also find
bugs have been kept on-by-default. Search this list for
-Wdelete-non-virtual-dtor.
Once again, the burden would be on application developers who are just
using a demonstrably mainstream idiom, all to prevent obvious bugs in
poorly written applications under very specific circumstances, that
will only occur a vanishingly small number of times, and will never be
noticed by GCC either.
This bug is in ICU, which is pretty commonly used. Someone (me) posted
an example where this warning found a bug within 2h of Chris asking
for it, so maybe it's not as vanishingly small as you think.
