Patchwork [3/5] don't restrict bit range for -fstrict-volatile-bitfields

login
register
mail settings
Submitter Sandra Loosemore
Date June 16, 2013, 7:08 p.m.
Message ID <51BE0D1C.9070404@codesourcery.com>
Download mbox | patch
Permalink /patch/251735/
State New
Headers show

Comments

Sandra Loosemore - June 16, 2013, 7:08 p.m.
This patch fixes the PR23623 regression.  In conjunction with part 2 of 
the series, it also fixes the new volatile-bitfields-3.c test case.

As I noted in previous discussion, there might be a better place to 
accomplish this effect, but hacking DECL_BIT_FIELD_REPRESENTATIVE can't 
work because the volatile-ness may be coming from a qualifier on the 
pointer or object from which the field is being extracted, rather than 
from a volatile qualifier on the bit field decl.  I think the choices 
are to do it in get_bit_range (as in this patch), in the callers of 
get_bit_range, or at the places where the bit range information is being 
used.

-Sandra
Jakub Jelinek - June 16, 2013, 7:26 p.m.
On Sun, Jun 16, 2013 at 01:08:12PM -0600, Sandra Loosemore wrote:
> This patch fixes the PR23623 regression.  In conjunction with part 2
> of the series, it also fixes the new volatile-bitfields-3.c test
> case.
> 
> As I noted in previous discussion, there might be a better place to
> accomplish this effect, but hacking DECL_BIT_FIELD_REPRESENTATIVE
> can't work because the volatile-ness may be coming from a qualifier
> on the pointer or object from which the field is being extracted,
> rather than from a volatile qualifier on the bit field decl.  I
> think the choices are to do it in get_bit_range (as in this patch),
> in the callers of get_bit_range, or at the places where the bit
> range information is being used.

So does this means you just always violate C++11 memory model requirements
with -fstrict-volatile-bitfields?  That doesn't seem to be a good idea.

> 2013-06-16  Sandra Loosemore  <sandra@codesourcery.com>
> 
> 	PR middle-end/23623
> 
> 	gcc/
> 	* expr.c (get_bit_range): Handle flag_strict_volatile_bitfields.

	Jakub
Richard Guenther - June 17, 2013, 11:12 a.m.
On Sun, Jun 16, 2013 at 9:26 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Sun, Jun 16, 2013 at 01:08:12PM -0600, Sandra Loosemore wrote:
>> This patch fixes the PR23623 regression.  In conjunction with part 2
>> of the series, it also fixes the new volatile-bitfields-3.c test
>> case.
>>
>> As I noted in previous discussion, there might be a better place to
>> accomplish this effect, but hacking DECL_BIT_FIELD_REPRESENTATIVE
>> can't work because the volatile-ness may be coming from a qualifier
>> on the pointer or object from which the field is being extracted,
>> rather than from a volatile qualifier on the bit field decl.  I
>> think the choices are to do it in get_bit_range (as in this patch),
>> in the callers of get_bit_range, or at the places where the bit
>> range information is being used.
>
> So does this means you just always violate C++11 memory model requirements
> with -fstrict-volatile-bitfields?  That doesn't seem to be a good idea.

Yeah, it's not clear to me that this patch fixes anything by design.  It
mainly changes things from limiting the access in some way to not
limiting it at all ...

Richard.

>> 2013-06-16  Sandra Loosemore  <sandra@codesourcery.com>
>>
>>       PR middle-end/23623
>>
>>       gcc/
>>       * expr.c (get_bit_range): Handle flag_strict_volatile_bitfields.
>
>         Jakub
Julian Brown - June 17, 2013, 11:31 a.m.
On Mon, 17 Jun 2013 13:12:09 +0200
Richard Biener <richard.guenther@gmail.com> wrote:

> On Sun, Jun 16, 2013 at 9:26 PM, Jakub Jelinek <jakub@redhat.com>
> wrote:
> > On Sun, Jun 16, 2013 at 01:08:12PM -0600, Sandra Loosemore wrote:
> >> This patch fixes the PR23623 regression.  In conjunction with part
> >> 2 of the series, it also fixes the new volatile-bitfields-3.c test
> >> case.
> >>
> >> As I noted in previous discussion, there might be a better place to
> >> accomplish this effect, but hacking DECL_BIT_FIELD_REPRESENTATIVE
> >> can't work because the volatile-ness may be coming from a qualifier
> >> on the pointer or object from which the field is being extracted,
> >> rather than from a volatile qualifier on the bit field decl.  I
> >> think the choices are to do it in get_bit_range (as in this patch),
> >> in the callers of get_bit_range, or at the places where the bit
> >> range information is being used.
> >
> > So does this means you just always violate C++11 memory model
> > requirements with -fstrict-volatile-bitfields?  That doesn't seem
> > to be a good idea.
> 
> Yeah, it's not clear to me that this patch fixes anything by design.
> It mainly changes things from limiting the access in some way to not
> limiting it at all ...

