[PATCH] c/67882 - improve -Warray-bounds for invalid offsetof

Martin Sebor msebor@gmail.com
Tue Oct 20 15:33:00 GMT 2015


On 10/20/2015 07:20 AM, Bernd Schmidt wrote:
> On 10/16/2015 09:34 PM, Martin Sebor wrote:
>> Thank you for the review. Attached is an updated patch that hopefully
>> addresses all your comments. I ran the check_GNU_style.sh script on
>> it to make sure I didn't miss something.  I've also added replies to
>> a few of your comments below.
>
> Ok, thanks. However - the logic around the context struct in the patch
> still seems fairly impenetrable to me, and I've been wondering whether a
> simpler approach wouldn't work just as well. I came up with the patch
> below, which just passes a tree_code context to recursive invocations
> and increasing the upper bound by one only if not in an ARRAY_REF or
> COMPONENT_REF context.
>
> As far as I can tell, this produces the same warnings on the testcase
> (modulo differences in type printout), except for the final few FA5_7
> testcases, which I had doubts about anyway:
>
> typedef struct FA5_7 {
>    int i;
>    char a5_7 [5][7];
> } FA5_7;
>
>      __builtin_offsetof (FA5_7, a5_7 [0][7]),         // { dg-warning
> "index" }
>      __builtin_offsetof (FA5_7, a5_7 [1][7]),         // { dg-warning
> "index" }
>      __builtin_offsetof (FA5_7, a5_7 [5][0]),         // { dg-warning
> "index" }
>      __builtin_offsetof (FA5_7, a5_7 [5][7]),         // { dg-warning
> "index" }
>
> Why wouldn't at least the first two be convered by the
> one-past-the-end-is-ok rule?

Thanks for taking the time to try to simplify the patch!

Diagnosing the cases above is at the core of the issue (and can
be seen by substituting a5_7 into the oversimplified test case
in the bug).

In a 5 by 7 array, the offset computed for the out-of-bounds
a_5_7[0][7] is the same as the offset computed for the in-bounds
a_5_7[1][0].  The result might be unexpected because it suggests
that two "distinct" elements of an array share the same offset.

The "one-past-the-end rule" applies only to the offset corresponding
to the address just past the end of the whole array because like its
address, the offset of the element just beyond the end of the array
is valid and cannot be mistaken for an offset of a valid element.

My initial approach was similar to the patch you attached.  Most
of the additional complexity (i.e., the recursive use of the context)
grew out of wanting to diagnose these less than trivial cases of
surprising offsets in multi-dimensional arrays.  I don't think it
can be done without passing some state down the recursive calls but
I'd be happy to be proven wrong.

FWIW, teh bug and my patch for it were precipitated by looking for
the problems behind PR 67872 - missing -Warray-bounds warning (which
was in turn prompted by developing and testing a solution for PR
67942 - diagnose placement new buffer overflow).

I think -Warray-bounds should emit consistent diagnostics for invalid
array references regardless of the contexts. I.e., given

     struct S {
         int A [5][7];
         int x;
     } s;

these should both be diagnosed:

     int i = offsetof (struct S, A [0][7]);

     int *p = &s.A [0][7];

because they are both undefined and both can lead to surprising
results when used.

With my patch for 67882, GCC diagnoses the first case. I hope to
work on 67872 in the near future to diagnose the second to bring
GCC on par with Clang. (Clang doesn't yet diagnose any offsetof
errors).

Martin



More information about the Gcc-patches mailing list