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
