On Tue, Aug 21, 2012 at 10:20 PM, Sandra Loosemore <san...@codesourcery.com> wrote: > 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?
+ 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; note that this is not a reliable way of determining if a field is packed (yes, the existing code is broken in the same way). I realize that you are only using this for diagnostic purposes, still something worth mentioning. I dislike passing around the packedp flag throughout the expander and to warn inside the expander. First, the flag has a name that can be confusing (it does not conservatively flag all packed accesses). Second, why don't you warn for this from inside the frontend when the bitfield access is generated? This way the diagnostic won't depend on optimization levels and you avoid uglifying the expander. Thanks, Richard. > -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. >
