Patchwork reimplement -fstrict-volatile-bitfields v4, part 1/2

login
register
mail settings
Submitter Sandra Loosemore
Date Oct. 31, 2013, 12:46 a.m.
Message ID <5271A851.6000808@codesourcery.com>
Download mbox | patch
Permalink /patch/287370/
State New
Headers show

Comments

Sandra Loosemore - Oct. 31, 2013, 12:46 a.m.
On 10/29/2013 02:51 AM, Bernd Edlinger wrote:
>
> On Mon, 28 Oct 2013 21:29:24, Sandra Loosemore wrote:
>> On 10/28/2013 03:20 AM, Bernd Edlinger wrote:
>>> I have attached an update to your patch, that should
>>> a) fix the recursion problem.
>>> b) restrict the -fstrict-volatile-bitfields to not violate the C++ memory model.

Here's a new version of the update patch.

>> Alternatively, if strict_volatile_bitfield_p returns false but
>> flag_strict_volatile_bitfields> 0, then always force to word_mode and
>> change the -fstrict-volatile-bitfields documentation to indicate that's
>> the fallback if the insertion/extraction cannot be done in the declared
>> mode, rather than claiming that it tries to do the same thing as if
>> -fstrict-volatile-bitfields were not enabled at all.

I decided that this approach was more expedient, after all.

I've tested this patch (in conjunction with my already-approved but 
not-yet-applied patch) on mainline for arm-none-eabi, x86_64-linux-gnu, 
and mips-linux gnu.  I also backported the entire series to GCC 4.8 and 
tested there on arm-none-eabi and x86_64-linux-gnu.  OK to apply?

-Sandra
Sandra Loosemore - Nov. 11, 2013, 1:41 a.m.
Can someone please review this patch?

http://gcc.gnu.org/ml/gcc-patches/2013-10/msg02637.html

I would like to commit the already-approved -fstrict-volatile-bitfields 
patch once we also have an approved fix for the infinite recursion 
problem I discovered while testing a backport of the patch series to a 
local copy of GCC 4.8.

-Sandra
Richard Guenther - Nov. 11, 2013, 12:10 p.m.
On Thu, Oct 31, 2013 at 1:46 AM, Sandra Loosemore
<sandra@codesourcery.com> wrote:
> On 10/29/2013 02:51 AM, Bernd Edlinger wrote:
>>
>>
>> On Mon, 28 Oct 2013 21:29:24, Sandra Loosemore wrote:
>>>
>>> On 10/28/2013 03:20 AM, Bernd Edlinger wrote:
>>>>
>>>> I have attached an update to your patch, that should
>>>> a) fix the recursion problem.
>>>> b) restrict the -fstrict-volatile-bitfields to not violate the C++
>>>> memory model.
>
>
> Here's a new version of the update patch.
>
>
>>> Alternatively, if strict_volatile_bitfield_p returns false but
>>> flag_strict_volatile_bitfields> 0, then always force to word_mode and
>>> change the -fstrict-volatile-bitfields documentation to indicate that's
>>> the fallback if the insertion/extraction cannot be done in the declared
>>> mode, rather than claiming that it tries to do the same thing as if
>>> -fstrict-volatile-bitfields were not enabled at all.
>
>
> I decided that this approach was more expedient, after all.
>
> I've tested this patch (in conjunction with my already-approved but
> not-yet-applied patch) on mainline for arm-none-eabi, x86_64-linux-gnu, and
> mips-linux gnu.  I also backported the entire series to GCC 4.8 and tested
> there on arm-none-eabi and x86_64-linux-gnu.  OK to apply?

Hm, I can't seem to find the context for

@@ -923,6 +935,14 @@
        store_fixed_bit_field (str_rtx, bitsize, bitnum, 0, 0, value);
       return;
     }
+  else if (MEM_P (str_rtx)
+          && MEM_VOLATILE_P (str_rtx)
+          && flag_strict_volatile_bitfields > 0)
+    /* This is a case where -fstrict-volatile-bitfields doesn't apply
+       because we can't do a single access in the declared mode of the field.
+       Since the incoming STR_RTX has already been adjusted to that mode,
+       fall back to word mode for subsequent logic.  */
+    str_rtx = adjust_address (str_rtx, word_mode, 0);

   /* 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

and the other similar hunk.  I suppose they apply to earlier patches
in the series?  I suppose the above applies to store_bit_field (diff -p
really helps!).  Why would using word_mode be any good as
fallback?  That is, why is "Since the incoming STR_RTX has already
been adjusted to that mode" not the thing to fix?

Richard.

> -Sandra
Bernd Edlinger - Nov. 14, 2013, 10:16 a.m.
Hi,

sorry, for the delay.
Sandra seems to be even more busy than me...

Attached is a combined patch of the original part 1, and the update,
in diff -up format.

On Mon, 11 Nov 2013 13:10:45, Richard Biener wrote:
>
> On Thu, Oct 31, 2013 at 1:46 AM, Sandra Loosemore
> <sandra@codesourcery.com> wrote:
>> On 10/29/2013 02:51 AM, Bernd Edlinger wrote:
>>>
>>>
>>> On Mon, 28 Oct 2013 21:29:24, Sandra Loosemore wrote:
>>>>
>>>> On 10/28/2013 03:20 AM, Bernd Edlinger wrote:
>>>>>
>>>>> I have attached an update to your patch, that should
>>>>> a) fix the recursion problem.
>>>>> b) restrict the -fstrict-volatile-bitfields to not violate the C++
>>>>> memory model.
>>
>>
>> Here's a new version of the update patch.
>>
>>
>>>> Alternatively, if strict_volatile_bitfield_p returns false but
>>>> flag_strict_volatile_bitfields> 0, then always force to word_mode and
>>>> change the -fstrict-volatile-bitfields documentation to indicate that's
>>>> the fallback if the insertion/extraction cannot be done in the declared
>>>> mode, rather than claiming that it tries to do the same thing as if
>>>> -fstrict-volatile-bitfields were not enabled at all.
>>
>>
>> I decided that this approach was more expedient, after all.
>>
>> I've tested this patch (in conjunction with my already-approved but
>> not-yet-applied patch) on mainline for arm-none-eabi, x86_64-linux-gnu, and
>> mips-linux gnu. I also backported the entire series to GCC 4.8 and tested
>> there on arm-none-eabi and x86_64-linux-gnu. OK to apply?
>
> Hm, I can't seem to find the context for
>
> @@ -923,6 +935,14 @@
> store_fixed_bit_field (str_rtx, bitsize, bitnum, 0, 0, value);
> return;
> }
> + else if (MEM_P (str_rtx)
> + && MEM_VOLATILE_P (str_rtx)
> + && flag_strict_volatile_bitfields> 0)
> + /* This is a case where -fstrict-volatile-bitfields doesn't apply
> + because we can't do a single access in the declared mode of the field.
> + Since the incoming STR_RTX has already been adjusted to that mode,
> + fall back to word mode for subsequent logic. */
> + str_rtx = adjust_address (str_rtx, word_mode, 0);
>
> /* 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
>
> and the other similar hunk. I suppose they apply to earlier patches
> in the series? I suppose the above applies to store_bit_field (diff -p
> really helps!). Why would using word_mode be any good as
> fallback? That is, why is "Since the incoming STR_RTX has already
> been adjusted to that mode" not the thing to fix?
>

Well, this hunk does not force the access to be in word_mode.

Instead it allows get_best_mode to choose the access to be in any mode from
QI to word_mode.

It is there to revert the effect of this weird code in expr.c (expand_assigment):

          if (volatilep && flag_strict_volatile_bitfields> 0)
            to_rtx = adjust_address (to_rtx, mode1, 0);

Note that this does not even check if the access is on a bit-field !

The problem with the strict_volatile_bitfields is that it is used already
before the code reaches store_bit_field or extract_bit_field.

It starts in get_inner_reference, (which is not only used in expand_assignment
and expand_expr_real_1)

Then this,

            if (volatilep && flag_strict_volatile_bitfields> 0)
              op0 = adjust_address (op0, mode1, 0);

and then this,

            /* If the field is volatile, we always want an aligned
               access.  Do this in following two situations:
               1. the access is not already naturally
               aligned, otherwise "normal" (non-bitfield) volatile fields
               become non-addressable.
               2. the bitsize is narrower than the access size. Need
               to extract bitfields from the access.  */
            || (volatilep && flag_strict_volatile_bitfields> 0
                && (bitpos % GET_MODE_ALIGNMENT (mode) != 0
                    || (mode1 != BLKmode
                        && bitsize < GET_MODE_SIZE (mode1) * BITS_PER_UNIT)))