IIUC, the incompatibility between the specified
-fstrict-volatile-bitfields behaviour and the C++ memory model is a
recognised deficiency in the ARM EABI. It might be an unpopular
suggestion, but how about disabling the option by default for C++ on
ARM [*]? Maybe even with a "sorry" if the user tries to turn it on
manually?

The assumption behind that idea is that people who most care about the
EABI-specified behaviour (to access hardware registers, etc.) are
probably using bare metal and plain C. The downside is the potential
for "surprise" for users though, I suppose.

Julian

[*] Or some other variant target-dependent hack, like depending on
-ansi, bare-metal vs. $operating system, etc....
Richard Guenther - June 17, 2013, 11:38 a.m.
On Mon, Jun 17, 2013 at 1:31 PM, Julian Brown <julian@codesourcery.com> wrote:
> On Mon, 17 Jun 2013 13:12:09 +0200
> Richard Biener <richard.guenther@gmail.com> wrote:
>
>> On Sun, Jun 16, 2013 at 9:26 PM, Jakub Jelinek <jakub@redhat.com>
>> wrote:
>> > On Sun, Jun 16, 2013 at 01:08:12PM -0600, Sandra Loosemore wrote:
>> >> This patch fixes the PR23623 regression.  In conjunction with part
>> >> 2 of the series, it also fixes the new volatile-bitfields-3.c test
>> >> case.
>> >>
>> >> As I noted in previous discussion, there might be a better place to
>> >> accomplish this effect, but hacking DECL_BIT_FIELD_REPRESENTATIVE
>> >> can't work because the volatile-ness may be coming from a qualifier
>> >> on the pointer or object from which the field is being extracted,
>> >> rather than from a volatile qualifier on the bit field decl.  I
>> >> think the choices are to do it in get_bit_range (as in this patch),
>> >> in the callers of get_bit_range, or at the places where the bit
>> >> range information is being used.
>> >
>> > So does this means you just always violate C++11 memory model
>> > requirements with -fstrict-volatile-bitfields?  That doesn't seem
>> > to be a good idea.
>>
>> Yeah, it's not clear to me that this patch fixes anything by design.
>> It mainly changes things from limiting the access in some way to not
>> limiting it at all ...
>
> IIUC, the incompatibility between the specified
> -fstrict-volatile-bitfields behaviour and the C++ memory model is a
> recognised deficiency in the ARM EABI. It might be an unpopular
> suggestion, but how about disabling the option by default for C++ on
> ARM [*]? Maybe even with a "sorry" if the user tries to turn it on
> manually?

How are they incompatible?  As far as I understand the
-fstrict-volatile-bitfields
at most restricts the size of the access further, no?  Can you write down an
example that shows the incompatibility?  (woudl be nice to see that as comment
before the relevant code).

Richard.

> The assumption behind that idea is that people who most care about the
> EABI-specified behaviour (to access hardware registers, etc.) are
> probably using bare metal and plain C. The downside is the potential
> for "surprise" for users though, I suppose.
>
> Julian
>
> [*] Or some other variant target-dependent hack, like depending on
> -ansi, bare-metal vs. $operating system, etc....
Julian Brown - June 17, 2013, 12:27 p.m.
On Mon, 17 Jun 2013 13:38:05 +0200
Richard Biener <richard.guenther@gmail.com> wrote:

