diff mbox

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

Message ID 52463D60.8040607@codesourcery.com
State New
Headers show

Commit Message

Sandra Loosemore Sept. 28, 2013, 2:22 a.m. UTC
This patch makes -fstrict-volatile-bitfields not be the default on any 
target.  It is unchanged from part 4 of the previous version (v3) of 
this patch set that I originally posted back in June and have been 
re-pinging many times since then.

Some reviewers of my original patch series argued quite strongly that 
the C/C++ language semantics ought to take precedence over any 
target-specific ABI in cases where they conflict.  I am neutral on this 
change myself, but if it is a requirement for approval of the other part 
of the patch that fixes the wrong-code bugs, I think users on targets 
such as ARM that currently default to enabling this flag would be better 
off specifying the flag explicitly to get code that does what they want, 
than getting broken code by default and no way to tell GCC to DTRT.  :-( 
  And that's the behavior we're stuck with if we do nothing or can't 
reach a consensus about what to do.  :-(

Bernd Edlinger has been working on a followup patch to issue warnings in 
cases where -fstrict-volatile-bitfields behavior conflicts with the new 
C/C++ memory model, which might help to ease the transition in the 
change of defaults.  I believe this was the last version posted:

http://gcc.gnu.org/ml/gcc-patches/2013-09/msg00100.html

-Sandra

Comments

DJ Delorie Sept. 30, 2013, 8:18 p.m. UTC | #1
As per my previous comments on this patch, I will not approve the
changes to the m32c backend, as they will cause real bugs in real
hardware, and violate the hardware's ABI.  The user may use
-fno-strict-volatile-bitfields if they do not desire this behavior and
understand the consequences.

I am not a maintainer for the rx and h8300 ports, but they are in the
same situation.

To reiterate my core position: if the user defines a proper "volatile
int" bitfield, and the compiler does anything other than an int-sized
access, the compiler is WRONG.  Any optimization that changes volatile
accesses to something other than what the user specified is a bug that
needs to be fixed before this option can be non-default.
Bernd Edlinger Oct. 9, 2013, 1:09 a.m. UTC | #2
Hi,

On Mon, 30 Sep 2013 16:18:30, DJ Delorie wrote:
>
> As per my previous comments on this patch, I will not approve the
> changes to the m32c backend, as they will cause real bugs in real
> hardware, and violate the hardware's ABI. The user may use
> -fno-strict-volatile-bitfields if they do not desire this behavior and
> understand the consequences.
>
> I am not a maintainer for the rx and h8300 ports, but they are in the
> same situation.
>
> To reiterate my core position: if the user defines a proper "volatile
> int" bitfield, and the compiler does anything other than an int-sized
> access, the compiler is WRONG. Any optimization that changes volatile
> accesses to something other than what the user specified is a bug that
> needs to be fixed before this option can be non-default.

hmm, I just tried to use the latest 4.9 trunk to compile the example from
the AAPCS document:

struct s
{
  volatile int a:8;
  volatile char b:2;
};

struct s ss;

int
main ()
{
  ss.a=1;
  ss.b=1;
  return 0;
}

and the resulting code is completely against the written AAPCS specification:

main:
        @ Function supports interworking.
        @ args = 0, pretend = 0, frame = 0
        @ frame_needed = 0, uses_anonymous_args = 0
        @ link register save eliminated.
        ldr     r3, .L2
        ldrh    r2, [r3]
        bic     r2, r2, #254
        orr     r2, r2, #1
        strh    r2, [r3]        @ movhi
        ldrh    r2, [r3]
        bic     r2, r2, #512
        orr     r2, r2, #256
        strh    r2, [r3]        @ movhi
        mov     r0, #0
        bx      lr

two half-word accesses, to my total surprise!

As it looks like, the -fstrict-volatile-bitfields are already totally broken,
apparently in favour of the C++ memory model, at least at the write-side.

These are aligned accesses, not the packed structures, that was the
only case where it used to work once.

This must be fixed. I do not understand why we cannot agree, that
at least the bug-fix part of Sandra's patch needs to be applied.

Regards
Bernd.
DJ Delorie Oct. 16, 2013, 11:46 p.m. UTC | #3
> As it looks like, the -fstrict-volatile-bitfields are already totally broken,

I tested your example on rl78-elf, with and without
-fstrict-volatile-bitfields, and it generates correct code with it and
incorrect code without it.  Same for m32c-elf.  Same for h8300-elf.
Seems to be working OK for me.

Either way, if -fstrict-volatile-bitfields does not do what it's
supposed to do, the correct action is to fix it - not to disable it on
targets that rely on it for correct operation.
Sandra Loosemore Oct. 17, 2013, 2:16 a.m. UTC | #4
On 10/16/2013 05:46 PM, DJ Delorie wrote:
>
>> As it looks like, the -fstrict-volatile-bitfields are already totally broken,
>
> I tested your example on rl78-elf, with and without
> -fstrict-volatile-bitfields, and it generates correct code with it and
> incorrect code without it.  Same for m32c-elf.  Same for h8300-elf.
> Seems to be working OK for me.

I'm curious; do all the test cases included in
http://gcc.gnu.org/ml/gcc-patches/2013-09/msg02058.html
work for you on those targets as well (without applying the rest of the 
patch)?

> Either way, if -fstrict-volatile-bitfields does not do what it's
> supposed to do, the correct action is to fix it - not to disable it on
> targets that rely on it for correct operation.

Unfortunately, "fixing" -fstrict-volatile-bitfields to conform to the 
AAPCS breaks conformance with the C11/C++11 standards, and we seem to be 
at an impasse where global reviewers refuse to consider anything that 
breaks language standard conformance and target maintainers refuse to 
consider anything that breaks ABI conformance.  Meanwhile, while we're 
bickering about what the default should be, ARM users (in particular) 
are stuck in a situation where GCC's default behavior is to generate 
unaligned accesses that fault at runtime or wrong code that silently 
truncates bitfields, and there is *no* command-line option or even 
build-time option they can provide to make GCC generate code that is 
correct according to the AAPCS.

FWIW, I don't care much myself in an abstract way about whether language 
standard conformance or ABI conformance should rule here.  But, looking 
at it practically, I think the battle over "correctness" was already 
over when the C and C++ standards committees voted to specify this 
behavior in a particular way, and that in time ABIs that specify some 
other behavior are going to be either revised or considered archaic or 
obsolete.

Anyway, at this point I've given up on thinking either side is going to 
convince the other in this debate, and I haven't seen much evidence that 
either side would be willing to compromise, either (e.g., Bernd's patch 
to add warnings in cases where the behavior differs, or my earlier 
proposal to make the defaults depend on the selected language standard 
as well as the ABI, don't seem to have placated either side).  About the 
only way forward I can see is if the GCC steering committee steps in 
with a general policy statement about what to do when language standards 
conflict with a target ABI.  And, of course, the patch itself still 
needs review....

-Sandra, who is feeling pretty discouraged about how hard it is to 
contribute to GCC :-(
DJ Delorie Oct. 17, 2013, 2:50 a.m. UTC | #5
> I'm curious; do all the test cases included in
> http://gcc.gnu.org/ml/gcc-patches/2013-09/msg02058.html
> work for you on those targets as well (without applying the rest of the 
> patch)?

Not all of them can work, because they describe something that can't
be done in hardware.  For example, the first test has an incomplete
bitfield - the fields do not completely describe an "int" so the
structure is smaller (one byte, according to sizeof()) than the access
type (2-8 bytes).

Looking through the tests, most of them combine "packed" with
mismatched types.  IMHO, those tests are invalid.

> > Either way, if -fstrict-volatile-bitfields does not do what it's
> > supposed to do, the correct action is to fix it - not to disable it on
> > targets that rely on it for correct operation.
> 
> Unfortunately, "fixing" -fstrict-volatile-bitfields to conform to the 
> AAPCS breaks conformance with the C11/C++11 standards,

The global default is -fno-strict-volatile-bitfields.  The flag was
added specifically to force the compiler to always use the
user-specified type when accessing bitfields, at least when it was
physically possible.  I don't see why there's a problem.  Without the
flag, you follow the standard.  With the flag, you do what the user
told you to do.  It's up to the targets to decide which way the flag
defaults if the user doesn't specify.  For my targets, the default
must be to do what the user told you to do, regardless of the
standards, because the standards lead to hardware failure.

> Meanwhile, while we're bickering about what the default should be,
> ARM users (in particular) are stuck in a situation where GCC's
> default behavior is to generate unaligned accesses that fault at
> runtime or wrong code that silently truncates bitfields,

Note that if the user defines a structure that intentionally mixes
types in a hardware word, the compiler can't "do what it's told".
It's still up to the user to write well-defined structures in those
cases.  Mixing packed, incomplete bitfields, and multiple types,
(i.e. pr48784-1.c in that patch) is an example of a case where gcc
can't do what the user asked, because what the user asked is nonsense.

> and there is *no* command-line option or even build-time option they
> can provide to make GCC generate code that is correct according to
> the AAPCS.

At the time I added -fstrict-volatile-bitfields, the ARM maintainers
seemed to agree that the new functionality was what they needed.  So,
-fstrict-volatile-bitfields *is* that command line option, it's just
either broken for them, or they're trying to do something not
physically possible.

> But, looking at it practically, I think the battle over
> "correctness" was already over when the C and C++ standards
> committees voted to specify this behavior in a particular way, and
> that in time ABIs that specify some other behavior are going to be
> either revised or considered archaic or obsolete.

My problem isn't some arbitrary ABI, it's a hardware peripheral
register that just can't be accessed in the wrong mode, because the
peripheral won't work if you do that.  Most MCU peripherals work this
way.  Just reading a byte or word outside the intended one could have
real consequences in the hardware.

> About the only way forward I can see is if the GCC steering
> committee steps in with a general policy statement about what to do
> when language standards conflict with a target ABI.

I've not objected to fixing -fstrict-volatile-bitfields, or making the
-fno-strict-volatile-bitfields case match the standard.  I've only
objected to breaking my targets by making that flag not the default.
Bernd Edlinger Oct. 17, 2013, 1:04 p.m. UTC | #6
On Wed, 16 Oct 2013 22:50:20, DJ Delorie wrote:
> Not all of them can work, because they describe something that can't
> be done in hardware. For example, the first test has an incomplete
> bitfield - the fields do not completely describe an "int" so the
> structure is smaller (one byte, according to sizeof()) than the access
> type (2-8 bytes).
>

where in the C standard did you read the requirement that every bit
field must be complete? (This is a serious question).
extern struct
{
  volatile unsigned int b : 1;
} bf3;

on my compiler this structure occupies 4 bytes.
and it is aligned at 4 bytes.
That is OK for me and AAPCS.

But the access "bf3.b=1" is SI mode with Sandra's patch (part 1, which just
obeys the AAPCS and does nothing else) 
and QI mode without this patch, which is just a BUG.
I am quite surprised how your target manages to avoid it?

It is as Sandra said, at least on ARM -fstrict-volatile-bitfields
does not function at all. And the C++11 memory model wins all the time.

> Looking through the tests, most of them combine "packed" with
> mismatched types. IMHO, those tests are invalid.

I dont think so. They are simply packed. And volatile just says that
the value may be changed by a different thread. It has a great
impact on loop optimizations.

>>> Either way, if -fstrict-volatile-bitfields does not do what it's
>>> supposed to do, the correct action is to fix it - not to disable it on
>>> targets that rely on it for correct operation.

Agreed. That is the number one priority here.

>...
> I've not objected to fixing -fstrict-volatile-bitfields, or making the
> -fno-strict-volatile-bitfields case match the standard. I've only
> objected to breaking my targets by making that flag not the default.

Fine. Why cant we just get this fixed?

Bernd.
DJ Delorie Oct. 17, 2013, 4:17 p.m. UTC | #7
> where in the C standard did you read the requirement that every bit
> field must be complete? (This is a serious question).

The spec doesn't say each field must be complete, but neither does it
say that the structure must be as big as the type used.  If you
specify "int foo:1" then the compile is allowed to make the struct
smaller than "int".

> extern struct
> {
>   volatile unsigned int b : 1;
> } bf3;
> 
> on my compiler this structure occupies 4 bytes.
> and it is aligned at 4 bytes.
> That is OK for me and AAPCS.

On my target, the structure occupies 1 byte.  I suspect gcc is
rounding up to WORD_MODE, which is 4 bytes for you and 1 for me.  If
so, you're relying on a coincidence, not a standard.

> But the access "bf3.b=1" is SI mode with Sandra's patch (part 1, which just
> obeys the AAPCS and does nothing else) 
> and QI mode without this patch, which is just a BUG.
> I am quite surprised how your target manages to avoid it?

It doesn't, but I wouldn't expect it to, because IMHO that test is
invalid - you have not given a precise enough test to expect
consistent results.

> It is as Sandra said, at least on ARM -fstrict-volatile-bitfields
> does not function at all. And the C++11 memory model wins all the time.

Are we talking about N2429?  I read through the changes and it didn't
preclude honoring the user's types.

> > Looking through the tests, most of them combine "packed" with
> > mismatched types. IMHO, those tests are invalid.
> 
> I dont think so. They are simply packed. And volatile just says that
> the value may be changed by a different thread. It has a great
> impact on loop optimizations.

Nothing you've said so far precludes honoring the user's types when
the user gives you a consistent structure.  Consider:

typedef struct { unsigned char b0:1, b1:1, b2:1, b3:1, b4:1, b5:1, b6:1, b7:1; } Bits;

extern volatile struct {
  Bits reg1; /* read status without resetting them */
  Bits reg2; /* read status and atomically reset them */
} interrupt_handler;

Without -fstrict-volatile-bitfields, gcc is allowed to use a 16-bit
access to read reg1, but this causes an unexpected read to volatile
reg2, which may have unintended consequences.  The spec doesn't say
the compiler *must* read reg2, it just *doesn't* say that it *can't*.
The -fstrict-volatile-bitfields flag tells the compiler that it
*can't*.  IMHO this doesn't violate the spec, it just limits what the
compiler can do within the spec.

If ARM wants fast word-sized volatile bitfields, use "int" and
structure your bitfields so that no "int" field overlaps a natural
"int" boundary.

When I added the option, I considered making it an error() to define a
strict volatile bitfield that spanned the allowed boundary of the type
specified, but I figured that would be too much.

> > I've not objected to fixing -fstrict-volatile-bitfields, or making the
> > -fno-strict-volatile-bitfields case match the standard. I've only
> > objected to breaking my targets by making that flag not the default.
> 
> Fine. Why cant we just get this fixed?

Dunno.  I'm only opposing the patch that disabled it on my targets.
Joseph Myers Oct. 17, 2013, 5:21 p.m. UTC | #8
On Thu, 17 Oct 2013, DJ Delorie wrote:

> > It is as Sandra said, at least on ARM -fstrict-volatile-bitfields
> > does not function at all. And the C++11 memory model wins all the time.
> 
> Are we talking about N2429?  I read through the changes and it didn't
> preclude honoring the user's types.

At least on ARM, you can e.g. have a non-bit-field "char" that occupies 
part of the same 4-byte unit as an "int" bit-field.  And the C11/C++11 
memory model prohibits stores to that bit-field from doing 
read/modify/write on the whole 4-byte unit, as that's a store data race.

If the user wants to allow that data race, they can rewrite their code by 
changing the "char" into a bit-field.  I believe that in all cases where 
strict-volatile-bitfields ABIs are incompatible with the default --param 
allow-store-data-races=0, the structure can be similarly rewritten by the 
user so the adjacent fields become bit-fields, if they want the 
strict-volatile-bitfields requirements for their code to be compatible 
with the memory model.
DJ Delorie Oct. 17, 2013, 5:29 p.m. UTC | #9
> At least on ARM, you can e.g. have a non-bit-field "char" that occupies 
> part of the same 4-byte unit as an "int" bit-field.

Ok, "when the user doesn't give the compiler conflicting requirements."
(which is why I contemplated making those conflicts errors at first)

I'm a big fan of blaming the user when they mix things together and
expect the compiler to read their mind.
Joseph Myers Oct. 17, 2013, 8:22 p.m. UTC | #10
On Thu, 17 Oct 2013, DJ Delorie wrote:

> > At least on ARM, you can e.g. have a non-bit-field "char" that occupies 
> > part of the same 4-byte unit as an "int" bit-field.
> 
> Ok, "when the user doesn't give the compiler conflicting requirements."
> (which is why I contemplated making those conflicts errors at first)

I don't consider them conflicting.

My default starting point is that the user has a GNU C program, using 
target-independent semantics of GNU C, and expects that program to be 
compiled and executed according to those semantics.  (In this case, it can 
be an ISO C program; no GNU extensions are required.)  Those semantics 
include the memory model.  The fact that the user happens to be compiling 
for an ARM processor shouldn't change those semantics.  If, say, a 
GNU/Linux distribution contains code expecting the memory model, we 
shouldn't make the distributor use special options for building that code 
on ARM; building for a new architecture should, as far as possible, just 
work (and new architecture porters are hardly likely to know if one of 
thousands of packages might have such a requirement).

You seem to be starting from a user having a program that instead of 
expecting the normal target-independent semantics expects some other 
semantics specified by an ABI document, where those semantics contradict 
the semantics of ISO C.
DJ Delorie Oct. 17, 2013, 8:41 p.m. UTC | #11
I'm starting from an MCU that doesn't work right if GCC doesn't do
what the user tells GCC to do.  I added -fstrict-volatile-bitfields to
tell gcc that it needs to be more strict than the standard allows for
bitfield access, because without that flag, there's no way to force
gcc to use a specific access width on bitfields.  When I added that
flag, some ARM folks chose to enable it in their target, because they
felt they needed it.  If different ARM folks feel otherwise, that's a
target problem.

If the user tells gcc that a particular 32-bit memory location should
be treated as both a char and a long, then gcc has been given
inconsistent information.  The spec says it can do what it wants to
access that memory, and it does.

If the user tells gcc that a particular 16-bit memory location
consists of 16-bits worth of "unsigned short", and the user has told
gcc that it needs to be strict about accessing that field in the type
specified, and gcc uses a 32-bit access anyway, gcc is wrong.

I will agree with you 100% that gcc can do whatever the spec allows if
the user does NOT specify -fstrict-volatile-bitfields, but the flag is
there to tell gcc that the user needs stricter control than the spec
demands.  If the user uses that flag, *and* gives gcc information that
is inconsistent with the use of that flag, then the user is wrong.

I note that you assume GNU/Linux is involved, perhaps that's part of
the problem.  Maybe the Linux kernel needs gcc to ignore its bitfield
types, but other ARM firmware may have other requirements.  If you and
the other ARM maintainers want to argue over whether
-fstrict-volatile-bitfields is enabled by default for the ARM target,
go for it.  Just leave my targets alone.

> where those semantics contradict the semantics of ISO C.

Where in the spec does it say that a compiler MUST access an "unsigned
short" bitfield with a 32-bit access?  I've seen places where it says
the compiler MAY or MIGHT do it, but not MUST.
Bernd Edlinger Oct. 18, 2013, 10:11 a.m. UTC | #12
Hi,

On Thu, 17 Oct 2013 16:41:13, DJ Delorie wrote:
> I'm starting from an MCU that doesn't work right if GCC doesn't do
> what the user tells GCC to do. I added -fstrict-volatile-bitfields to
> tell gcc that it needs to be more strict than the standard allows for
> bitfield access, because without that flag, there's no way to force
> gcc to use a specific access width on bitfields. When I added that
> flag, some ARM folks chose to enable it in their target, because they
> felt they needed it. If different ARM folks feel otherwise, that's a
> target problem.
>
> If the user tells gcc that a particular 32-bit memory location should
> be treated as both a char and a long, then gcc has been given
> inconsistent information. The spec says it can do what it wants to
> access that memory, and it does.
>
> If the user tells gcc that a particular 16-bit memory location
> consists of 16-bits worth of "unsigned short", and the user has told
> gcc that it needs to be strict about accessing that field in the type
> specified, and gcc uses a 32-bit access anyway, gcc is wrong.
>
> I will agree with you 100% that gcc can do whatever the spec allows if
> the user does NOT specify -fstrict-volatile-bitfields, but the flag is
> there to tell gcc that the user needs stricter control than the spec
> demands. If the user uses that flag, *and* gives gcc information that
> is inconsistent with the use of that flag, then the user is wrong.
>
> I note that you assume GNU/Linux is involved, perhaps that's part of
> the problem. Maybe the Linux kernel needs gcc to ignore its bitfield
> types, but other ARM firmware may have other requirements. If you and
> the other ARM maintainers want to argue over whether
> -fstrict-volatile-bitfields is enabled by default for the ARM target,
> go for it. Just leave my targets alone.
>
>> where those semantics contradict the semantics of ISO C.
>
> Where in the spec does it say that a compiler MUST access an "unsigned
> short" bitfield with a 32-bit access? I've seen places where it says
> the compiler MAY or MIGHT do it, but not MUST.

Well, I'm starting to think that we can never follow the ARM AAPCS by the letter.

But maybe we could implement -fstrict-volatile-bitfields in this way:

We use the container mode where possible.
It is always possible for well-formed bit-fields.

If necessary the user has to add anonymous bit field members, or
convert normal members to bit-fields.

But when the bit field is conflict with the either the hardware's alignment
requirements or with the C++11 memory model, we follow the latter.

This means that, even for volatile data:
1. we never write anything outside the BIT_FIELD_REPRESENTATIVE.
2. we allow unaligned packed data, but we may use multiple accesses for a
    single read/write op. Also in this case: no data store races outside the member.

Example:

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

This is not well-formed, and -fstrict-volatile-bitfields will have no effect on it.

The user has to re-write this structure to:

struct {
  volatile int a : 24;
  volatile char b : 8;
};

Maybe a warning for "not well-formed" bit-fields could be helpful...

What do you think?

Bernd.
Richard Biener Oct. 18, 2013, 10:11 a.m. UTC | #13
On Wed, Oct 9, 2013 at 3:09 AM, Bernd Edlinger
<bernd.edlinger@hotmail.de> wrote:
> Hi,
>
> On Mon, 30 Sep 2013 16:18:30, DJ Delorie wrote:
>>
>> As per my previous comments on this patch, I will not approve the
>> changes to the m32c backend, as they will cause real bugs in real
>> hardware, and violate the hardware's ABI. The user may use
>> -fno-strict-volatile-bitfields if they do not desire this behavior and
>> understand the consequences.
>>
>> I am not a maintainer for the rx and h8300 ports, but they are in the
>> same situation.
>>
>> To reiterate my core position: if the user defines a proper "volatile
>> int" bitfield, and the compiler does anything other than an int-sized
>> access, the compiler is WRONG. Any optimization that changes volatile
>> accesses to something other than what the user specified is a bug that
>> needs to be fixed before this option can be non-default.
>
> hmm, I just tried to use the latest 4.9 trunk to compile the example from
> the AAPCS document:
>
> struct s
> {
>   volatile int a:8;
>   volatile char b:2;
> };
>
> struct s ss;
>
> int
> main ()
> {
>   ss.a=1;
>   ss.b=1;
>   return 0;
> }
>
> and the resulting code is completely against the written AAPCS specification:
>
> main:
>         @ Function supports interworking.
>         @ args = 0, pretend = 0, frame = 0
>         @ frame_needed = 0, uses_anonymous_args = 0
>         @ link register save eliminated.
>         ldr     r3, .L2
>         ldrh    r2, [r3]
>         bic     r2, r2, #254
>         orr     r2, r2, #1
>         strh    r2, [r3]        @ movhi
>         ldrh    r2, [r3]
>         bic     r2, r2, #512
>         orr     r2, r2, #256
>         strh    r2, [r3]        @ movhi
>         mov     r0, #0
>         bx      lr
>
> two half-word accesses, to my total surprise!
>
> As it looks like, the -fstrict-volatile-bitfields are already totally broken,
> apparently in favour of the C++ memory model, at least at the write-side.

Note that the C++ memory model restricts the _maximum_ memory region
we may touch - it does not constrain the minimum as AAPCS does.

What I would suggest is to have a -fgnu-strict-volatile-bit-fields
(or two levels of it) enabled by default on AAPCS targets which will
follow the AAPCS if it doesn't violate the maximum memory region
constraints of the C++ memory model.

I never claimed that the C++ memory model is good enough for AAPCS
but there was consensus that the default on AAPCS should not violate
the C++ memory model by default.

-fgnu-strict-volatile-bit-fields should be fully implementable in
get_best_mode if you pass down the desired AAPCS mode.

Richard.

> These are aligned accesses, not the packed structures, that was the
> only case where it used to work once.
>
> This must be fixed. I do not understand why we cannot agree, that
> at least the bug-fix part of Sandra's patch needs to be applied.
>
> Regards
> Bernd.
DJ Delorie Oct. 18, 2013, 6:14 p.m. UTC | #14
> We use the container mode where possible.
> It is always possible for well-formed bit-fields.

This is the only part I really need.

> If necessary the user has to add anonymous bit field members, or
> convert normal members to bit-fields.

IIRC I added code to support normal members as well, the "bitfields"
part of the option name is not entirely accurate.

> But when the bit field is conflict with the either the hardware's alignment
> requirements or with the C++11 memory model, we follow the latter.

Fine with me.

> 2. we allow unaligned packed data, but we may use multiple accesses
>  for a single read/write op. Also in this case: no data store races
>  outside the member.

We should note that volatile != atomic in these cases.  Perhaps a
warning would be in order?

> Example:
> 
> struct {
>  volatile int a : 24;
>  volatile char b;
> };
> 
> This is not well-formed, and -fstrict-volatile-bitfields will have
> no effect on it.

I agree this isn't well-formed, but -fs-v-b could still make a best
effort since none of the fields *individually* span a natural
boundary.

> The user has to re-write this structure to:
> 
> struct {
>  volatile int a : 24;
>  volatile char b : 8;
> };

More accurate would be these:

struct {
  volatile int a : 24;
  volatile int b : 8;    /* overlap, should be the same type */
};

struct {
  volatile int a : 24;
  volatile int : 0;
  volatile char b;	/* no overlap, make the padding explicit */
};
DJ Delorie Oct. 18, 2013, 6:21 p.m. UTC | #15
> What I would suggest is to have a -fgnu-strict-volatile-bit-fields

Why a new option?  The -fstrict-volatile-bitfields option is already
GCC-specific, and I think it can do what you want anyway.
Bernd Edlinger Oct. 20, 2013, 5:12 a.m. UTC | #16
Hi,

>> What I would suggest is to have a -fgnu-strict-volatile-bit-fields
>
> Why a new option? The -fstrict-volatile-bitfields option is already
> GCC-specific, and I think it can do what you want anyway.

As I understand Richard's comment, he proposes to
have an option for true AAPCS compliance, which will
be allowed to break the C++11 memory model and
which will _not_ be the default on any target.
Name it -fstrict-volatile-bitfields.

And an option that addresses your requirements,
which will _not_ break the C++11 memory model
and which will be the default on some targets,
dependent on the respective ABI requirements.
Name it -fgnu-strict-volatile-bit-fields.


Bernd.
Richard Biener Oct. 20, 2013, 12:54 p.m. UTC | #17
Bernd Edlinger <bernd.edlinger@hotmail.de> wrote:
>Hi,
>
>>> What I would suggest is to have a -fgnu-strict-volatile-bit-fields
>>
>> Why a new option? The -fstrict-volatile-bitfields option is already
>> GCC-specific, and I think it can do what you want anyway.
>
>As I understand Richard's comment, he proposes to
>have an option for true AAPCS compliance, which will
>be allowed to break the C++11 memory model and
>which will _not_ be the default on any target.
>Name it -fstrict-volatile-bitfields.
>
>And an option that addresses your requirements,
>which will _not_ break the C++11 memory model
>and which will be the default on some targets,
>dependent on the respective ABI requirements.
>Name it -fgnu-strict-volatile-bit-fields.

Yes. You could also make it -fstrict-volatile-bitfields={off,gnu,aacps} if you think that's better.

Richard.

>
>Bernd.
DJ Delorie Oct. 21, 2013, 8:51 p.m. UTC | #18
> have an option for true AAPCS compliance, which will
> be allowed to break the C++11 memory model and

> And an option that addresses your requirements,
> which will _not_ break the C++11 memory model

So the problem isn't that what *I* need conflicts with C++11, it's
that what AAPCS needs conflicts?
Bernd Edlinger Oct. 22, 2013, 1:51 a.m. UTC | #19
>
>> have an option for true AAPCS compliance, which will
>> be allowed to break the C++11 memory model and
>
>> And an option that addresses your requirements,
>> which will _not_ break the C++11 memory model
>
> So the problem isn't that what *I* need conflicts with C++11, it's
> that what AAPCS needs conflicts?

Yes, there are two written specifications which are in conflict
AAPCS and C++11. We cannot follow both at the same time.

But from this discussion I've learned, that your target's requirements
can easily co-exist with the C++ memory model.

Because if you only use well-formed bit-fields, the C++ memory
model just allows everything, and we can choose what to do.

Regards
Bernd.
diff mbox

Patch

Index: gcc/config/aarch64/aarch64.c
===================================================================
--- gcc/config/aarch64/aarch64.c	(revision 203002)
+++ gcc/config/aarch64/aarch64.c	(working copy)
@@ -5141,10 +5141,6 @@  aarch64_override_options (void)
 
   aarch64_build_bitmask_table ();
 
-  /* This target defaults to strict volatile bitfields.  */
-  if (flag_strict_volatile_bitfields < 0 && abi_version_at_least (2))
-    flag_strict_volatile_bitfields = 1;
-
   /* If the user did not specify a processor, choose the default
      one for them.  This will be the CPU set during configuration using
      --with-cpu, otherwise it is "generic".  */
Index: gcc/config/arm/arm.c
===================================================================
--- gcc/config/arm/arm.c	(revision 203002)
+++ gcc/config/arm/arm.c	(working copy)
@@ -2136,11 +2136,6 @@  arm_option_override (void)
 			   global_options.x_param_values,
 			   global_options_set.x_param_values);
 
-  /* ARM EABI defaults to strict volatile bitfields.  */
-  if (TARGET_AAPCS_BASED && flag_strict_volatile_bitfields < 0
-      && abi_version_at_least(2))
-    flag_strict_volatile_bitfields = 1;
-
   /* Enable sw prefetching at -O3 for CPUS that have prefetch, and we have deemed
      it beneficial (signified by setting num_prefetch_slots to 1 or more.)  */
   if (flag_prefetch_loop_arrays < 0
Index: gcc/config/h8300/h8300.c
===================================================================
--- gcc/config/h8300/h8300.c	(revision 203002)
+++ gcc/config/h8300/h8300.c	(working copy)
@@ -437,10 +437,6 @@  h8300_option_override (void)
 	 restore er6 though, so bump up the cost.  */
       h8300_move_ratio = 6;
     }
-
-  /* This target defaults to strict volatile bitfields.  */
-  if (flag_strict_volatile_bitfields < 0 && abi_version_at_least(2))
-    flag_strict_volatile_bitfields = 1;
 }
 
 /* Return the byte register name for a register rtx X.  B should be 0
Index: gcc/config/m32c/m32c.c
===================================================================
--- gcc/config/m32c/m32c.c	(revision 203002)
+++ gcc/config/m32c/m32c.c	(working copy)
@@ -416,10 +416,6 @@  m32c_option_override (void)
   if (TARGET_A24)
     flag_ivopts = 0;
 
-  /* This target defaults to strict volatile bitfields.  */
-  if (flag_strict_volatile_bitfields < 0 && abi_version_at_least(2))
-    flag_strict_volatile_bitfields = 1;
-
   /* r8c/m16c have no 16-bit indirect call, so thunks are involved.
      This is always worse than an absolute call.  */
   if (TARGET_A16)