As a result, a read access to an unaligned volatile data member does
not even reach the expand_bit_field if flag_strict_volatile_bitfields <= 0,
and instead goes through convert_move (target, op0, unsignedp).

I still believe the proposed patch is guaranteed to not change anything if
-fno-strict-volatile-bitfields is used, and even if we can not guarantee
that it creates exactly the same code for cases where the strict-volatile-bitfields
does not apply, it certainly generates valid code, where we had invalid code,
or ICEs without the patch.

OK for trunk?

Bernd.

> Richard.
>
>> -Sandra
Richard Guenther - Nov. 14, 2013, 3:31 p.m.
On Thu, Nov 14, 2013 at 11:16 AM, Bernd Edlinger
<bernd.edlinger@hotmail.de> wrote:
> Hi,
>
> sorry, for the delay.
> Sandra seems to be even more busy than me...
>
> Attached is a combined patch of the original part 1, and the update,
> in diff -up format.
>
> On Mon, 11 Nov 2013 13:10:45, Richard Biener wrote:
>>
>> On Thu, Oct 31, 2013 at 1:46 AM, Sandra Loosemore
>> <sandra@codesourcery.com> wrote:
>>> On 10/29/2013 02:51 AM, Bernd Edlinger wrote:
>>>>
>>>>
>>>> On Mon, 28 Oct 2013 21:29:24, Sandra Loosemore wrote:
>>>>>
>>>>> On 10/28/2013 03:20 AM, Bernd Edlinger wrote:
>>>>>>
>>>>>> I have attached an update to your patch, that should
>>>>>> a) fix the recursion problem.
>>>>>> b) restrict the -fstrict-volatile-bitfields to not violate the C++
>>>>>> memory model.
>>>
>>>
>>> Here's a new version of the update patch.
>>>
>>>
>>>>> Alternatively, if strict_volatile_bitfield_p returns false but
>>>>> flag_strict_volatile_bitfields> 0, then always force to word_mode and
>>>>> change the -fstrict-volatile-bitfields documentation to indicate that's
>>>>> the fallback if the insertion/extraction cannot be done in the declared
>>>>> mode, rather than claiming that it tries to do the same thing as if
>>>>> -fstrict-volatile-bitfields were not enabled at all.
>>>
>>>
>>> I decided that this approach was more expedient, after all.
>>>
>>> I've tested this patch (in conjunction with my already-approved but
>>> not-yet-applied patch) on mainline for arm-none-eabi, x86_64-linux-gnu, and
>>> mips-linux gnu. I also backported the entire series to GCC 4.8 and tested
>>> there on arm-none-eabi and x86_64-linux-gnu. OK to apply?
>>
>> Hm, I can't seem to find the context for
>>
>> @@ -923,6 +935,14 @@
>> store_fixed_bit_field (str_rtx, bitsize, bitnum, 0, 0, value);
>> return;
>> }
>> + else if (MEM_P (str_rtx)
>> + && MEM_VOLATILE_P (str_rtx)
>> + && flag_strict_volatile_bitfields> 0)
>> + /* This is a case where -fstrict-volatile-bitfields doesn't apply
>> + because we can't do a single access in the declared mode of the field.
>> + Since the incoming STR_RTX has already been adjusted to that mode,
>> + fall back to word mode for subsequent logic. */
>> + str_rtx = adjust_address (str_rtx, word_mode, 0);
>>
>> /* 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
>>
>> and the other similar hunk. I suppose they apply to earlier patches
>> in the series? I suppose the above applies to store_bit_field (diff -p
>> really helps!). Why would using word_mode be any good as
>> fallback? That is, why is "Since the incoming STR_RTX has already
>> been adjusted to that mode" not the thing to fix?
>>
>
> Well, this hunk does not force the access to be in word_mode.
>
> Instead it allows get_best_mode to choose the access to be in any mode from
> QI to word_mode.
>
> It is there to revert the effect of this weird code in expr.c (expand_assigment):
>
>           if (volatilep && flag_strict_volatile_bitfields> 0)
>             to_rtx = adjust_address (to_rtx, mode1, 0);
>
> Note that this does not even check if the access is on a bit-field !

Then why not remove that ...

> The problem with the strict_volatile_bitfields is that it is used already
> before the code reaches store_bit_field or extract_bit_field.
>
> It starts in get_inner_reference, (which is not only used in expand_assignment
> and expand_expr_real_1)
>
> Then this,
>
>             if (volatilep && flag_strict_volatile_bitfields> 0)
>               op0 = adjust_address (op0, mode1, 0);

... and this ...

> and then this,
>
>             /* If the field is volatile, we always want an aligned
>                access.  Do this in following two situations:
>                1. the access is not already naturally
>                aligned, otherwise "normal" (non-bitfield) volatile fields
>                become non-addressable.
>                2. the bitsize is narrower than the access size. Need
>                to extract bitfields from the access.  */
>             || (volatilep && flag_strict_volatile_bitfields> 0
>                 && (bitpos % GET_MODE_ALIGNMENT (mode) != 0
>                     || (mode1 != BLKmode
>                         && bitsize < GET_MODE_SIZE (mode1) * BITS_PER_UNIT)))