> On Mon, Jun 17, 2013 at 1:31 PM, Julian Brown
> <julian@codesourcery.com> wrote:
> > On Mon, 17 Jun 2013 13:12:09 +0200
> > Richard Biener <richard.guenther@gmail.com> wrote:
> >
> >> On Sun, Jun 16, 2013 at 9:26 PM, Jakub Jelinek <jakub@redhat.com>
> >> wrote:
> >> > On Sun, Jun 16, 2013 at 01:08:12PM -0600, Sandra Loosemore wrote:
> >> >> This patch fixes the PR23623 regression.  In conjunction with
> >> >> part 2 of the series, it also fixes the new
> >> >> volatile-bitfields-3.c test case.
> >> >>
> >> >> As I noted in previous discussion, there might be a better
> >> >> place to accomplish this effect, but hacking
> >> >> DECL_BIT_FIELD_REPRESENTATIVE can't work because the
> >> >> volatile-ness may be coming from a qualifier on the pointer or
> >> >> object from which the field is being extracted, rather than
> >> >> from a volatile qualifier on the bit field decl.  I think the
> >> >> choices are to do it in get_bit_range (as in this patch), in
> >> >> the callers of get_bit_range, or at the places where the bit
> >> >> range information is being used.
> >> >
> >> > So does this means you just always violate C++11 memory model
> >> > requirements with -fstrict-volatile-bitfields?  That doesn't seem
> >> > to be a good idea.
> >>
> >> Yeah, it's not clear to me that this patch fixes anything by
> >> design. It mainly changes things from limiting the access in some
> >> way to not limiting it at all ...
> >
> > IIUC, the incompatibility between the specified
> > -fstrict-volatile-bitfields behaviour and the C++ memory model is a
> > recognised deficiency in the ARM EABI. It might be an unpopular
> > suggestion, but how about disabling the option by default for C++ on
> > ARM [*]? Maybe even with a "sorry" if the user tries to turn it on
> > manually?
> 
> How are they incompatible?  As far as I understand the
> -fstrict-volatile-bitfields
> at most restricts the size of the access further, no?  Can you write
> down an example that shows the incompatibility?  (woudl be nice to
> see that as comment before the relevant code).

Well -- I'm certainly no expert on the C++ memory model, but I am under
the impression (that I can't seem to verify by googling ;-)) that
accesses to adjacent bitfields during volatile access of a particular
bitfield are forbidden. So simply, for the following:

struct foo {
  int a : 8;
  int b : 8;
  int c : 16;
};

volatile struct foo x;

void bar (void) { x.b++; }

to satisfy the ARM EABI, 'bar' will access all three of a, b and c
using read-modify-write (using int-sized accesses). IIUC for the C++
memory model, only 'b' may be accessed, e.g. using byte-sized
loads/stores.

I'm quite prepared to be wrong though, in which case sorry for the
noise :-).

Julian
Jakub Jelinek - June 17, 2013, 12:37 p.m.
On Mon, Jun 17, 2013 at 01:27:38PM +0100, Julian Brown wrote:
> Well -- I'm certainly no expert on the C++ memory model, but I am under
> the impression (that I can't seem to verify by googling ;-)) that
> accesses to adjacent bitfields during volatile access of a particular
> bitfield are forbidden. So simply, for the following:
> 
> struct foo {
>   int a : 8;
>   int b : 8;
>   int c : 16;
> };
> 
> volatile struct foo x;
> 
> void bar (void) { x.b++; }

I believe in the above it is ok in C++ memory model if the RMW cycle is
using 32-bit type, but in
struct foo {
  int a : 8;
  int b : 8;
  char c, d;
};
  
volatile struct foo x;

void bar (void) { x.b++; }
it is not (but it is laid out the same), because modification to x.a or x.b
must not create data races on x.c and/or x.d.

	Jakub
Bernd Schmidt - June 17, 2013, 12:38 p.m.
On 06/17/2013 02:27 PM, Julian Brown wrote:
> On Mon, 17 Jun 2013 13:38:05 +0200
> Richard Biener <richard.guenther@gmail.com> wrote:
> 
>> On Mon, Jun 17, 2013 at 1:31 PM, Julian Brown
>> <julian@codesourcery.com> wrote:
>>> IIUC, the incompatibility between the specified
>>> -fstrict-volatile-bitfields behaviour and the C++ memory model is a
>>> recognised deficiency in the ARM EABI. It might be an unpopular
>>> suggestion, but how about disabling the option by default for C++ on
>>> ARM [*]? Maybe even with a "sorry" if the user tries to turn it on
>>> manually?
>>
>> How are they incompatible?  As far as I understand the
>> -fstrict-volatile-bitfields
>> at most restricts the size of the access further, no?  Can you write
>> down an example that shows the incompatibility?  (woudl be nice to
>> see that as comment before the relevant code).
> 
> Well -- I'm certainly no expert on the C++ memory model, but I am under
> the impression (that I can't seem to verify by googling ;-)) that
> accesses to adjacent bitfields during volatile access of a particular
> bitfield are forbidden. So simply, for the following:
> 
> struct foo {
>   int a : 8;
>   int b : 8;
>   int c : 16;
> };
> 
> volatile struct foo x;
> 
> void bar (void) { x.b++; }
> 
> to satisfy the ARM EABI, 'bar' will access all three of a, b and c
> using read-modify-write (using int-sized accesses). IIUC for the C++
> memory model, only 'b' may be accessed, e.g. using byte-sized
> loads/stores.

The one I came up with after reading about the C++ memory model spec is
struct x
{
  int x: 8;
  char : 0;
  short y : 8;
} z;

I interpret the C++ rules to say that due to the zero-sized bitfield, x
and y are different memory locations, and modifying one shouldn't affect
the other. However, with -fstrict-volatile-bitfields, x must be accessed
as an int, so it overlaps y.

> I'm quite prepared to be wrong though, in which case sorry for the
> noise :-).

Same here :)


