This patch is a followup to the addition of support for
-fstrict-volatile-bitfields (required by the ARM EABI); see this thread
http://gcc.gnu.org/ml/gcc-patches/2010-10/msg01889.html
for discussion of the original patch.
That patch only addressed the behavior when extracting the value of a
volatile bit field, but the same problems affect storing values into a
volatile bit field (or a field of a packed structure, which is
effectively implemented as a bit field). This patch makes the code for
bitfield stores mirror that for bitfield loads.
Although the fix is in target-independent code, it's really for ARM;
hence the test case, which (without this patch) generates wrong code.
Code to determine the access width was correctly preserving the
user-specified width, but was incorrectly falling through to code that
assumes word mode.
As well as regression testing on arm-none-eabi, I've bootstrapped and
regression-tested this patch on x86_64 Linux. Earlier versions of this
patch have also been present in our local 4.5 and 4.6 GCC trees for some
time, so it's been well-tested on a variety of other platforms. OK to
check in on mainline?
-Sandra
2012-08-21 Paul Brook <p...@codesourcery.com>
Joseph Myers <jos...@codesourcery.com>
Sandra Loosemore <san...@codesourcery.com>
gcc/
* expr.h (store_bit_field): Add packedp parameter to prototype.
* expmed.c (store_bit_field, store_bit_field_1): Add packedp
parameter. Adjust all callers.
(warn_misaligned_bitfield): New function, split from
extract_fixed_bit_field.
(store_fixed_bit_field): Add packedp parameter. Fix wrong-code
behavior for the combination of misaligned bitfield and
-fstrict-volatile-bitfields. Use warn_misaligned_bitfield.
(extract_fixed_bit_field): Use warn_misaligned_bitfield.
* expr.c: Adjust calls to store_bit_field.
(expand_assignment): Identify accesses to packed structures.
(store_field): Add packedp parameter. Adjust callers.
* calls.c: Adjust calls to store_bit_field.
* ifcvt.c: Likewise.
* config/s390/s390.c: Likewise.
gcc/testsuite/
* gcc.target/arm/volatile-bitfields-5.c: New test case.
Index: gcc/expr.h
===================================================================
--- gcc/expr.h (revision 190541)
+++ gcc/expr.h (working copy)
@@ -693,7 +693,7 @@ extern void store_bit_field (rtx, unsign
unsigned HOST_WIDE_INT,
unsigned HOST_WIDE_INT,
unsigned HOST_WIDE_INT,
- enum machine_mode, rtx);
+ bool, enum machine_mode, rtx);
extern rtx extract_bit_field (rtx, unsigned HOST_WIDE_INT,
unsigned HOST_WIDE_INT, int, bool, rtx,
enum machine_mode, enum machine_mode);
Index: gcc/expmed.c
===================================================================
--- gcc/expmed.c (revision 190541)
+++ gcc/expmed.c (working copy)
@@ -50,7 +50,7 @@ static void store_fixed_bit_field (rtx,
unsigned HOST_WIDE_INT,
unsigned HOST_WIDE_INT,
unsigned HOST_WIDE_INT,
- rtx);
+ rtx, bool);
static void store_split_bit_field (rtx, unsigned HOST_WIDE_INT,
unsigned HOST_WIDE_INT,
unsigned HOST_WIDE_INT,
@@ -406,7 +406,7 @@ store_bit_field_1 (rtx str_rtx, unsigned
unsigned HOST_WIDE_INT bitnum,
unsigned HOST_WIDE_INT bitregion_start,
unsigned HOST_WIDE_INT bitregion_end,
- enum machine_mode fieldmode,
+ bool packedp, enum machine_mode fieldmode,
rtx value, bool fallback_p)
{
unsigned int unit
@@ -638,7 +638,7 @@ store_bit_field_1 (rtx str_rtx, unsigned
if (!store_bit_field_1 (op0, new_bitsize,
bitnum + bit_offset,
bitregion_start, bitregion_end,
- word_mode,
+ false, word_mode,
value_word, fallback_p))
{
delete_insns_since (last);
@@ -859,7 +859,7 @@ store_bit_field_1 (rtx str_rtx, unsigned
tempreg = copy_to_reg (xop0);
if (store_bit_field_1 (tempreg, bitsize, xbitpos,
bitregion_start, bitregion_end,
- fieldmode, orig_value, false))
+ false, fieldmode, orig_value, false))
{
emit_move_insn (xop0, tempreg);
return true;
@@ -872,7 +872,7 @@ store_bit_field_1 (rtx str_rtx, unsigned
return false;
store_fixed_bit_field (op0, offset, bitsize, bitpos,
- bitregion_start, bitregion_end, value);
+ bitregion_start, bitregion_end, value, packedp);
return true;
}
@@ -885,6 +885,8 @@ store_bit_field_1 (rtx str_rtx, unsigned
These two fields are 0, if the C++ memory model does not apply,
or we are not interested in keeping track of bitfield regions.
+ PACKEDP is true for fields with the packed attribute.
+
FIELDMODE is the machine-mode of the FIELD_DECL node for this field. */
void
@@ -892,6 +894,7 @@ store_bit_field (rtx str_rtx, unsigned H
unsigned HOST_WIDE_INT bitnum,
unsigned HOST_WIDE_INT bitregion_start,
unsigned HOST_WIDE_INT bitregion_end,
+ bool packedp,
enum machine_mode fieldmode,
rtx value)
{
@@ -924,9 +927,48 @@ store_bit_field (rtx str_rtx, unsigned H
if (!store_bit_field_1 (str_rtx, bitsize, bitnum,
bitregion_start, bitregion_end,
- fieldmode, value, true))
+ packedp, fieldmode, value, true))
gcc_unreachable ();
}
+
+static void
+warn_misaligned_bitfield (bool struct_member, bool packedp)
+{
+ static bool informed_about_misalignment = false;
+ bool warned;
+
+ if (packedp)
+ {
+ if (struct_member)
+ warning_at (input_location, OPT_fstrict_volatile_bitfields,
+ "multiple accesses to volatile structure member"
+ " because of packed attribute");
+ else
+ warning_at (input_location, OPT_fstrict_volatile_bitfields,
+ "multiple accesses to volatile structure bitfield"
+ " because of packed attribute");
+ }
+ else
+ {
+ if (struct_member)
+ warned = warning_at (input_location, OPT_fstrict_volatile_bitfields,
+ "mis-aligned access used for structure member");
+ else
+ warned = warning_at (input_location, OPT_fstrict_volatile_bitfields,
+ "mis-aligned access used for structure bitfield");
+ if (! informed_about_misalignment && warned)
+ {
+ informed_about_misalignment = true;
+ inform (input_location,
+ "When a volatile object spans multiple type-sized"
+ " locations, the compiler must choose between using a"
+ " single mis-aligned access to preserve the volatility,"
+ " or using multiple aligned accesses to avoid runtime"
+ " faults. This code may fail at runtime if the hardware"
+ " does not allow this access.");
+ }
+ }
+}
/* Use shifts and boolean operations to store VALUE
into a bit field of width BITSIZE
@@ -943,7 +985,8 @@ store_fixed_bit_field (rtx op0, unsigned
unsigned HOST_WIDE_INT bitpos,
unsigned HOST_WIDE_INT bitregion_start,
unsigned HOST_WIDE_INT bitregion_end,
- rtx value)
+ rtx value,
+ bool packedp)
{
enum machine_mode mode;
unsigned int total_bits = BITS_PER_WORD;
@@ -981,6 +1024,7 @@ store_fixed_bit_field (rtx op0, unsigned
includes the entire field. If such a mode would be larger than
a word, we won't be doing the extraction the normal way.
We don't want a mode bigger than the destination. */
+ bool realign = true;
mode = GET_MODE (op0);
if (GET_MODE_BITSIZE (mode) == 0
@@ -991,7 +1035,25 @@ store_fixed_bit_field (rtx op0, unsigned
&& GET_MODE_BITSIZE (GET_MODE (op0)) > 0
&& GET_MODE_BITSIZE (GET_MODE (op0)) <= maxbits
&& flag_strict_volatile_bitfields > 0)
- mode = GET_MODE (op0);
+ {
+ /* We must use the specified access size. */
+ mode = GET_MODE (op0);
+ total_bits = GET_MODE_BITSIZE (mode);
+ if (bitpos + bitsize <= total_bits
+ && bitpos + bitsize
+ + (offset % (total_bits / BITS_PER_UNIT)) * BITS_PER_UNIT
+ > total_bits)
+ {
+ realign = false;
+ if (STRICT_ALIGNMENT)
+ {
+ warn_misaligned_bitfield (bitsize == GET_MODE_BITSIZE (mode),
+ packedp);
+ if (packedp)
+ mode = VOIDmode;
+ }
+ }
+ }
else
mode = get_best_mode (bitsize, bitpos + offset * BITS_PER_UNIT,
bitregion_start, bitregion_end,
@@ -1000,7 +1062,8 @@ store_fixed_bit_field (rtx op0, unsigned
if (mode == VOIDmode)
{
/* The only way this should occur is if the field spans word
- boundaries. */
+ boundaries, or container boundaries with
+ -fstrict-volatile-bitfields. */
store_split_bit_field (op0, bitsize, bitpos + offset * BITS_PER_UNIT,
bitregion_start, bitregion_end, value);
return;
@@ -1018,12 +1081,15 @@ store_fixed_bit_field (rtx op0, unsigned
* BITS_PER_UNIT);
}
- /* Get ref to an aligned byte, halfword, or word containing the field.
- Adjust BITPOS to be position within a word,
- and OFFSET to be the offset of that word.
- Then alter OP0 to refer to that word. */
- bitpos += (offset % (total_bits / BITS_PER_UNIT)) * BITS_PER_UNIT;
- offset -= (offset % (total_bits / BITS_PER_UNIT));
+ if (realign)
+ {
+ /* Get ref to an aligned byte, halfword, or word containing the field.
+ Adjust BITPOS to be position within a word,
+ and OFFSET to be the offset of that word.
+ Then alter OP0 to refer to that word. */
+ bitpos += (offset % (total_bits / BITS_PER_UNIT)) * BITS_PER_UNIT;
+ offset -= (offset % (total_bits / BITS_PER_UNIT));
+ }
op0 = adjust_address (op0, mode, offset);
}
@@ -1250,7 +1316,8 @@ store_split_bit_field (rtx op0, unsigned
it is just an out-of-bounds access. Ignore it. */
if (word != const0_rtx)
store_fixed_bit_field (word, offset * unit / BITS_PER_UNIT, thissize,
- thispos, bitregion_start, bitregion_end, part);
+ thispos, bitregion_start, bitregion_end, part,
+ false);
bitsdone += thissize;
}
}
@@ -1857,42 +1924,13 @@ extract_fixed_bit_field (enum machine_mo
{
if (STRICT_ALIGNMENT)
{
- static bool informed_about_misalignment = false;
- bool warned;
-
+ warn_misaligned_bitfield (bitsize == total_bits, packedp);
if (packedp)
{
- if (bitsize == total_bits)
- warned = warning_at (input_location, OPT_fstrict_volatile_bitfields,
- "multiple accesses to volatile structure member"
- " because of packed attribute");
- else
- warned = warning_at (input_location, OPT_fstrict_volatile_bitfields,
- "multiple accesses to volatile structure bitfield"
- " because of packed attribute");
-
return extract_split_bit_field (op0, bitsize,
bitpos + offset * BITS_PER_UNIT,
unsignedp);
}
-
- if (bitsize == total_bits)
- warned = warning_at (input_location, OPT_fstrict_volatile_bitfields,
- "mis-aligned access used for structure member");
- else
- warned = warning_at (input_location, OPT_fstrict_volatile_bitfields,
- "mis-aligned access used for structure bitfield");
-
- if (! informed_about_misalignment && warned)
- {
- informed_about_misalignment = true;
- inform (input_location,
- "when a volatile object spans multiple type-sized locations,"
- " the compiler must choose between using a single mis-aligned access to"
- " preserve the volatility, or using multiple aligned accesses to avoid"
- " runtime faults; this code may fail at runtime if the hardware does"
- " not allow this access");
- }
}
}
else
Index: gcc/expr.c
===================================================================
--- gcc/expr.c (revision 190541)
+++ gcc/expr.c (working copy)
@@ -142,7 +142,7 @@ static void store_constructor (tree, rtx
static rtx store_field (rtx, HOST_WIDE_INT, HOST_WIDE_INT,
unsigned HOST_WIDE_INT, unsigned HOST_WIDE_INT,
enum machine_mode,
- tree, tree, alias_set_type, bool);
+ tree, tree, alias_set_type, bool, bool);
static unsigned HOST_WIDE_INT highest_pow2_factor_for_target (const_tree, const_tree);
@@ -2076,7 +2076,7 @@ emit_group_store (rtx orig_dst, rtx src,
emit_move_insn (adjust_address (dest, mode, bytepos), tmps[i]);
else
store_bit_field (dest, bytelen * BITS_PER_UNIT, bytepos * BITS_PER_UNIT,
- 0, 0, mode, tmps[i]);
+ 0, 0, false, mode, tmps[i]);
}
/* Copy from the pseudo into the (probable) hard reg. */
@@ -2170,7 +2170,8 @@ copy_blkmode_from_reg (rtx tgtblk, rtx s
/* Use xbitpos for the source extraction (right justified) and
bitpos for the destination store (left justified). */
- store_bit_field (dst, bitsize, bitpos % BITS_PER_WORD, 0, 0, copy_mode,
+ store_bit_field (dst, bitsize, bitpos % BITS_PER_WORD, 0, 0,
+ false, copy_mode,
extract_bit_field (src, bitsize,
xbitpos % BITS_PER_WORD, 1, false,
NULL_RTX, copy_mode, copy_mode));
@@ -2249,7 +2250,7 @@ copy_blkmode_to_reg (enum machine_mode m
/* Use bitpos for the source extraction (left justified) and
xbitpos for the destination store (right justified). */
store_bit_field (dst_word, bitsize, xbitpos % BITS_PER_WORD,
- 0, 0, word_mode,
+ 0, 0, false, word_mode,
extract_bit_field (src_word, bitsize,
bitpos % BITS_PER_WORD, 1, false,
NULL_RTX, word_mode, word_mode));
@@ -2933,7 +2934,8 @@ write_complex_part (rtx cplx, rtx val, b
gcc_assert (MEM_P (cplx) && ibitsize < BITS_PER_WORD);
}
- store_bit_field (cplx, ibitsize, imag_p ? ibitsize : 0, 0, 0, imode, val);
+ store_bit_field (cplx, ibitsize, imag_p ? ibitsize : 0, 0, 0,
+ false, imode, val);
}
/* Extract one of the components of the complex value CPLX. Extract the
@@ -4614,7 +4616,7 @@ expand_assignment (tree to, tree from, b
}
else
store_bit_field (mem, GET_MODE_BITSIZE (mode),
- 0, 0, 0, mode, reg);
+ 0, 0, 0, false, mode, reg);
return;
}
@@ -4759,14 +4761,14 @@ expand_assignment (tree to, tree from, b
result = store_field (XEXP (to_rtx, 0), bitsize, bitpos,
bitregion_start, bitregion_end,
mode1, from, TREE_TYPE (tem),
- get_alias_set (to), nontemporal);
+ get_alias_set (to), nontemporal, false);
else if (bitpos >= mode_bitsize / 2)
result = store_field (XEXP (to_rtx, 1), bitsize,
bitpos - mode_bitsize / 2,
bitregion_start, bitregion_end,
mode1, from,
TREE_TYPE (tem), get_alias_set (to),
- nontemporal);
+ nontemporal, false);
else if (bitpos == 0 && bitsize == mode_bitsize)
{
rtx from_rtx;
@@ -4788,7 +4790,7 @@ expand_assignment (tree to, tree from, b
bitregion_start, bitregion_end,
mode1, from,
TREE_TYPE (tem), get_alias_set (to),
- nontemporal);
+ nontemporal, false);
emit_move_insn (XEXP (to_rtx, 0), read_complex_part (temp, false));
emit_move_insn (XEXP (to_rtx, 1), read_complex_part (temp, true));
}
@@ -4817,11 +4819,21 @@ expand_assignment (tree to, tree from, b
to_rtx, to, from))
result = NULL;
else
- result = store_field (to_rtx, bitsize, bitpos,
- bitregion_start, bitregion_end,
- mode1, from,
- TREE_TYPE (tem), get_alias_set (to),
- nontemporal);
+ {
+ bool packedp = false;
+
+ if (TREE_CODE(to) == COMPONENT_REF
+ && (TYPE_PACKED (TREE_TYPE (TREE_OPERAND (to, 0)))
+ || (TREE_CODE (TREE_OPERAND (to, 1)) == FIELD_DECL
+ && DECL_PACKED (TREE_OPERAND (to, 1)))))
+ packedp = true;
+
+ result = store_field (to_rtx, bitsize, bitpos,
+ bitregion_start, bitregion_end,
+ mode1, from,
+ TREE_TYPE (tem), get_alias_set (to),
+ nontemporal, packedp);
+ }
}
if (misalignp)
@@ -5220,7 +5232,7 @@ store_expr (tree exp, rtx target, int ca
: BLOCK_OP_NORMAL));
else if (GET_MODE (target) == BLKmode)
store_bit_field (target, INTVAL (expr_size (exp)) * BITS_PER_UNIT,
- 0, 0, 0, GET_MODE (temp), temp);
+ 0, 0, 0, false, GET_MODE (temp), temp);
else
convert_move (target, temp, unsignedp);
}
@@ -5688,7 +5700,7 @@ store_constructor_field (rtx target, uns
}
else
store_field (target, bitsize, bitpos, 0, 0, mode, exp, type, alias_set,
- false);
+ false, false);
}
/* Store the value of constructor EXP into the rtx TARGET.
@@ -6275,14 +6287,16 @@ store_constructor (tree exp, rtx target,
(in general) be different from that for TARGET, since TARGET is a
reference to the containing structure.
- If NONTEMPORAL is true, try generating a nontemporal store. */
+ If NONTEMPORAL is true, try generating a nontemporal store.
+
+ PACKEDP is true if this field has the packed attribute. */
static rtx
store_field (rtx target, HOST_WIDE_INT bitsize, HOST_WIDE_INT bitpos,
unsigned HOST_WIDE_INT bitregion_start,
unsigned HOST_WIDE_INT bitregion_end,
enum machine_mode mode, tree exp, tree type,
- alias_set_type alias_set, bool nontemporal)
+ alias_set_type alias_set, bool nontemporal, bool packedp)
{
if (TREE_CODE (exp) == ERROR_MARK)
return const0_rtx;
@@ -6315,7 +6329,8 @@ store_field (rtx target, HOST_WIDE_INT b
store_field (blk_object, bitsize, bitpos,
bitregion_start, bitregion_end,
- mode, exp, type, MEM_ALIAS_SET (blk_object), nontemporal);
+ mode, exp, type, MEM_ALIAS_SET (blk_object), nontemporal,
+ false);
emit_move_insn (target, object);
@@ -6432,7 +6447,7 @@ store_field (rtx target, HOST_WIDE_INT b
/* Store the value in the bitfield. */
store_bit_field (target, bitsize, bitpos,
bitregion_start, bitregion_end,
- mode, temp);
+ packedp, mode, temp);
return const0_rtx;
}
@@ -8024,7 +8039,7 @@ expand_expr_real_2 (sepops ops, rtx targ
* BITS_PER_UNIT),
(HOST_WIDE_INT) GET_MODE_BITSIZE (mode)),
0, 0, 0, TYPE_MODE (valtype), treeop0,
- type, 0, false);
+ type, 0, false, false);
}
/* Return the entire union. */
Index: gcc/calls.c
===================================================================
--- gcc/calls.c (revision 190541)
+++ gcc/calls.c (working copy)
@@ -1051,7 +1051,7 @@ store_unaligned_arguments_into_pseudos (
bytes -= bitsize / BITS_PER_UNIT;
store_bit_field (reg, bitsize, endian_correction, 0, 0,
- word_mode, word);
+ false, word_mode, word);
}
}
}
Index: gcc/ifcvt.c
===================================================================
--- gcc/ifcvt.c (revision 190541)
+++ gcc/ifcvt.c (working copy)
@@ -896,7 +896,7 @@ noce_emit_move_insn (rtx x, rtx y)
}
gcc_assert (start < (MEM_P (op) ? BITS_PER_UNIT : BITS_PER_WORD));
- store_bit_field (op, size, start, 0, 0, GET_MODE (x), y);
+ store_bit_field (op, size, start, 0, 0, false, GET_MODE (x), y);
return;
}
@@ -951,7 +951,7 @@ noce_emit_move_insn (rtx x, rtx y)
outmode = GET_MODE (outer);
bitpos = SUBREG_BYTE (outer) * BITS_PER_UNIT;
store_bit_field (inner, GET_MODE_BITSIZE (outmode), bitpos,
- 0, 0, outmode, y);
+ 0, 0, false, outmode, y);
}
/* Return sequence of instructions generated by if conversion. This
Index: gcc/config/s390/s390.c
===================================================================
--- gcc/config/s390/s390.c (revision 190541)
+++ gcc/config/s390/s390.c (working copy)
@@ -4944,7 +4944,7 @@ s390_expand_atomic (enum machine_mode mo
case SET:
if (ac.aligned && MEM_P (val))
store_bit_field (new_rtx, GET_MODE_BITSIZE (mode), 0,
- 0, 0, SImode, val);
+ 0, 0, false, SImode, val);
else
{
new_rtx = expand_simple_binop (SImode, AND, new_rtx, ac.modemaski,
Index: gcc/testsuite/gcc.target/arm/volatile-bitfields-5.c
===================================================================
--- gcc/testsuite/gcc.target/arm/volatile-bitfields-5.c (revision 0)
+++ gcc/testsuite/gcc.target/arm/volatile-bitfields-5.c (revision 0)
@@ -0,0 +1,37 @@
+/* { dg-require-effective-target arm_eabi } */
+/* { dg-do run } */
+/* { dg-options "-O2 -fno-strict-aliasing" } */
+
+#include <stdlib.h>
+
+#pragma pack(1) // no space between structure members
+volatile typedef struct {
+ unsigned char byte;
+ /* Packed field members get converted to bitfields internally.
+ On most other these will either be split into smaller accesses,
+ or a single large aligned access. With -fstrict-volatile-bitfields
+ store_bitfield was assuming a single aligned user-specified-size access
+ covered the whole field. */
+ unsigned short halfword;
+} S;
+#pragma pack() // resume default structure packing
+
+void __attribute__((noinline)) foo(S *p)
+{
+ p->halfword++; /* { dg-message "note: When a volatile" } */
+ /* { dg-warning "mis-aligned access" "" { target *-*-* } 21 } */
+}
+
+int main()
+{
+/* Make sure the halfword is actually aligned in practice. */
+ short buf[3];
+ buf[0] = 0x5a5a;
+ buf[1] = 0x42ff;
+ buf[2] = 0x5a5a;
+
+ foo((S *)(((char *)buf) + 1));
+ if (buf[0] != 0x5a5a || buf[1] != 0x4300 || buf[2] != 0x5a5a)
+ abort();
+ return 0;
+}