... or this ...

> As a result, a read access to an unaligned volatile data member does
> not even reach the expand_bit_field if flag_strict_volatile_bitfields <= 0,
> and instead goes through convert_move (target, op0, unsignedp).
>
> I still believe the proposed patch is guaranteed to not change anything if
> -fno-strict-volatile-bitfields is used, and even if we can not guarantee
> that it creates exactly the same code for cases where the strict-volatile-bitfields
> does not apply, it certainly generates valid code, where we had invalid code,
> or ICEs without the patch.
>
> OK for trunk?

Again, most of the patch is ok (and nice), the
store_bit_field/extract_bit_field changes
point to the above issues which we should rather fix than trying
to revert them after the fact.

Why is that not possible?

That said,

+  else if (MEM_P (str_rtx)
+          && MEM_VOLATILE_P (str_rtx)
+          && flag_strict_volatile_bitfields > 0)
+    /* This is a case where -fstrict-volatile-bitfields doesn't apply
+       because we can't do a single access in the declared mode of the field.
+       Since the incoming STR_RTX has already been adjusted to that mode,
+       fall back to word mode for subsequent logic.  */
+    str_rtx = adjust_address (str_rtx, word_mode, 0);

we are looking at an access with bitsize / bitregion properties so plainly
choosing word_mode here looks wrong to me.  Yes, only
-fstrict-volatile-bitfields is affected but still ...

The patch is ok if you look at the above as followup.

Thanks,
Richard.



> Bernd.
>
>> Richard.
>>
>>> -Sandra
Bernd Edlinger - Nov. 15, 2013, 9:28 a.m.
Hi,

On Thu, 14 Nov 2013 16:31:10, Richard Biener wrote:
>
> On Thu, Nov 14, 2013 at 11:16 AM, Bernd Edlinger
> <bernd.edlinger@hotmail.de> wrote:
>> Hi,
>>
>> sorry, for the delay.
>> Sandra seems to be even more busy than me...
>>
>> Attached is a combined patch of the original part 1, and the update,
>> in diff -up format.
>>
>> On Mon, 11 Nov 2013 13:10:45, Richard Biener wrote:
>>>
>>> On Thu, Oct 31, 2013 at 1:46 AM, Sandra Loosemore
>>> <sandra@codesourcery.com> wrote:
>>>> On 10/29/2013 02:51 AM, Bernd Edlinger wrote:
>>>>>
>>>>>
>>>>> On Mon, 28 Oct 2013 21:29:24, Sandra Loosemore wrote:
>>>>>>
>>>>>> On 10/28/2013 03:20 AM, Bernd Edlinger wrote:
>>>>>>>
>>>>>>> I have attached an update to your patch, that should
>>>>>>> a) fix the recursion problem.
>>>>>>> b) restrict the -fstrict-volatile-bitfields to not violate the C++
>>>>>>> memory model.
>>>>
>>>>
>>>> Here's a new version of the update patch.
>>>>
>>>>
>>>>>> Alternatively, if strict_volatile_bitfield_p returns false but
>>>>>> flag_strict_volatile_bitfields> 0, then always force to word_mode and
>>>>>> change the -fstrict-volatile-bitfields documentation to indicate that's
>>>>>> the fallback if the insertion/extraction cannot be done in the declared
>>>>>> mode, rather than claiming that it tries to do the same thing as if
>>>>>> -fstrict-volatile-bitfields were not enabled at all.
>>>>
>>>>
>>>> I decided that this approach was more expedient, after all.
>>>>
>>>> I've tested this patch (in conjunction with my already-approved but
>>>> not-yet-applied patch) on mainline for arm-none-eabi, x86_64-linux-gnu, and
>>>> mips-linux gnu. I also backported the entire series to GCC 4.8 and tested
>>>> there on arm-none-eabi and x86_64-linux-gnu. OK to apply?
>>>
>>> Hm, I can't seem to find the context for
>>>
>>> @@ -923,6 +935,14 @@
>>> store_fixed_bit_field (str_rtx, bitsize, bitnum, 0, 0, value);
>>> return;
>>> }
>>> + else if (MEM_P (str_rtx)
>>> + && MEM_VOLATILE_P (str_rtx)
>>> + && flag_strict_volatile_bitfields> 0)
>>> + /* This is a case where -fstrict-volatile-bitfields doesn't apply
>>> + because we can't do a single access in the declared mode of the field.
>>> + Since the incoming STR_RTX has already been adjusted to that mode,
>>> + fall back to word mode for subsequent logic. */
>>> + str_rtx = adjust_address (str_rtx, word_mode, 0);
>>>
>>> /* 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
>>>
>>> and the other similar hunk. I suppose they apply to earlier patches
>>> in the series? I suppose the above applies to store_bit_field (diff -p
>>> really helps!). Why would using word_mode be any good as
>>> fallback? That is, why is "Since the incoming STR_RTX has already
>>> been adjusted to that mode" not the thing to fix?
>>>
>>
>> Well, this hunk does not force the access to be in word_mode.
>>
>> Instead it allows get_best_mode to choose the access to be in any mode from
>> QI to word_mode.
>>
>> It is there to revert the effect of this weird code in expr.c (expand_assigment):
>>
>> if (volatilep && flag_strict_volatile_bitfields> 0)
>> to_rtx = adjust_address (to_rtx, mode1, 0);
>>
>> Note that this does not even check if the access is on a bit-field !
>
> Then why not remove that ...
>
>> The problem with the strict_volatile_bitfields is that it is used already
>> before the code reaches store_bit_field or extract_bit_field.
>>
>> It starts in get_inner_reference, (which is not only used in expand_assignment
>> and expand_expr_real_1)
>>
>> Then this,
>>
>> if (volatilep && flag_strict_volatile_bitfields> 0)
>> op0 = adjust_address (op0, mode1, 0);
>
> ... and this ...
>
>> and then this,
>>
>> /* If the field is volatile, we always want an aligned
>> access. Do this in following two situations:
>> 1. the access is not already naturally
>> aligned, otherwise "normal" (non-bitfield) volatile fields
>> become non-addressable.
>> 2. the bitsize is narrower than the access size. Need
>> to extract bitfields from the access. */
>> || (volatilep && flag_strict_volatile_bitfields> 0
>> && (bitpos % GET_MODE_ALIGNMENT (mode) != 0
>> || (mode1 != BLKmode
>> && bitsize < GET_MODE_SIZE (mode1) * BITS_PER_UNIT)))
>
> ... or this ...
>
>> As a result, a read access to an unaligned volatile data member does
>> not even reach the expand_bit_field if flag_strict_volatile_bitfields <= 0,
>> and instead goes through convert_move (target, op0, unsignedp).
>>
>> I still believe the proposed patch is guaranteed to not change anything if
>> -fno-strict-volatile-bitfields is used, and even if we can not guarantee
>> that it creates exactly the same code for cases where the strict-volatile-bitfields
>> does not apply, it certainly generates valid code, where we had invalid code,
>> or ICEs without the patch.
>>
>> OK for trunk?
>
> Again, most of the patch is ok (and nice), the
> store_bit_field/extract_bit_field changes
> point to the above issues which we should rather fix than trying
> to revert them after the fact.
>
> Why is that not possible?
>
> That said,
>
> + else if (MEM_P (str_rtx)
> + && MEM_VOLATILE_P (str_rtx)
> + && flag_strict_volatile_bitfields> 0)
> + /* This is a case where -fstrict-volatile-bitfields doesn't apply
> + because we can't do a single access in the declared mode of the field.
> + Since the incoming STR_RTX has already been adjusted to that mode,
> + fall back to word mode for subsequent logic. */
> + str_rtx = adjust_address (str_rtx, word_mode, 0);
>
> we are looking at an access with bitsize / bitregion properties so plainly
> choosing word_mode here looks wrong to me. Yes, only
> -fstrict-volatile-bitfields is affected but still ...
>
> The patch is ok if you look at the above as followup.
>
> Thanks,
> Richard.
>
>