Index: gcc/config/rx/rx.c
===================================================================
--- gcc/config/rx/rx.c	(revision 203002)
+++ gcc/config/rx/rx.c	(working copy)
@@ -2691,10 +2691,6 @@  rx_option_override (void)
 	  }
       }
 
-  /* This target defaults to strict volatile bitfields.  */
-  if (flag_strict_volatile_bitfields < 0 && abi_version_at_least(2))
-    flag_strict_volatile_bitfields = 1;
-
   rx_override_options_after_change ();
 
   if (align_jumps == 0 && ! optimize_size)
Index: gcc/config/sh/sh.c
===================================================================
--- gcc/config/sh/sh.c	(revision 203002)
+++ gcc/config/sh/sh.c	(working copy)
@@ -1014,10 +1014,6 @@  sh_option_override (void)
   if (sh_fixed_range_str)
     sh_fix_range (sh_fixed_range_str);
 
-  /* This target defaults to strict volatile bitfields.  */
-  if (flag_strict_volatile_bitfields < 0 && abi_version_at_least(2))
-    flag_strict_volatile_bitfields = 1;
-
   /* Parse atomic model option and make sure it is valid for the current
      target CPU.  */
   selected_atomic_model_
Index: gcc/doc/invoke.texi
===================================================================
--- gcc/doc/invoke.texi	(revision 203003)
+++ gcc/doc/invoke.texi	(working copy)
@@ -21175,8 +21175,11 @@ 
 case GCC falls back to generating multiple accesses 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
