On Thu, 27 Apr 2017, Alexander Monakov wrote:
> On Thu, 27 Apr 2017, Richard Biener wrote:
> > struct q { int n; long o[100]; };
> > struct r { int n; long o[0]; };
> >
> > union {
> > struct r r;
> > struct q q;
> > } u;
> >
> > int foo (int i, int j)
> > {
> > long *q = u.r.o;
> > u.r.o[i/j] = 1;
> > return q[2];
> > }
> >
> > but nothing convinced scheduling to move the load before the store ;)
> > The two memory references are seen as not aliasing though. Stupid
> > scheduler.
>
> On x86 there's an anti-dependence on %eax that prevents the second scheduler
> from performing the breaking motion; with -fschedule-insns, pre-RA scheduler
> is actually able to move the load too early, but then IRA immediately undoes
> that. Also, -fselective-scheduling2 is able to move the load early via
> renaming
> (and uncovers the miscompile, as nothing undoes it later).
>
> Applying division to the result of the load, rather than the address of the
> store, also produces code demonstrating the miscompile:
>
> struct q { int n; long o[100]; };
> struct r { int n; long o[0]; };
>
> union {
> struct r r;
> struct q q;
> } u;
>
> int foo (int i, int j)
> {
> long *q = u.r.o;
> u.r.o[i] = 1;
> return q[2]/j;
> }
Thanks. It also is still miscompiled because array_at_struct_end_p
is somewhat confused as to what its semantics are supposed to be.
Multiple callers (including the new one) assume when it returns false
they can trust the array domain while in reality when it returns false
it says we can constrain the access even if only because we know an
underlying decls size ...
It also raises the question whether for
struct X
{
double x;
int a[1];
} x;
x.a is an array at struct end -- there's enough padding to provide
space for x.a[1] even though its size doesn't include that (and
whether it can depends on the alignment of 'double'). This is
sort-of what happens for the union case above -- u.r.o can be
extended into the padding (which is quite large due to u.q.o).
The question is whether we allow such access to padding in general
(not only when the array is at struct end). Likewise do we allow, in
struct Y
{
struct X[4][4] a;
} y;
for y.a[i][j].a[k] k == 3? that is, allow the "last" element of
an array to be flexibly extended? Or do we instead allow
y.a[i][j] with j == 4, thus the last dimension of a multidimensional
array to be "over-allocated"? Or do we just allow i > 3?
There's another PR where array-bound warnings are confused by
the weird answer from array_at_struct_end_p and two other users
I skimmed would generate wrong code because they trust the array
domain after it.
Looks like I have to refactor that a bit, like with the following
which makes things a bit more explicit, and _not_ allowing to
use padding appart when using flexarrays (though that can't
happen, you can't instantiate those with decls) or the zero-size
array GNU extension.
It also corrects IMHO wrong behavior with VIEW_CONVERT_EXPRs
and the overly pessimistic handling of arrays of structs with
arrays at struct end or multi-dimensional arrays when not
dealing with the outermost dimension.
So I'm testing the following.
Richard.
2017-04-28 Richard Biener <rguent...@suse.de>
PR middle-end/80533
* tree.c (array_at_struct_end_p): Handle arrays at struct
end with size zero more conservatively. Refactor and treat
arrays of arrays or aggregates more strict. Fix
VIEW_CONVERT_EXPR handling.
* gcc.dg/torture/pr80533.c: New testcase.
Index: gcc/tree.c
===================================================================
--- gcc/tree.c (revision 247362)
+++ gcc/tree.c (working copy)
@@ -13222,9 +13230,19 @@ array_ref_up_bound (tree exp)
bool
array_at_struct_end_p (tree ref, bool allow_compref)
{
- if (TREE_CODE (ref) != ARRAY_REF
- && TREE_CODE (ref) != ARRAY_RANGE_REF
- && (!allow_compref || TREE_CODE (ref) != COMPONENT_REF))
+ tree atype;
+
+ if (TREE_CODE (ref) == ARRAY_REF
+ || TREE_CODE (ref) == ARRAY_RANGE_REF)
+ {
+ atype = TREE_TYPE (TREE_OPERAND (ref, 0));
+ ref = TREE_OPERAND (ref, 0);
+ }
+ else if (allow_compref
+ && TREE_CODE (ref) == COMPONENT_REF
+ && TREE_CODE (TREE_TYPE (TREE_OPERAND (ref, 1))) == ARRAY_TYPE)
+ atype = TREE_TYPE (TREE_OPERAND (ref, 1));
+ else
return false;
while (handled_component_p (ref))
@@ -13232,19 +13250,43 @@ array_at_struct_end_p (tree ref, bool al
/* If the reference chain contains a component reference to a
non-union type and there follows another field the reference
is not at the end of a structure. */
- if (TREE_CODE (ref) == COMPONENT_REF
- && TREE_CODE (TREE_TYPE (TREE_OPERAND (ref, 0))) == RECORD_TYPE)
+ if (TREE_CODE (ref) == COMPONENT_REF)
{
- tree nextf = DECL_CHAIN (TREE_OPERAND (ref, 1));
- while (nextf && TREE_CODE (nextf) != FIELD_DECL)
- nextf = DECL_CHAIN (nextf);
- if (nextf)
- return false;
+ if (TREE_CODE (TREE_TYPE (TREE_OPERAND (ref, 0))) == RECORD_TYPE)
+ {
+ tree nextf = DECL_CHAIN (TREE_OPERAND (ref, 1));
+ while (nextf && TREE_CODE (nextf) != FIELD_DECL)
+ nextf = DECL_CHAIN (nextf);
+ if (nextf)
+ return false;
+ }
}
+ /* If we have a multi-dimensional array we do not consider
+ a non-innermost dimension as flex array if the whole
+ multi-dimensional array is at struct end.
+ Same for an array of aggregates with a trailing array
+ member. */
+ else if (TREE_CODE (ref) == ARRAY_REF)
+ return false;
+ else if (TREE_CODE (ref) == ARRAY_RANGE_REF)
+ ;
+ /* If we view an underlying object as sth else then what we
+ gathered up to now is what we have to rely on. */
+ else if (TREE_CODE (ref) == VIEW_CONVERT_EXPR)
+ break;
+ else
+ gcc_unreachable ();
ref = TREE_OPERAND (ref, 0);
}
+ /* The array now is at struct end. Treat flexible arrays and
+ zero-sized arrays as always subject to extend, even into
+ just padding constrained by an underlying decl. */
+ if (! TYPE_SIZE (atype)
+ || integer_zerop (TYPE_SIZE (atype)))
+ return true;
+
tree size = NULL;
if (TREE_CODE (ref) == MEM_REF
Index: gcc/testsuite/gcc.dg/torture/pr80533.c
===================================================================
--- gcc/testsuite/gcc.dg/torture/pr80533.c (nonexistent)
+++ gcc/testsuite/gcc.dg/torture/pr80533.c (working copy)
@@ -0,0 +1,27 @@
+/* { dg-do run } */
+
+extern void abort (void);
+
+struct q { int n; long o[100]; };
+struct r { int n; long o[0]; };
+
+union {
+ struct r r;
+ struct q q;
+} u;
+
+int __attribute__((noclone,noinline))
+foo (int i, int j)
+{
+ long *q = u.r.o;
+ u.r.o[i] = 1;
+ return q[2]/j;
+}
+
+int
+main()
+{
+ if (foo (2, 1) != 1)
+ abort ();
+ return 0;
+}