hmm...
the above change is just not aggressive enough.


with a slightly changed test case I have now got a 
recursion on the extract_fixed_bit_fields on ARM but
interestingly not on powerpc:

#include <stdlib.h>

typedef struct {
  char pad;
  int arr[0];
} __attribute__((packed)) str;

str *
foo (int* src)
{
  str *s = malloc (sizeof (str) + sizeof (int));
  *src = s->arr[0];
  s->arr[0] = 0x12345678;
  return s;
}

Now I think about reverting that hunk back to what I had in mind initially:

  else if (MEM_P (str_rtx)
           && GET_MODE_BITSIZE (GET_MODE (str_rtx)) != 0
           && GET_MODE_BITSIZE (GET_MODE (str_rtx)) < GET_MODE_BITSIZE (word_mode)
           && bitnum % GET_MODE_BITSIZE (GET_MODE (str_rtx)) + bitsize
             > GET_MODE_BITSIZE (GET_MODE (str_rtx)))
    /* If the mode of str_rtx is too small or unaligned
       fall back to word mode for subsequent logic.  */
    str_rtx = adjust_address (str_rtx, word_mode, 0);

this fixes the recursion on the read side too. And it is limited to cases
where the mode does not match the bitnum and bitsize parameters.


Bernd.


>
>> Bernd.
>>
>>> Richard.
>>>
>>>> -Sandra
Richard Guenther - Nov. 15, 2013, 10:16 a.m.
On Fri, Nov 15, 2013 at 10:28 AM, Bernd Edlinger
<bernd.edlinger@hotmail.de> wrote:
> Hi,
>
> On Thu, 14 Nov 2013 16:31:10, Richard Biener wrote:
>>
>> On Thu, Nov 14, 2013 at 11:16 AM, Bernd Edlinger
>> <bernd.edlinger@hotmail.de> wrote:
>>> Hi,
>>>
>>> sorry, for the delay.
>>> Sandra seems to be even more busy than me...
>>>
>>> Attached is a combined patch of the original part 1, and the update,
>>> in diff -up format.
>>>
>>> On Mon, 11 Nov 2013 13:10:45, Richard Biener wrote:
>>>>
>>>> On Thu, Oct 31, 2013 at 1:46 AM, Sandra Loosemore
>>>> <sandra@codesourcery.com> wrote:
>>>>> On 10/29/2013 02:51 AM, Bernd Edlinger wrote:
>>>>>>
>>>>>>
>>>>>> On Mon, 28 Oct 2013 21:29:24, Sandra Loosemore wrote:
>>>>>>>
>>>>>>> On 10/28/2013 03:20 AM, Bernd Edlinger wrote:
>>>>>>>>
>>>>>>>> I have attached an update to your patch, that should
>>>>>>>> a) fix the recursion problem.
>>>>>>>> b) restrict the -fstrict-volatile-bitfields to not violate the C++
>>>>>>>> memory model.
>>>>>
>>>>>
>>>>> Here's a new version of the update patch.
>>>>>
>>>>>
>>>>>>> Alternatively, if strict_volatile_bitfield_p returns false but
>>>>>>> flag_strict_volatile_bitfields> 0, then always force to word_mode and
>>>>>>> change the -fstrict-volatile-bitfields documentation to indicate that's
>>>>>>> the fallback if the insertion/extraction cannot be done in the declared
>>>>>>> mode, rather than claiming that it tries to do the same thing as if
>>>>>>> -fstrict-volatile-bitfields were not enabled at all.
>>>>>
>>>>>
>>>>> I decided that this approach was more expedient, after all.
>>>>>
>>>>> I've tested this patch (in conjunction with my already-approved but
>>>>> not-yet-applied patch) on mainline for arm-none-eabi, x86_64-linux-gnu, and
>>>>> mips-linux gnu. I also backported the entire series to GCC 4.8 and tested
>>>>> there on arm-none-eabi and x86_64-linux-gnu. OK to apply?
>>>>
>>>> Hm, I can't seem to find the context for
>>>>
>>>> @@ -923,6 +935,14 @@
>>>> store_fixed_bit_field (str_rtx, bitsize, bitnum, 0, 0, value);
>>>> return;
>>>> }
>>>> + else if (MEM_P (str_rtx)
>>>> + && MEM_VOLATILE_P (str_rtx)
>>>> + && flag_strict_volatile_bitfields> 0)
>>>> + /* This is a case where -fstrict-volatile-bitfields doesn't apply
>>>> + because we can't do a single access in the declared mode of the field.
>>>> + Since the incoming STR_RTX has already been adjusted to that mode,
>>>> + fall back to word mode for subsequent logic. */
>>>> + str_rtx = adjust_address (str_rtx, word_mode, 0);
>>>>
>>>> /* 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
>>>>
>>>> and the other similar hunk. I suppose they apply to earlier patches
>>>> in the series? I suppose the above applies to store_bit_field (diff -p
>>>> really helps!). Why would using word_mode be any good as
>>>> fallback? That is, why is "Since the incoming STR_RTX has already
>>>> been adjusted to that mode" not the thing to fix?
>>>>
>>>
>>> Well, this hunk does not force the access to be in word_mode.
>>>
>>> Instead it allows get_best_mode to choose the access to be in any mode from
>>> QI to word_mode.
>>>
>>> It is there to revert the effect of this weird code in expr.c (expand_assigment):
>>>
>>> if (volatilep && flag_strict_volatile_bitfields> 0)
>>> to_rtx = adjust_address (to_rtx, mode1, 0);
>>>
>>> Note that this does not even check if the access is on a bit-field !
>>
>> Then why not remove that ...
>>
>>> The problem with the strict_volatile_bitfields is that it is used already
>>> before the code reaches store_bit_field or extract_bit_field.
>>>
>>> It starts in get_inner_reference, (which is not only used in expand_assignment
>>> and expand_expr_real_1)
>>>
>>> Then this,
>>>
>>> if (volatilep && flag_strict_volatile_bitfields> 0)
>>> op0 = adjust_address (op0, mode1, 0);
>>
>> ... and this ...
>>
>>> and then this,
>>>
>>> /* If the field is volatile, we always want an aligned
>>> access. Do this in following two situations:
>>> 1. the access is not already naturally
>>> aligned, otherwise "normal" (non-bitfield) volatile fields
>>> become non-addressable.
>>> 2. the bitsize is narrower than the access size. Need
>>> to extract bitfields from the access. */
>>> || (volatilep && flag_strict_volatile_bitfields> 0
>>> && (bitpos % GET_MODE_ALIGNMENT (mode) != 0
>>> || (mode1 != BLKmode
>>> && bitsize < GET_MODE_SIZE (mode1) * BITS_PER_UNIT)))
>>
>> ... or this ...
>>
>>> As a result, a read access to an unaligned volatile data member does
>>> not even reach the expand_bit_field if flag_strict_volatile_bitfields <= 0,
>>> and instead goes through convert_move (target, op0, unsignedp).
>>>
>>> I still believe the proposed patch is guaranteed to not change anything if
>>> -fno-strict-volatile-bitfields is used, and even if we can not guarantee
>>> that it creates exactly the same code for cases where the strict-volatile-bitfields
>>> does not apply, it certainly generates valid code, where we had invalid code,
>>> or ICEs without the patch.
>>>
>>> OK for trunk?
>>
>> Again, most of the patch is ok (and nice), the
>> store_bit_field/extract_bit_field changes
>> point to the above issues which we should rather fix than trying
>> to revert them after the fact.
>>
>> Why is that not possible?
>>
>> That said,
>>
>> + else if (MEM_P (str_rtx)
>> + && MEM_VOLATILE_P (str_rtx)
>> + && flag_strict_volatile_bitfields> 0)
>> + /* This is a case where -fstrict-volatile-bitfields doesn't apply
>> + because we can't do a single access in the declared mode of the field.
>> + Since the incoming STR_RTX has already been adjusted to that mode,
>> + fall back to word mode for subsequent logic. */
>> + str_rtx = adjust_address (str_rtx, word_mode, 0);
>>
>> we are looking at an access with bitsize / bitregion properties so plainly
>> choosing word_mode here looks wrong to me. Yes, only
>> -fstrict-volatile-bitfields is affected but still ...
>>
>> The patch is ok if you look at the above as followup.
>>
>> Thanks,
>> Richard.
>>
>>
>
> hmm...
> the above change is just not aggressive enough.
>
>
> with a slightly changed test case I have now got a
> recursion on the extract_fixed_bit_fields on ARM but
> interestingly not on powerpc:
>
> #include <stdlib.h>
>
> typedef struct {
>   char pad;
>   int arr[0];
> } __attribute__((packed)) str;
>
> str *
> foo (int* src)
> {
>   str *s = malloc (sizeof (str) + sizeof (int));
>   *src = s->arr[0];
>   s->arr[0] = 0x12345678;
>   return s;
> }
>
> Now I think about reverting that hunk back to what I had in mind initially:
>
>   else if (MEM_P (str_rtx)
>            && GET_MODE_BITSIZE (GET_MODE (str_rtx)) != 0
>            && GET_MODE_BITSIZE (GET_MODE (str_rtx)) < GET_MODE_BITSIZE (word_mode)
>            && bitnum % GET_MODE_BITSIZE (GET_MODE (str_rtx)) + bitsize
>              > GET_MODE_BITSIZE (GET_MODE (str_rtx)))
>     /* If the mode of str_rtx is too small or unaligned
>        fall back to word mode for subsequent logic.  */
>     str_rtx = adjust_address (str_rtx, word_mode, 0);
>
> this fixes the recursion on the read side too. And it is limited to cases
> where the mode does not match the bitnum and bitsize parameters.

