[Bug c/50521] New: -fstrict-volatile-bitfields is not strict

[Bug c/50521] New: -fstrict-volatile-bitfields is not strict

[kikairoya at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50521

             Bug #: 50521
           Summary: -fstrict-volatile-bitfields is not strict
    Classification: Unclassified
           Product: gcc
           Version: 4.6.1
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: kikairoya@gmail.com


gcc -fstrict-volatile-bitfields does not align volatile bitfield whose
bit-width=8, and generates byte-access code.


In this testcase:

// btf.c
volatile union {
    unsigned int all;
    struct {
        unsigned int a: 8;
        unsigned int : 7;
        unsigned int b: 1;
        unsigned int : 8;
        unsigned int c: 8;
    } bits;
} bitfield;

int main() {
    bitfield.bits.b = 1;
    bitfield.bits.c = 1;
    return 0;
}


command line:
$ gcc -S -O -o - btf.c -fstrict-volatile-bitfields

...compiles to below:
        .file   "btf.c"
        .text
        .globl  main
        .type   main, @function
main:
.LFB0:
        .cfi_startproc
        movl    bitfield(%rip), %eax
        orb     $128, %ah
        movl    %eax, bitfield(%rip) ; ok, aligned 32bit access
        movb    $1, bitfield+3(%rip) ; *** byte access ***
        movl    $0, %eax
        ret
        .cfi_endproc
.LFE0:
        .size   main, .-main
        .comm   bitfield,4,4
        .ident  "GCC: (Debian 4.6.1-3) 4.6.1"
        .section        .note.GNU-stack,"",@progbits


other arch:
$ rx-elf-gcc -S -O -o - btf.c -fstrict-volatile-bitfields
        .file   "btf.c"
        .section P,"ax"
        .global _main
        .type   _main, @function
_main:
        mov.L   #_bitfield, r14
        mov.L   [r14], r4
        bset    #15, r4
        mov.L   r4, [r14]  ; ok, aligned 32bit access
        mov.B   #1, 3[r14] ; *** byte access to misaligned address ***
        mov.L   #0, r1
        rts
        .size   _main, .-_main
        .comm   _bitfield,4,4
        .ident  "GCC: (GNU) 4.6.1"




Can make this access aligned?

[Bug c/50521] -fstrict-volatile-bitfields is not strict

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50521

--- Comment #1 from Richard Guenther <rguenth at gcc dot gnu.org> 2011-09-26 10:48:15 UTC ---
I don't think volatile union bitfield constitutes a "volatile bitfield" as
per what GCC implements.

[Bug c/50521] -fstrict-volatile-bitfields is not strict

[kikairoya at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50521

--- Comment #2 from Tomohiro Kashiwada <kikairoya at gmail dot com> 2011-09-26 11:42:37 UTC ---
Variable 'bitfield' declared as volatile, so all bitfield's members are
volatile.
Even if declare 'bits' as volatile, gcc dumps same code.
However, adding option -fno-strict-volatile-bitfields instead of
-fstrict-volatile-bitfields, gcc ignores type of bit-field.

[Bug c/50521] -fstrict-volatile-bitfields is not strict

[kikairoya at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50521

--- Comment #3 from Tomohiro Kashiwada <kikairoya at gmail dot com> 2011-09-27 04:15:19 UTC ---
Other test case (gcc generates wrong code):

volatile union {
    // add volatiles tenaciously, but...
    volatile unsigned int all;
    volatile struct {
        volatile unsigned int a: 8;
        volatile unsigned int : 7;
        volatile unsigned int b: 1;
        volatile unsigned int : 8;
        volatile unsigned int c: 8;
    } bits;
} bitfield;

int main() {
    bitfield.bits.a = 1;
    bitfield.bits.b = 1;
    bitfield.bits.c = 1;
    return 0;
}


$ gcc -S -O -o - btf.c -fstrict-volatile-bitfields
        .file   "btf.c"
        .text
        .globl  main
        .type   main, @function
main:
.LFB0:
        .cfi_startproc
        movl    $1, bitfield(%rip) ; **** breaks other area! ****
        movl    bitfield(%rip), %eax
        orb     $128, %ah
        movl    %eax, bitfield(%rip)
        movl    $1, bitfield+3(%rip) ; **** unaligned, and breaks other area!
****
        movl    $0, %eax
        ret
        .cfi_endproc
.LFE0:
        .size   main, .-main
        .comm   bitfield,4,4
        .ident  "GCC: (Debian 4.6.1-3) 4.6.1"
        .section        .note.GNU-stack,"",@progbits



$ rx-elf-gcc -S -O -o - btf.c -fstrict-volatile-bitfields
        .file   "btf.c"
        .section P,"ax"
        .global _main
        .type   _main, @function
_main:
        mov.L   #_bitfield, r14
        mov.L   #1, [r14]        ; **** breaks other area! ****
        mov.L   [r14], r4
        bset    #15, r4
        mov.L   r4, [r14]
        mov.L   [r14], r4
        and     #0xffffff, r4
        or      #0x1000000, r4
        mov.L   r4, [r14]        ; ok, aligned (my misunderstanding).
        mov.L   #0, r1
        rts
        .size   _main, .-_main
        .comm   _bitfield,4,4
        .ident  "GCC: (GNU) 4.6.1"

[Bug c/50521] -fstrict-volatile-bitfields is not strict

[kikairoya at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50521

--- Comment #4 from Tomohiro Kashiwada <kikairoya at gmail dot com> 2011-10-20 01:53:33 UTC ---
Created attachment 25559
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=25559
proposal patch

fix generating wrong code. no longer volatile-bitfield breaks other field.

[Bug c/50521] -fstrict-volatile-bitfields is not strict

[henrik at henriknordstrom dot net]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50521

Henrik Nordström <henrik at henriknordstrom dot net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |henrik at henriknordstrom
                   |                            |dot net

--- Comment #5 from Henrik Nordström <henrik at henriknordstrom dot net> 2011-10-27 23:23:13 UTC ---
Is this related to the strict volatile bitfields change in trunk revision
171347?
http://gcc.gnu.org/viewcvs/trunk/gcc/expr.c?view=log&pathrev=171347

It's quite similar changes but at slightly different locations.

[Bug c/50521] -fstrict-volatile-bitfields is not strict

[kikairoya at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50521

--- Comment #6 from Tomohiro Kashiwada <kikairoya at gmail dot com> 2011-10-28 01:32:42 UTC ---
(In reply to comment #5)
> Is this related to the strict volatile bitfields change in trunk revision
> 171347?
> http://gcc.gnu.org/viewcvs/trunk/gcc/expr.c?view=log&pathrev=171347

No, r171347 and r171346 generates same wrong code.

[Bug c/50521] -fstrict-volatile-bitfields is not strict

[henrik at henriknordstrom dot net]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50521

--- Comment #7 from Henrik Nordström <henrik at henriknordstrom dot net> 2011-10-28 01:59:34 UTC ---
Right.  r171347 seem to be about fetches from bitfields while this change is
about stores?

An interesting test would be 

  bitfield.bits.a = bitfield.bits.c

which should load the int to a register, load the int again to another
register, copy c to a between them and store the result.  I guess the double
load may be optimized away as it's an sideeffect of the aassignment.

[Bug c/50521] -fstrict-volatile-bitfields is not strict

[kikairoya at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50521

--- Comment #8 from Tomohiro Kashiwada <kikairoya at gmail dot com> 2011-10-28 03:58:45 UTC ---
(In reply to comment #7)
> Right.  r171347 seem to be about fetches from bitfields while this change is
> about stores?
> 
> An interesting test would be 
> 
>   bitfield.bits.a = bitfield.bits.c
> 
> which should load the int to a register, load the int again to another
> register, copy c to a between them and store the result.  I guess the double
> load may be optimized away as it's an sideeffect of the aassignment.

Both r171346 and r171347 (target=rx-elf, byte and bit little-endian) generates:
        mov.L   #_bitfield, r14
        mov.L   [r14], r4
        shlr    #24, r4
        mov.L   r4, [r14]

r171347 with my patch generates:
        mov.L   #_bitfield, r14
        mov.L   [r14], r3
        shlr    #24, r3
        mov.L   [r14], r4
        and     #0xffffffffffffff00, r4
        or      r3, r4
        mov.L   r4, [r14]
in this code, loads twice. I don't know the Standard C or C++ requires double
load or not. 

When -fno-strict-volatile-bitfields, generates
        mov.L   #_bitfield, r14
        mov.B   3[r14], r4
        mov.B   r4, [r14]
this code is seems ok but GCC ignores alignment (configuration of rx-elf has
STRICT_ALIGNMENT).

Another test,
  bitfield.bits.a = bitfield.bits.a;

r171346 and r171347 generates:
        mov.L   #_bitfield, r14
        mov.L   [r14], r4
        movu.B  r4, r4
        mov.L   r4, [r14]

r171347 with my patch:
        mov.L   #_bitfield, r14
        mov.L   [r14], r3
        movu.B  r3, r3
        mov.L   [r14], r4
        and     #0xffffffffffffff00, r4
        or      r3, r4
        mov.L   r4, [r14]

[Bug c/50521] -fstrict-volatile-bitfields is not strict

[henrik at henriknordstrom dot net]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50521

--- Comment #9 from Henrik Nordström <henrik at henriknordstrom dot net> 2011-10-28 07:32:48 UTC ---
C standard does not define any of this It's all implementation and platform ABI
dependent. 


The C standard does define not storage size of a bit-field other than that it's
sufficiently large, or bit-fields of other types than _Bool and int
(+qualifiers), or if bits outside the specific bit-field is accessed as side
effect when operating on a bit-field.


For example the ARM ABI defines volatile bitfield memory access in full detail
as being equal to the base type of the bitfield, and I see now that it actually
requires a double load in the mentioned test case. See
http://infocenter.arm.com/help/topic/com.arm.doc.ihi0042d/IHI0042D_aapcs.pdf
section 7.1.7.5.

[Bug c/50521] -fstrict-volatile-bitfields is not strict

[kikairoya at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50521

--- Comment #10 from Tomohiro Kashiwada <kikairoya at gmail dot com> 2011-10-28 08:13:31 UTC ---
(In reply to comment #9)
> http://infocenter.arm.com/help/topic/com.arm.doc.ihi0042d/IHI0042D_aapcs.pdf
> section 7.1.7.5.

Thanks, I see.
On ARM ABI, reading or writing to volatile-bitfields should not cause double
access and regardless of volatileness, access should be aligned as container's
type.

I think that to avoid double access needs treatment of volatile object.

[Bug c/50521] -fstrict-volatile-bitfields is not strict

[kikairoya at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50521

--- Comment #11 from Tomohiro Kashiwada <kikairoya at gmail dot com> 2011-10-28 08:15:39 UTC ---
Created attachment 25642
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=25642
patch to honor STRICT_ALIGNMENT

honor STRICT_ALIGNMENT when accessing non-volatile-bitfields

[Bug c/50521] -fstrict-volatile-bitfields is not strict

[henrik at henriknordstrom dot net]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50521

--- Comment #12 from Henrik Nordström <henrik at henriknordstrom dot net> 2011-10-28 17:46:15 UTC ---
Regarding the double load. In a statement like a = b, both a & be should be
individually accessed even if they refer to the same storage. So  
bitfield.bits.a = bitfield.bits.c should load the bitfield variable twice, once
for reading the rvalue and once for masking the lvalue assignment.

See 7.1.7.5 second and third paragraph and the note just after.


Regarding STRICT_ALIGNMENT, not strictly needed on ARM i think. Smaller
accesses than the base type is acceptable, as long as it's aligned to the
matching access size (8/16/32/64 bit) and on ARMv7 unaligned access is allowed,
but at a performance penalty. And this change is technically unrelated to
strict-volatile-bitfields even if there is overlap.

[Bug c/50521] -fstrict-volatile-bitfields is not strict

[kikairoya at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50521

--- Comment #13 from Tomohiro Kashiwada <kikairoya at gmail dot com> 2011-10-29 06:50:11 UTC ---
(In reply to comment #12)
> Regarding the double load. In a statement like a = b, both a & be should be
> individually accessed even if they refer to the same storage. So  
> bitfield.bits.a = bitfield.bits.c should load the bitfield variable twice, once
> for reading the rvalue and once for masking the lvalue assignment.
> 
> See 7.1.7.5 second and third paragraph and the note just after.

Is that means a statement
  a = b;
always should be treat as if
  tmp = b;
  a = tmp;
two individual statements?


> Regarding STRICT_ALIGNMENT, not strictly needed on ARM i think. Smaller
> accesses than the base type is acceptable, as long as it's aligned to the
> matching access size (8/16/32/64 bit) and on ARMv7 unaligned access is allowed,
> but at a performance penalty. And this change is technically unrelated to
> strict-volatile-bitfields even if there is overlap.

I think STRICT_ALIGNMENT is not only for ARM, but also MIPS, SH and others.
I'll create new ticket later about STRICT_ALIGNMENT.

[Bug c/50521] -fstrict-volatile-bitfields is not strict

[henrik at henriknordstrom dot net]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50521

--- Comment #14 from Henrik Nordström <henrik at henriknordstrom dot net> 2011-10-29 10:45:53 UTC ---
(In reply to comment #13)

> > See 7.1.7.5 second and third paragraph and the note just after.
> 
> Is that means a statement
>   a = b;
> always should be treat as if
>   tmp = b;
>   a = tmp;
> two individual statements?

That's my understanding of the text.

Further, given

struct {
   volatile int bits:32;
} a;
int result;

my understanding is that the note means that

  result = ++a.bits;

should be translated into

  int tmp = a.bits;
  a.bits = tmp + 1;
  result = a.bits;

and a++ into

  result = a.bits;
  tmp = a.bits;
  a.bits = tmp + 1;

suitable expanded for aligned container loads & stores on each access to
a.bits, with each access to the a container int handled as a volatile access.

Also, from in the second sentence of the note the load of the container on
write may not be optimized away even if it's entirely masked by the write
operation. I.e. 

a.bits = x;

translates into

  int tmp = *(int *)(int aligned address of a.bits);
  tmp &= ~0xFFFFFFFF;
  tmp |= x;
  *(int *)(int aligned address of a.bits) = tmp;

which is the same load & store memory access sequence as used when a.bits is
not filling the entire container.

  int tmp = *(int *)(int aligned address of a.bits);
  tmp &= ~a_bits_mask;
  tmp |= (x << shift) & ~a_bits_mask;
  *(int *)(int aligned address of a.bits) = tmp;

where it's not allowed to optimize away the initial load if the result of that
load is entirely masked away by the bit-field assignment (32 bit ~0xFFFFFFFF ==
0). Operations on tmp between the load & store of the a.bits container may be
optimized freely as tmp itself is not a volatile.

> I think STRICT_ALIGNMENT is not only for ARM, but also MIPS, SH and others.
> I'll create new ticket later about STRICT_ALIGNMENT.

agreed

none of this is only about ARM I think. But the ARM AAPCS specification is
suitable to use as reference for the implementation as it's very detailed on
how to operate on volatile bit-fields and also alignment requirements on
bit-field accesses in general. Not sure the others are as detailed, and it's
very likely the rules from the ARM specification can be applied there as well.

[Bug c/50521] -fstrict-volatile-bitfields is not strict

[kikairoya at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50521

--- Comment #15 from Tomohiro Kashiwada <kikairoya at gmail dot com> 2011-10-29 14:37:15 UTC ---
I see. Thanks detail exposition.
I think the behavior of my patch seems correct and it should be merged.