Ping: https://gcc.gnu.org/ml/gcc-patches/2019-10/msg00860.html
Should I add something like the -Wzero-length-array-bounds option
to allow some of the questionable idioms seen in the kernel?
On 10/11/2019 10:34 AM, Martin Sebor wrote:
On 9/10/19 4:35 PM, Jeff Law wrote:
On 9/6/19 1:27 PM, Martin Sebor wrote:
Recent enhancements to -Wstringop-overflow improved the warning
to the point that it detects a superset of the problems -Warray-
bounds is intended detect in character accesses. Because both
warnings detect overlapping sets of problems, and because the IL
they work with tends to change in subtle ways from target to
targer, tests designed to verify one or the other sometimes fail
with a target where the warning isn't robust enough to detect
the problem given the IL representation.
To reduce these test suite failures the attached patch extends
-Warray-bounds to handle some of the same problems -Wstringop-
overflow does, pecifically, out-of-bounds accesses to array
members of structs, including zero-length arrays and flexible
array members of defined objects.
In the process of testing the enhancement I realized that
the recently added component_size() function doesn't work as
intended for non-character array members (see below). The patch
corrects this by reverting back to the original implementation
of the function until the better/simpler solution can be put in
place as mentioned below.
Tested on x86_64-linux.
Martin
[*] component_size() happens to work for char arrays because those
are transformed to STRING_CSTs, but not for arrays that are not.
E.g., in
struct S { int64_t i; int16_t j; int16_t a[]; }
s = { 0, 0, { 1, 0 } };
unless called with type set to int16_t[2], fold_ctor_reference
will return s.a[0] rather than all of s.a. But set type to
int16_t[2] we would need to know that s.a's initializer has two
elements, and that's just what we're using fold_ctor_reference
to find out.
I think this could probably be made to work somehow by extending
useless_type_conversion_p to handle this case as special somehow,
but it doesn't seem worth the effort given that there should be
an easier way to do it as you noted below.
Given the above, the long term solution should be to rely on
DECL_SIZE_UNIT(decl) - TYPE_SIZE_UNIT(decl_type) as Richard
suggested in the review of its initial implementation.
Unfortunately, because of bugs in both the C and C++ front ends
(I just opened PR 65403 with the details) the simple formula
doesn't give the right answers either. So until the bugs are
fixed, the patch reverts back to the original loopy solution.
It's no more costly than the current fold_ctor_reference
approach.
...
So no concerns with the patch itself, just the fallout you mentioned in
a follow-up message. Ideally we'd have glibc and the kernel fixed
before this goes in, but I'd settle for just getting glibc fixed since
we have more influence there.
Half of the issues there were due to a bug in the warning. The rest
are caused by Glibc's use of interior zero-length arrays to access
subsequent members. It works in simple cases but it's very brittle
because GCC assumes that even such members don't alias. If it's meant
to be a supported feature then aliasing would have to be changed to
take it into account. But I'd rather encourage projects to move away
from these dangerous hacks and towards cleaner, safer code.
I've fixed the bug in the attached patch. The rest can be suppressed
by replacing the zero-length arrays with flexible array members but
that's just trading one misuse for another. If the code can't be
changed to avoid this (likely not an option since the arrays are in
visible in the public API) I think the best way to deal with them is
to suppress them by #pragma GCC diagnostic ignored. I opened BZ 25097
in Glibc Bugzilla to track this.
Out of curiosity are the kernel issues you raised due to flexible arrays
or just cases where we're doing a better job on normal objects? I'd be
a bit surprised to find flexible arrays in the kernel.
I don't think I've come across any flexible arrays in the kernel.
The patch triggers 94 instances of -Warray-bounds (60 of which
are for distinct code) in 21 .c files. I haven't looked at all
of them but some of the patterns I noticed are:
1) Intentionally using an interior zero-length array to access
(e.g., memset) one or more subsequent members. E.g.,
_dbgp_external_startup in drivers/usb/early/ehci-dbgp.c and
quite a few others. This is pretty pervasive but seems easily
avoidable.
2) Overwriting a member array with more data (e.g., function
cxio_rdev_open in
drivers/infiniband/hw/cxgb3/cxio_hal.c or in function
pk_probe in drivers/hid/hid-prodikeys.c). At first glance
some of these look like bugs but with stuff obscured by macros
and no comments it's hard to tell.
3) Uses of the container_of() macro to access one member given
the address of another. This is undefined (and again breaks
the aliasing rules) but the macro is used all over the place
in the kernel. I count over 15,000 references to it.
4) Uses of one-element arrays as members of other one-element
arrays (in include/scsi/fc/fc_ms.h). Was this ever meant
to be supported by GCC? (It isn't by _FORTIFY_SOURCE=2.)
5) Possible false positives due to the recent loop unrolling
change.
It will be a quite a bit of work to clean this up. To make it
easier we would introduce a new option to control the warning
for some of the most common idioms, such as
-Wzero-length-array-bounds. I'm not too wild about this because
it would just paper over the problem. A better solution would
also involve avoiding the aliasing assumptions for overlapping
zero-length member arrays.
Anyway, attached is the updated patch with just the one fix
I mentioned above, retested on x86_64-linux.
Martin