But that's not conditional on -fstrict-volatile-bitfields and frankly it doesn't
make sense to me.  Why is the recursion not there for
-fno-strict-volatile-bitfields?

Richard.

>
> Bernd.
>
>
>>
>>> Bernd.
>>>
>>>> Richard.
>>>>
>>>>> -Sandra
Bernd Edlinger - Nov. 15, 2013, 12:08 p.m.
>>
>> hmm...
>> the above change is just not aggressive enough.
>>
>>
>> with a slightly changed test case I have now got a
>> recursion on the extract_fixed_bit_fields on ARM but
>> interestingly not on powerpc:
>>
>> #include <stdlib.h>
>>
>> typedef struct {
>> char pad;
>> int arr[0];
>> } __attribute__((packed)) str;
>>
>> str *
>> foo (int* src)
>> {
>> str *s = malloc (sizeof (str) + sizeof (int));
>> *src = s->arr[0];
>> s->arr[0] = 0x12345678;
>> return s;
>> }
>>
>> Now I think about reverting that hunk back to what I had in mind initially:
>>
>> else if (MEM_P (str_rtx)
>> && GET_MODE_BITSIZE (GET_MODE (str_rtx)) != 0
>> && GET_MODE_BITSIZE (GET_MODE (str_rtx)) < GET_MODE_BITSIZE (word_mode)
>> && bitnum % GET_MODE_BITSIZE (GET_MODE (str_rtx)) + bitsize
>>> GET_MODE_BITSIZE (GET_MODE (str_rtx)))
>> /* If the mode of str_rtx is too small or unaligned
>> fall back to word mode for subsequent logic. */
>> str_rtx = adjust_address (str_rtx, word_mode, 0);
>>
>> this fixes the recursion on the read side too. And it is limited to cases
>> where the mode does not match the bitnum and bitsize parameters.
>
> But that's not conditional on -fstrict-volatile-bitfields and frankly it doesn't
> make sense to me. Why is the recursion not there for
> -fno-strict-volatile-bitfields?
>

