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.
>
> gcc-91647.diff
>
> PR middle-end/91679 - missing -Warray-bounds accessing a member array in a
> local buffer
> PR middle-end/91647 - new FAILs for Warray-bounds-8 and Wstringop-overflow-3.C
> PR middle-end/91463 - missing -Warray-bounds accessing past the end of a
> statically initialized flexible array member
>
> gcc/ChangeLog:
>
> PR middle-end/91679
> PR middle-end/91647
> PR middle-end/91463
> * builtins.c (component_size): Rename to component_ref_size and move...
> (compute_objsize): Adjust to callee name change.
> * tree-vrp.c (vrp_prop::check_array_ref): Handle trailing arrays with
> initializers.
> (vrp_prop::check_mem_ref): Handle declared struct objects.
> * tree.c (last_field): New function.
> (array_at_struct_end_p): Handle MEM_REF.
> (get_initializer_for, get_flexarray_size): New helpers.
> (component_ref_size): ...move here from builtins.c. Make extern.
> Use get_flexarray_size instead of fold_ctor_reference.
> * tree.h (component_ref_size): Declare.
> * wide-int.h (generic_wide_int <storage>::sign_mask): Assert invariant.
>
> gcc/testsuite/ChangeLog:
>
> PR middle-end/91679
> PR middle-end/91647
> PR middle-end/91463
> * c-c++-common/Warray-bounds-2.c: Disable VRP. Adjust expected
> messages.
> * gcc.dg/Warray-bounds-44.c: New test.
> * gcc.dg/Warray-bounds-45.c: New test.
> * gcc.dg/Wstringop-overflow-16.c: Adjust text of expected messages.
> * gcc.dg/pr36902.c: Remove xfail.
> * gcc.dg/strlenopt-57.c: Add an expected warning.
>
> Index: gcc/tree.c
> ===================================================================
> --- gcc/tree.c (revision 275387)
> +++ gcc/tree.c (working copy)
> @@ -13860,6 +13902,134 @@ component_ref_field_offset (tree exp)
[ ... ]
> + /* If the flexible array member has an known size use the greater
> + of it and the tail padding in the enclosing struct.
> + Otherwise, when the size of the flexible array member is unknown
> + and the referenced object is not a struct, use the size of its
> + type when known. This detects sizes of array buffers when cast
> + to struct tyoes with flexible array members. */
s/tyoes/types/
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.
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.
jeff