-interface for the target processor.
+Note that in some cases this option overrides the memory model
+specified in recent versions of the C and C++ standards.  For this
+reason, @option{-fstrict-volatile-bitfields} is not enabled by default
+on any target, even those where the application binary interface for
+the target processor requires this behavior.
 
 @item -fsync-libcalls
 @opindex fsync-libcalls
Index: gcc/testsuite/gcc.target/arm/volatile-bitfields-1.c
===================================================================
--- gcc/testsuite/gcc.target/arm/volatile-bitfields-1.c	(revision 203002)
+++ gcc/testsuite/gcc.target/arm/volatile-bitfields-1.c	(working copy)
@@ -1,6 +1,6 @@ 
 /* { dg-require-effective-target arm_eabi } */
 /* { dg-do compile } */
-/* { dg-options "-O2" } */
+/* { dg-options "-O2 -fstrict-volatile-bitfields" } */
 
 typedef struct {
   char a:1;
Index: gcc/testsuite/gcc.target/arm/volatile-bitfields-2.c
===================================================================
--- gcc/testsuite/gcc.target/arm/volatile-bitfields-2.c	(revision 203002)
+++ gcc/testsuite/gcc.target/arm/volatile-bitfields-2.c	(working copy)
@@ -1,6 +1,6 @@ 
 /* { dg-require-effective-target arm_eabi } */
 /* { dg-do compile } */
-/* { dg-options "-O2" } */
+/* { dg-options "-O2 -fstrict-volatile-bitfields" } */
 
 typedef struct {
   volatile unsigned long a:8;
Index: gcc/testsuite/gcc.target/arm/volatile-bitfields-3.c
===================================================================
--- gcc/testsuite/gcc.target/arm/volatile-bitfields-3.c	(revision 203002)
+++ gcc/testsuite/gcc.target/arm/volatile-bitfields-3.c	(working copy)
@@ -1,6 +1,6 @@ 
 /* { dg-require-effective-target arm_eabi } */
 /* { dg-do compile } */
-/* { dg-options "-O2" } */
+/* { dg-options "-O2 -fstrict-volatile-bitfields" } */
 
 typedef struct {
   volatile unsigned long a:8;
Index: gcc/testsuite/gcc.target/arm/volatile-bitfields-4.c
===================================================================
--- gcc/testsuite/gcc.target/arm/volatile-bitfields-4.c	(revision 203002)
+++ gcc/testsuite/gcc.target/arm/volatile-bitfields-4.c	(working copy)
@@ -1,6 +1,6 @@ 
 /* { dg-require-effective-target arm_eabi } */
 /* { dg-do compile } */
-/* { dg-options "-O2" } */
+/* { dg-options "-O2 -fstrict-volatile-bitfields" } */
 /* { dg-final { scan-assembler-times "ldr\[\\t \]+\[^\n\]*,\[\\t \]*\\\[\[^\n\]*\\\]" 2 } } */
 /* { dg-final { scan-assembler-times "str\[\\t \]+\[^\n\]*,\[\\t \]*\\\[\[^\n\]*\\\]" 2 } } */
 /* { dg-final { scan-assembler-not "strb" } } */