the problem here is different, than we thought. Both recursions have been
observed initially only with volatile accesses and -fstrict-volatile-bitfields.
That's why it looked like the right thing to do. But PR59134 shows clearly
that both recursions have nothing to do with strict-volatile-bitfields. They
happen just because the hardware is not always able to access unaligned data
on one instruction. And the mode in str_rtx is sometimes too restrictive.

Now in this test case, we have s, a packed structure, but malloc retruns a
BIGGEST_ALIGNMENT. And at -O2 the back-end sees the larger alignment.
But the mode of str_rtx is QImode, because that is the mode of the struct str.
This mode is only good for accessing pad, but not for arr[0].
It does never make sense to access 32 bits in QImode, especially if the memory
is un-aligned. That is how this hunk resolves the issue. 

> Richard.
>
>>
>> Bernd.
>>
>>
>>>
>>>> Bernd.
>>>>
>>>>> Richard.
>>>>>
>>>>>> -Sandra
Richard Guenther - Nov. 15, 2013, 12:32 p.m.
On Fri, Nov 15, 2013 at 1:08 PM, Bernd Edlinger
<bernd.edlinger@hotmail.de> wrote:
>>>
>>> hmm...
>>> the above change is just not aggressive enough.
>>>
>>>
>>> with a slightly changed test case I have now got a
>>> recursion on the extract_fixed_bit_fields on ARM but
>>> interestingly not on powerpc:
>>>
>>> #include <stdlib.h>
>>>
>>> typedef struct {
>>> char pad;
>>> int arr[0];
>>> } __attribute__((packed)) str;
>>>
>>> str *
>>> foo (int* src)
>>> {
>>> str *s = malloc (sizeof (str) + sizeof (int));
>>> *src = s->arr[0];
>>> s->arr[0] = 0x12345678;
>>> return s;
>>> }
>>>
>>> Now I think about reverting that hunk back to what I had in mind initially:
>>>
>>> else if (MEM_P (str_rtx)
>>> && GET_MODE_BITSIZE (GET_MODE (str_rtx)) != 0
>>> && GET_MODE_BITSIZE (GET_MODE (str_rtx)) < GET_MODE_BITSIZE (word_mode)
>>> && bitnum % GET_MODE_BITSIZE (GET_MODE (str_rtx)) + bitsize
>>>> GET_MODE_BITSIZE (GET_MODE (str_rtx)))
>>> /* If the mode of str_rtx is too small or unaligned
>>> fall back to word mode for subsequent logic. */
>>> str_rtx = adjust_address (str_rtx, word_mode, 0);
>>>
>>> this fixes the recursion on the read side too. And it is limited to cases
>>> where the mode does not match the bitnum and bitsize parameters.
>>
>> But that's not conditional on -fstrict-volatile-bitfields and frankly it doesn't
>> make sense to me. Why is the recursion not there for
>> -fno-strict-volatile-bitfields?
>>
>
> the problem here is different, than we thought. Both recursions have been
> observed initially only with volatile accesses and -fstrict-volatile-bitfields.
> That's why it looked like the right thing to do. But PR59134 shows clearly
> that both recursions have nothing to do with strict-volatile-bitfields. They
> happen just because the hardware is not always able to access unaligned data
> on one instruction. And the mode in str_rtx is sometimes too restrictive.
>
> Now in this test case, we have s, a packed structure, but malloc retruns a
> BIGGEST_ALIGNMENT. And at -O2 the back-end sees the larger alignment.
> But the mode of str_rtx is QImode, because that is the mode of the struct str.
> This mode is only good for accessing pad, but not for arr[0].
> It does never make sense to access 32 bits in QImode, especially if the memory
> is un-aligned. That is how this hunk resolves the issue.

But then why is the mode QImode in the first place?  The access is
definitely of SImode.

Richard.

>> Richard.
>>
>>>
>>> Bernd.
>>>
>>>
>>>>
>>>>> Bernd.
>>>>>
>>>>>> Richard.
>>>>>>
>>>>>>> -Sandra
Bernd Edlinger - Nov. 15, 2013, 1:24 p.m.
>
> But then why is the mode QImode in the first place? The access is
> definitely of SImode.
>

that's in the test case:

  s->arr[0] = 0x12345678;


it is QImode from that in expand_assignment:

      to_rtx = expand_expr (tem, NULL_RTX, VOIDmode, EXPAND_WRITE);

tem is "s", a MEM_REF, of QImode, perfectly aligned. the mode is only
OK to access *s or s->pad. It is wrong for s->srr[0].
in store_bit_field the mode is used in store_fixed_bit_field:

      mode = GET_MODE (op0);
      if (GET_MODE_BITSIZE (mode) == 0
          || GET_MODE_BITSIZE (mode)> GET_MODE_BITSIZE (word_mode))
        mode = word_mode;
      mode = get_best_mode (bitsize, bitnum, bitregion_start, bitregion_end,
                            MEM_ALIGN (op0), mode, MEM_VOLATILE_P (op0));

      if (mode == VOIDmode)
        goto boom;

so this restricts the possible access mode. word_mode, means no restriction.
Everything would be OK if MEM_ALIGN(op0) was byte-aligned, but we have
a problem if MEM_ALIGN(op0)>=WORD_MODE.

Do you understand?

Bernd.
Richard Guenther - Nov. 15, 2013, 2:10 p.m.
On Fri, Nov 15, 2013 at 2:24 PM, Bernd Edlinger
<bernd.edlinger@hotmail.de> wrote:
>>
>> But then why is the mode QImode in the first place? The access is
>> definitely of SImode.
>>
>
> that's in the test case:
>
>   s->arr[0] = 0x12345678;
>
>
> it is QImode from that in expand_assignment:
>
>       to_rtx = expand_expr (tem, NULL_RTX, VOIDmode, EXPAND_WRITE);
>
> tem is "s", a MEM_REF, of QImode, perfectly aligned. the mode is only
> OK to access *s or s->pad. It is wrong for s->srr[0].

I undestand that, but why isn't that immediately adjusted to use
TYPE_MODE (TREE_TYPE (to))?  The above expands *s, not s->arr[0].
Using the mode of *s if it is not equal to that of the destination doesn't
make any sense (usually it will be BLKmode anyway).  This seems to
be the source of multiple problems.