Bernd
Richard Guenther - June 17, 2013, 12:59 p.m.
On Mon, Jun 17, 2013 at 2:38 PM, Bernd Schmidt <bernds@codesourcery.com> wrote:
> On 06/17/2013 02:27 PM, Julian Brown wrote:
>> On Mon, 17 Jun 2013 13:38:05 +0200
>> Richard Biener <richard.guenther@gmail.com> wrote:
>>
>>> On Mon, Jun 17, 2013 at 1:31 PM, Julian Brown
>>> <julian@codesourcery.com> wrote:
>>>> IIUC, the incompatibility between the specified
>>>> -fstrict-volatile-bitfields behaviour and the C++ memory model is a
>>>> recognised deficiency in the ARM EABI. It might be an unpopular
>>>> suggestion, but how about disabling the option by default for C++ on
>>>> ARM [*]? Maybe even with a "sorry" if the user tries to turn it on
>>>> manually?
>>>
>>> How are they incompatible?  As far as I understand the
>>> -fstrict-volatile-bitfields
>>> at most restricts the size of the access further, no?  Can you write
>>> down an example that shows the incompatibility?  (woudl be nice to
>>> see that as comment before the relevant code).
>>
>> Well -- I'm certainly no expert on the C++ memory model, but I am under
>> the impression (that I can't seem to verify by googling ;-)) that
>> accesses to adjacent bitfields during volatile access of a particular
>> bitfield are forbidden. So simply, for the following:
>>
>> struct foo {
>>   int a : 8;
>>   int b : 8;
>>   int c : 16;
>> };
>>
>> volatile struct foo x;
>>
>> void bar (void) { x.b++; }
>>
>> to satisfy the ARM EABI, 'bar' will access all three of a, b and c
>> using read-modify-write (using int-sized accesses). IIUC for the C++
>> memory model, only 'b' may be accessed, e.g. using byte-sized
>> loads/stores.
>
> The one I came up with after reading about the C++ memory model spec is
> struct x
> {
>   int x: 8;
>   char : 0;
>   short y : 8;
> } z;
>
> I interpret the C++ rules to say that due to the zero-sized bitfield, x
> and y are different memory locations, and modifying one shouldn't affect
> the other.

True.  IIRC the char : 0 isn't even necessary for the limitation to take
place (just the different underlying types is enough).

> However, with -fstrict-volatile-bitfields, x must be accessed
> as an int, so it overlaps y.

Ok, that's a good example then.  Looking at the mode of the FIELD_DECL
for x isn't a good way to query the access mode then, but the mode
of DECL_BIT_FIELD_TYPE would.  Note that for z.x you are not guaranteed
to end up with a COMPONENT_REF at RTL expansion time at all but it
may be lowered to a MEM_REF by arbitrary optimization passes (unlikely
because most simply go away when they see a volatile access), because
it's bitsize is a multiple of BITS_PER_UNIT and it's boundary is byte-aligned.

Richard.

>> I'm quite prepared to be wrong though, in which case sorry for the
>> noise :-).
>
> Same here :)
>
>
> Bernd
>
Joseph S. Myers - June 17, 2013, 2:41 p.m.
On Mon, 17 Jun 2013, Julian Brown wrote:

> IIUC, the incompatibility between the specified
> -fstrict-volatile-bitfields behaviour and the C++ memory model is a
> recognised deficiency in the ARM EABI. It might be an unpopular
> suggestion, but how about disabling the option by default for C++ on
> ARM [*]? Maybe even with a "sorry" if the user tries to turn it on
> manually?

The C11 and C++11 memory models are the same, this is nothing to do with 
C++ as opposed to C.

