struct { unsigned int b : 1; } bf1; volatile struct { unsigned int b : 1; } bf2; void test(void) { bf1.b = 1; bf2.b = 1; } Access to bf1.b is correctly done as 32-bits (lwz/stw opcodes), bf2.b is accessed as 8-bits (lbz/stb opcodes). GCC3.4.3 shows the same behaviour, can't go back any further. The same happens when the bitfield itself is made volatile, not the whole struct.
(In reply to comment #0) > Access to bf1.b is correctly done as 32-bits (lwz/stw opcodes), bf2.b is > accessed as 8-bits (lbz/stb opcodes). GCC3.4.3 shows the same behaviour, can't > go back any further. The same happens when the bitfield itself is made volatile, > not the whole struct. This is intentional. The idea is not to touch any more of that volatile stuff than absolutely needed. Why do you think it is a bug?
(In reply to comment #1) > (In reply to comment #0) > > > Access to bf1.b is correctly done as 32-bits (lwz/stw opcodes), bf2.b is > > accessed as 8-bits (lbz/stb opcodes). GCC3.4.3 shows the same behaviour, can't > > go back any further. The same happens when the bitfield itself is made volatile, > > not the whole struct. > > This is intentional. The idea is not to touch any more of that volatile stuff > than absolutely needed. Why do you think it is a bug? When dealing with peripherals in embedded systems, the use of bitfields makes the code much more readable. Ex.: 3 bits of a peripheral register define a timer prescaler It can be s.th. like #define PS_VAL_05 0x00140000 #define PS_MASK 0x001c0000 ... per_reg = (per_reg & ~PS_MASK) | PS_VAL_05; But could be as simple as per_reg.ps = 5;, or even per_reg.ps = func(...); Try to set up the latter w/o bitfields and you end up per_reg = (per_reg & ~PS_MASK) | (func(...) << PS_SHIFT); Most (admittedly not all) modern peripherals allow the bitfield approach, but correct access size is a must (misalignment traps, access triggered buffering etc.). IMHO the compiler/code generator should always use the basic type when a variable is declared as volatile, i.e. volatile unsigned int ps:3; should enforce 32-bit-access, probably even for non-bitfields. All compilers for embedded systems I know of act this way, so I assumed this was a bug. In other cases, e.g. communicating between threads through memory, access size is not an issue and even if so, could be enforced by appropriate declaration or, god help me, typecasting as a last resort. Unfortunately, C does not provide a qualifier for access size enforcement, "volatile" seems to be the closest friend. The current implementation puts me between a rock and a hard place, as the "core volatile functionality" is needed as well. What do you think?
(In reply to comment #2) > (In reply to comment #1) > > (In reply to comment #0) > When dealing with peripherals in embedded systems, the use of bitfields makes > the code much more readable. I can see that. > Most (admittedly not all) modern peripherals allow the bitfield approach, but > correct access size is a must (misalignment traps, access triggered buffering > etc.). I can see that too, but I don't see what it has to do with this PR. > IMHO the compiler/code generator should always use the basic type when a > variable is declared as volatile, i.e. volatile unsigned int ps:3; should > enforce 32-bit-access, probably even for non-bitfields. OK, but why? You still haven't explained. What exactly is the problem with your code the way gcc does things currently? > All compilers for > embedded systems I know of act this way, so I assumed this was a bug. The only other compiler I have around (DEC C) behaves the same way as gcc (which is, to take the narrowest possible access that can be still done in one instruction). In any case, even if the current behavior isn't perfect, we would need an extremely and utterly good reason to change it, lest we screw over lots of users relying on the current behavior.
Note this also happens on ARM where (in the EABI) it is definitely a bug becuase the procedure call standard says it is. Quoting from the AAPCS: 7.1.7.5 Volatile bit-fieldspreserving number and width of container accesses When a volatile bit-field is read, its container must be read exactly once using the access width appropriate to the type of the container. When a volatile bit-field is written, its container must be read exactly once and written exactly once using the access width appropriate to the type of the container. The two accesses are not atomic.
(In reply to comment #4) > Note this also happens on ARM where (in the EABI) it is definitely a bug I will try and dig up the EABI for PowerPC, but it's not just about sticking to a paper. It simply does not work for me (and probably others) the way it is. My system traps out on me or, worce, writes garbage to the 'untouched' register parts for some peripherals. NEC V850 and, I think, MIPS do the same. I can't quite see what can be gained by minimizing access size, but not knowing the complete rationale (why are non-bitfields NOT minimized, e.g. volatile int x |= 1), how about a command line switch to let the user select?
(In reply to comment #5) > I will try and dig up the EABI for PowerPC, but it's not just about sticking to > a paper. It simply does not work for me (and probably others) the way it is. Saying that because it doesn't work for you is not really addressing the issue. The point of specifications like this are so that they create a level playing field for all to start from (and thus avoid 'arguments' as to which one is correct). Ultimately, if your code doesn't work with the prescribed model, then you have to find another way of expressing the problem. Note, I'm not trying to judge here whether your expectations of the PPC ABI are right or wrong, I'm trying to set the rational for having ABI specifications and then trying to stick to them. > I can't quite see what can be gained by minimizing access size, but not knowing > the complete rationale (why are non-bitfields NOT minimized, e.g. volatile int x > |= 1), how about a command line switch to let the user select? I suspect it's historical. On a machine that can do arbitrary alignment (eg, traditional CISC processors) then a common layout rule for a bit-field is to place it as close as possible to the previous field such that it all fits in one 'access' (but probably multiple memory fetches -- afterall, the memory bus is still aligned). If that access happens to be volatile, then you still don't want the memory system to be accessed twice (even transparently) and you can normally guarantee that by 'narrowing' the object. For RISC processors the alignment tends to be more critical in the normal case, so bit-fields are generally laid out with more padding. In this situation it then makes more sense to say that the bit-field's type governs the nature of the memory access.
(In reply to comment #6) > (In reply to comment #5) > > I will try and dig up the EABI for PowerPC > > > Note this also happens on ARM where (in the EABI) it is definitely a bug None of the documents I found makes a statement as clear as the ARM EABI does. Can someone point me to the source file where the "narrowing" is done, to maybe simply "comment it out"?; I don't feel capable of providing a real fix, see below, but I also don't want to rewrite large portions of my code when migrating to GCC. Having given more thought to the problem, isn't the "narrowing" an optimization which should not be a fixed property of the compiler, given that even for the same type of machine ("target") there can exist various different memory interfaces? Hence, shouldn't it be an optimization option, "on" by default to be backwards compatible?
Subject: Bug 23623 Author: pbrook Date: Tue Mar 28 18:03:06 2006 New Revision: 112460 URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=112460 Log: 2006-03-28 Paul Brook <paul@codesourcery.com> PR middle-end/23623 * gcc/targhooks.c (default_narrow_bitfield): New fuction. * gcc/targhooks.h (default_narrow_bitfield): add prototype. * gcc/target-def.h (TARGET_NARROW_VOLATILE_BITFIELD): Define. * gcc/doc/tm.texi: Document TARGET_NARROW_VOLATILE_BITFIELDS. * gcc/config/arm/arm.c (TARGET_NARROW_VOLATILE_BITFIELD): Define. Modified: branches/csl/sourcerygxx-4_1/ChangeLog.csl branches/csl/sourcerygxx-4_1/gcc/config/arm/arm.c branches/csl/sourcerygxx-4_1/gcc/doc/tm.texi branches/csl/sourcerygxx-4_1/gcc/stor-layout.c branches/csl/sourcerygxx-4_1/gcc/target-def.h branches/csl/sourcerygxx-4_1/gcc/target.h branches/csl/sourcerygxx-4_1/gcc/targhooks.c branches/csl/sourcerygxx-4_1/gcc/targhooks.h
Subject: Bug 23623 Author: pbrook Date: Wed Mar 29 15:21:13 2006 New Revision: 112493 URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=112493 Log: 2006-03-29 Paul Brook <paul@codesourcery.com> PR middle-end/23623 * targhooks.c (default_narrow_bitfield): New fuction. * targhooks.h (default_narrow_bitfield): add prototype. * target.h (gcc_target): Add narrow_volatile_bitfield. * target-def.h (TARGET_NARROW_VOLATILE_BITFIELD): Define. * stor-layout.c (get_best_mode): Use targetm.narrow_volatile_bitfield. * doc/tm.texi: Document TARGET_NARROW_VOLATILE_BITFIELDS. * config/arm/arm.c (TARGET_NARROW_VOLATILE_BITFIELD): Define. Modified: trunk/gcc/ChangeLog trunk/gcc/config/arm/arm.c trunk/gcc/doc/tm.texi trunk/gcc/stor-layout.c trunk/gcc/target-def.h trunk/gcc/target.h trunk/gcc/targhooks.c trunk/gcc/targhooks.h
Should be fixed now.
*** Bug 28568 has been marked as a duplicate of this bug. ***
Created attachment 17085 [details] patch_for_volatile_bitfields_gcc400.tgz The following patches help to honour the container type of the volatile bitfield when the GCC internal: TARGET_NARROW_VOLATILE_BITFIELD is false. The patch is for GCC 4.0.0 Included in the tgz-file is the disassembly for the application: main.c, with the original GCC 4.0.0 source and the patched source.
Please note: The patch was for GCC 4.4.0, not GCC 4.0.0 as mentioned in the previous post.
Created attachment 17087 [details] GCC 4.3.2 patch for volatile bitfields The following patches help to honour the container type of the volatile bitfield when the GCC internal: TARGET_NARROW_VOLATILE_BITFIELD is false. The patch is for GCC 4.3.2 Included in the tgz-file is the whole project, GCC patches and disassembly for the application when using the original GCC 4.3.2 source and the patched source.
This bug regressed sometime around GCC 4.7, when the C++ bitfield range support was added. I'm working on a fix that makes it work again in conjunction with -fstrict-volatile-bitfields.
Patch that fixes regression posted here: http://gcc.gnu.org/ml/gcc-patches/2013-06/msg00750.html
Updated patch series: http://gcc.gnu.org/ml/gcc-patches/2013-09/msg02057.html http://gcc.gnu.org/ml/gcc-patches/2013-09/msg02058.html http://gcc.gnu.org/ml/gcc-patches/2013-09/msg02059.html Unfortunately, it seems that fixing bugs with -fstrict-volatile-bitfields has been blocked by disagreement between global reviewers and target maintainers who can't agree on whether the C/C++11 memory model should take precedence over target-specific ABIs by default. :-(
Author: edlinger Date: Wed Dec 11 16:50:05 2013 New Revision: 205896 URL: http://gcc.gnu.org/viewcvs?rev=205896&root=gcc&view=rev Log: 2013-12-11 Sandra Loosemore <sandra@codesourcery.com> PR middle-end/23623 PR middle-end/48784 PR middle-end/56341 PR middle-end/56997 gcc/ * expmed.c (strict_volatile_bitfield_p): New function. (store_bit_field_1): Don't special-case strict volatile bitfields here. (store_bit_field): Handle strict volatile bitfields here instead. (store_fixed_bit_field): Don't special-case strict volatile bitfields here. (extract_bit_field_1): Don't special-case strict volatile bitfields here. (extract_bit_field): Handle strict volatile bitfields here instead. (extract_fixed_bit_field): Don't special-case strict volatile bitfields here. Simplify surrounding code to resemble that in store_fixed_bit_field. * doc/invoke.texi (Code Gen Options): Update -fstrict-volatile-bitfields description. gcc/testsuite/ * gcc.dg/pr23623.c: New test. * gcc.dg/pr48784-1.c: New test. * gcc.dg/pr48784-2.c: New test. * gcc.dg/pr56341-1.c: New test. * gcc.dg/pr56341-2.c: New test. * gcc.dg/pr56997-1.c: New test. * gcc.dg/pr56997-2.c: New test. * gcc.dg/pr56997-3.c: New test. Added: trunk/gcc/testsuite/gcc.dg/pr23623.c trunk/gcc/testsuite/gcc.dg/pr48784-1.c trunk/gcc/testsuite/gcc.dg/pr48784-2.c trunk/gcc/testsuite/gcc.dg/pr56341-1.c trunk/gcc/testsuite/gcc.dg/pr56341-2.c trunk/gcc/testsuite/gcc.dg/pr56997-1.c trunk/gcc/testsuite/gcc.dg/pr56997-2.c trunk/gcc/testsuite/gcc.dg/pr56997-3.c Modified: trunk/gcc/ChangeLog trunk/gcc/doc/invoke.texi trunk/gcc/expmed.c trunk/gcc/testsuite/ChangeLog
Author: edlinger Date: Wed Dec 11 16:59:24 2013 New Revision: 205897 URL: http://gcc.gnu.org/viewcvs?rev=205897&root=gcc&view=rev Log: 2013-12-11 Bernd Edlinger <bernd.edlinger@hotmail.de> Sandra Loosemore <sandra@codesourcery.com> PR middle-end/23623 PR middle-end/48784 PR middle-end/56341 PR middle-end/56997 * expmed.c (strict_volatile_bitfield_p): Add bitregion_start and bitregion_end parameters. Test for compliance with C++ memory model. (store_bit_field): Adjust call to strict_volatile_bitfield_p. Add fallback logic for cases where -fstrict-volatile-bitfields is supposed to apply, but cannot. (extract_bit_field): Likewise. Use narrow_bit_field_mem and extract_fixed_bit_field_1 to do the extraction. (extract_fixed_bit_field): Revert to previous mode selection algorithm. Call extract_fixed_bit_field_1 to do the real work. (extract_fixed_bit_field_1): New function. testsuite: * gcc.dg/pr23623.c: Update to test interaction with C++ memory model. Modified: trunk/gcc/ChangeLog trunk/gcc/expmed.c trunk/gcc/testsuite/ChangeLog trunk/gcc/testsuite/gcc.dg/pr23623.c
Author: jye2 Date: Thu Feb 27 07:28:06 2014 New Revision: 208195 URL: http://gcc.gnu.org/viewcvs?rev=208195&root=gcc&view=rev Log: 2014-02-27 Joey Ye <joey.ye@arm.com> Backport mainline strict-volatile-bitfields fixes 2013-09-28 Sandra Loosemore <sandra@codesourcery.com> gcc/ * expr.h (extract_bit_field): Remove packedp parameter. * expmed.c (extract_fixed_bit_field): Remove packedp parameter from forward declaration. (store_split_bit_field): Remove packedp arg from calls to extract_fixed_bit_field. (extract_bit_field_1): Remove packedp parameter and packedp argument from recursive calls and calls to extract_fixed_bit_field. (extract_bit_field): Remove packedp parameter and corresponding arg to extract_bit_field_1. (extract_fixed_bit_field): Remove packedp parameter. Remove code to issue warnings. (extract_split_bit_field): Remove packedp arg from call to extract_fixed_bit_field. * expr.c (emit_group_load_1): Adjust calls to extract_bit_field. (copy_blkmode_from_reg): Likewise. (copy_blkmode_to_reg): Likewise. (read_complex_part): Likewise. (store_field): Likewise. (expand_expr_real_1): Likewise. * calls.c (store_unaligned_arguments_into_pseudos): Adjust call to extract_bit_field. * config/tilegx/tilegx.c (tilegx_expand_unaligned_load): Adjust call to extract_bit_field. * config/tilepro/tilepro.c (tilepro_expand_unaligned_load): Adjust call to extract_bit_field. * doc/invoke.texi (Code Gen Options): Remove mention of warnings and special packedp behavior from -fstrict-volatile-bitfields documentation. 2013-12-11 Bernd Edlinger <bernd.edlinger@hotmail.de> * expr.c (expand_assignment): Remove dependency on flag_strict_volatile_bitfields. Always set the memory access mode. (expand_expr_real_1): Likewise. 2013-12-11 Sandra Loosemore <sandra@codesourcery.com> PR middle-end/23623 PR middle-end/48784 PR middle-end/56341 PR middle-end/56997 gcc/ * expmed.c (strict_volatile_bitfield_p): New function. (store_bit_field_1): Don't special-case strict volatile bitfields here. (store_bit_field): Handle strict volatile bitfields here instead. (store_fixed_bit_field): Don't special-case strict volatile bitfields here. (extract_bit_field_1): Don't special-case strict volatile bitfields here. (extract_bit_field): Handle strict volatile bitfields here instead. (extract_fixed_bit_field): Don't special-case strict volatile bitfields here. Simplify surrounding code to resemble that in store_fixed_bit_field. * doc/invoke.texi (Code Gen Options): Update -fstrict-volatile-bitfields description. gcc/testsuite/ * gcc.dg/pr23623.c: New test. * gcc.dg/pr48784-1.c: New test. * gcc.dg/pr48784-2.c: New test. * gcc.dg/pr56341-1.c: New test. * gcc.dg/pr56341-2.c: New test. * gcc.dg/pr56997-1.c: New test. * gcc.dg/pr56997-2.c: New test. * gcc.dg/pr56997-3.c: New test. 2013-12-11 Bernd Edlinger <bernd.edlinger@hotmail.de> Sandra Loosemore <sandra@codesourcery.com> PR middle-end/23623 PR middle-end/48784 PR middle-end/56341 PR middle-end/56997 * expmed.c (strict_volatile_bitfield_p): Add bitregion_start and bitregion_end parameters. Test for compliance with C++ memory model. (store_bit_field): Adjust call to strict_volatile_bitfield_p. Add fallback logic for cases where -fstrict-volatile-bitfields is supposed to apply, but cannot. (extract_bit_field): Likewise. Use narrow_bit_field_mem and extract_fixed_bit_field_1 to do the extraction. (extract_fixed_bit_field): Revert to previous mode selection algorithm. Call extract_fixed_bit_field_1 to do the real work. (extract_fixed_bit_field_1): New function. testsuite: * gcc.dg/pr23623.c: Update to test interaction with C++ memory model. 2013-12-11 Bernd Edlinger <bernd.edlinger@hotmail.de> PR middle-end/59134 * expmed.c (store_bit_field): Use narrow_bit_field_mem and store_fixed_bit_field_1 for -fstrict-volatile-bitfields. (store_fixed_bit_field): Split up. Call store_fixed_bit_field_1 to do the real work. (store_fixed_bit_field_1): New function. (store_split_bit_field): Limit the unit size to the memory mode size, to prevent recursion. testsuite: * gcc.c-torture/compile/pr59134.c: New test. * gnat.dg/misaligned_volatile.adb: New test. Added: branches/ARM/embedded-4_8-branch/gcc/testsuite/gcc.c-torture/compile/pr59134.c branches/ARM/embedded-4_8-branch/gcc/testsuite/gcc.dg/pr23623.c branches/ARM/embedded-4_8-branch/gcc/testsuite/gcc.dg/pr48784-1.c branches/ARM/embedded-4_8-branch/gcc/testsuite/gcc.dg/pr48784-2.c branches/ARM/embedded-4_8-branch/gcc/testsuite/gcc.dg/pr56341-1.c branches/ARM/embedded-4_8-branch/gcc/testsuite/gcc.dg/pr56341-2.c branches/ARM/embedded-4_8-branch/gcc/testsuite/gcc.dg/pr56997-1.c branches/ARM/embedded-4_8-branch/gcc/testsuite/gcc.dg/pr56997-2.c branches/ARM/embedded-4_8-branch/gcc/testsuite/gcc.dg/pr56997-3.c branches/ARM/embedded-4_8-branch/gcc/testsuite/gnat.dg/misaligned_volatile.adb Modified: branches/ARM/embedded-4_8-branch/gcc/ChangeLog.arm branches/ARM/embedded-4_8-branch/gcc/calls.c branches/ARM/embedded-4_8-branch/gcc/config/tilegx/tilegx.c branches/ARM/embedded-4_8-branch/gcc/config/tilepro/tilepro.c branches/ARM/embedded-4_8-branch/gcc/doc/invoke.texi branches/ARM/embedded-4_8-branch/gcc/expmed.c branches/ARM/embedded-4_8-branch/gcc/expr.c branches/ARM/embedded-4_8-branch/gcc/expr.h branches/ARM/embedded-4_8-branch/gcc/testsuite/ChangeLog.arm