[Bug middle-end/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 #18 from CVS Commits --- The releases/gcc-11 branch has been updated by Jakub Jelinek : https://gcc.gnu.org/g:3587c2c241eda0f3ab54ea60d46e9caf12d69b5a commit r11-8615-g3587c2c241eda0f3ab54ea60d46e9caf12d69b5a Author: Jakub Jelinek Date: Fri Jun 18 11:20:40 2021 +0200 stor-layout: Don't create DECL_BIT_FIELD_REPRESENTATIVE for QUAL_UNION_TYPE [PR101062] > The following patch does create them, but treats all such bitfields as if > they were in a structure where the particular bitfield is the only field. While the patch passed bootstrap/regtest on the trunk, when trying to backport it to 11 branch the bootstrap failed with atree.ads:3844:34: size for "Node_Record" too small errors. Turns out the error is not about size being too small, but actually about size being non-constant, and comes from: /* In a FIELD_DECL of a RECORD_TYPE, this is a pointer to the storage representative FIELD_DECL. */ #define DECL_BIT_FIELD_REPRESENTATIVE(NODE) \ (FIELD_DECL_CHECK (NODE)->field_decl.qualifier) /* For a FIELD_DECL in a QUAL_UNION_TYPE, records the expression, which if nonzero, indicates that the field occupies the type. */ #define DECL_QUALIFIER(NODE) (FIELD_DECL_CHECK (NODE)->field_decl.qualifier) so by setting up DECL_BIT_FIELD_REPRESENTATIVE in QUAL_UNION_TYPE we actually set or modify DECL_QUALIFIER and then construct size as COND_EXPRs with those bit field representatives (e.g. with array type) as conditions which doesn't fold into constant. The following patch fixes it by not creating DECL_BIT_FIELD_REPRESENTATIVEs for QUAL_UNION_TYPE as there is nowhere to store them, Shall we change tree.h to document that DECL_BIT_FIELD_REPRESENTATIVE is valid also on UNION_TYPE? I see: tree-ssa-alias.c- if (TREE_CODE (type1) == RECORD_TYPE tree-ssa-alias.c: && DECL_BIT_FIELD_REPRESENTATIVE (field1)) tree-ssa-alias.c:field1 = DECL_BIT_FIELD_REPRESENTATIVE (field1); tree-ssa-alias.c- if (TREE_CODE (type2) == RECORD_TYPE tree-ssa-alias.c: && DECL_BIT_FIELD_REPRESENTATIVE (field2)) tree-ssa-alias.c:field2 = DECL_BIT_FIELD_REPRESENTATIVE (field2); Shall we change that to || == UNION_TYPE or do we assume all fields are overlapping in a UNION_TYPE already? At other spots (asan, ubsan, expr.c) it is unclear what will happen if they see a QUAL_UNION_TYPE with a DECL_QUALIFIER (or does the Ada FE lower that somehow)? 2021-06-18 Jakub Jelinek PR middle-end/101062 * stor-layout.c (finish_bitfield_layout): Don't add bitfield representatives in QUAL_UNION_TYPE.
[Bug middle-end/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 #17 from CVS Commits --- The releases/gcc-11 branch has been updated by Jakub Jelinek : https://gcc.gnu.org/g:c63b440cda7449fb6079831db3911ab3dde7c9f0 commit r11-8614-gc63b440cda7449fb6079831db3911ab3dde7c9f0 Author: Jakub Jelinek Date: Wed Jun 16 12:17:55 2021 +0200 stor-layout: Create DECL_BIT_FIELD_REPRESENTATIVE even for bitfields in unions [PR101062] The following testcase is miscompiled on x86_64-linux, the bitfield store is implemented as a RMW 64-bit operation at d+24 when the d variable has size of only 28 bytes and scheduling moves in between the R and W part a store to a different variable that happens to be right after the d variable. The reason for this is that we weren't creating DECL_BIT_FIELD_REPRESENTATIVEs for bitfields in unions. The following patch does create them, but treats all such bitfields as if they were in a structure where the particular bitfield is the only field. 2021-06-16 Jakub Jelinek PR middle-end/101062 * stor-layout.c (finish_bitfield_representative): For fields in unions assume nextf is always NULL. (finish_bitfield_layout): Compute bit field representatives also in unions, but handle it as if each bitfield was the only field in the aggregate. * gcc.dg/pr101062.c: New test. (cherry picked from commit b4b50bf2864e09f028a39a3f460222632c4d7348)
[Bug middle-end/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 #16 from CVS Commits --- The master branch has been updated by Jakub Jelinek : https://gcc.gnu.org/g:76e990fd211cbb20bf74ce074eb8b2d7b096d3b7 commit r12-1640-g76e990fd211cbb20bf74ce074eb8b2d7b096d3b7 Author: Jakub Jelinek Date: Fri Jun 18 11:20:40 2021 +0200 stor-layout: Don't create DECL_BIT_FIELD_REPRESENTATIVE for QUAL_UNION_TYPE [PR101062] > The following patch does create them, but treats all such bitfields as if > they were in a structure where the particular bitfield is the only field. While the patch passed bootstrap/regtest on the trunk, when trying to backport it to 11 branch the bootstrap failed with atree.ads:3844:34: size for "Node_Record" too small errors. Turns out the error is not about size being too small, but actually about size being non-constant, and comes from: /* In a FIELD_DECL of a RECORD_TYPE, this is a pointer to the storage representative FIELD_DECL. */ #define DECL_BIT_FIELD_REPRESENTATIVE(NODE) \ (FIELD_DECL_CHECK (NODE)->field_decl.qualifier) /* For a FIELD_DECL in a QUAL_UNION_TYPE, records the expression, which if nonzero, indicates that the field occupies the type. */ #define DECL_QUALIFIER(NODE) (FIELD_DECL_CHECK (NODE)->field_decl.qualifier) so by setting up DECL_BIT_FIELD_REPRESENTATIVE in QUAL_UNION_TYPE we actually set or modify DECL_QUALIFIER and then construct size as COND_EXPRs with those bit field representatives (e.g. with array type) as conditions which doesn't fold into constant. The following patch fixes it by not creating DECL_BIT_FIELD_REPRESENTATIVEs for QUAL_UNION_TYPE as there is nowhere to store them, Shall we change tree.h to document that DECL_BIT_FIELD_REPRESENTATIVE is valid also on UNION_TYPE? I see: tree-ssa-alias.c- if (TREE_CODE (type1) == RECORD_TYPE tree-ssa-alias.c: && DECL_BIT_FIELD_REPRESENTATIVE (field1)) tree-ssa-alias.c:field1 = DECL_BIT_FIELD_REPRESENTATIVE (field1); tree-ssa-alias.c- if (TREE_CODE (type2) == RECORD_TYPE tree-ssa-alias.c: && DECL_BIT_FIELD_REPRESENTATIVE (field2)) tree-ssa-alias.c:field2 = DECL_BIT_FIELD_REPRESENTATIVE (field2); Shall we change that to || == UNION_TYPE or do we assume all fields are overlapping in a UNION_TYPE already? At other spots (asan, ubsan, expr.c) it is unclear what will happen if they see a QUAL_UNION_TYPE with a DECL_QUALIFIER (or does the Ada FE lower that somehow)? 2021-06-18 Jakub Jelinek PR middle-end/101062 * stor-layout.c (finish_bitfield_layout): Don't add bitfield representatives in QUAL_UNION_TYPE.
[Bug middle-end/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 #15 from Eric Botcazou --- > Eric, any idea what's going on? Shall I readd if (TREE_CODE (t) == > QUAL_UNION_TYPE) return; at the start function and thus ignore > QUAL_UNION_TYPE and handle only UNION_TYPEs, or should the bitfield > representatives in QUAL_UNION_TYPE get some DECL_QUALIFIER (either one that > always evaluates to false, or the same as corresponding bitfield), something > else? Let's try the former, i.e. disable the new code for QUAL_UNION_TYPE.
[Bug middle-end/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 #14 from Jakub Jelinek --- Sorry for bad paste, on QUAL_UNION_TYPE atree__atree_private_part__node_record___is_extension___XVN
[Bug middle-end/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 #13 from Jakub Jelinek --- I have tried to backport this patch to the 11 branch, but unlike the trunk, it doesn't build there, with atree.ads:3844:34: size for "Node_Record" too small make[3]: *** [../../gcc/ada/gcc-interface/Make-lang.in:139: ada/errout.o] Error 1 (and many further occurrences of the same error). Seems the stor-layout.c changes trigger during ada/errout.o compilation just once, on QUAL_UNION_TYPE atree.ads:3844:34: size for "Node_Record" too small make[3]: *** [../../gcc/ada/gcc-interface/Make-lang.in:139: ada/errout.o] Error 1 which has byte size 28 and bitfield FIELD_DECL S0 with the same byte size (224 bits) and another bitfield FIELD_DECL O with the same size. The code then adds two bit field representatives with the same size. Eric, any idea what's going on? Shall I readd if (TREE_CODE (t) == QUAL_UNION_TYPE) return; at the start function and thus ignore QUAL_UNION_TYPE and handle only UNION_TYPEs, or should the bitfield representatives in QUAL_UNION_TYPE get some DECL_QUALIFIER (either one that always evaluates to false, or the same as corresponding bitfield), something else? Wonder what is different that it works on the trunk...
[Bug middle-end/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 #12 from CVS Commits --- The master branch has been updated by Jakub Jelinek : https://gcc.gnu.org/g:b4b50bf2864e09f028a39a3f460222632c4d7348 commit r12-1527-gb4b50bf2864e09f028a39a3f460222632c4d7348 Author: Jakub Jelinek Date: Wed Jun 16 12:17:55 2021 +0200 stor-layout: Create DECL_BIT_FIELD_REPRESENTATIVE even for bitfields in unions [PR101062] The following testcase is miscompiled on x86_64-linux, the bitfield store is implemented as a RMW 64-bit operation at d+24 when the d variable has size of only 28 bytes and scheduling moves in between the R and W part a store to a different variable that happens to be right after the d variable. The reason for this is that we weren't creating DECL_BIT_FIELD_REPRESENTATIVEs for bitfields in unions. The following patch does create them, but treats all such bitfields as if they were in a structure where the particular bitfield is the only field. 2021-06-16 Jakub Jelinek PR middle-end/101062 * stor-layout.c (finish_bitfield_representative): For fields in unions assume nextf is always NULL. (finish_bitfield_layout): Compute bit field representatives also in unions, but handle it as if each bitfield was the only field in the aggregate. * gcc.dg/pr101062.c: New test.
[Bug middle-end/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 #11 from Richard Biener --- (In reply to Jakub Jelinek from comment #10) > Created attachment 51023 [details] > gcc12-pr101062.patch > > Untested fix for this in stor-layout.c. LGTM
[Bug middle-end/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 Status|NEW |ASSIGNED Assignee|unassigned at gcc dot gnu.org |jakub at gcc dot gnu.org --- Comment #10 from Jakub Jelinek --- Created attachment 51023 --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=51023=edit gcc12-pr101062.patch Untested fix for this in stor-layout.c.
[Bug middle-end/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 #9 from rguenther at suse dot de ---
On Tue, 15 Jun 2021, jakub at gcc dot gnu.org wrote:
> 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 (_start, _end, to, ,
> > );
> > /* 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).
Yes, as said - the caller of get_bit_range seems to expect it to always
handle cases that do not run into the byte-align case to make sure to
not cross to next objects. I don't remember why I didn't handle
UNION_TYPE (I guess because it was about bitfield groups, not single
bitfields), so maybe we should indeed handle them.
There are also other callers of get_bit_range it seems, even on
!DECL_BIT_FIELD_TYPE fields.
So let's try handling [QUAL_]UNION_TYPE in finish_bitfield_layout?
