[PATCH] Fix C++0x memory model for -fno-strict-volatile-bitfields on ARM

[PATCH] Fix C++0x memory model for -fno-strict-volatile-bitfields on ARM

[Bernd Edlinger]

[-- Attachment #1: Type: text/plain, Size: 845 bytes --]

Hello,

this patch fixes the recently discovered data store race on arm-eabi-gcc with -fno-strict-volatile-bitfields
for structures like this:

#define test_type unsigned short

typedef struct s{
 unsigned char Prefix[1];
 test_type Type;
}__attribute((__packed__,__aligned__(4))) ss;

volatile ss v;

void __attribute__((noinline))
foo (test_type u)
{
  v.Type = u;
}

test_type __attribute__((noinline))
bar (void)
{
  return v.Type;
}


I've manually confirmed the correct code generation using variations of the
example above on an ARM cross-compiler for -fno-strict-volatile-bitfields.

Note, that this example is still causes ICE's for -fstrict-volatile-bitfields,
but I'd like to fix that separately.

Boot-strapped and regression-tested on x86_64-linux-gnu.

Ok for trunk?

Thanks
Bernd. 		 	   		  

[-- Attachment #2: changelog-unaligned-data.txt --]
[-- Type: text/plain, Size: 296 bytes --]

2013-10-25  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	Fix C++0x memory model for unaligned fields in packed, aligned(4)
	structures with -fno-strict-volatile-bitfields on STRICT_ALIGNMENT
	targets like arm-none-eabi.
	* expmed.c (store_bit_field): Handle unaligned fields like
	bit regions.


[-- Attachment #3: patch-unaligned-data.diff --]
[-- Type: application/octet-stream, Size: 929 bytes --]

Index: gcc/expmed.c
===================================================================
--- gcc/expmed.c	(revision 203898)
+++ gcc/expmed.c	(working copy)
@@ -871,6 +871,18 @@ store_bit_field (rtx str_rtx, unsigned HOST_WIDE_I
 		 enum machine_mode fieldmode,
 		 rtx value)
 {
+  /* Treat unaligned fields like bit regions otherwise we might
+     touch bits outside the field.  */
+  if (MEM_P (str_rtx) && bitregion_start == 0 && bitregion_end == 0
+      && bitsize > 0 && bitsize % BITS_PER_UNIT == 0
+      && bitnum % BITS_PER_UNIT == 0
+      && (bitsize % MIN (MEM_ALIGN (str_rtx), BITS_PER_WORD) > 0
+	|| bitnum % MIN (MEM_ALIGN (str_rtx), BITS_PER_WORD) > 0))
+    {
+      bitregion_start = bitnum;
+      bitregion_end = bitnum + bitsize - 1;
+    }
+
   /* Under the C++0x memory model, we must not touch bits outside the
      bit region.  Adjust the address to start at the beginning of the
      bit region.  */

Re: [PATCH] Fix C++0x memory model for -fno-strict-volatile-bitfields on ARM

[Richard Biener]

On Fri, Oct 25, 2013 at 10:40 AM, Bernd Edlinger
<bernd.edlinger@hotmail.de> wrote:
> Hello,
>
> this patch fixes the recently discovered data store race on arm-eabi-gcc with -fno-strict-volatile-bitfields
> for structures like this:
>
> #define test_type unsigned short
>
> typedef struct s{
>  unsigned char Prefix[1];
>  test_type Type;
> }__attribute((__packed__,__aligned__(4))) ss;
>
> volatile ss v;
>
> void __attribute__((noinline))
> foo (test_type u)
> {
>   v.Type = u;
> }
>
> test_type __attribute__((noinline))
> bar (void)
> {
>   return v.Type;
> }
>
>
> I've manually confirmed the correct code generation using variations of the
> example above on an ARM cross-compiler for -fno-strict-volatile-bitfields.
>
> Note, that this example is still causes ICE's for -fstrict-volatile-bitfields,
> but I'd like to fix that separately.
>
> Boot-strapped and regression-tested on x86_64-linux-gnu.
>
> Ok for trunk?

Isn't it more appropriate to fix it here:

      if (TREE_CODE (to) == COMPONENT_REF
          && DECL_BIT_FIELD_TYPE (TREE_OPERAND (to, 1)))
        get_bit_range (&bitregion_start, &bitregion_end, to, &bitpos, &offset);

?

Btw, the C++ standard doesn't cover packed or aligned attributes so
we could declare this a non-issue.  Any opinion on that?

Thanks,
Richard.

> Thanks
> Bernd.

RE: [PATCH] Fix C++0x memory model for -fno-strict-volatile-bitfields on ARM

[Bernd Edlinger]

Hi,

On Fri, 25 Oct 2013 11:26:20, Richard Biener wrote:
>
> On Fri, Oct 25, 2013 at 10:40 AM, Bernd Edlinger
> <bernd.edlinger@hotmail.de> wrote:
>> Hello,
>>
>> this patch fixes the recently discovered data store race on arm-eabi-gcc with -fno-strict-volatile-bitfields
>> for structures like this:
>>
>> #define test_type unsigned short
>>
>> typedef struct s{
>> unsigned char Prefix[1];
>> test_type Type;
>> }__attribute((__packed__,__aligned__(4))) ss;
>>
>> volatile ss v;
>>
>> void __attribute__((noinline))
>> foo (test_type u)
>> {
>> v.Type = u;
>> }
>>
>> test_type __attribute__((noinline))
>> bar (void)
>> {
>> return v.Type;
>> }
>>
>>
>> I've manually confirmed the correct code generation using variations of the
>> example above on an ARM cross-compiler for -fno-strict-volatile-bitfields.
>>
>> Note, that this example is still causes ICE's for -fstrict-volatile-bitfields,
>> but I'd like to fix that separately.
>>
>> Boot-strapped and regression-tested on x86_64-linux-gnu.
>>
>> Ok for trunk?
>
> Isn't it more appropriate to fix it here:
>
> if (TREE_CODE (to) == COMPONENT_REF
> && DECL_BIT_FIELD_TYPE (TREE_OPERAND (to, 1)))
> get_bit_range (&bitregion_start, &bitregion_end, to, &bitpos, &offset);
>
> ?
>

Honestly, I'd call this is a work-around, not a design.

Therefore I would not move that workaround to expr.c.

Also the block below is only a work-around IMHO.

  if (MEM_P (str_rtx) && bitregion_start> 0)
    {
      enum machine_mode bestmode;
      HOST_WIDE_INT offset, size;

      gcc_assert ((bitregion_start % BITS_PER_UNIT) == 0);

      offset = bitregion_start / BITS_PER_UNIT;
      bitnum -= bitregion_start;
      size = (bitnum + bitsize + BITS_PER_UNIT - 1) / BITS_PER_UNIT;
      bitregion_end -= bitregion_start;
      bitregion_start = 0;
      bestmode = get_best_mode (bitsize, bitnum,
                                bitregion_start, bitregion_end,
                                MEM_ALIGN (str_rtx), VOIDmode,
                                MEM_VOLATILE_P (str_rtx));
      str_rtx = adjust_bitfield_address_size (str_rtx, bestmode, offset, size);
    }

Here, if bitregion_start = 8, we have a 4 byte aligned memory context,
and whoops, now it is only 1 byte aligned.

this example:

struct s
{
  char a;
  int b:24;
};

struct s ss;

void foo(int b)
{
  ss.b = b;
}


gets compiled (at -O3) to:

foo:
    @ Function supports interworking.
    @ args = 0, pretend = 0, frame = 0
    @ frame_needed = 0, uses_anonymous_args = 0
    @ link register save eliminated.
    ldr    r3, .L2
    mov    r1, r0, lsr #8
    mov    r2, r0, lsr #16
    strb    r1, [r3, #2]
    strb    r0, [r3, #1]
    strb    r2, [r3, #3]
    bx    lr

while...

struct s
{
  char a;
  int b:24;
};

struct s ss;

void foo(int b)
{
  ss.b = b;
}


gets compiled (at -O3) to

foo:
    @ Function supports interworking.
    @ args = 0, pretend = 0, frame = 0
    @ frame_needed = 0, uses_anonymous_args = 0
    @ link register save eliminated.
    ldr    r3, .L2
    mov    r2, r0, lsr #16
    strb    r2, [r3, #2]
    strh    r0, [r3]    @ movhi
    bx    lr

which is more efficient, but only because the memory context is still
aligned in this case.

> Btw, the C++ standard doesn't cover packed or aligned attributes so
> we could declare this a non-issue. Any opinion on that?
>
> Thanks,
> Richard.
>
>> Thanks
>> Bernd. 		 	   		  

Re: [PATCH] Fix C++0x memory model for -fno-strict-volatile-bitfields on ARM

[Joseph S. Myers]

On Fri, 25 Oct 2013, Richard Biener wrote:

> Btw, the C++ standard doesn't cover packed or aligned attributes so
> we could declare this a non-issue.  Any opinion on that?

I think the memory model naturally applies to packed structures (i.e., 
writes to fields in them should not write to any other fields except as 
part of a sequence of consecutive non-zero-width bit-fields, unless 
allow-store-data-races is set).

-- 
Joseph S. Myers
joseph@codesourcery.com

RE: [PATCH] Fix C++0x memory model for -fno-strict-volatile-bitfields on ARM

[Bernd Edlinger]

[-- Attachment #1: Type: text/plain, Size: 3884 bytes --]

Hello,

meanwhile, I have added a test case to that patch.

Boot-strapped and regression-tested as usual.

OK for trunk?

Bernd.

> Hi,
>
> On Fri, 25 Oct 2013 11:26:20, Richard Biener wrote:
>>
>> On Fri, Oct 25, 2013 at 10:40 AM, Bernd Edlinger
>> <bernd.edlinger@hotmail.de> wrote:
>>> Hello,
>>>
>>> this patch fixes the recently discovered data store race on arm-eabi-gcc with -fno-strict-volatile-bitfields
>>> for structures like this:
>>>
>>> #define test_type unsigned short
>>>
>>> typedef struct s{
>>> unsigned char Prefix[1];
>>> test_type Type;
>>> }__attribute((__packed__,__aligned__(4))) ss;
>>>
>>> volatile ss v;
>>>
>>> void __attribute__((noinline))
>>> foo (test_type u)
>>> {
>>> v.Type = u;
>>> }
>>>
>>> test_type __attribute__((noinline))
>>> bar (void)
>>> {
>>> return v.Type;
>>> }
>>>
>>>
>>> I've manually confirmed the correct code generation using variations of the
>>> example above on an ARM cross-compiler for -fno-strict-volatile-bitfields.
>>>
>>> Note, that this example is still causes ICE's for -fstrict-volatile-bitfields,
>>> but I'd like to fix that separately.
>>>
>>> Boot-strapped and regression-tested on x86_64-linux-gnu.
>>>
>>> Ok for trunk?
>>
>> Isn't it more appropriate to fix it here:
>>
>> if (TREE_CODE (to) == COMPONENT_REF
>> && DECL_BIT_FIELD_TYPE (TREE_OPERAND (to, 1)))
>> get_bit_range (&bitregion_start, &bitregion_end, to, &bitpos, &offset);
>>
>> ?
>>
>
> Honestly, I'd call this is a work-around, not a design.
>
> Therefore I would not move that workaround to expr.c.
>
> Also the block below is only a work-around IMHO.
>
>   if (MEM_P (str_rtx) && bitregion_start> 0)
>     {
>       enum machine_mode bestmode;
>       HOST_WIDE_INT offset, size;
>
>       gcc_assert ((bitregion_start % BITS_PER_UNIT) == 0);
>
>       offset = bitregion_start / BITS_PER_UNIT;
>       bitnum -= bitregion_start;
>       size = (bitnum + bitsize + BITS_PER_UNIT - 1) / BITS_PER_UNIT;
>       bitregion_end -= bitregion_start;
>       bitregion_start = 0;
>       bestmode = get_best_mode (bitsize, bitnum,
>                                 bitregion_start, bitregion_end,
>                                 MEM_ALIGN (str_rtx), VOIDmode,
>                                 MEM_VOLATILE_P (str_rtx));
>       str_rtx = adjust_bitfield_address_size (str_rtx, bestmode, offset, size);
>     }
>
> Here, if bitregion_start = 8, we have a 4 byte aligned memory context,
> and whoops, now it is only 1 byte aligned.
>
> this example:
>
> struct s
> {
>   char a;
>   int b:24;
> };
>
> struct s ss;
>
> void foo(int b)
> {
>   ss.b = b;
> }
>
>
> gets compiled (at -O3) to:
>
> foo:
>     @ Function supports interworking.
>     @ args = 0, pretend = 0, frame = 0
>     @ frame_needed = 0, uses_anonymous_args = 0
>     @ link register save eliminated.
>     ldr    r3, .L2
>     mov    r1, r0, lsr #8
>     mov    r2, r0, lsr #16
>     strb    r1, [r3, #2]
>     strb    r0, [r3, #1]
>     strb    r2, [r3, #3]
>     bx    lr
>
> while...
>
> struct s
> {
>   char a;
>   int b:24;
> };
>
> struct s ss;
>
> void foo(int b)
> {
>   ss.b = b;
> }
>
>
> gets compiled (at -O3) to
>
> foo:
>     @ Function supports interworking.
>     @ args = 0, pretend = 0, frame = 0
>     @ frame_needed = 0, uses_anonymous_args = 0
>     @ link register save eliminated.
>     ldr    r3, .L2
>     mov    r2, r0, lsr #16
>     strb    r2, [r3, #2]
>     strh    r0, [r3]    @ movhi
>     bx    lr
>
> which is more efficient, but only because the memory context is still
> aligned in this case.
>
>> Btw, the C++ standard doesn't cover packed or aligned attributes so
>> we could declare this a non-issue. Any opinion on that?
>>
>> Thanks,
>> Richard.
>>
>>> Thanks
>>> Bernd. 		 	   		  

[-- Attachment #2: changelog-unaligned-data.txt --]
[-- Type: text/plain, Size: 401 bytes --]

2013-10-31  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	Fix C++0x memory model for unaligned fields in packed, aligned(4)
	structures with -fno-strict-volatile-bitfields on STRICT_ALIGNMENT
	targets like arm-none-eabi.
	* expmed.c (store_bit_field): Handle unaligned fields like
	bit regions.

testsuite:
2013-10-31  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* gcc.dg/pr56997-4.c: New testcase.

[-- Attachment #3: patch-unaligned-data.diff --]
[-- Type: application/octet-stream, Size: 1697 bytes --]

Index: gcc/expmed.c
===================================================================
--- gcc/expmed.c	(revision 203898)
+++ gcc/expmed.c	(working copy)
@@ -871,6 +871,18 @@ store_bit_field (rtx str_rtx, unsigned HOST_WIDE_I
 		 enum machine_mode fieldmode,
 		 rtx value)
 {
+  /* Treat unaligned fields like bit regions otherwise we might
+     touch bits outside the field.  */
+  if (MEM_P (str_rtx) && bitregion_start == 0 && bitregion_end == 0
+      && bitsize > 0 && bitsize % BITS_PER_UNIT == 0
+      && bitnum % BITS_PER_UNIT == 0
+      && (bitsize % MIN (MEM_ALIGN (str_rtx), BITS_PER_WORD) > 0
+	|| bitnum % MIN (MEM_ALIGN (str_rtx), BITS_PER_WORD) > 0))
+    {
+      bitregion_start = bitnum;
+      bitregion_end = bitnum + bitsize - 1;
+    }
+
   /* Under the C++0x memory model, we must not touch bits outside the
      bit region.  Adjust the address to start at the beginning of the
      bit region.  */
--- gcc/testsuite/gcc.dg/pr56997-4.c	(working copy)
+++ gcc/testsuite/gcc.dg/pr56997-4.c	(working copy)
@@ -0,0 +1,23 @@
+/* Test volatile access to unaligned field.  */
+/* { dg-do compile } */
+/* { dg-options "-fno-strict-volatile-bitfields -fdump-rtl-final" } */
+
+#define test_type unsigned short
+
+typedef struct s{
+ unsigned char Prefix[1];
+ volatile test_type Type;
+}__attribute((__packed__,__aligned__(4))) ss;
+
+extern volatile ss v;
+
+void
+foo (test_type u)
+{
+  v.Type = u;
+}
+
+/* The C++ memory model forbids data store race conditions outside the
+   unaligned data member, therefore only QI or HI access is allowed, no SI.  */
+/* { dg-final { scan-rtl-dump-not "mem/v(/.)*:SI" "final" } } */
+/* { dg-final { cleanup-rtl-dump "final" } } */

[PING] [PATCH] Fix C++0x memory model for -fno-strict-volatile-bitfields on ARM

[Bernd Edlinger]

Hello,

could you please approve this patch: http://gcc.gnu.org/ml/gcc-patches/2013-10/msg02664.html

As it looks like, it fixes the problem reported under PR59134,
which is the expander enters infinite recursion while it tries to violate the C++ memory model.

The memory context has still the structure mode, which is QImode, and due to this patch the
memory accesses are no longer tried in SImode but in QImode, which is fixes the ICE here.

Maybe I should add the test case from PR59134 ?


Thanks
Bernd.

>
> Hello,
>
> meanwhile, I have added a test case to that patch.
>
> Boot-strapped and regression-tested as usual.
>
> OK for trunk?
>
> Bernd.
>
>> Hi,
>>
>> On Fri, 25 Oct 2013 11:26:20, Richard Biener wrote:
>>>
>>> On Fri, Oct 25, 2013 at 10:40 AM, Bernd Edlinger
>>> <bernd.edlinger@hotmail.de> wrote:
>>>> Hello,
>>>>
>>>> this patch fixes the recently discovered data store race on arm-eabi-gcc with -fno-strict-volatile-bitfields
>>>> for structures like this:
>>>>
>>>> #define test_type unsigned short
>>>>
>>>> typedef struct s{
>>>> unsigned char Prefix[1];
>>>> test_type Type;
>>>> }__attribute((__packed__,__aligned__(4))) ss;
>>>>
>>>> volatile ss v;
>>>>
>>>> void __attribute__((noinline))
>>>> foo (test_type u)
>>>> {
>>>> v.Type = u;
>>>> }
>>>>
>>>> test_type __attribute__((noinline))
>>>> bar (void)
>>>> {
>>>> return v.Type;
>>>> }
>>>>
>>>>
>>>> I've manually confirmed the correct code generation using variations of the
>>>> example above on an ARM cross-compiler for -fno-strict-volatile-bitfields.
>>>>
>>>> Note, that this example is still causes ICE's for -fstrict-volatile-bitfields,
>>>> but I'd like to fix that separately.
>>>>
>>>> Boot-strapped and regression-tested on x86_64-linux-gnu.
>>>>
>>>> Ok for trunk?
>>>
>>> Isn't it more appropriate to fix it here:
>>>
>>> if (TREE_CODE (to) == COMPONENT_REF
>>> && DECL_BIT_FIELD_TYPE (TREE_OPERAND (to, 1)))
>>> get_bit_range (&bitregion_start, &bitregion_end, to, &bitpos, &offset);
>>>
>>> ?
>>>
>>
>> Honestly, I'd call this is a work-around, not a design.
>>
>> Therefore I would not move that workaround to expr.c.
>>
>> Also the block below is only a work-around IMHO.
>>
>> if (MEM_P (str_rtx) && bitregion_start> 0)
>> {
>> enum machine_mode bestmode;
>> HOST_WIDE_INT offset, size;
>>
>> gcc_assert ((bitregion_start % BITS_PER_UNIT) == 0);
>>
>> offset = bitregion_start / BITS_PER_UNIT;
>> bitnum -= bitregion_start;
>> size = (bitnum + bitsize + BITS_PER_UNIT - 1) / BITS_PER_UNIT;
>> bitregion_end -= bitregion_start;
>> bitregion_start = 0;
>> bestmode = get_best_mode (bitsize, bitnum,
>> bitregion_start, bitregion_end,
>> MEM_ALIGN (str_rtx), VOIDmode,
>> MEM_VOLATILE_P (str_rtx));
>> str_rtx = adjust_bitfield_address_size (str_rtx, bestmode, offset, size);
>> }
>>
>> Here, if bitregion_start = 8, we have a 4 byte aligned memory context,
>> and whoops, now it is only 1 byte aligned.
>>
>> this example:
>>
>> struct s
>> {
>> char a;
>> int b:24;
>> };
>>
>> struct s ss;
>>
>> void foo(int b)
>> {
>> ss.b = b;
>> }
>>
>>
>> gets compiled (at -O3) to:
>>
>> foo:
>> @ Function supports interworking.
>> @ args = 0, pretend = 0, frame = 0
>> @ frame_needed = 0, uses_anonymous_args = 0
>> @ link register save eliminated.
>> ldr r3, .L2
>> mov r1, r0, lsr #8
>> mov r2, r0, lsr #16
>> strb r1, [r3, #2]
>> strb r0, [r3, #1]
>> strb r2, [r3, #3]
>> bx lr
>>
>> while...
>>
>> struct s
>> {
>> char a;
>> int b:24;
>> };
>>
>> struct s ss;
>>
>> void foo(int b)
>> {
>> ss.b = b;
>> }
>>
>>
>> gets compiled (at -O3) to
>>
>> foo:
>> @ Function supports interworking.
>> @ args = 0, pretend = 0, frame = 0
>> @ frame_needed = 0, uses_anonymous_args = 0
>> @ link register save eliminated.
>> ldr r3, .L2
>> mov r2, r0, lsr #16
>> strb r2, [r3, #2]
>> strh r0, [r3] @ movhi
>> bx lr
>>
>> which is more efficient, but only because the memory context is still
>> aligned in this case.
>>
>>> Btw, the C++ standard doesn't cover packed or aligned attributes so
>>> we could declare this a non-issue. Any opinion on that?
>>>
>>> Thanks,
>>> Richard.
>>>
>>>> Thanks
>>>> Bernd. 		 	   		  

Re: [PATCH] Fix C++0x memory model for -fno-strict-volatile-bitfields on ARM

[Richard Biener]

On Thu, Oct 31, 2013 at 11:27 AM, Bernd Edlinger
<bernd.edlinger@hotmail.de> wrote:
> Hello,
>
> meanwhile, I have added a test case to that patch.
>
> Boot-strapped and regression-tested as usual.
>
> OK for trunk?

Err, well.  This just means that the generic C++ memory model
handling isn't complete.  We do

      if (TREE_CODE (to) == COMPONENT_REF
          && DECL_BIT_FIELD_TYPE (TREE_OPERAND (to, 1)))
        get_bit_range (&bitregion_start, &bitregion_end, to, &bitpos, &offset);

and thus restrict ourselves to bitfields to initialize the region we may touch.
This just lacks computing bitregion_start / bitregion_end for other
fields.

Btw, what does the C++ memory model say for

struct { char x; short a; short b; } a __attribute__((packed));

short *p = &a.a;

*p = 1;

I suppose '*p' may not touch bits outside of a.a either?  Or are
memory accesses via pointers less constrained?  What about
array element accesses?

That is, don't we need

     else
        {
           bitregion_start = 0;
           bitregion_end = bitsize;
        }

?

Richard.

> Bernd.
>
>> Hi,
>>
>> On Fri, 25 Oct 2013 11:26:20, Richard Biener wrote:
>>>
>>> On Fri, Oct 25, 2013 at 10:40 AM, Bernd Edlinger
>>> <bernd.edlinger@hotmail.de> wrote:
>>>> Hello,
>>>>
>>>> this patch fixes the recently discovered data store race on arm-eabi-gcc with -fno-strict-volatile-bitfields
>>>> for structures like this:
>>>>
>>>> #define test_type unsigned short
>>>>
>>>> typedef struct s{
>>>> unsigned char Prefix[1];
>>>> test_type Type;
>>>> }__attribute((__packed__,__aligned__(4))) ss;
>>>>
>>>> volatile ss v;
>>>>
>>>> void __attribute__((noinline))
>>>> foo (test_type u)
>>>> {
>>>> v.Type = u;
>>>> }
>>>>
>>>> test_type __attribute__((noinline))
>>>> bar (void)
>>>> {
>>>> return v.Type;
>>>> }
>>>>
>>>>
>>>> I've manually confirmed the correct code generation using variations of the
>>>> example above on an ARM cross-compiler for -fno-strict-volatile-bitfields.
>>>>
>>>> Note, that this example is still causes ICE's for -fstrict-volatile-bitfields,
>>>> but I'd like to fix that separately.
>>>>
>>>> Boot-strapped and regression-tested on x86_64-linux-gnu.
>>>>
>>>> Ok for trunk?
>>>
>>> Isn't it more appropriate to fix it here:
>>>
>>> if (TREE_CODE (to) == COMPONENT_REF
>>> && DECL_BIT_FIELD_TYPE (TREE_OPERAND (to, 1)))
>>> get_bit_range (&bitregion_start, &bitregion_end, to, &bitpos, &offset);
>>>
>>> ?
>>>
>>
>> Honestly, I'd call this is a work-around, not a design.
>>
>> Therefore I would not move that workaround to expr.c.
>>
>> Also the block below is only a work-around IMHO.
>>
>>   if (MEM_P (str_rtx) && bitregion_start> 0)
>>     {
>>       enum machine_mode bestmode;
>>       HOST_WIDE_INT offset, size;
>>
>>       gcc_assert ((bitregion_start % BITS_PER_UNIT) == 0);
>>
>>       offset = bitregion_start / BITS_PER_UNIT;
>>       bitnum -= bitregion_start;
>>       size = (bitnum + bitsize + BITS_PER_UNIT - 1) / BITS_PER_UNIT;
>>       bitregion_end -= bitregion_start;
>>       bitregion_start = 0;
>>       bestmode = get_best_mode (bitsize, bitnum,
>>                                 bitregion_start, bitregion_end,
>>                                 MEM_ALIGN (str_rtx), VOIDmode,
>>                                 MEM_VOLATILE_P (str_rtx));
>>       str_rtx = adjust_bitfield_address_size (str_rtx, bestmode, offset, size);
>>     }
>>
>> Here, if bitregion_start = 8, we have a 4 byte aligned memory context,
>> and whoops, now it is only 1 byte aligned.
>>
>> this example:
>>
>> struct s
>> {
>>   char a;
>>   int b:24;
>> };
>>
>> struct s ss;
>>
>> void foo(int b)
>> {
>>   ss.b = b;
>> }
>>
>>
>> gets compiled (at -O3) to:
>>
>> foo:
>>     @ Function supports interworking.
>>     @ args = 0, pretend = 0, frame = 0
>>     @ frame_needed = 0, uses_anonymous_args = 0
>>     @ link register save eliminated.
>>     ldr    r3, .L2
>>     mov    r1, r0, lsr #8
>>     mov    r2, r0, lsr #16
>>     strb    r1, [r3, #2]
>>     strb    r0, [r3, #1]
>>     strb    r2, [r3, #3]
>>     bx    lr
>>
>> while...
>>
>> struct s
>> {
>>   char a;
>>   int b:24;
>> };
>>
>> struct s ss;
>>
>> void foo(int b)
>> {
>>   ss.b = b;
>> }
>>
>>
>> gets compiled (at -O3) to
>>
>> foo:
>>     @ Function supports interworking.
>>     @ args = 0, pretend = 0, frame = 0
>>     @ frame_needed = 0, uses_anonymous_args = 0
>>     @ link register save eliminated.
>>     ldr    r3, .L2
>>     mov    r2, r0, lsr #16
>>     strb    r2, [r3, #2]
>>     strh    r0, [r3]    @ movhi
>>     bx    lr
>>
>> which is more efficient, but only because the memory context is still
>> aligned in this case.
>>
>>> Btw, the C++ standard doesn't cover packed or aligned attributes so
>>> we could declare this a non-issue. Any opinion on that?
>>>
>>> Thanks,
>>> Richard.
>>>
>>>> Thanks
>>>> Bernd.

Re: [PATCH] Fix C++0x memory model for -fno-strict-volatile-bitfields on ARM

[Jakub Jelinek]

On Fri, Nov 15, 2013 at 11:12:39AM +0100, Richard Biener wrote:
> Btw, what does the C++ memory model say for
> 
> struct { char x; short a; short b; } a __attribute__((packed));

Nothing, because packed is outside of the standard?

	Jakub

Re: [PATCH] Fix C++0x memory model for -fno-strict-volatile-bitfields on ARM

[Richard Biener]

On Fri, Nov 15, 2013 at 11:14 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Fri, Nov 15, 2013 at 11:12:39AM +0100, Richard Biener wrote:
>> Btw, what does the C++ memory model say for
>>
>> struct { char x; short a; short b; } a __attribute__((packed));
>
> Nothing, because packed is outside of the standard?

Ok, then make it not packed.  Then I bring in Josephs earlier comment

"I think the memory model naturally applies to packed structures (i.e.,
writes to fields in them should not write to any other fields except as
part of a sequence of consecutive non-zero-width bit-fields, unless
allow-store-data-races is set)."

Richard.

>         Jakub

RE: [PATCH] Fix C++0x memory model for -fno-strict-volatile-bitfields on ARM

[Bernd Edlinger]

>
> Err, well. This just means that the generic C++ memory model
> handling isn't complete. We do
>
> if (TREE_CODE (to) == COMPONENT_REF
> && DECL_BIT_FIELD_TYPE (TREE_OPERAND (to, 1)))
> get_bit_range (&bitregion_start, &bitregion_end, to, &bitpos, &offset);
>
> and thus restrict ourselves to bitfields to initialize the region we may touch.
> This just lacks computing bitregion_start / bitregion_end for other
> fields.
>
> Btw, what does the C++ memory model say for
>
> struct { char x; short a; short b; } a __attribute__((packed));
>
> short *p = &a.a;
>
> *p = 1;
>

this is not allowed. It should get at least a warning, that p is assigned
a value, which violates the alignment restrictions.
with packed structures you always have to access as "a.a".

> I suppose '*p' may not touch bits outside of a.a either? Or are
> memory accesses via pointers less constrained? What about
> array element accesses?
>

of course, *p may not access anything else than a short.
but the code generation may assume the pointer is aligned.

> That is, don't we need
>
> else
> {
> bitregion_start = 0;
> bitregion_end = bitsize;
> }
>
> ?
>
> Richard.
>

That looks like always pretending it is a bit field.
But it is not a bit field, and bitregion_start=bitregion_end=0
means it is an ordinary value.

It is just the bottom half of the expansion in expmed.c that does
not handle that really well, most of that code seems to ignore the
C++ memory model. For instance store_split_bit_field only handles
bitregion_end and not bitregion_start. And I do not see, why it
tries to write beyond a packed field at all. But that was OK in the past.

And then there is this:

      if (get_best_mem_extraction_insn (&insv, EP_insv, bitsize, bitnum,
                                        fieldmode)
          && store_bit_field_using_insv (&insv, op0, bitsize, bitnum, value))
        return true;

which dies not even care about bitregion_start/end.

An look at that code in store_bit_field:

  if (MEM_P (str_rtx) && bitregion_start> 0)
    {
      enum machine_mode bestmode;
      HOST_WIDE_INT offset, size;

      gcc_assert ((bitregion_start % BITS_PER_UNIT) == 0);

      offset = bitregion_start / BITS_PER_UNIT;
      bitnum -= bitregion_start;
      size = (bitnum + bitsize + BITS_PER_UNIT - 1) / BITS_PER_UNIT;
      bitregion_end -= bitregion_start;
      bitregion_start = 0;
      bestmode = get_best_mode (bitsize, bitnum,
                                bitregion_start, bitregion_end,
                                MEM_ALIGN (str_rtx), VOIDmode,
                                MEM_VOLATILE_P (str_rtx));
      str_rtx = adjust_bitfield_address_size (str_rtx, bestmode, offset, size);
    }

I'd bet 1 Euro, that the person who wrote that, did that because it was too difficult and error-prone
to re-write all the code below, and found it much safer to change the bitregion_start/end
here instead.

My patch is just layered on this if-block to accomplish the task. And that is why I think it is the
right place here.

Bernd. 		 	   		  

Re: [PATCH] Fix C++0x memory model for -fno-strict-volatile-bitfields on ARM

[Richard Biener]

On Fri, Nov 15, 2013 at 12:38 PM, Bernd Edlinger
<bernd.edlinger@hotmail.de> wrote:
>>
>> Err, well. This just means that the generic C++ memory model
>> handling isn't complete. We do
>>
>> if (TREE_CODE (to) == COMPONENT_REF
>> && DECL_BIT_FIELD_TYPE (TREE_OPERAND (to, 1)))
>> get_bit_range (&bitregion_start, &bitregion_end, to, &bitpos, &offset);
>>
>> and thus restrict ourselves to bitfields to initialize the region we may touch.
>> This just lacks computing bitregion_start / bitregion_end for other
>> fields.
>>
>> Btw, what does the C++ memory model say for
>>
>> struct { char x; short a; short b; } a __attribute__((packed));
>>
>> short *p = &a.a;
>>
>> *p = 1;
>>
>
> this is not allowed. It should get at least a warning, that p is assigned
> a value, which violates the alignment restrictions.
> with packed structures you always have to access as "a.a".
>
>> I suppose '*p' may not touch bits outside of a.a either? Or are
>> memory accesses via pointers less constrained? What about
>> array element accesses?
>>
>
> of course, *p may not access anything else than a short.
> but the code generation may assume the pointer is aligned.
>
>> That is, don't we need
>>
>> else
>> {
>> bitregion_start = 0;
>> bitregion_end = bitsize;
>> }
>>
>> ?
>>
>> Richard.
>>
>
> That looks like always pretending it is a bit field.
> But it is not a bit field, and bitregion_start=bitregion_end=0
> means it is an ordinary value.

I don't think it is supposed to mean that.  It's supposed to mean
"the access is unconstrained".

> It is just the bottom half of the expansion in expmed.c that does
> not handle that really well, most of that code seems to ignore the
> C++ memory model. For instance store_split_bit_field only handles
> bitregion_end and not bitregion_start. And I do not see, why it
> tries to write beyond a packed field at all. But that was OK in the past.
>
> And then there is this:
>
>       if (get_best_mem_extraction_insn (&insv, EP_insv, bitsize, bitnum,
>                                         fieldmode)
>           && store_bit_field_using_insv (&insv, op0, bitsize, bitnum, value))
>         return true;
>
> which dies not even care about bitregion_start/end.

Hopefully insv targets exactly match bitnum/bitsize.

> An look at that code in store_bit_field:
>
>   if (MEM_P (str_rtx) && bitregion_start> 0)
>     {
>       enum machine_mode bestmode;
>       HOST_WIDE_INT offset, size;
>
>       gcc_assert ((bitregion_start % BITS_PER_UNIT) == 0);
>
>       offset = bitregion_start / BITS_PER_UNIT;
>       bitnum -= bitregion_start;
>       size = (bitnum + bitsize + BITS_PER_UNIT - 1) / BITS_PER_UNIT;
>       bitregion_end -= bitregion_start;
>       bitregion_start = 0;
>       bestmode = get_best_mode (bitsize, bitnum,
>                                 bitregion_start, bitregion_end,
>                                 MEM_ALIGN (str_rtx), VOIDmode,
>                                 MEM_VOLATILE_P (str_rtx));
>       str_rtx = adjust_bitfield_address_size (str_rtx, bestmode, offset, size);
>     }
>
> I'd bet 1 Euro, that the person who wrote that, did that because it was too difficult and error-prone
> to re-write all the code below, and found it much safer to change the bitregion_start/end
> here instead.

It wasn't me ;)

> My patch is just layered on this if-block to accomplish the task. And that is why I think it is the
> right place here.

Well.  You are looking at the alignment of str_rtx which I'm not sure
whether it's the correct alignment at this point (we at least start
with the alignment of the base).

+  /* Treat unaligned fields like bit regions otherwise we might
+     touch bits outside the field.  */
+  if (MEM_P (str_rtx) && bitregion_start == 0 && bitregion_end == 0
+      && bitsize > 0 && bitsize % BITS_PER_UNIT == 0

bitsize is always > 0, why restrict it to byte-sized accesses?

+      && bitnum % BITS_PER_UNIT == 0

and byte-aligned accesses?

+      && (bitsize % MIN (MEM_ALIGN (str_rtx), BITS_PER_WORD) > 0
+ || bitnum % MIN (MEM_ALIGN (str_rtx), BITS_PER_WORD) > 0))

and what's this BITS_PER_WORD doing here?  It seems what you
are testing for (and should say so in the comment) is non-power-of-two
size accesses or accesses that are not naturally aligned.

But as said, I fail to see why doing this here instead of at the
point we compute the initial bitregion_start/end is appropriate - you
are after all affecting callers such as

calls.c:            store_bit_field (reg, bitsize, endian_correction, 0, 0,

and not only those that will possibly start from a non-zero bitregion_start/end.

Richard.

+    {
+      bitregion_start = bitnum;
+      bitregion_end = bitnum + bitsize - 1;
+    }



> Bernd.

RE: [PATCH] Fix C++0x memory model for -fno-strict-volatile-bitfields on ARM

[Bernd Edlinger]

[-- Attachment #1: Type: text/plain, Size: 616 bytes --]

Hi,

On Fri, 15 Nov 2013 13:30:51, Richard Biener wrote:
>> That looks like always pretending it is a bit field.
>> But it is not a bit field, and bitregion_start=bitregion_end=0
>> means it is an ordinary value.
>
> I don't think it is supposed to mean that. It's supposed to mean
> "the access is unconstrained".
>

Ok, agreed, I did not regard that as a feature.
And apparently only the code path in expand_assigment
really has a problem with it.

So here my second attempt at fixing this.

Boot-strapped and regression-tested on x86_64-linux-gnu.

OK for trunk?


Thanks
Bernd. 		 	   		  

[-- Attachment #2: changelog-unaligned-data.txt --]
[-- Type: text/plain, Size: 398 bytes --]

2013-11-18  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	Fix C++0x memory model for unaligned fields in packed, aligned(4)
	structures with -fno-strict-volatile-bitfields on STRICT_ALIGNMENT
	targets like arm-none-eabi.
	* expr.c (expand_assignment): Handle normal fields like bit regions.

testsuite:
2013-11-18  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* gcc.dg/pr56997-4.c: New testcase.


[-- Attachment #3: patch-unaligned-data.diff --]
[-- Type: application/octet-stream, Size: 1341 bytes --]

Index: gcc/expr.c
===================================================================
--- gcc/expr.c	(revision 204913)
+++ gcc/expr.c	(working copy)
@@ -4722,6 +4722,11 @@ expand_assignment (tree to, tree from, bool nontem
       if (TREE_CODE (to) == COMPONENT_REF
 	  && DECL_BIT_FIELD_TYPE (TREE_OPERAND (to, 1)))
 	get_bit_range (&bitregion_start, &bitregion_end, to, &bitpos, &offset);
+      else if (bitsize > 0)
+	{
+	  bitregion_start = bitpos;
+	  bitregion_end = bitpos + bitsize - 1;
+	}
 
       to_rtx = expand_expr (tem, NULL_RTX, VOIDmode, EXPAND_WRITE);
 
--- gcc/testsuite/gcc.dg/pr56997-4.c	(working copy)
+++ gcc/testsuite/gcc.dg/pr56997-4.c	(working copy)
@@ -0,0 +1,23 @@
+/* Test volatile access to unaligned field.  */
+/* { dg-do compile } */
+/* { dg-options "-fno-strict-volatile-bitfields -fdump-rtl-final" } */
+
+#define test_type unsigned short
+
+typedef struct s{
+ unsigned char Prefix[1];
+ volatile test_type Type;
+}__attribute((__packed__,__aligned__(4))) ss;
+
+extern volatile ss v;
+
+void
+foo (test_type u)
+{
+  v.Type = u;
+}
+
+/* The C++ memory model forbids data store race conditions outside the
+   unaligned data member, therefore only QI or HI access is allowed, no SI.  */
+/* { dg-final { scan-rtl-dump-not "mem/v(/.)*:SI" "final" } } */
+/* { dg-final { cleanup-rtl-dump "final" } } */

RE: [PATCH] Fix C++0x memory model for -fno-strict-volatile-bitfields on ARM

[Bernd Edlinger]

[-- Attachment #1: Type: text/plain, Size: 1113 bytes --]

Hello,

I had forgotten to run the Ada test suite when I submitted the previous version of this patch.
And indeed there were some Ada test cases failing because in Ada packed structures are
like bit fields, but without the DECL_BIT_FIELD_TYPE attribute.

Please find attached the updated version of this patch.

Boot-strapped and regression-tested on x86_64-linux-gnu.
Ok for trunk?

Bernd.

> On Mon, 18 Nov 2013 11:37:05, Bernd Edlinger wrote:
>
> Hi,
>
> On Fri, 15 Nov 2013 13:30:51, Richard Biener wrote:
>>> That looks like always pretending it is a bit field.
>>> But it is not a bit field, and bitregion_start=bitregion_end=0
>>> means it is an ordinary value.
>>
>> I don't think it is supposed to mean that. It's supposed to mean
>> "the access is unconstrained".
>>
>
> Ok, agreed, I did not regard that as a feature.
> And apparently only the code path in expand_assigment
> really has a problem with it.
>
> So here my second attempt at fixing this.
>
> Boot-strapped and regression-tested on x86_64-linux-gnu.
>
> OK for trunk?
>
>
> Thanks
> Bernd. 		 	   		  

[-- Attachment #2: changelog-unaligned-data.txt --]
[-- Type: text/plain, Size: 398 bytes --]

2013-11-25  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	Fix C++0x memory model for unaligned fields in packed, aligned(4)
	structures with -fno-strict-volatile-bitfields on STRICT_ALIGNMENT
	targets like arm-none-eabi.
	* expr.c (expand_assignment): Handle normal fields like bit regions.

testsuite:
2013-11-25  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* gcc.dg/pr56997-4.c: New testcase.


[-- Attachment #3: patch-unaligned-data.diff --]
[-- Type: application/octet-stream, Size: 1699 bytes --]

Index: gcc/expr.c
===================================================================
--- gcc/expr.c	(revision 205234)
+++ gcc/expr.c	(working copy)
@@ -4811,6 +4811,17 @@ expand_assignment (tree to, tree from, bool nontem
       if (TREE_CODE (to) == COMPONENT_REF
 	  && DECL_BIT_FIELD_TYPE (TREE_OPERAND (to, 1)))
 	get_bit_range (&bitregion_start, &bitregion_end, to, &bitpos, &offset);
+      /* The C++ memory model naturally applies to byte-aligned fields.
+	 However, if we do not have a DECL_BIT_FIELD_TYPE but BITPOS or
+	 BITSIZE are not byte-aligned, there is no need to limit the range
+	 we can access.  This can occur with packed structures in Ada.  */
+      else if (bitsize > 0
+	       && bitsize % BITS_PER_UNIT == 0
+	       && bitpos % BITS_PER_UNIT == 0)
+	{
+	  bitregion_start = bitpos;
+	  bitregion_end = bitpos + bitsize - 1;
+	}
 
       to_rtx = expand_expr (tem, NULL_RTX, VOIDmode, EXPAND_WRITE);
 
--- gcc/testsuite/gcc.dg/pr56997-4.c	(working copy)
+++ gcc/testsuite/gcc.dg/pr56997-4.c	(working copy)
@@ -0,0 +1,23 @@
+/* Test volatile access to unaligned field.  */
+/* { dg-do compile } */
+/* { dg-options "-fno-strict-volatile-bitfields -fdump-rtl-final" } */
+
+#define test_type unsigned short
+
+typedef struct s{
+ unsigned char Prefix[1];
+ volatile test_type Type;
+}__attribute((__packed__,__aligned__(4))) ss;
+
+extern volatile ss v;
+
+void
+foo (test_type u)
+{
+  v.Type = u;
+}
+
+/* The C++ memory model forbids data store race conditions outside the
+   unaligned data member, therefore only QI or HI access is allowed, no SI.  */
+/* { dg-final { scan-rtl-dump-not "mem/v(/.)*:SI" "final" } } */
+/* { dg-final { cleanup-rtl-dump "final" } } */

Re: [PATCH] Fix C++0x memory model for -fno-strict-volatile-bitfields on ARM

[Richard Biener]

On Mon, Nov 25, 2013 at 1:07 PM, Bernd Edlinger
<bernd.edlinger@hotmail.de> wrote:
> Hello,
>
> I had forgotten to run the Ada test suite when I submitted the previous version of this patch.
> And indeed there were some Ada test cases failing because in Ada packed structures are
> like bit fields, but without the DECL_BIT_FIELD_TYPE attribute.

I think they may have DECL_BIT_FIELD set though, not sure.

> Please find attached the updated version of this patch.
>
> Boot-strapped and regression-tested on x86_64-linux-gnu.
> Ok for trunk?

So you mimic what Eric added in get_bit_range?  Btw, I'm not sure
the "conservative" way of failing get_bit_range with not limiting the
access at all is good.

That is, we may want to do

+      /* The C++ memory model naturally applies to byte-aligned fields.
+        However, if we do not have a DECL_BIT_FIELD_TYPE but BITPOS or
+        BITSIZE are not byte-aligned, there is no need to limit the range
+        we can access.  This can occur with packed structures in Ada.  */
+      if (bitregion_start == 0 && bitregion_end == 0
+          && bitsize > 0
+          && bitsize % BITS_PER_UNIT == 0
+          && bitpos % BITS_PER_UNIT == 0)
+       {
+         bitregion_start = bitpos;
+         bitregion_end = bitpos + bitsize - 1;
+       }

thus not else if but also apply it when get_bit_range "failed" (as it may
fail for other reasons).  A better fallback would be to track down
the outermost byte-aligned handled-component and limit the access
to that (though I guess Ada doesn't care at all about the C++ memory
model and only Ada has bit-aligned aggregates).

That said, the patch looks ok as-is to me, let's see if we can clean
things up for the next stage1.

Thanks,
Richard.

> Bernd.
>
>> On Mon, 18 Nov 2013 11:37:05, Bernd Edlinger wrote:
>>
>> Hi,
>>
>> On Fri, 15 Nov 2013 13:30:51, Richard Biener wrote:
>>>> That looks like always pretending it is a bit field.
>>>> But it is not a bit field, and bitregion_start=bitregion_end=0
>>>> means it is an ordinary value.
>>>
>>> I don't think it is supposed to mean that. It's supposed to mean
>>> "the access is unconstrained".
>>>
>>
>> Ok, agreed, I did not regard that as a feature.
>> And apparently only the code path in expand_assigment
>> really has a problem with it.
>>
>> So here my second attempt at fixing this.
>>
>> Boot-strapped and regression-tested on x86_64-linux-gnu.
>>
>> OK for trunk?
>>
>>
>> Thanks
>> Bernd.

RE: [PATCH] Fix C++0x memory model for -fno-strict-volatile-bitfields on ARM

[Bernd Edlinger]

Hi,

On Mon, 2 Dec 2013 15:55:08Richard Biener wrote:
>
> On Mon, Nov 25, 2013 at 1:07 PM, Bernd Edlinger
> <bernd.edlinger@hotmail.de> wrote:
>> Hello,
>>
>> I had forgotten to run the Ada test suite when I submitted the previous version of this patch.
>> And indeed there were some Ada test cases failing because in Ada packed structures are
>> like bit fields, but without the DECL_BIT_FIELD_TYPE attribute.
>
> I think they may have DECL_BIT_FIELD set though, not sure.
>
>> Please find attached the updated version of this patch.
>>
>> Boot-strapped and regression-tested on x86_64-linux-gnu.
>> Ok for trunk?
>
> So you mimic what Eric added in get_bit_range? Btw, I'm not sure
> the "conservative" way of failing get_bit_range with not limiting the
> access at all is good.
>
> That is, we may want to do
>
> + /* The C++ memory model naturally applies to byte-aligned fields.
> + However, if we do not have a DECL_BIT_FIELD_TYPE but BITPOS or
> + BITSIZE are not byte-aligned, there is no need to limit the range
> + we can access. This can occur with packed structures in Ada. */
> + if (bitregion_start == 0 && bitregion_end == 0
> + && bitsize> 0
> + && bitsize % BITS_PER_UNIT == 0
> + && bitpos % BITS_PER_UNIT == 0)
> + {
> + bitregion_start = bitpos;
> + bitregion_end = bitpos + bitsize - 1;
> + }
>
> thus not else if but also apply it when get_bit_range "failed" (as it may
> fail for other reasons). A better fallback would be to track down
> the outermost byte-aligned handled-component and limit the access
> to that (though I guess Ada doesn't care at all about the C++ memory
> model and only Ada has bit-aligned aggregates).
>

Good question. Most of the time the expansion can not know if it expands 
Ada, C, or Fortran. In this case we know it can only be Ada, so the C++ memory
model is not mandatory. Maybe Eric can tell, if a data store race condition
may be an issue in Ada if  structure is laid out like __attribute((packed,aligned(1)))
I mean, if that is at all possible.

> That said, the patch looks ok as-is to me, let's see if we can clean
> things up for the next stage1.
>

Ok, applied as-is.

Thanks
Bernd.

> Thanks,
> Richard.
>
>> Bernd.
>>
>>> On Mon, 18 Nov 2013 11:37:05, Bernd Edlinger wrote:
>>>
>>> Hi,
>>>
>>> On Fri, 15 Nov 2013 13:30:51, Richard Biener wrote:
>>>>> That looks like always pretending it is a bit field.
>>>>> But it is not a bit field, and bitregion_start=bitregion_end=0
>>>>> means it is an ordinary value.
>>>>
>>>> I don't think it is supposed to mean that. It's supposed to mean
>>>> "the access is unconstrained".
>>>>
>>>
>>> Ok, agreed, I did not regard that as a feature.
>>> And apparently only the code path in expand_assigment
>>> really has a problem with it.
>>>
>>> So here my second attempt at fixing this.
>>>
>>> Boot-strapped and regression-tested on x86_64-linux-gnu.
>>>
>>> OK for trunk?
>>>
>>>
>>> Thanks
>>> Bernd. 		 	   		  

Re: [PATCH] Fix C++0x memory model for -fno-strict-volatile-bitfields on ARM

[Eric Botcazou]

> Good question. Most of the time the expansion can not know if it expands
> Ada, C, or Fortran. In this case we know it can only be Ada, so the C++
> memory model is not mandatory. Maybe Eric can tell, if a data store race
> condition may be an issue in Ada if  structure is laid out like
> __attribute((packed,aligned(1))) I mean, if that is at all possible.

Unlike in C++, all bets are off in Ada as soon as you have non-byte-aligned 
objects.  For byte-aligned objects, there is the following implementation 
advice (C.6 2/22):

"A load or store of a volatile object whose size is a multiple of 
System.Storage_Unit and whose alignment is nonzero, should be implemented by 
accessing exactly the bits of the object and no others."

so the answer is (theoritically) yes for volatile fields.  But, in practice, 
we probably reject the potentially problematic volatile fields in gigi.

-- 
Eric Botcazou