> in store_bit_field the mode is used in store_fixed_bit_field:
>
>       mode = GET_MODE (op0);
>       if (GET_MODE_BITSIZE (mode) == 0
>           || GET_MODE_BITSIZE (mode)> GET_MODE_BITSIZE (word_mode))
>         mode = word_mode;
>       mode = get_best_mode (bitsize, bitnum, bitregion_start, bitregion_end,
>                             MEM_ALIGN (op0), mode, MEM_VOLATILE_P (op0));
>
>       if (mode == VOIDmode)
>         goto boom;
>
> so this restricts the possible access mode. word_mode, means no restriction.
> Everything would be OK if MEM_ALIGN(op0) was byte-aligned, but we have
> a problem if MEM_ALIGN(op0)>=WORD_MODE.
>
> Do you understand?
>
> Bernd.
Bernd Edlinger - Nov. 18, 2013, 12:11 p.m.
Hi,


This modified test case exposes a bug in the already approved part of the strict-volatile-bitfields patch:

#include <stdlib.h>

typedef struct {
  char pad;
  int arr[0];
} __attribute__((packed)) str;

str *
foo (int* src)
{
  str *s = malloc (sizeof (str) + sizeof (int));
  s->arr[0] = 0x12345678;
  asm volatile("":::"memory");
  *src = s->arr[0];
  return s;
}


As we know this test case triggered a recursion in the store_bit_field on ARM and on PowerPC,
which is no longer reproducible after this patch is applied: http://gcc.gnu.org/ml/gcc-patches/2013-11/msg02025.html

Additionally it triggered a recursion on extract_bit_field, but _only_ on my local copy of the trunk.
I had this patch installed, but did not expect it to change anything unless the values are volatile.
That was cased by this hunk in the strict-volatile-bitfields v4 patch:


@@ -1691,45 +1736,19 @@ extract_fixed_bit_field (enum machine_mo
         includes the entire field.  If such a mode would be larger than
         a word, we won't be doing the extraction the normal way.  */

-      if (MEM_VOLATILE_P (op0)
-         && flag_strict_volatile_bitfields> 0)
-       {
-         if (GET_MODE_BITSIZE (GET_MODE (op0))> 0)
-           mode = GET_MODE (op0);
-         else if (target && GET_MODE_BITSIZE (GET_MODE (target))> 0)
-           mode = GET_MODE (target);
-         else
-           mode = tmode;
-       }
-      else
-       mode = get_best_mode (bitsize, bitnum, 0, 0,
-                             MEM_ALIGN (op0), word_mode, MEM_VOLATILE_P (op0));
+      mode = GET_MODE (op0);
+      if (GET_MODE_BITSIZE (mode) == 0
+         || GET_MODE_BITSIZE (mode)> GET_MODE_BITSIZE (word_mode))
+       mode = word_mode;
+      mode = get_best_mode (bitsize, bitnum, 0, 0,
+                           MEM_ALIGN (op0), mode, MEM_VOLATILE_P (op0));

       if (mode == VOIDmode)
        /* The only way this should occur is if the field spans word
           boundaries.  */
        return extract_split_bit_field (op0, bitsize, bitnum, unsignedp);

So the problem started, because initially this function did not look at GET_MODE(op0)
and always used word_mode. That was changed, but now also affected non-volatile data.


Now, if we solve this differently and install the C++ memory model patch,
we can avoid to introduce the recursion in the extract path,
and remove these two hunks in the update patch at the same time:

+  else if (MEM_P (str_rtx)
+          && MEM_VOLATILE_P (str_rtx)
+          && flag_strict_volatile_bitfields> 0)
+    /* This is a case where -fstrict-volatile-bitfields doesn't apply
+       because we can't do a single access in the declared mode of the field.
+       Since the incoming STR_RTX has already been adjusted to that mode,
+       fall back to word mode for subsequent logic.  */
+    str_rtx = adjust_address (str_rtx, word_mode, 0);



Attached you'll find a new version of the bitfields-update patch,
it is again relative to the already approved version of the volatile-bitfields patch v4, part 1/2.

Boot-strapped and regression-tested on X86_64-linux-gnu.
additionally tested with an ARM cross-compiler.


OK for trunk?


Thanks
Bernd.
2013-11-18  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:
2013-11-18  Bernd Edlinger  <bernd.edlinger@hotmail.de>
	    Sandra Loosemore  <sandra@codesourcery.com>

	* gcc.dg/pr23623.c: Update to test interaction with C++
	memory model.
Richard Guenther - Dec. 2, 2013, 2:58 p.m.
On Mon, Nov 18, 2013 at 1:11 PM, Bernd Edlinger
<bernd.edlinger@hotmail.de> wrote:
> Hi,
>
>
> This modified test case exposes a bug in the already approved part of the strict-volatile-bitfields patch:
>
> #include <stdlib.h>
>
> typedef struct {
>   char pad;
>   int arr[0];
> } __attribute__((packed)) str;
>
> str *
> foo (int* src)
> {
>   str *s = malloc (sizeof (str) + sizeof (int));
>   s->arr[0] = 0x12345678;
>   asm volatile("":::"memory");
>   *src = s->arr[0];
>   return s;
> }
>
>
> As we know this test case triggered a recursion in the store_bit_field on ARM and on PowerPC,
> which is no longer reproducible after this patch is applied: http://gcc.gnu.org/ml/gcc-patches/2013-11/msg02025.html
>
> Additionally it triggered a recursion on extract_bit_field, but _only_ on my local copy of the trunk.
> I had this patch installed, but did not expect it to change anything unless the values are volatile.
> That was cased by this hunk in the strict-volatile-bitfields v4 patch:
>
>
> @@ -1691,45 +1736,19 @@ extract_fixed_bit_field (enum machine_mo
>          includes the entire field.  If such a mode would be larger than
>          a word, we won't be doing the extraction the normal way.  */
>
> -      if (MEM_VOLATILE_P (op0)
> -         && flag_strict_volatile_bitfields> 0)
> -       {
> -         if (GET_MODE_BITSIZE (GET_MODE (op0))> 0)
> -           mode = GET_MODE (op0);
> -         else if (target && GET_MODE_BITSIZE (GET_MODE (target))> 0)
> -           mode = GET_MODE (target);
> -         else
> -           mode = tmode;
> -       }
> -      else
> -       mode = get_best_mode (bitsize, bitnum, 0, 0,
> -                             MEM_ALIGN (op0), word_mode, MEM_VOLATILE_P (op0));
> +      mode = GET_MODE (op0);
> +      if (GET_MODE_BITSIZE (mode) == 0
> +         || GET_MODE_BITSIZE (mode)> GET_MODE_BITSIZE (word_mode))
> +       mode = word_mode;
> +      mode = get_best_mode (bitsize, bitnum, 0, 0,
> +                           MEM_ALIGN (op0), mode, MEM_VOLATILE_P (op0));
>
>        if (mode == VOIDmode)
>         /* The only way this should occur is if the field spans word
>            boundaries.  */
>         return extract_split_bit_field (op0, bitsize, bitnum, unsignedp);
>
> So the problem started, because initially this function did not look at GET_MODE(op0)
> and always used word_mode. That was changed, but now also affected non-volatile data.
>
>
> Now, if we solve this differently and install the C++ memory model patch,
> we can avoid to introduce the recursion in the extract path,
> and remove these two hunks in the update patch at the same time:
>
> +  else if (MEM_P (str_rtx)
> +          && MEM_VOLATILE_P (str_rtx)
> +          && flag_strict_volatile_bitfields> 0)
> +    /* This is a case where -fstrict-volatile-bitfields doesn't apply
> +       because we can't do a single access in the declared mode of the field.
> +       Since the incoming STR_RTX has already been adjusted to that mode,
> +       fall back to word mode for subsequent logic.  */
> +    str_rtx = adjust_address (str_rtx, word_mode, 0);
>
>
>
> Attached you'll find a new version of the bitfields-update patch,
> it is again relative to the already approved version of the volatile-bitfields patch v4, part 1/2.
>
> Boot-strapped and regression-tested on X86_64-linux-gnu.
> additionally tested with an ARM cross-compiler.
>
>
> OK for trunk?

Ok.

Thanks,
Richard.

>
> Thanks
> Bernd.

Patch

diff -u gcc/doc/invoke.texi gcc/doc/invoke.texi
--- gcc/doc/invoke.texi	(working copy)
+++ gcc/doc/invoke.texi	(working copy)
@@ -21659,7 +21659,8 @@ 
 In some cases, such as when the @code{packed} attribute is applied to a 
 structure field, it may not be possible to access the field with a single
 read or write that is correctly aligned for the target machine.  In this
-case GCC falls back to generating multiple accesses rather than code that 
+case GCC falls back to generating either multiple accesses
+or an access in a larger mode, rather than code that 
 will fault or truncate the result at run time.
 
 The default value of this option is determined by the application binary
diff -u gcc/testsuite/gcc.dg/pr23623.c gcc/testsuite/gcc.dg/pr23623.c
--- gcc/testsuite/gcc.dg/pr23623.c	(revision 0)
+++ gcc/testsuite/gcc.dg/pr23623.c	(revision 0)
@@ -8,16 +8,19 @@ 
 extern struct
 {
   unsigned int b : 1;
+  unsigned int : 31;
 } bf1;
 
 extern volatile struct
 {
   unsigned int b : 1;
+  unsigned int : 31;
 } bf2;
 
 extern struct
 {
   volatile unsigned int b : 1;
+  volatile unsigned int : 31;
 } bf3;
 
 void writeb(void)
diff -u gcc/expmed.c gcc/expmed.c
--- gcc/expmed.c	(working copy)
+++ gcc/expmed.c	(working copy)
@@ -416,12 +416,17 @@ 
 }
 
 /* Return true if -fstrict-volatile-bitfields applies an access of OP0
-   containing BITSIZE bits starting at BITNUM, with field mode FIELDMODE.  */
+   containing BITSIZE bits starting at BITNUM, with field mode FIELDMODE.
+   Return false if the access would touch memory outside the range
+   BITREGION_START to BITREGION_END for conformance to the C++ memory
+   model.  */
 
 static bool
 strict_volatile_bitfield_p (rtx op0, unsigned HOST_WIDE_INT bitsize,
 			    unsigned HOST_WIDE_INT bitnum,
-			    enum machine_mode fieldmode)
+			    enum machine_mode fieldmode,
+			    unsigned HOST_WIDE_INT bitregion_start,
+			    unsigned HOST_WIDE_INT bitregion_end)
 {
   unsigned HOST_WIDE_INT modesize = GET_MODE_BITSIZE (fieldmode);
 
@@ -448,6 +453,12 @@ 
 	  && bitnum % GET_MODE_ALIGNMENT (fieldmode) + bitsize > modesize))
     return false;
 
