Re: [PATCH] Fix PRs 52080, 52097 and 48124, rewrite bitfield expansion, enable the C++ memory model wrt bitfields everywhere
Hmm, yeah. Or something like Index: expr.c === --- expr.c (revision 186082) +++ expr.c (working copy) @@ -4490,8 +4490,8 @@ get_bit_range (unsigned HOST_WIDE_INT *b bitoffset += (tree_low_cst (DECL_FIELD_BIT_OFFSET (field), 1) - tree_low_cst (DECL_FIELD_BIT_OFFSET (repr), 1)); - *bitstart = bitpos - bitoffset; - *bitend = *bitstart + tree_low_cst (DECL_SIZE (repr), 1) - 1; + *bitstart = bitpos (HOST_WIDE_INT) bitoffset ? 0 : bitpos - bitoffset; + *bitend = bitpos + tree_low_cst (DECL_SIZE (repr), 1) - bitoffset - 1; } /* Returns true if the MEM_REF REF refers to an object that does not which conservatively truncates the bitrange. What do you think about allowing get_bit_range to adjust offset and bitpos? tem = get_inner_reference (to, bitsize, bitpos, offset, mode1, unsignedp, volatilep, true); if (TREE_CODE (to) == COMPONENT_REF DECL_BIT_FIELD_TYPE (TREE_OPERAND (to, 1))) get_bit_range (bitregion_start, bitregion_end, offset, bitpos, to); so as to have a really non-negative bitregion_start/bitregion_end? It would assert that offset is already non-null in that case. -- Eric Botcazou
Re: [PATCH] Fix PRs 52080, 52097 and 48124, rewrite bitfield expansion, enable the C++ memory model wrt bitfields everywhere
On Tue, 3 Apr 2012, Eric Botcazou wrote: Hmm, yeah. Or something like Index: expr.c === --- expr.c (revision 186082) +++ expr.c (working copy) @@ -4490,8 +4490,8 @@ get_bit_range (unsigned HOST_WIDE_INT *b bitoffset += (tree_low_cst (DECL_FIELD_BIT_OFFSET (field), 1) - tree_low_cst (DECL_FIELD_BIT_OFFSET (repr), 1)); - *bitstart = bitpos - bitoffset; - *bitend = *bitstart + tree_low_cst (DECL_SIZE (repr), 1) - 1; + *bitstart = bitpos (HOST_WIDE_INT) bitoffset ? 0 : bitpos - bitoffset; + *bitend = bitpos + tree_low_cst (DECL_SIZE (repr), 1) - bitoffset - 1; } /* Returns true if the MEM_REF REF refers to an object that does not which conservatively truncates the bitrange. What do you think about allowing get_bit_range to adjust offset and bitpos? tem = get_inner_reference (to, bitsize, bitpos, offset, mode1, unsignedp, volatilep, true); if (TREE_CODE (to) == COMPONENT_REF DECL_BIT_FIELD_TYPE (TREE_OPERAND (to, 1))) get_bit_range (bitregion_start, bitregion_end, offset, bitpos, to); so as to have a really non-negative bitregion_start/bitregion_end? It would assert that offset is already non-null in that case. For the case in question offset is (D.2640_7 + -1) * 20 + 16. I wonder why DECL_FIELD_OFFSET of the outermost COMPONENT_REF is not added to bitpos by get_inner_reference (that is what get_bit_range assumes ...). The FIELD_DECL is arg 1 field_decl 0x75c5 n type integer_type 0x75c4f2a0 natural___XDLU_0__2147483647 unsigned external packed bit-field nonaddressable SI file pack18_pkg.ads line 10 col 7 size integer_cst 0x75ae4c80 constant visited 31 unit size integer_cst 0x75ae4260 4 align 1 offset_align 128 offset integer_cst 0x75ae4420 constant visited 16 bit offset integer_cst 0x75ae4560 constant visited 1 bit_field_type integer_type 0x75c4f2a0 natural___XDLU_0__2147483647 context record_type 0x75c4f0a8 pack18_pkg__rec and TREE_OPERAND (to, 2) is NULL and component_ref_field_offset returns 16. In the alias-oracle variants we add constant field-offsets to the bitposition part (at least that avoids the need to dissect the constant part of the offset from the non-constant part). So, how would you make sure this works? Match the fact that get_inner_reference does _not_ add DECL_FIELD_OFFSET to bitpos, and, if DECL_FIELD_OFFSET is an INTEGER_CST simply subtract that from offset and add it to bitpos? I suppose that would work. Though doing that in get_inner_reference for DECL_BIT_FIELD_TYPE fields may make sense as well. Richard.
Re: [PATCH] Fix PRs 52080, 52097 and 48124, rewrite bitfield expansion, enable the C++ memory model wrt bitfields everywhere
For the case in question offset is (D.2640_7 + -1) * 20 + 16. I wonder
why DECL_FIELD_OFFSET of the outermost COMPONENT_REF is not added
to bitpos by get_inner_reference (that is what get_bit_range assumes ...).
DECL_FIELD_OFFSET is added to offset and DECL_FIELD_BIT_OFFSET to bitpos.
So, how would you make sure this works? Match the fact that
get_inner_reference does _not_ add DECL_FIELD_OFFSET to bitpos,
and, if DECL_FIELD_OFFSET is an INTEGER_CST simply subtract that
from offset and add it to bitpos? I suppose that would work.
Yes, but the amount is simply the negative bitstart (which is a multiple of
BITS_PER_UNIT). This is the same kind of adjustment now done at the end of
get_inner_reference to avoid negative bit positions there too.
Though doing that in get_inner_reference for DECL_BIT_FIELD_TYPE
fields may make sense as well.
That would be more complicated, as we would need to split the offset into
variable and fixed part.
Tentative patch attached, regtested for Ada on x86 and x86-64. I'll do a full
testing cycle if it is approved.
* expr.c (get_bit_range): Add OFFSET parameter and adjust BITPOS.
Change type of BITOFFSET to signed. Make sure the lower bound of
the computed range is non-negative by adjusting OFFSET and BITPOS.
(expand_assignment): Adjust call to get_bit_range.
--
Eric Botcazou
Index: expr.c
===
--- expr.c (revision 186054)
+++ expr.c (working copy)
@@ -4431,19 +4431,22 @@ optimize_bitfield_assignment_op (unsigne
/* In the C++ memory model, consecutive bit fields in a structure are
considered one memory location.
- Given a COMPONENT_REF EXP at bit position BITPOS, this function
+ Given a COMPONENT_REF EXP at position (OFFSET, BITPOS), this function
returns the bit range of consecutive bits in which this COMPONENT_REF
- belongs in. The values are returned in *BITSTART and *BITEND.
+ belongs in. The values are returned in *BITSTART and *BITEND. Both
+ OFFSET and BITPOS may be adjusted in the process.
+
If the access does not need to be restricted 0 is returned in
*BITSTART and *BITEND. */
static void
get_bit_range (unsigned HOST_WIDE_INT *bitstart,
unsigned HOST_WIDE_INT *bitend,
- tree exp,
- HOST_WIDE_INT bitpos)
+ tree *offset,
+ HOST_WIDE_INT *bitpos,
+ tree exp)
{
- unsigned HOST_WIDE_INT bitoffset;
+ HOST_WIDE_INT bitoffset;
tree field, repr;
gcc_assert (TREE_CODE (exp) == COMPONENT_REF);
@@ -4490,7 +4493,25 @@ get_bit_range (unsigned HOST_WIDE_INT *b
bitoffset += (tree_low_cst (DECL_FIELD_BIT_OFFSET (field), 1)
- tree_low_cst (DECL_FIELD_BIT_OFFSET (repr), 1));
- *bitstart = bitpos - bitoffset;
+ /* If the adjustment is larger than bitpos, we would have a negative bit
+ position for the lower bound and this may wreak havoc later. This can
+ occur only if we have a non-null offset, so adjust offset and bitpos
+ to make the lower bound non-negative. */
+ if (bitoffset *bitpos)
+{
+ HOST_WIDE_INT adjust = bitoffset - *bitpos;
+
+ gcc_assert ((adjust % BITS_PER_UNIT) == 0);
+ gcc_assert (*offset != NULL_TREE);
+
+ *bitpos += adjust;
+ *offset
+ = size_binop (MINUS_EXPR, *offset, size_int (adjust / BITS_PER_UNIT));
+ *bitstart = 0;
+}
+ else
+*bitstart = *bitpos - bitoffset;
+
*bitend = *bitstart + tree_low_cst (DECL_SIZE (repr), 1) - 1;
}
@@ -4595,7 +4616,7 @@ expand_assignment (tree to, tree from, b
if (TREE_CODE (to) == COMPONENT_REF
DECL_BIT_FIELD_TYPE (TREE_OPERAND (to, 1)))
- get_bit_range (bitregion_start, bitregion_end, to, bitpos);
+ get_bit_range (bitregion_start, bitregion_end, offset, bitpos, to);
/* If we are going to use store_bit_field and extract_bit_field,
make sure to_rtx will be safe for multiple use. */
Re: [PATCH] Fix PRs 52080, 52097 and 48124, rewrite bitfield expansion, enable the C++ memory model wrt bitfields everywhere
On Tue, 3 Apr 2012, Eric Botcazou wrote: For the case in question offset is (D.2640_7 + -1) * 20 + 16. I wonder why DECL_FIELD_OFFSET of the outermost COMPONENT_REF is not added to bitpos by get_inner_reference (that is what get_bit_range assumes ...). DECL_FIELD_OFFSET is added to offset and DECL_FIELD_BIT_OFFSET to bitpos. So, how would you make sure this works? Match the fact that get_inner_reference does _not_ add DECL_FIELD_OFFSET to bitpos, and, if DECL_FIELD_OFFSET is an INTEGER_CST simply subtract that from offset and add it to bitpos? I suppose that would work. Yes, but the amount is simply the negative bitstart (which is a multiple of BITS_PER_UNIT). This is the same kind of adjustment now done at the end of get_inner_reference to avoid negative bit positions there too. Though doing that in get_inner_reference for DECL_BIT_FIELD_TYPE fields may make sense as well. That would be more complicated, as we would need to split the offset into variable and fixed part. Tentative patch attached, regtested for Ada on x86 and x86-64. I'll do a full testing cycle if it is approved. Yes, the patch is ok. Thanks, Richard. * expr.c (get_bit_range): Add OFFSET parameter and adjust BITPOS. Change type of BITOFFSET to signed. Make sure the lower bound of the computed range is non-negative by adjusting OFFSET and BITPOS. (expand_assignment): Adjust call to get_bit_range.
Re: [PATCH] Fix PRs 52080, 52097 and 48124, rewrite bitfield expansion, enable the C++ memory model wrt bitfields everywhere
On Fri, 30 Mar 2012, Eric Botcazou wrote: May I apply the patch I posted? It boostrapped/regtested fine on x86-64/Linux. Yes. Thanks. Unfortunately, while this was the last identified problem on x86, another issue is visible on x86-64 as a miscompilation of XML/Ada at -O0. Reduced testcase attached: gnat.dg/pack18.adb gnat.dg/pack18_pkg.ads The executable segfaults because it attempts a read at 0x2000. The scenario is a follows: Rec is packed record so its fields are bit fields, N being at bit offset 129. The representative is at offset 0. get_bit_range is invoked on N with a bitpos of 1, because there is variable offset and its DECL_FIELD_OFFFSET is added to it instead of bitpos. Hence bitpos - bitoffset is (unsigned HOST_WIDE_INT) -128. This value enters unchanged the new code in store_bit_field and the division: offset = bitregion_start / BITS_PER_UNIT; yields the problematic big number. It would therefore appear that bitstart and bitend need to be signed offsets, at least until they are adjusted by store_bit_field. Hmm, yeah. Or something like Index: expr.c === --- expr.c (revision 186082) +++ expr.c (working copy) @@ -4490,8 +4490,8 @@ get_bit_range (unsigned HOST_WIDE_INT *b bitoffset += (tree_low_cst (DECL_FIELD_BIT_OFFSET (field), 1) - tree_low_cst (DECL_FIELD_BIT_OFFSET (repr), 1)); - *bitstart = bitpos - bitoffset; - *bitend = *bitstart + tree_low_cst (DECL_SIZE (repr), 1) - 1; + *bitstart = bitpos (HOST_WIDE_INT) bitoffset ? 0 : bitpos - bitoffset; + *bitend = bitpos + tree_low_cst (DECL_SIZE (repr), 1) - bitoffset - 1; } /* Returns true if the MEM_REF REF refers to an object that does not which conservatively truncates the bitrange. Richard.
Re: [PATCH] Fix PRs 52080, 52097 and 48124, rewrite bitfield expansion, enable the C++ memory model wrt bitfields everywhere
May I apply the patch I posted? It boostrapped/regtested fine on
x86-64/Linux.
Yes.
Thanks. Unfortunately, while this was the last identified problem on x86,
another issue is visible on x86-64 as a miscompilation of XML/Ada at -O0.
Reduced testcase attached:
gnat.dg/pack18.adb
gnat.dg/pack18_pkg.ads
The executable segfaults because it attempts a read at 0x2000.
The scenario is a follows: Rec is packed record so its fields are bit fields, N
being at bit offset 129. The representative is at offset 0.
get_bit_range is invoked on N with a bitpos of 1, because there is variable
offset and its DECL_FIELD_OFFFSET is added to it instead of bitpos. Hence
bitpos - bitoffset
is (unsigned HOST_WIDE_INT) -128. This value enters unchanged the new code in
store_bit_field and the division:
offset = bitregion_start / BITS_PER_UNIT;
yields the problematic big number.
It would therefore appear that bitstart and bitend need to be signed offsets,
at least until they are adjusted by store_bit_field.
--
Eric Botcazou
-- { dg-do run }
with Pack18_Pkg; use Pack18_Pkg;
procedure Pack18 is
use Pack18_Pkg.Attributes_Tables;
Table : Instance;
begin
Init (Table);
Set_Last (Table, 1);
Table.Table (Last (Table)).N := 0;
end;
with GNAT.Dynamic_Tables;
package Pack18_Pkg is
type String_Access is access String;
type Rec is record
S : String_Access;
B : Boolean;
N : Natural;
end record;
pragma Pack (Rec);
package Attributes_Tables is new GNAT.Dynamic_Tables
(Table_Component_Type = Rec,
Table_Index_Type = Natural,
Table_Low_Bound = 1,
Table_Initial= 200,
Table_Increment = 200);
end Pack18_Pkg;
Re: [PATCH] Fix PRs 52080, 52097 and 48124, rewrite bitfield expansion, enable the C++ memory model wrt bitfields everywhere
On Mon, 26 Mar 2012, Eric Botcazou wrote: Uh. When is a field a bit field though? At least stor-layout.c resets DECL_BIT_FIELD when local relative alignment is proper and the filed has an integer mode. That's overly optimistic if the record is placed at a bit position. Of course we still have DECL_BIT_FIELD_TYPE, but I wonder what the DECL_BIT_FIELD flag is for if we cannot really know whether the field in the end is a bitfield anyway... (what value do the various field-decls in a component-ref chain have anyway if the real interpretation is subject to the containing object, which might be even only specified by the type used for an indirect ref ...) We're talking about a bit field with record type here, so anything calculated within the record type in isolation is essentially invalidated for the bit field in question. Fortunately this only occurs in Ada, which doesn't use the very questionable C++ memory model (fingers crossed for the future. :-) ;) Btw, I put things in stor-layout.c exactly to allow a langhook eventually controlling things for the bitfield representative. Mind adding one that simply disables them completely for Ada? Or maybe, for selected record types, so that we do if (lang_hooks.types.lower_bitfield_layout (rli-t)) finish_bitfield_layout (rli-t); ? Letting the FE decide when to punt is certainly better than trying to figure it out from stor-layout.c code. I suppose for types that are supposed to interoperate with C / C++ using the C / C++ rules makes sense (I suppose there is sth like C/C++ interoperability with Ada). Richard.
Re: [PATCH] Fix PRs 52080, 52097 and 48124, rewrite bitfield expansion, enable the C++ memory model wrt bitfields everywhere
On Mon, 26 Mar 2012, Eric Botcazou wrote: The patch looks reasonable - can we compute this backward from the result of the outer get_inner_reference call and the outermost field-decl though? Or make get_inner_reference compute that while analyzing the full reference and return a flag? OTOH it shouldn't be too expensive. There are a lot of calls to get_inner_reference throughout the compiler though. And it is cheap if you compare it with the 4.7 code, which essentially does the same processing for _every_ field in the record (and even if the record object isn't a handled_component_p). Yes, indeed. One of the main motivation was to get rid of that quadratic complexity ... Richard.
Re: [PATCH] Fix PRs 52080, 52097 and 48124, rewrite bitfield expansion, enable the C++ memory model wrt bitfields everywhere
Btw, I put things in stor-layout.c exactly to allow a langhook eventually controlling things for the bitfield representative. Mind adding one that simply disables them completely for Ada? Or maybe, for selected record types, so that we do if (lang_hooks.types.lower_bitfield_layout (rli-t)) finish_bitfield_layout (rli-t); ? Letting the FE decide when to punt is certainly better than trying to figure it out from stor-layout.c code. I suppose for types that are supposed to interoperate with C / C++ using the C / C++ rules makes sense (I suppose there is sth like C/C++ interoperability with Ada). Yes, there is C/C++ interoperability in Ada. But GNAT's policy is to be compatible with C/C++ by default, so I'd rather not deviate from the common code for such a central thing as record layout if we can avoid it. -- Eric Botcazo
Re: [PATCH] Fix PRs 52080, 52097 and 48124, rewrite bitfield expansion, enable the C++ memory model wrt bitfields everywhere
On Tue, 27 Mar 2012, Eric Botcazou wrote: Btw, I put things in stor-layout.c exactly to allow a langhook eventually controlling things for the bitfield representative. Mind adding one that simply disables them completely for Ada? Or maybe, for selected record types, so that we do if (lang_hooks.types.lower_bitfield_layout (rli-t)) finish_bitfield_layout (rli-t); ? Letting the FE decide when to punt is certainly better than trying to figure it out from stor-layout.c code. I suppose for types that are supposed to interoperate with C / C++ using the C / C++ rules makes sense (I suppose there is sth like C/C++ interoperability with Ada). Yes, there is C/C++ interoperability in Ada. But GNAT's policy is to be compatible with C/C++ by default, so I'd rather not deviate from the common code for such a central thing as record layout if we can avoid it. I see. Though the code does not affect layout but only access layout for bitfields. I'm fine with fixing the issues we run into elsewhere, just the langhook is a possibility I had in mind from the start, in case frontends differ in their memory model for bitfields. Richard.
Re: [PATCH] Fix PRs 52080, 52097 and 48124, rewrite bitfield expansion, enable the C++ memory model wrt bitfields everywhere
I see. Though the code does not affect layout but only access layout for bitfields. I'm fine with fixing the issues we run into elsewhere, just the langhook is a possibility I had in mind from the start, in case frontends differ in their memory model for bitfields. Understood. According to our internal testing, the issue we're discussing was the last problem introduced by the bitfield change, and I think that using the C/C++ model for C/C++-compatible bit fields is fine for GNAT. May I apply the patch I posted? It boostrapped/regtested fine on x86-64/Linux. -- Eric Botcazou
Re: [PATCH] Fix PRs 52080, 52097 and 48124, rewrite bitfield expansion, enable the C++ memory model wrt bitfields everywhere
Huh? If they have DECL_FIELD_BIT_OFFSET of zero they are at a byte boundary, no? Wait - the RECORD_TYPE itself is at non-zero DECL_FIELD_BIT_OFFSET and thus a zero DECL_FIELD_BIT_OFFSET for its fields does not mean anything?! DECL_FIELD_BIT_OFFSET is relative to the enclosing record type. But how can DECL_OFFSET_ALIGN be still valid for such field? Likewise, it is relative to DECL_FIELD_OFFSET, which is relative to the enclosing record type. Obviously if DECL_FIELD_OFFSET == 0, DECL_FIELD_BIT_OFFSET == 0 then the offset needs to be aligned to DECL_OFFSET_ALIGN. Which then means DECL_OFFSET_ALIGN is a bit-alignment? It's just a relative alignment, not an absolute one. Anyway, since we are trying to compute a nice mode to use for the bitfield representative we can give up in the second that we do not know how to reach BITS_PER_UNIT alignment. Or we can simply only try to ensure MIN (BITS_PER_UNIT, DECL_OFFSET_ALIGN) alignment/size of the representative. Of course the bitfield expansion code has to deal with non-byte-aligned representatives then, and we'd always have to use BLKmode for them. But BLKmode forces byte alignment. Anything not byte-aligned must use an integral mode. I think we can't change it to be on a byte-boundary, the same record may be used at different bit-positions, no? Sure. Sure, we acknowledge it can cross a record boundary. I just was not aware we cannot statically compute the bit-offset to the previous byte for a record type. We can if we look into the entire object, not just the enclosing record. The problem is that, while get_inner_reference and friends do the former, the new code in stor-layout.c only does the latter. OK, I think we should enter some sort of a degraded mode for these non-byte aligned records, as they cannot occur in C/C++. I'll try to come up with something. -- Eric Botcazou
Re: [PATCH] Fix PRs 52080, 52097 and 48124, rewrite bitfield expansion, enable the C++ memory model wrt bitfields everywhere
Btw, now checking with gdb, DECL_OFFSET_ALIGN is always 128 for all of the fields - that looks bogus. DECL_ALIGN is 1, but that doesn't mean DECL_OFFSET_ALIGN should not be 1 as well, no? DECL_OFFSET_ALIGN is set to BIGGEST_ALIGNMENT for the first field, see start_record_layout. If DECL_FIELD_OFFSET doesn't change for the next fields, DECL_OFFSET_ALIGN will very likely not either. -- Eric Botcazou
Re: [PATCH] Fix PRs 52080, 52097 and 48124, rewrite bitfield expansion, enable the C++ memory model wrt bitfields everywhere
I think we indeed can't really in stor-layout, so the only place is very
likely get_bit_range.
Something like that for example. I think the expmed.c hunk should be applied
to the 4.7 branch as well, because the new code in store_bit_field is quite
dangerous without it.
* expmed.c (store_bit_field): Assert that BITREGION_START is a multiple
of a unit before computing the offset in units.
* expr.c (get_bit_range): Return the null range if the enclosing record
is part of a larger bit field.
--
Eric Botcazou
Index: expr.c
===
--- expr.c (revision 185767)
+++ expr.c (working copy)
@@ -4462,6 +4462,25 @@ get_bit_range (unsigned HOST_WIDE_INT *b
return;
}
+ /* If we have a DECL_BIT_FIELD_REPRESENTATIVE but the enclosing record is
+ part of a larger bit field, then the representative does not serve any
+ useful purpose. This can occur in Ada. */
+ if (handled_component_p (TREE_OPERAND (exp, 0)))
+{
+ enum machine_mode rmode;
+ HOST_WIDE_INT rbitsize, rbitpos;
+ tree roffset;
+ int unsignedp;
+ int volatilep = 0;
+ get_inner_reference (TREE_OPERAND (exp, 0), rbitsize, rbitpos,
+ roffset, rmode, unsignedp, volatilep, false);
+ if ((rbitpos % BITS_PER_UNIT) != 0)
+ {
+ *bitstart = *bitend = 0;
+ 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,
Index: expmed.c
===
--- expmed.c (revision 185570)
+++ expmed.c (working copy)
@@ -828,8 +828,7 @@ store_bit_field (rtx str_rtx, unsigned H
/* Under the C++0x memory model, we must not touch bits outside the
bit region. Adjust the address to start at the beginning of the
bit region. */
- if (MEM_P (str_rtx)
- bitregion_start 0)
+ if (MEM_P (str_rtx) bitregion_start 0)
{
enum machine_mode bestmode;
enum machine_mode op_mode;
@@ -839,6 +838,8 @@ store_bit_field (rtx str_rtx, unsigned H
if (op_mode == MAX_MACHINE_MODE)
op_mode = VOIDmode;
+ gcc_assert ((bitregion_start % BITS_PER_UNIT) == 0);
+
offset = bitregion_start / BITS_PER_UNIT;
bitnum -= bitregion_start;
bitregion_end -= bitregion_start;
Re: [PATCH] Fix PRs 52080, 52097 and 48124, rewrite bitfield expansion, enable the C++ memory model wrt bitfields everywhere
OTOH if DECL_ALIGN is absolute then if the first field of a record type has DECL_ALIGN that is a multiple of BITS_PER_UNIT we know we can use the present stor-layout code? So we can check that and give up computing representatives at all for a record type if that does not hold. I don't think so, DECL_ALIGN of a field is probably honored only if TYPE_ALIGN of the record is, and TYPE_ALIGN isn't honored for a bit field. -- Eric Botcazou
Re: [PATCH] Fix PRs 52080, 52097 and 48124, rewrite bitfield expansion, enable the C++ memory model wrt bitfields everywhere
On Mon, Mar 26, 2012 at 10:17 AM, Eric Botcazou ebotca...@adacore.com wrote: OTOH if DECL_ALIGN is absolute then if the first field of a record type has DECL_ALIGN that is a multiple of BITS_PER_UNIT we know we can use the present stor-layout code? So we can check that and give up computing representatives at all for a record type if that does not hold. I don't think so, DECL_ALIGN of a field is probably honored only if TYPE_ALIGN of the record is, and TYPE_ALIGN isn't honored for a bit field. Uh. When is a field a bit field though? At least stor-layout.c resets DECL_BIT_FIELD when local relative alignment is proper and the filed has an integer mode. That's overly optimistic if the record is placed at a bit position. Of course we still have DECL_BIT_FIELD_TYPE, but I wonder what the DECL_BIT_FIELD flag is for if we cannot really know whether the field in the end is a bitfield anyway... (what value do the various field-decls in a component-ref chain have anyway if the real interpretation is subject to the containing object, which might be even only specified by the type used for an indirect ref ...) Richard. -- Eric Botcazou
Re: [PATCH] Fix PRs 52080, 52097 and 48124, rewrite bitfield expansion, enable the C++ memory model wrt bitfields everywhere
On Mon, 26 Mar 2012, Eric Botcazou wrote: I think we indeed can't really in stor-layout, so the only place is very likely get_bit_range. Something like that for example. I think the expmed.c hunk should be applied to the 4.7 branch as well, because the new code in store_bit_field is quite dangerous without it. * expmed.c (store_bit_field): Assert that BITREGION_START is a multiple of a unit before computing the offset in units. * expr.c (get_bit_range): Return the null range if the enclosing record is part of a larger bit field. The patch looks reasonable - can we compute this backward from the result of the outer get_inner_reference call and the outermost field-decl though? Or make get_inner_reference compute that while analyzing the full reference and return a flag? OTOH it shouldn't be too expensive. I agree the assert should go to the banch as well, though the code only ever triggers there with --param allow-store-data-races=0. Thanks, Richard.
Re: [PATCH] Fix PRs 52080, 52097 and 48124, rewrite bitfield expansion, enable the C++ memory model wrt bitfields everywhere
Uh. When is a field a bit field though? At least stor-layout.c resets DECL_BIT_FIELD when local relative alignment is proper and the filed has an integer mode. That's overly optimistic if the record is placed at a bit position. Of course we still have DECL_BIT_FIELD_TYPE, but I wonder what the DECL_BIT_FIELD flag is for if we cannot really know whether the field in the end is a bitfield anyway... (what value do the various field-decls in a component-ref chain have anyway if the real interpretation is subject to the containing object, which might be even only specified by the type used for an indirect ref ...) We're talking about a bit field with record type here, so anything calculated within the record type in isolation is essentially invalidated for the bit field in question. Fortunately this only occurs in Ada, which doesn't use the very questionable C++ memory model (fingers crossed for the future. :-) -- Eric Botcazou
Re: [PATCH] Fix PRs 52080, 52097 and 48124, rewrite bitfield expansion, enable the C++ memory model wrt bitfields everywhere
The patch looks reasonable - can we compute this backward from the result of the outer get_inner_reference call and the outermost field-decl though? Or make get_inner_reference compute that while analyzing the full reference and return a flag? OTOH it shouldn't be too expensive. There are a lot of calls to get_inner_reference throughout the compiler though. And it is cheap if you compare it with the 4.7 code, which essentially does the same processing for _every_ field in the record (and even if the record object isn't a handled_component_p). -- Eric Botcazou
Re: [PATCH] Fix PRs 52080, 52097 and 48124, rewrite bitfield expansion, enable the C++ memory model wrt bitfields everywhere
On Thu, 22 Mar 2012, Richard Guenther wrote: On Thu, 22 Mar 2012, Eric Botcazou wrote: bitregion_start == 11 looks bogus. The representative is starting at DECL_FIELD_BIT_OFFSET (repr) = size_binop (BIT_AND_EXPR, DECL_FIELD_BIT_OFFSET (field), bitsize_int (~(BITS_PER_UNIT - 1))); which looks ok It cannot be OK if you want it to be on a byte boundary, since the field isn't on a byte boundary itself and they have the same DECL_FIELD_BIT_OFFSET (0). Huh? If they have DECL_FIELD_BIT_OFFSET of zero they are at a byte boundary, no? Wait - the RECORD_TYPE itself is at non-zero DECL_FIELD_BIT_OFFSET and thus a zero DECL_FIELD_BIT_OFFSET for its fields does not mean anything?! But how can DECL_OFFSET_ALIGN be still valid for such field? Obviously if DECL_FIELD_OFFSET == 0, DECL_FIELD_BIT_OFFSET == 0 then the offset needs to be aligned to DECL_OFFSET_ALIGN. Which then means DECL_OFFSET_ALIGN is a bit-alignment? Anyway, since we are trying to compute a nice mode to use for the bitfield representative we can give up in the second that we do not know how to reach BITS_PER_UNIT alignment. Or we can simply only try to ensure MIN (BITS_PER_UNIT, DECL_OFFSET_ALIGN) alignment/size of the representative. Of course the bitfield expansion code has to deal with non-byte-aligned representatives then, and we'd always have to use BLKmode for them. Btw, now checking with gdb, DECL_OFFSET_ALIGN is always 128 for all of the fields - that looks bogus. DECL_ALIGN is 1, but that doesn't mean DECL_OFFSET_ALIGN should not be 1 as well, no? Thanks, Richard.
Re: [PATCH] Fix PRs 52080, 52097 and 48124, rewrite bitfield expansion, enable the C++ memory model wrt bitfields everywhere
This is what I have applied after bootstrapping and testing on
x86_64-unknown-linux-gnu.
Thanks. Last identified problem: the miscompilation with variant record.
gnat.dg/pack17.adb
raised PROGRAM_ERROR : pack17.adb:36 explicit raise
FAIL: gnat.dg/pack17.adb execution test
Things start to go wrong at line 28:
24 procedure Fill (R : out Record_With_QImode_Variants) is
25 begin
26 R.C_Filler := (True, False, True, False, True, False, True);
27 R.C_Map := (True, False, True);
28 R.T_Int := 17;
29 end;
R is a packed record with variant part and -gnatR1 gives its layout:
for Record_With_Qimode_Variants'Object_Size use 24;
for Record_With_Qimode_Variants'Value_Size use 19;
for Record_With_Qimode_Variants'Alignment use 1;
for Record_With_Qimode_Variants use record
Dat 0 range 0 .. 0;
C_Filler at 0 range 1 .. 7;
C_Mapat 1 range 0 .. 2;
F_Bitat 1 range 3 .. 3;
F_Filler at 1 range 4 .. 10;
T_Intat 1 range 3 .. 10;
Now, since T_Int is within a variant, the assignment is translated into:
r_1(D)-d___XVN.O.t_int = 17;
d___XVN is QUAL_UNION_TYPE and O a RECORD_TYPE (fields of QUAL_UNION_TYPE are
always of RECORD_TYPE, that's why the overloading of qualifier is indeed OK).
The compiler now generates:
movl8(%ebp), %eax
movb$17, 1(%eax)
which is of course wrong given the layout above.
It seems to me that the new code implicitly assumes that the first field in a
bitfield group starts on a byte boundary, which doesn't hold in Ada.
--
Eric Botcazou
-- { dg-do run }
procedure Pack17 is
type Bitmap_T is array (Natural range ) of Boolean;
pragma Pack (Bitmap_T);
type Uint8 is range 0 .. 2 ** 8 - 1;
for Uint8'Size use 8;
type Record_With_QImode_Variants (D : Boolean) is record
C_Filler : Bitmap_T (1..7);
C_Map : Bitmap_T (1..3);
case D is
when False =
F_Bit : Boolean;
F_Filler : Bitmap_T (1..7);
when True =
T_Int : Uint8;
end case;
end record;
pragma Pack (Record_With_QImode_Variants);
procedure Fill (R : out Record_With_QImode_Variants) is
begin
R.C_Filler := (True, False, True, False, True, False, True);
R.C_Map := (True, False, True);
R.T_Int := 17;
end;
RT : Record_With_QImode_Variants (D = True);
begin
Fill (RT);
if RT.T_Int /= 17 then
raise Program_Error;
end if;
end;
Re: [PATCH] Fix PRs 52080, 52097 and 48124, rewrite bitfield expansion, enable the C++ memory model wrt bitfields everywhere
On Thu, 22 Mar 2012, Eric Botcazou wrote: This is what I have applied after bootstrapping and testing on x86_64-unknown-linux-gnu. Thanks. Last identified problem: the miscompilation with variant record. gnat.dg/pack17.adb raised PROGRAM_ERROR : pack17.adb:36 explicit raise FAIL: gnat.dg/pack17.adb execution test Things start to go wrong at line 28: 24 procedure Fill (R : out Record_With_QImode_Variants) is 25 begin 26 R.C_Filler := (True, False, True, False, True, False, True); 27 R.C_Map := (True, False, True); 28 R.T_Int := 17; 29 end; R is a packed record with variant part and -gnatR1 gives its layout: for Record_With_Qimode_Variants'Object_Size use 24; for Record_With_Qimode_Variants'Value_Size use 19; for Record_With_Qimode_Variants'Alignment use 1; for Record_With_Qimode_Variants use record Dat 0 range 0 .. 0; C_Filler at 0 range 1 .. 7; C_Mapat 1 range 0 .. 2; F_Bitat 1 range 3 .. 3; F_Filler at 1 range 4 .. 10; T_Intat 1 range 3 .. 10; Now, since T_Int is within a variant, the assignment is translated into: r_1(D)-d___XVN.O.t_int = 17; d___XVN is QUAL_UNION_TYPE and O a RECORD_TYPE (fields of QUAL_UNION_TYPE are always of RECORD_TYPE, that's why the overloading of qualifier is indeed OK). The compiler now generates: movl8(%ebp), %eax movb$17, 1(%eax) which is of course wrong given the layout above. It seems to me that the new code implicitly assumes that the first field in a bitfield group starts on a byte boundary, which doesn't hold in Ada. Yes, I have noticed that from what you can see in the code. So the issue is that get_bit_range tells us that it's ok to touch bits outside of the field - but that's obviously required. We may not change bits that are not covered by the FIELD_DECL which now somehow happens? That sounds like a latent bug in the bitfield expander to me - it should never _change_ bits outside of bitpos/bitsize as returned by get_inner_reference. I guess I'll have to debug this if you don't beat me on it. Richard.
Re: [PATCH] Fix PRs 52080, 52097 and 48124, rewrite bitfield expansion, enable the C++ memory model wrt bitfields everywhere
Yes, I have noticed that from what you can see in the code. So the
issue is that get_bit_range tells us that it's ok to touch bits
outside of the field - but that's obviously required. We may not
change bits that are not covered by the FIELD_DECL which now
somehow happens? That sounds like a latent bug in the bitfield
expander to me - it should never _change_ bits outside of bitpos/bitsize
as returned by get_inner_reference.
It's the new C++0x memory model stuff:
void
store_bit_field (rtx str_rtx, unsigned HOST_WIDE_INT bitsize,
unsigned HOST_WIDE_INT bitnum,
unsigned HOST_WIDE_INT bitregion_start,
unsigned HOST_WIDE_INT bitregion_end,
enum machine_mode fieldmode,
rtx value)
{
/* Under the C++0x memory model, we must not touch bits outside the
bit region. Adjust the address to start at the beginning of the
bit region. */
if (MEM_P (str_rtx)
bitregion_start 0)
{
enum machine_mode bestmode;
enum machine_mode op_mode;
unsigned HOST_WIDE_INT offset;
op_mode = mode_for_extraction (EP_insv, 3);
if (op_mode == MAX_MACHINE_MODE)
op_mode = VOIDmode;
offset = bitregion_start / BITS_PER_UNIT;
bitnum -= bitregion_start;
bitregion_end -= bitregion_start;
bitregion_start = 0;
which assumes that a bitfield group starts on a byte boundary. So this is
probably indeed latent in GCC 4.7 as well.
--
Eric Botcazou
Re: [PATCH] Fix PRs 52080, 52097 and 48124, rewrite bitfield expansion, enable the C++ memory model wrt bitfields everywhere
On Thu, 22 Mar 2012, Eric Botcazou wrote:
Yes, I have noticed that from what you can see in the code. So the
issue is that get_bit_range tells us that it's ok to touch bits
outside of the field - but that's obviously required. We may not
change bits that are not covered by the FIELD_DECL which now
somehow happens? That sounds like a latent bug in the bitfield
expander to me - it should never _change_ bits outside of bitpos/bitsize
as returned by get_inner_reference.
It's the new C++0x memory model stuff:
void
store_bit_field (rtx str_rtx, unsigned HOST_WIDE_INT bitsize,
unsigned HOST_WIDE_INT bitnum,
unsigned HOST_WIDE_INT bitregion_start,
unsigned HOST_WIDE_INT bitregion_end,
enum machine_mode fieldmode,
rtx value)
{
/* Under the C++0x memory model, we must not touch bits outside the
bit region. Adjust the address to start at the beginning of the
bit region. */
if (MEM_P (str_rtx)
bitregion_start 0)
{
enum machine_mode bestmode;
enum machine_mode op_mode;
unsigned HOST_WIDE_INT offset;
op_mode = mode_for_extraction (EP_insv, 3);
if (op_mode == MAX_MACHINE_MODE)
op_mode = VOIDmode;
offset = bitregion_start / BITS_PER_UNIT;
bitnum -= bitregion_start;
bitregion_end -= bitregion_start;
bitregion_start = 0;
which assumes that a bitfield group starts on a byte boundary. So this is
probably indeed latent in GCC 4.7 as well.
But the bitfield group _does_ start on a byte boundary. At least
that's what the new code in stor-layout ensures.
It's ok to assume the group starts on a byte boundary. But it's not
ok to modify bits outside of the access, so the RMW cycle has to mask
them properly.
Richard.
Re: [PATCH] Fix PRs 52080, 52097 and 48124, rewrite bitfield expansion, enable the C++ memory model wrt bitfields everywhere
But the bitfield group _does_ start on a byte boundary. At least that's what the new code in stor-layout ensures. No, it doesn't, since it does the computation on a record by record basis. Here we have a bitfield group that starts in a record and ends up in a nested record. It's ok to assume the group starts on a byte boundary. But it's not ok to modify bits outside of the access, so the RMW cycle has to mask them properly. We have 2 options: 1. the GCC 4.7 approach where get_bit_range returns 0 for bitstart, which doesn't make much sense but is in keeping with store_bit_field. 2. the GCC 4.8 approach where get_bit_range attempts to return a more correct value, but is currently wrong (bitregion_start=11, bitregion_end=18) because the representative of the bitfield is wrong. The real representative of the bitfield is outside the record type. -- Eric Botcazou
Re: [PATCH] Fix PRs 52080, 52097 and 48124, rewrite bitfield expansion, enable the C++ memory model wrt bitfields everywhere
On Thu, Mar 22, 2012 at 12:47 PM, Eric Botcazou ebotca...@adacore.com wrote: But the bitfield group _does_ start on a byte boundary. At least that's what the new code in stor-layout ensures. No, it doesn't, since it does the computation on a record by record basis. Here we have a bitfield group that starts in a record and ends up in a nested record. It's ok to assume the group starts on a byte boundary. But it's not ok to modify bits outside of the access, so the RMW cycle has to mask them properly. We have 2 options: 1. the GCC 4.7 approach where get_bit_range returns 0 for bitstart, which doesn't make much sense but is in keeping with store_bit_field. 2. the GCC 4.8 approach where get_bit_range attempts to return a more correct value, but is currently wrong (bitregion_start=11, bitregion_end=18) because the representative of the bitfield is wrong. The real representative of the bitfield is outside the record type. bitregion_start == 11 looks bogus. The representative is starting at DECL_FIELD_BIT_OFFSET (repr) = size_binop (BIT_AND_EXPR, DECL_FIELD_BIT_OFFSET (field), bitsize_int (~(BITS_PER_UNIT - 1))); which looks ok, the size of the representative is (at minimum) size = size_diffop (DECL_FIELD_OFFSET (field), DECL_FIELD_OFFSET (repr)); gcc_assert (host_integerp (size, 1)); bitsize = (tree_low_cst (size, 1) * BITS_PER_UNIT + tree_low_cst (DECL_FIELD_BIT_OFFSET (field), 1) - tree_low_cst (DECL_FIELD_BIT_OFFSET (repr), 1) + tree_low_cst (DECL_SIZE (field), 1)); /* Round up bitsize to multiples of BITS_PER_UNIT. */ bitsize = (bitsize + BITS_PER_UNIT - 1) ~(BITS_PER_UNIT - 1); that looks ok to me as well. Is the issue that we, in get_bit_range, compute bitregion_start relative to the byte-aligned offset of the representative? At least I still can't see where things go wrong from inspecting the code ... Richard. -- Eric Botcazou
Re: [PATCH] Fix PRs 52080, 52097 and 48124, rewrite bitfield expansion, enable the C++ memory model wrt bitfields everywhere
bitregion_start == 11 looks bogus. The representative is starting at DECL_FIELD_BIT_OFFSET (repr) = size_binop (BIT_AND_EXPR, DECL_FIELD_BIT_OFFSET (field), bitsize_int (~(BITS_PER_UNIT - 1))); which looks ok It cannot be OK if you want it to be on a byte boundary, since the field isn't on a byte boundary itself and they have the same DECL_FIELD_BIT_OFFSET (0). the size of the representative is (at minimum) size = size_diffop (DECL_FIELD_OFFSET (field), DECL_FIELD_OFFSET (repr)); gcc_assert (host_integerp (size, 1)); bitsize = (tree_low_cst (size, 1) * BITS_PER_UNIT + tree_low_cst (DECL_FIELD_BIT_OFFSET (field), 1) - tree_low_cst (DECL_FIELD_BIT_OFFSET (repr), 1) + tree_low_cst (DECL_SIZE (field), 1)); /* Round up bitsize to multiples of BITS_PER_UNIT. */ bitsize = (bitsize + BITS_PER_UNIT - 1) ~(BITS_PER_UNIT - 1); that looks ok to me as well. Is the issue that we, in get_bit_range, compute bitregion_start relative to the byte-aligned offset of the representative? The issue is that the representative is assumed to be on a byte boundary in get_bit_range, but it isn't in the case at hand. So either we cope with that (this is the GCC 4.7 approach) or we change the representative somehow. IOW, either we pretend that a bitfield group is entirely contained within a single record type or we acknowledge that a bitfield group can cross a record boundary. -- Eric Botcazou
Re: [PATCH] Fix PRs 52080, 52097 and 48124, rewrite bitfield expansion, enable the C++ memory model wrt bitfields everywhere
On Thu, 22 Mar 2012, Eric Botcazou wrote: bitregion_start == 11 looks bogus. The representative is starting at DECL_FIELD_BIT_OFFSET (repr) = size_binop (BIT_AND_EXPR, DECL_FIELD_BIT_OFFSET (field), bitsize_int (~(BITS_PER_UNIT - 1))); which looks ok It cannot be OK if you want it to be on a byte boundary, since the field isn't on a byte boundary itself and they have the same DECL_FIELD_BIT_OFFSET (0). Huh? If they have DECL_FIELD_BIT_OFFSET of zero they are at a byte boundary, no? Wait - the RECORD_TYPE itself is at non-zero DECL_FIELD_BIT_OFFSET and thus a zero DECL_FIELD_BIT_OFFSET for its fields does not mean anything?! But how can DECL_OFFSET_ALIGN be still valid for such field? Obviously if DECL_FIELD_OFFSET == 0, DECL_FIELD_BIT_OFFSET == 0 then the offset needs to be aligned to DECL_OFFSET_ALIGN. Which then means DECL_OFFSET_ALIGN is a bit-alignment? Anyway, since we are trying to compute a nice mode to use for the bitfield representative we can give up in the second that we do not know how to reach BITS_PER_UNIT alignment. Or we can simply only try to ensure MIN (BITS_PER_UNIT, DECL_OFFSET_ALIGN) alignment/size of the representative. Of course the bitfield expansion code has to deal with non-byte-aligned representatives then, and we'd always have to use BLKmode for them. the size of the representative is (at minimum) size = size_diffop (DECL_FIELD_OFFSET (field), DECL_FIELD_OFFSET (repr)); gcc_assert (host_integerp (size, 1)); bitsize = (tree_low_cst (size, 1) * BITS_PER_UNIT + tree_low_cst (DECL_FIELD_BIT_OFFSET (field), 1) - tree_low_cst (DECL_FIELD_BIT_OFFSET (repr), 1) + tree_low_cst (DECL_SIZE (field), 1)); /* Round up bitsize to multiples of BITS_PER_UNIT. */ bitsize = (bitsize + BITS_PER_UNIT - 1) ~(BITS_PER_UNIT - 1); that looks ok to me as well. Is the issue that we, in get_bit_range, compute bitregion_start relative to the byte-aligned offset of the representative? The issue is that the representative is assumed to be on a byte boundary in get_bit_range, but it isn't in the case at hand. So either we cope with that (this is the GCC 4.7 approach) or we change the representative somehow. I think we can't change it to be on a byte-boundary, the same record may be used at different bit-positions, no? IOW, either we pretend that a bitfield group is entirely contained within a single record type or we acknowledge that a bitfield group can cross a record boundary. Sure, we acknowledge it can cross a record boundary. I just was not aware we cannot statically compute the bit-offset to the previous byte for a record type. Richard.
Re: [PATCH] Fix PRs 52080, 52097 and 48124, rewrite bitfield expansion, enable the C++ memory model wrt bitfields everywhere
On Mon, 19 Mar 2012, Richard Guenther wrote:
On Mon, 19 Mar 2012, Eric Botcazou wrote:
But it's only ever computed for RECORD_TYPEs where DECL_QUALIFIER is
unused.
OK, that could work indeed.
For now giving up seems to be easiest (just give up when
DECL_FIELD_OFFSET is not equal for all of the bitfield members).
That will at most get you the miscompiles for the PRs back, for
languages with funny structure layout.
I have another variant of the DECL_FIELD_OFFSET problem:
FAIL: gnat.dg/specs/pack8.ads (test for excess errors)
Excess errors:
+===GNAT BUG DETECTED==+
| 4.8.0 20120314 (experimental) [trunk revision 185395] (i586-suse-linux)
GCC
error:|
| in finish_bitfield_representative, at stor-layout.c:1762 |
| Error detected at pack8.ads:17:4
Testcase attached:
gnat.dg/specs/pack8.ads
gnat.dg/specs/pack8_pkg.ads
Thanks. That one indeed has different DECL_FIELD_OFFSET,
((sizetype) MAX_EXPR (integer) pack8__R1s, 0 + (sizetype) MAX_EXPR
(integer) pack8__R1s, 0) + 1
vs.
(sizetype) MAX_EXPR (integer) pack8__R1s, 0 + (sizetype) MAX_EXPR
(integer) pack8__R1s, 0
we're not putting the 1 byte offset into DECL_FIELD_BIT_OFFSET
because DECL_OFFSET_ALIGN is 8 in this case. Eventually we should
be able to relax how many bits we push into DECL_FIELD_BIT_OFFSET.
I agree that giving up (for now) is a sensible option. Thanks.
Done with the patch below. We're actually not going to generate
possibly wrong-code again but sub-optimal code.
Bootstrap regtest pending on x86_64-unknown-linux-gnu.
This is what I have applied after bootstrapping and testing on
x86_64-unknown-linux-gnu.
Richard.
2012-03-20 Richard Guenther rguent...@suse.de
* stor-layout.c (finish_bitfield_representative): Fallback
to conservative maximum size if the padding up to the next
field cannot be computed as a constant.
(finish_bitfield_layout): If we cannot compute the distance
between the start of the bitfield representative and the
bitfield member start a new representative.
* expr.c (get_bit_range): The distance between the start of
the bitfield representative and the bitfield member is zero
if the field offsets are not constants.
* gnat.dg/pack16.adb: New testcase.
* gnat.dg/pack16_pkg.ads: Likewise.
* gnat.dg/specs/pack8.ads: Likewise.
* gnat.dg/specs/pack8_pkg.ads: Likewise.
Index: gcc/stor-layout.c
===
*** gcc/stor-layout.c (revision 185518)
--- gcc/stor-layout.c (working copy)
*** finish_bitfield_representative (tree rep
*** 1781,1790
return;
maxsize = size_diffop (DECL_FIELD_OFFSET (nextf),
DECL_FIELD_OFFSET (repr));
! gcc_assert (host_integerp (maxsize, 1));
! maxbitsize = (tree_low_cst (maxsize, 1) * BITS_PER_UNIT
! + tree_low_cst (DECL_FIELD_BIT_OFFSET (nextf), 1)
! - tree_low_cst (DECL_FIELD_BIT_OFFSET (repr), 1));
}
else
{
--- 1781,1797
return;
maxsize = size_diffop (DECL_FIELD_OFFSET (nextf),
DECL_FIELD_OFFSET (repr));
! if (host_integerp (maxsize, 1))
! {
! maxbitsize = (tree_low_cst (maxsize, 1) * BITS_PER_UNIT
! + tree_low_cst (DECL_FIELD_BIT_OFFSET (nextf), 1)
! - tree_low_cst (DECL_FIELD_BIT_OFFSET (repr), 1));
! /* If the group ends within a bitfield nextf does not need to be
!aligned to BITS_PER_UNIT. Thus round up. */
! maxbitsize = (maxbitsize + BITS_PER_UNIT - 1) ~(BITS_PER_UNIT - 1);
! }
! else
! maxbitsize = bitsize;
}
else
{
*** finish_bitfield_layout (record_layout_in
*** 1888,1893
--- 1895,1902
}
else if (DECL_BIT_FIELD_TYPE (field))
{
+ gcc_assert (repr != NULL_TREE);
+
/* Zero-size bitfields finish off a representative and
do not have a representative themselves. This is
required by the C++ memory model. */
*** finish_bitfield_layout (record_layout_in
*** 1896,1901
--- 1905,1928
finish_bitfield_representative (repr, prev);
repr = NULL_TREE;
}
+
+ /* We assume that either DECL_FIELD_OFFSET of the representative
+and each bitfield member is a constant or they are equal.
+This is because we need to be able to compute the bit-offset
+of each field relative to the representative in get_bit_range
+during RTL expansion.
+If these constraints are not met, simply force a new
+representative to be generated. That will at most
+
Re: [PATCH] Fix PRs 52080, 52097 and 48124, rewrite bitfield expansion, enable the C++ memory model wrt bitfields everywhere
But it's only ever computed for RECORD_TYPEs where DECL_QUALIFIER is unused. OK, that could work indeed. For now giving up seems to be easiest (just give up when DECL_FIELD_OFFSET is not equal for all of the bitfield members). That will at most get you the miscompiles for the PRs back, for languages with funny structure layout. I have another variant of the DECL_FIELD_OFFSET problem: FAIL: gnat.dg/specs/pack8.ads (test for excess errors) Excess errors: +===GNAT BUG DETECTED==+ | 4.8.0 20120314 (experimental) [trunk revision 185395] (i586-suse-linux) GCC error:| | in finish_bitfield_representative, at stor-layout.c:1762 | | Error detected at pack8.ads:17:4 Testcase attached: gnat.dg/specs/pack8.ads gnat.dg/specs/pack8_pkg.ads I agree that giving up (for now) is a sensible option. Thanks. -- Eric Botcazou with Pack8_Pkg; package Pack8 is subtype Index_Type is Integer range 1 .. Pack8_Pkg.N; subtype Str is String( Index_Type); subtype Str2 is String (1 .. 11); type Rec is record S1 : Str; S2 : Str; B : Boolean; S3 : Str2; end record; pragma Pack (Rec); end Pack8; package Pack8_Pkg is N : Natural := 1; end Pack8_Pkg;
Re: [PATCH] Fix PRs 52080, 52097 and 48124, rewrite bitfield expansion, enable the C++ memory model wrt bitfields everywhere
On Mon, 19 Mar 2012, Eric Botcazou wrote:
But it's only ever computed for RECORD_TYPEs where DECL_QUALIFIER is
unused.
OK, that could work indeed.
For now giving up seems to be easiest (just give up when
DECL_FIELD_OFFSET is not equal for all of the bitfield members).
That will at most get you the miscompiles for the PRs back, for
languages with funny structure layout.
I have another variant of the DECL_FIELD_OFFSET problem:
FAIL: gnat.dg/specs/pack8.ads (test for excess errors)
Excess errors:
+===GNAT BUG DETECTED==+
| 4.8.0 20120314 (experimental) [trunk revision 185395] (i586-suse-linux) GCC
error:|
| in finish_bitfield_representative, at stor-layout.c:1762 |
| Error detected at pack8.ads:17:4
Testcase attached:
gnat.dg/specs/pack8.ads
gnat.dg/specs/pack8_pkg.ads
Thanks. That one indeed has different DECL_FIELD_OFFSET,
((sizetype) MAX_EXPR (integer) pack8__R1s, 0 + (sizetype) MAX_EXPR
(integer) pack8__R1s, 0) + 1
vs.
(sizetype) MAX_EXPR (integer) pack8__R1s, 0 + (sizetype) MAX_EXPR
(integer) pack8__R1s, 0
we're not putting the 1 byte offset into DECL_FIELD_BIT_OFFSET
because DECL_OFFSET_ALIGN is 8 in this case. Eventually we should
be able to relax how many bits we push into DECL_FIELD_BIT_OFFSET.
I agree that giving up (for now) is a sensible option. Thanks.
Done with the patch below. We're actually not going to generate
possibly wrong-code again but sub-optimal code.
Bootstrap regtest pending on x86_64-unknown-linux-gnu.
Richard.
2012-03-19 Richard Guenther rguent...@suse.de
* stor-layout.c (finish_bitfield_representative): Fallback
to conservative maximum size if the padding up to the next
field cannot be computed as a constant.
(finish_bitfield_layout): If we cannot compute the distance
between the start of the bitfield representative and the
bitfield member start a new representative.
* expr.c (get_bit_range): The distance between the start of
the bitfield representative and the bitfield member is zero
if the field offsets are not constants.
* gnat.dg/pack16.adb: New testcase.
* gnat.dg/pack16_pkg.ads: Likewise.
* gnat.dg/specs/pack8.ads: Likewise.
* gnat.dg/specs/pack8_pkg.ads: Likewise.
Index: gcc/stor-layout.c
===
*** gcc/stor-layout.c (revision 185518)
--- gcc/stor-layout.c (working copy)
*** finish_bitfield_representative (tree rep
*** 1781,1790
return;
maxsize = size_diffop (DECL_FIELD_OFFSET (nextf),
DECL_FIELD_OFFSET (repr));
! gcc_assert (host_integerp (maxsize, 1));
! maxbitsize = (tree_low_cst (maxsize, 1) * BITS_PER_UNIT
! + tree_low_cst (DECL_FIELD_BIT_OFFSET (nextf), 1)
! - tree_low_cst (DECL_FIELD_BIT_OFFSET (repr), 1));
}
else
{
--- 1781,1792
return;
maxsize = size_diffop (DECL_FIELD_OFFSET (nextf),
DECL_FIELD_OFFSET (repr));
! if (host_integerp (maxsize, 1))
! maxbitsize = (tree_low_cst (maxsize, 1) * BITS_PER_UNIT
! + tree_low_cst (DECL_FIELD_BIT_OFFSET (nextf), 1)
! - tree_low_cst (DECL_FIELD_BIT_OFFSET (repr), 1));
! else
! maxbitsize = bitsize;
}
else
{
*** finish_bitfield_layout (record_layout_in
*** 1888,1893
--- 1890,1897
}
else if (DECL_BIT_FIELD_TYPE (field))
{
+ gcc_assert (repr != NULL_TREE);
+
/* Zero-size bitfields finish off a representative and
do not have a representative themselves. This is
required by the C++ memory model. */
*** finish_bitfield_layout (record_layout_in
*** 1896,1901
--- 1900,1923
finish_bitfield_representative (repr, prev);
repr = NULL_TREE;
}
+
+ /* We assume that either DECL_FIELD_OFFSET of the representative
+and each bitfield member is a constant or they are equal.
+This is because we need to be able to compute the bit-offset
+of each field relative to the representative in get_bit_range
+during RTL expansion.
+If these constraints are not met, simply force a new
+representative to be generated. That will at most
+generate worse code but still maintain correctness with
+respect to the C++ memory model. */
+ if (!((host_integerp (DECL_FIELD_OFFSET (repr), 1)
+ host_integerp (DECL_FIELD_OFFSET (field), 1))
+ || operand_equal_p (DECL_FIELD_OFFSET (repr),
+ DECL_FIELD_OFFSET (field), 0)))
+ {
+ finish_bitfield_representative
Re: [PATCH] Fix PRs 52080, 52097 and 48124, rewrite bitfield expansion, enable the C++ memory model wrt bitfields everywhere
On Thu, 15 Mar 2012, Eric Botcazou wrote: Computing the offset in stor-layout.c and storing it in DECL_INITIAL? Ugh. I just realized that the DECL_BIT_FIELD_REPRESENTATIVE is built during layout... but is overloaded with DECL_QUALIFIER. That's probably the source of the miscompilation I talked about earlier. But it's only ever computed for RECORD_TYPEs where DECL_QUALIFIER is unused. Can we delay it until after gimplification ? Then we could use DECL_INITIAL. No, I don't think so. We can store the bit-offset relative to the start of the group in tree_base.address_space (ugh), if 8 bits are enough for that (ugh). For now giving up seems to be easiest (just give up when DECL_FIELD_OFFSET is not equal for all of the bitfield members). That will at most get you the miscompiles for the PRs back, for languages with funny structure layout. Well. I'll think about this some more and in the meantime install the fix for the easy problem. Richard.
Re: [PATCH] Fix PRs 52080, 52097 and 48124, rewrite bitfield expansion, enable the C++ memory model wrt bitfields everywhere
1. is easy, see patch below. 2. is much harder - we need to compute the bit-offset relative to the bitfield group start, thus in get_bit_range we do /* Compute the adjustment to bitpos from the offset of the field relative to the representative. */ offset = size_diffop (DECL_FIELD_OFFSET (field), DECL_FIELD_OFFSET (repr)); bitoffset = (tree_low_cst (offset, 1) * BITS_PER_UNIT + tree_low_cst (DECL_FIELD_BIT_OFFSET (field), 1) - tree_low_cst (DECL_FIELD_BIT_OFFSET (repr), 1)); and we cannot generally assume offset is zero (well, maybe we could arrange that though, at least we could assert that for all fields in a bitfield group DECL_FIELD_OFFSET is the same?). I'm skeptical. Any suggestion? Apart from trying to make sure that offset will be zero by construction? Or by simply not handling bitfields properly that start at a variable offset? Computing the offset in stor-layout.c and storing it in DECL_INITIAL? The finish_bitfield_representative hunk implements the fix for 1., the rest the proposed zero-by-construction solution for 2. Thanks! -- Eric Botcazou
Re: [PATCH] Fix PRs 52080, 52097 and 48124, rewrite bitfield expansion, enable the C++ memory model wrt bitfields everywhere
Computing the offset in stor-layout.c and storing it in DECL_INITIAL? Ugh. I just realized that the DECL_BIT_FIELD_REPRESENTATIVE is built during layout... but is overloaded with DECL_QUALIFIER. That's probably the source of the miscompilation I talked about earlier. Can we delay it until after gimplification ? Then we could use DECL_INITIAL. -- Eric Botcazou
Re: [PATCH] Fix PRs 52080, 52097 and 48124, rewrite bitfield expansion, enable the C++ memory model wrt bitfields everywhere
On Fri, 9 Mar 2012, Eric Botcazou wrote:
This patch also completely replaces get_bit_range (which is where
PR52097 ICEs) by a trivial implementation.
How does it short-circuit the decision made by get_best_mode exactly? By
making get_bit_range return non-zero in more cases?
It will make get_bit_range return non-zero in all cases (well, in
all cases where there is a COMPONENT_REF with a FIELD_DECL that
was part of a RECORD_TYPE at the time of finish_record_layout)
There is PR52134 which will make this patch cause 1 gnat regression.
This looks rather benign to me.
Yeah, it should be easy to fix - the question is whether we can
really rely on TYPE_SIZE_UNIT (DECL_CONTEXT (field)) - DECL_FIELD_OFFSET
(field) to fold to a constant if field is the first field of a bitfield
group. We can as well easily avoid the ICE by not computing a
DECL_BIT_FIELD_REPRESENTATIVE for such field in some way.
In the end how we divide bitfield groups will probably get some
control via a langhook.
* gimplify.c (gimplify_expr): Translate bitfield accesses
to BIT_FIELD_REFs of the representative.
This part isn't in the patch.
Oops. And it should not be (I did that for experimentation), ChangeLog
piece dropped.
+ /* Return a new underlying object for a bitfield started with FIELD. */
+
+ static tree
+ start_bitfield_representative (tree field)
+ {
+ tree repr = make_node (FIELD_DECL);
+ DECL_FIELD_OFFSET (repr) = DECL_FIELD_OFFSET (field);
+ /* Force the representative to begin at an BITS_PER_UNIT aligned
...at a BITS_PER_UNIT aligned...
Fixed.
+ boundary - C++ may use tail-padding of a base object to
+ continue packing bits so the bitfield region does not start
+ at bit zero (see g++.dg/abi/bitfield5.C for example).
+ Unallocated bits may happen for other reasons as well,
+ for example Ada which allows explicit bit-granular structure layout.
*/ + DECL_FIELD_BIT_OFFSET (repr)
+ = size_binop (BIT_AND_EXPR,
+ DECL_FIELD_BIT_OFFSET (field),
+ bitsize_int (~(BITS_PER_UNIT - 1)));
+ SET_DECL_OFFSET_ALIGN (repr, DECL_OFFSET_ALIGN (field));
+ DECL_SIZE (repr) = DECL_SIZE (field);
+ DECL_PACKED (repr) = DECL_PACKED (field);
+ DECL_CONTEXT (repr) = DECL_CONTEXT (field);
+ return repr;
Any particular reason to set DECL_SIZE but not DECL_SIZE_UNIT here? They are
generally set together.
No reason, fixed (I just set those fields I will use during updating
of 'repr', the other fields are set once the field has final size).
+
+ /* Finish up a bitfield group that was started by creating the underlying
+object REPR with the last fied in the bitfield group FIELD. */
...the last field...
Fixed.
+ /* Round up bitsize to multiples of BITS_PER_UNIT. */
+ bitsize = (bitsize + BITS_PER_UNIT - 1) ~(BITS_PER_UNIT - 1);
+
+ /* Find the smallest nice mode to use.
+ ??? Possibly use get_best_mode with appropriate arguments instead
+ (which would eventually require splitting representatives here). */
+ for (modesize = bitsize; modesize = maxbitsize; modesize +=
BITS_PER_UNIT) + {
+ mode = mode_for_size (modesize, MODE_INT, 1);
+ if (mode != BLKmode)
+ break;
+ }
The double loop looks superfluous. Why not re-using the implementation of
smallest_mode_for_size?
Ah, indeed. Matching the current implementation would be
for (mode = GET_CLASS_NARROWEST_MODE (MODE_INT); mode != VOIDmode;
mode = GET_MODE_WIDER_MODE (mode))
if (GET_MODE_PRECISION (mode) = bitsize)
break;
if (mode != VOIDmode
(GET_MODE_PRECISION (mode) maxbitsize
|| GET_MODE_PRECISION (mode) MAX_FIXED_MODE_SIZE))
mode = VOIDmode;
if (mode == VOIDmode)
...
Btw, I _think_ I want GET_MODE_BITSIZE here - we cannot allow
GET_MODE_BITSIZE GET_MODE_PRECISION as that would possibly
access memory that is not allowed. Thus, what GET_MODE_* would
identify the access size used for a MEM of that mode?
Anyway, fixed as you suggested, with the MAX_FIXED_MODE_SIZE check.
I'll split out the tree-sra.c piece, re-test and re-post.
Thanks,
Richard.
Re: [PATCH] Fix PRs 52080, 52097 and 48124, rewrite bitfield expansion, enable the C++ memory model wrt bitfields everywhere
Btw, I _think_ I want GET_MODE_BITSIZE here - we cannot allow GET_MODE_BITSIZE GET_MODE_PRECISION as that would possibly access memory that is not allowed. Thus, what GET_MODE_* would identify the access size used for a MEM of that mode? I agree that GET_MODE_BITSIZE makes more sense than GET_MODE_PRECISION here. -- Eric Botcazou
Re: [PATCH] Fix PRs 52080, 52097 and 48124, rewrite bitfield expansion, enable the C++ memory model wrt bitfields everywhere
This patch also completely replaces get_bit_range (which is where
PR52097 ICEs) by a trivial implementation.
How does it short-circuit the decision made by get_best_mode exactly? By
making get_bit_range return non-zero in more cases?
There is PR52134 which will make this patch cause 1 gnat regression.
This looks rather benign to me.
* gimplify.c (gimplify_expr): Translate bitfield accesses
to BIT_FIELD_REFs of the representative.
This part isn't in the patch.
+ /* Return a new underlying object for a bitfield started with FIELD. */
+
+ static tree
+ start_bitfield_representative (tree field)
+ {
+ tree repr = make_node (FIELD_DECL);
+ DECL_FIELD_OFFSET (repr) = DECL_FIELD_OFFSET (field);
+ /* Force the representative to begin at an BITS_PER_UNIT aligned
...at a BITS_PER_UNIT aligned...
+ boundary - C++ may use tail-padding of a base object to
+ continue packing bits so the bitfield region does not start
+ at bit zero (see g++.dg/abi/bitfield5.C for example).
+ Unallocated bits may happen for other reasons as well,
+ for example Ada which allows explicit bit-granular structure layout.
*/ + DECL_FIELD_BIT_OFFSET (repr)
+ = size_binop (BIT_AND_EXPR,
+ DECL_FIELD_BIT_OFFSET (field),
+ bitsize_int (~(BITS_PER_UNIT - 1)));
+ SET_DECL_OFFSET_ALIGN (repr, DECL_OFFSET_ALIGN (field));
+ DECL_SIZE (repr) = DECL_SIZE (field);
+ DECL_PACKED (repr) = DECL_PACKED (field);
+ DECL_CONTEXT (repr) = DECL_CONTEXT (field);
+ return repr;
Any particular reason to set DECL_SIZE but not DECL_SIZE_UNIT here? They are
generally set together.
+
+ /* Finish up a bitfield group that was started by creating the underlying
+object REPR with the last fied in the bitfield group FIELD. */
...the last field...
+ /* Round up bitsize to multiples of BITS_PER_UNIT. */
+ bitsize = (bitsize + BITS_PER_UNIT - 1) ~(BITS_PER_UNIT - 1);
+
+ /* Find the smallest nice mode to use.
+ ??? Possibly use get_best_mode with appropriate arguments instead
+ (which would eventually require splitting representatives here). */
+ for (modesize = bitsize; modesize = maxbitsize; modesize +=
BITS_PER_UNIT) + {
+ mode = mode_for_size (modesize, MODE_INT, 1);
+ if (mode != BLKmode)
+ break;
+ }
The double loop looks superfluous. Why not re-using the implementation of
smallest_mode_for_size?
for (mode = GET_CLASS_NARROWEST_MODE (MODE_INT); mode != VOIDmode;
mode = GET_MODE_WIDER_MODE (mode))
if (GET_MODE_PRECISION (mode) = bitsize)
break;
if (mode != VOIDmode GET_MODE_PRECISION (mode) maxbitsize)
mode = VOIDmode;
if (mode == VOIDmode)
...
--
Eric Botcazou
