User account creation filtered due to spam.

Bug 23623 - volatile keyword changes bitfield access size from 32bit to 8bit
Summary: volatile keyword changes bitfield access size from 32bit to 8bit
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: middle-end (show other bugs)
Version: 4.0.1
: P2 normal
Target Milestone: 4.2.0
Assignee: Not yet assigned to anyone
URL:
Keywords:
: 28568 (view as bug list)
Depends on:
Blocks:
 
Reported: 2005-08-29 14:02 UTC by Martin Reszat
Modified: 2014-02-27 07:28 UTC (History)
5 users (show)

See Also:
Host: i686-pc-cygwin
Target: powerpc-elf-eabispe
Build:
Known to work:
Known to fail: 3.1 3.3 4.1.0 4.0.0
Last reconfirmed: 2005-08-30 12:43:25


Attachments
patch_for_volatile_bitfields_gcc400.tgz (3.90 KB, application/octet-stream)
2009-01-13 10:12 UTC, Frikkie Thirion
Details
GCC 4.3.2 patch for volatile bitfields (6.79 KB, application/octet-stream)
2009-01-13 15:51 UTC, Frikkie Thirion
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Reszat 2005-08-29 14:02:27 UTC
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.
Comment 1 Falk Hueffner 2005-08-29 21:10:59 UTC
(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?
Comment 2 Martin Reszat 2005-08-30 07:43:03 UTC
(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?
Comment 3 Falk Hueffner 2005-08-30 12:26:26 UTC
(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.
Comment 4 Richard Earnshaw 2005-08-30 12:43:22 UTC
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&#63719;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.
Comment 5 Martin Reszat 2005-08-30 13:40:15 UTC
(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?
Comment 6 Richard Earnshaw 2005-08-30 14:00:54 UTC
(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.
Comment 7 Martin Reszat 2005-09-08 15:12:49 UTC
(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?
Comment 8 Paul Brook 2006-03-28 18:03:10 UTC
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

Comment 9 Paul Brook 2006-03-29 15:21:46 UTC
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

Comment 10 Paul Brook 2006-03-29 17:44:55 UTC
Should be fixed now.
Comment 11 Jason Morgan 2006-08-02 11:30:13 UTC
*** Bug 28568 has been marked as a duplicate of this bug. ***
Comment 12 Frikkie Thirion 2009-01-13 10:12:29 UTC
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.
Comment 13 Frikkie Thirion 2009-01-13 10:42:04 UTC
Please note: The patch was for GCC 4.4.0, not GCC 4.0.0 as mentioned in the previous post.
Comment 14 Frikkie Thirion 2009-01-13 15:51:52 UTC
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.
Comment 15 Sandra Loosemore 2013-06-08 19:57:35 UTC
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.
Comment 16 Sandra Loosemore 2013-06-14 02:59:00 UTC
Patch that fixes regression posted here:

http://gcc.gnu.org/ml/gcc-patches/2013-06/msg00750.html
Comment 17 Sandra Loosemore 2013-10-07 15:40:06 UTC
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.  :-(
Comment 18 Bernd Edlinger 2013-12-11 16:50:07 UTC
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
Comment 19 Bernd Edlinger 2013-12-11 16:59:26 UTC
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
Comment 20 jye2 2014-02-27 07:28:39 UTC
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