+  /* Check for cases where the C++ memory model applies.  */
+  if (bitregion_end != 0
+      && (bitnum - bitnum % modesize < bitregion_start
+	  || bitnum - bitnum % modesize + modesize > bitregion_end))
+    return false;
+
   return true;
 }
 
@@ -904,7 +915,8 @@ 
 		 rtx value)
 {
   /* Handle -fstrict-volatile-bitfields in the cases where it applies.  */
-  if (strict_volatile_bitfield_p (str_rtx, bitsize, bitnum, fieldmode))
+  if (strict_volatile_bitfield_p (str_rtx, bitsize, bitnum, fieldmode,
+				  bitregion_start, bitregion_end))
     {
 
       /* Storing any naturally aligned field can be done with a simple
@@ -923,6 +935,14 @@ 
 	store_fixed_bit_field (str_rtx, bitsize, bitnum, 0, 0, value);
       return;
     }
+  else if (MEM_P (str_rtx)
+	   && MEM_VOLATILE_P (str_rtx)
+	   && flag_strict_volatile_bitfields > 0)
+    /* This is a case where -fstrict-volatile-bitfields doesn't apply
+       because we can't do a single access in the declared mode of the field.
+       Since the incoming STR_RTX has already been adjusted to that mode,
+       fall back to word mode for subsequent logic.  */
+    str_rtx = adjust_address (str_rtx, word_mode, 0);
 
   /* 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
@@ -1695,7 +1715,7 @@ 
   else
     mode1 = tmode;
 
-  if (strict_volatile_bitfield_p (str_rtx, bitsize, bitnum, mode1))
+  if (strict_volatile_bitfield_p (str_rtx, bitsize, bitnum, mode1, 0, 0))
     {
       rtx result;
 
@@ -1709,6 +1729,14 @@ 
 					  target, unsignedp);
       return convert_extracted_bit_field (result, mode, tmode, unsignedp);
     }
+  else if (MEM_P (str_rtx)
+	   && MEM_VOLATILE_P (str_rtx)
+	   && flag_strict_volatile_bitfields > 0)
+    /* This is a case where -fstrict-volatile-bitfields doesn't apply
+       because we can't do a single access in the declared mode of the field.
+       Since the incoming STR_RTX has already been adjusted to that mode,
+       fall back to word mode for subsequent logic.  */
+    str_rtx = adjust_address (str_rtx, word_mode, 0);
   
   return extract_bit_field_1 (str_rtx, bitsize, bitnum, unsignedp,
 			      target, mode, tmode, true);