Someone from ARM will need to answer as to what their plans are to make 
AAPCS compatible with the memory model.  I'd say strict-volatile-bitfields 
should only ever apply in cases where it does not conflict with language 
semantics - so just as "packed" should take precedence, so the memory 
model should also take precedence (in the absence of --param 
allow-store-data-races=1, anyway) in those cases where AAPCS would 
indicate writing to an object outside the maximal sequence of consecutive 
non-zero-width bit-fields.
Sandra Loosemore - June 17, 2013, 4:52 p.m.
On 06/17/2013 08:41 AM, Joseph S. Myers wrote:
> On Mon, 17 Jun 2013, Julian Brown wrote:
>
>> IIUC, the incompatibility between the specified
>> -fstrict-volatile-bitfields behaviour and the C++ memory model is a
>> recognised deficiency in the ARM EABI. It might be an unpopular
>> suggestion, but how about disabling the option by default for C++ on
>> ARM [*]? Maybe even with a "sorry" if the user tries to turn it on
>> manually?
>
> The C11 and C++11 memory models are the same, this is nothing to do with
> C++ as opposed to C.
>
> Someone from ARM will need to answer as to what their plans are to make
> AAPCS compatible with the memory model.  I'd say strict-volatile-bitfields
> should only ever apply in cases where it does not conflict with language
> semantics - so just as "packed" should take precedence, so the memory
> model should also take precedence (in the absence of --param
> allow-store-data-races=1, anyway) in those cases where AAPCS would
> indicate writing to an object outside the maximal sequence of consecutive
> non-zero-width bit-fields.

The problem with always choosing C++ memory model compliance over ABI 
compliance, no matter what the setting of -fstrict-volatile-bitfields, 
is that some users may have legacy code out there where they really want 
the ABI-compliant behavior.  Perhaps we should not make 
-fstrict-volatile-bitfields not be the default on any target any more, 
but if you supply it explicitly it tells GCC that you really want the 
ABI-compliant behavior to override the language-standard-compliant behavior?

I don't think there's an actual conflict there between 
-fstrict-volatile-bitfields and packed structure extensions.  AAPCS 
allows for the possibility of packed structures but says conforming 
programs don't use them for external interfaces (this is the note at the 
end of 7.1.7.1).  As I noted previously, my understanding is that the 
volatile access requirements in the AAPCS only apply to non-packed 
struct fields whose layout and alignment is fully specified by AAPCS. 
The various packed-structure bugs addressed by part 4 of my patch series 
are mostly cases where the required single access *cannot* be emitted 
because the field is not aligned; instead, we're emitting code that is 
totally wrong even under GCC's normal access rules, because it faults at 
runtime or only accesses part of the bit-field.

-Sandra
Sandra Loosemore - June 18, 2013, 12:02 a.m.
Earlier today I wrote:

> On 06/17/2013 08:41 AM, Joseph S. Myers wrote:
>> On Mon, 17 Jun 2013, Julian Brown wrote:
>>
>>> IIUC, the incompatibility between the specified
>>> -fstrict-volatile-bitfields behaviour and the C++ memory model is a
>>> recognised deficiency in the ARM EABI. It might be an unpopular
>>> suggestion, but how about disabling the option by default for C++ on
>>> ARM [*]? Maybe even with a "sorry" if the user tries to turn it on
>>> manually?
>>
>> The C11 and C++11 memory models are the same, this is nothing to do with
>> C++ as opposed to C.
>
> The problem with always choosing C++ memory model compliance over ABI
> compliance, no matter what the setting of -fstrict-volatile-bitfields,
> is that some users may have legacy code out there where they really want
> the ABI-compliant behavior.  Perhaps we should not make
> -fstrict-volatile-bitfields not be the default on any target any more,
> but if you supply it explicitly it tells GCC that you really want the
> ABI-compliant behavior to override the language-standard-compliant
> behavior?

I had another thought:  perhaps -fstrict-volatile-bitfields could remain 
the default on targets where it currently is, but it can be overridden 
by an appropriate -std= option.  Perhaps also GCC could give an error if 
-fstrict-volatile-bitfields is given explicitly with an incompatible 
-std= option.

-Sandra

Patch

Index: gcc/expr.c
===================================================================
--- gcc/expr.c	(revision 199963)
+++ gcc/expr.c	(working copy)
@@ -4508,6 +4508,16 @@  get_bit_range (unsigned HOST_WIDE_INT *b
 
   gcc_assert (TREE_CODE (exp) == COMPONENT_REF);
 
+  /* If -fstrict-volatile-bitfields was given and this is a volatile
+     access, then we must ignore any DECL_BIT_FIELD_REPRESENTATIVE and
+     do the access in the mode of the field.  */
+  if (TREE_THIS_VOLATILE (exp)
+      && flag_strict_volatile_bitfields > 0)
+    {
+      *bitstart = *bitend = 0;
+      return;
+    }
+
   field = TREE_OPERAND (exp, 1);
   repr = DECL_BIT_FIELD_REPRESENTATIVE (field);
   /* If we do not have a DECL_BIT_FIELD_REPRESENTATIVE there is no