[Bug tree-optimization/101062] [10/11/12 Regression] wrong code with "-O2 -fno-toplevel-reorder -frename-registers"
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101062
--- Comment #8 from Jakub Jelinek ---
(In reply to Richard Biener from comment #7)
> Now, it looks to me this is rather an issue that the access is larger than
> the object and thus a general bug - at least I don't see how it should only
> manifest with bitfields in unions?
>
> Note we do
>
> if (TREE_CODE (to) == COMPONENT_REF
> && DECL_BIT_FIELD_TYPE (TREE_OPERAND (to, 1)))
> get_bit_range (&bitregion_start, &bitregion_end, to, &bitpos,
> &offset);
> /* The C++ memory model naturally applies to byte-aligned fields.
> However, if we do not have a DECL_BIT_FIELD_TYPE but BITPOS or
> BITSIZE are not byte-aligned, there is no need to limit the range
> we can access. This can occur with packed structures in Ada. */
> else if (maybe_gt (bitsize, 0)
>&& multiple_p (bitsize, BITS_PER_UNIT)
>&& multiple_p (bitpos, BITS_PER_UNIT))
> {
> bitregion_start = bitpos;
> bitregion_end = bitpos + bitsize - 1;
> }
>
> but if we assume that for DECL_BIT_FIELD_TYPE there's a representative
> then we miss the else if, so - maybe get_bit_range should return whether
> it handled things or the else if part should be done unconditionally
> in case bitregion_start/end is not {0,0}?
This wouldn't help us, bitsize is > 0, but not a multiple of BITS_PER_UNIT in
this case. Furthermore, even if we add there bitregion_start/end for the base
variable if any as further fallthrough, I think most C/C++ programmers will
expect that with
union U { int a; int b : 5; } u[64];
u[4].b = 1; can be done safely in one thread and
u[5].a = 2; in another one.
My patch fixes that (or another possibility would be to compute the
representative even in UNION_TYPE (no idea about QUAL_UNION_TYPE) - could be as
simple as removing the early out and instead of doing prev = field; in the loop
do if (TREE_CODE (t) != RECORD_TYPE) { finish_bitfield_representative (repr,
field); repr = NULL_TREE; } else prev = field; and in
finish_bitfield_representative override nextf to NULL_TREE).
Improving expand_assignment can be done too, sure, but independently to this.
[Bug tree-optimization/101062] [10/11/12 Regression] wrong code with "-O2 -fno-toplevel-reorder -frename-registers"
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101062
--- Comment #7 from Richard Biener ---
(In reply to Eric Botcazou from comment #6)
> > Does Ada allow bitfields in unions and if yes, what does it want for those?
>
> Yes, it does, and I don't think there is any specific need so the default
> should be OK like for structures.
I also think since there's at most a single active union member the default
should work.
Now, it looks to me this is rather an issue that the access is larger than
the object and thus a general bug - at least I don't see how it should only
manifest with bitfields in unions?
Note we do
if (TREE_CODE (to) == COMPONENT_REF
&& DECL_BIT_FIELD_TYPE (TREE_OPERAND (to, 1)))
get_bit_range (&bitregion_start, &bitregion_end, to, &bitpos, &offset);
/* The C++ memory model naturally applies to byte-aligned fields.
However, if we do not have a DECL_BIT_FIELD_TYPE but BITPOS or
BITSIZE are not byte-aligned, there is no need to limit the range
we can access. This can occur with packed structures in Ada. */
else if (maybe_gt (bitsize, 0)
&& multiple_p (bitsize, BITS_PER_UNIT)
&& multiple_p (bitpos, BITS_PER_UNIT))
{
bitregion_start = bitpos;
bitregion_end = bitpos + bitsize - 1;
}
but if we assume that for DECL_BIT_FIELD_TYPE there's a representative
then we miss the else if, so - maybe get_bit_range should return whether
it handled things or the else if part should be done unconditionally
in case bitregion_start/end is not {0,0}?
[Bug tree-optimization/101062] [10/11/12 Regression] wrong code with "-O2 -fno-toplevel-reorder -frename-registers"
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101062 --- Comment #6 from Eric Botcazou --- > Does Ada allow bitfields in unions and if yes, what does it want for those? Yes, it does, and I don't think there is any specific need so the default should be OK like for structures.
[Bug tree-optimization/101062] [10/11/12 Regression] wrong code with "-O2 -fno-toplevel-reorder -frename-registers"
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101062
--- Comment #5 from Jakub Jelinek ---
--- gcc/expr.c.jj 2021-06-14 12:27:02.0 +0200
+++ gcc/expr.c 2021-06-14 22:18:56.852524237 +0200
@@ -5120,15 +5120,18 @@ get_bit_range (poly_uint64_pod *bitstart
poly_int64_pod *bitpos, tree *offset)
{
poly_int64 bitoffset;
- tree field, repr;
gcc_assert (TREE_CODE (exp) == COMPONENT_REF);
- field = TREE_OPERAND (exp, 1);
- repr = DECL_BIT_FIELD_REPRESENTATIVE (field);
- /* If we do not have a DECL_BIT_FIELD_REPRESENTATIVE there is no
- need to limit the range we can access. */
- if (!repr)
+ tree field = TREE_OPERAND (exp, 1);
+ tree repr = DECL_BIT_FIELD_REPRESENTATIVE (field);
+ tree type = TREE_TYPE (TREE_OPERAND (exp, 0));
+ /* If we do not have a DECL_BIT_FIELD_REPRESENTATIVE (except for bitfields
+ in unions) there is no need to limit the range we can access. */
+ if (!repr
+ && (TREE_CODE (type) != UNION_TYPE
+ || !TYPE_SIZE (type)
+ || !tree_fits_poly_uint64_p (TYPE_SIZE (type
{
*bitstart = *bitend = 0;
return;
@@ -5153,6 +5156,14 @@ get_bit_range (poly_uint64_pod *bitstart
}
}
+ /* For bitfields in unions, return bitsize of the whole union. */
+ if (!repr)
+{
+ *bitstart = *bitpos;
+ *bitend = *bitstart + tree_to_poly_uint64 (TYPE_SIZE (type)) - 1;
+ return;
+}
+
/* Compute the adjustment to bitpos from the offset of the field
relative to the representative. DECL_FIELD_OFFSET of field and
repr are the same by construction if they are not constants,
fixes it for me.
[Bug tree-optimization/101062] [10/11/12 Regression] wrong code with "-O2 -fno-toplevel-reorder -frename-registers"
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101062
Jakub Jelinek changed:
What|Removed |Added
CC||ebotcazou at gcc dot gnu.org
--- Comment #4 from Jakub Jelinek ---
Ah, and there is also a problem that for bitfields in unions we don't create
DECL_BIT_FIELD_REPRESENTATIVE in finish_bitfield_layout and so then
get_bit_range returns { 0, 0 } as bitstart, bitend and so the access isn't
really restricted.
So, wonder, shall we for bitfields in UNION_TYPE in get_bit_range return the
range of the union? Or is it ok for d[4].b RMW to update also d[5].b next to
it, just not anything beyond end of the variable (so essentially set
bitregion_start/bitregion_end to the start and end of d variable), something
else?
At least for multithreaded apps, even the modification of another element of
the same array seems bad to me.
Does Ada allow bitfields in unions and if yes, what does it want for those?
Changing the testcase with sed -i -e s/union/struct/g fixes the bug...
[Bug tree-optimization/101062] [10/11/12 Regression] wrong code with "-O2 -fno-toplevel-reorder -frename-registers"
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101062
--- Comment #3 from Jakub Jelinek ---
And
union U { signed b : 5; };
int c;
volatile union U d[7] = {{8}};
short e = 1;
__attribute__((noipa, noinline, noclone)) void
foo ()
{
d[6].b = 0;
d[6].b = 0;
d[6].b = 0;
d[6].b = 0;
d[6].b = 0;
e = 0;
c = 0;
}
int
main ()
{
foo ();
if (e != 0)
__builtin_abort ();
return 0;
}
it started even in between r0-74314-gc245c134da2fdced8608d3e992c969ae22932752
and r0-74353-g5cd88d6857dffe4f10c834c773c300881ec20e32
Seems the problem is the d[6].b store, which incorrectly uses a 64-bit load
from d+24 and then later on stores adjusted value to that. But d is only 28
bytes long and with -fno-toplevel-reorder the e variable is right after it and
scheduling moves the load from *(long long *)(((char *)&d) + 24) before store
to e and its update after store to e.
[Bug tree-optimization/101062] [10/11/12 Regression] wrong code with "-O2 -fno-toplevel-reorder -frename-registers"
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101062 --- Comment #2 from Jakub Jelinek --- Actually, with noinline,noclone,noipa it started already with r0-102336-g8f0fe813790d58066714c8f38f4916925c83517d
[Bug tree-optimization/101062] [10/11/12 Regression] wrong code with "-O2 -fno-toplevel-reorder -frename-registers"
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101062
Jakub Jelinek changed:
What|Removed |Added
Priority|P3 |P2
Status|UNCONFIRMED |NEW
Ever confirmed|0 |1
Target Milestone|--- |10.4
CC||jakub at gcc dot gnu.org
Last reconfirmed||2021-06-14
Summary|wrong code with "-O2|[10/11/12 Regression] wrong
|-fno-toplevel-reorder |code with "-O2
|-frename-registers" |-fno-toplevel-reorder
||-frename-registers"
--- Comment #1 from Jakub Jelinek ---
Started with r10-3542-g0b92cf305dcf34387a8e2564e55ca8948df3b47a
so likely latent before.
Slightly adjusted testcase:
/* { dg-do run } */
/* { dg-options "-O2 -fno-toplevel-reorder -frename-registers" } */
union U { signed b : 5; };
int c;
volatile union U d[7] = {{8}};
short e = 1;
__attribute__((noipa)) void
foo ()
{
for (c = 5; c; c--)
e = d[6].b = 0;
}
int
main ()
{
foo ();
if (e != 0)
__builtin_abort ();
return 0;
}
