Patchwork patch [6/5] check for conflict with -fstrict-volatile-bitfields and -std=

login
register
mail settings
Submitter Sandra Loosemore
Date June 19, 2013, 8:32 p.m.
Message ID <51C2157B.9090707@codesourcery.com>
Download mbox | patch
Permalink /patch/252653/
State New
Headers show

Comments

Sandra Loosemore - June 19, 2013, 8:32 p.m.
On 06/17/2013 06:02 PM, Sandra Loosemore wrote:
>
> 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.

Like this.  This patch is intended to be applied on top of the other 5 
pieces in this series, although in theory it's independent of them.  OK 
to commit, and does this resolve the objection to part 3?

-Sandra
Joseph S. Myers - June 19, 2013, 11:10 p.m.
On Wed, 19 Jun 2013, Sandra Loosemore wrote:

> On 06/17/2013 06:02 PM, Sandra Loosemore wrote:
> > 
> > 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.
> 
> Like this.  This patch is intended to be applied on top of the other 5 pieces
> in this series, although in theory it's independent of them.  OK to commit,
> and does this resolve the objection to part 3?

I don't think it's right to depend on the standard version like this.  The 
existing semantics for GNU C and C++ follow the memory model for all 
standard versions, and that's the sort of thing that shouldn't depend on 
the target architecture.  In the absence of explicit 
-fstrict-volatile-bitfields, semantics conflicting with the memory model 
should only be enabled by one of the --param options to allow data races, 
and not by some default option relating to something in a target ABI 
that's incompatible with the normal language semantics.
Sandra Loosemore - June 20, 2013, 2:55 a.m.
On 06/19/2013 05:10 PM, Joseph S. Myers wrote:
>
> I don't think it's right to depend on the standard version like this.  The
> existing semantics for GNU C and C++ follow the memory model for all
> standard versions, and that's the sort of thing that shouldn't depend on
> the target architecture.  In the absence of explicit
> -fstrict-volatile-bitfields, semantics conflicting with the memory model
> should only be enabled by one of the --param options to allow data races,
> and not by some default option relating to something in a target ABI
> that's incompatible with the normal language semantics.

Hmmmm.  Well, I'm willing to hack up a patch to remove 
-fstrict-volatile-bitfields from the defaults for all backends, if it is 
the consensus of the GCC community to do that and it unblocks 
consideration of the other wrong-code bug fix patches in the series.

I'm concerned, though, that we should consider the perspective of GCC 
users on the affected targets as well as that of a standards committee 
member.  E.g., suppose I am an ARM user with some code manipulating 
memory-mapped I/O registers that was originally developed with an older 
version of GCC, or with a different compiler.  Maybe it is not even my 
own code, but something I got from a chip vendor SDK.  People working 
with such target-specific, low-level code are far more likely to be 
familiar with and conforming to ARM's published guidelines for volatile 
bit-field access than obscure details of a language standard that is not 
even being selected as the dialect for compiling the code.  I think 
there's a good argument here that by retroactively applying the 
C11/C++11 memory model to older standard versions, GCC has simply broken 
access to memory-mapped registers on ARM.  After all, the AAPCS predates 
these newer standards and older versions of GCC at least made an effort 
to implement the behavior AAPCS required, and if the pr23623 testcase 
had been added at the time that issue was originally resolved back in 
2006, the regression would have been caught immediately when the 
bitfield range patches were committed.

I hope that by the time GCC switches to C11 and C++11 as its default 
dialects, ARM will have revised its ABI or clarified how this conflict 
should be resolved.  :-)

Anyway, what do other people think?

-Sandra
Andrew Pinski - June 20, 2013, 4:22 a.m.
On Wed, Jun 19, 2013 at 7:55 PM, Sandra Loosemore
<sandra@codesourcery.com> wrote:
> On 06/19/2013 05:10 PM, Joseph S. Myers wrote:
>>
>>
>> I don't think it's right to depend on the standard version like this.  The
>> existing semantics for GNU C and C++ follow the memory model for all
>> standard versions, and that's the sort of thing that shouldn't depend on
>> the target architecture.  In the absence of explicit
>> -fstrict-volatile-bitfields, semantics conflicting with the memory model
>> should only be enabled by one of the --param options to allow data races,
>> and not by some default option relating to something in a target ABI
>> that's incompatible with the normal language semantics.
>
>
> Hmmmm.  Well, I'm willing to hack up a patch to remove
> -fstrict-volatile-bitfields from the defaults for all backends, if it is the
> consensus of the GCC community to do that and it unblocks consideration of
> the other wrong-code bug fix patches in the series.
>
> I'm concerned, though, that we should consider the perspective of GCC users
> on the affected targets as well as that of a standards committee member.
> E.g., suppose I am an ARM user with some code manipulating memory-mapped I/O
> registers that was originally developed with an older version of GCC, or
> with a different compiler.  Maybe it is not even my own code, but something
> I got from a chip vendor SDK.  People working with such target-specific,
> low-level code are far more likely to be familiar with and conforming to
> ARM's published guidelines for volatile bit-field access than obscure
> details of a language standard that is not even being selected as the
> dialect for compiling the code.  I think there's a good argument here that
> by retroactively applying the C11/C++11 memory model to older standard
> versions, GCC has simply broken access to memory-mapped registers on ARM.
> After all, the AAPCS predates these newer standards and older versions of
> GCC at least made an effort to implement the behavior AAPCS required, and if
> the pr23623 testcase had been added at the time that issue was originally
> resolved back in 2006, the regression would have been caught immediately
> when the bitfield range patches were committed.
>
> I hope that by the time GCC switches to C11 and C++11 as its default
> dialects, ARM will have revised its ABI or clarified how this conflict
> should be resolved.  :-)
>
> Anyway, what do other people think?

I rather ARM think about the issues.  This effects even AARCH64 where
most programs are going to be following the C11/C++11 memory model due
to the market that is being aimed.  I think it is a good idea to have
ARM resolves the issues before even breaking C11/C++11 memory model.

I know for Cavium's AARCH64 GCC I am going to turn off
-fstrict-volatile-bitfields for AARCH64 even though it violates the
ABI since it violates the C/c++ standard first.  The C/c++ standard in
my mind is what takes precedence over the ABI.

Thanks,
Andrew


>
> -Sandra
>

Patch

Index: gcc/c-family/c-opts.c
===================================================================
--- gcc/c-family/c-opts.c	(revision 199963)
+++ gcc/c-family/c-opts.c	(working copy)
@@ -813,6 +813,18 @@  c_common_post_options (const char **pfil
   C_COMMON_OVERRIDE_OPTIONS;
 #endif
 
+  /* C11 and C++11 specify a memory model that is incompatible with
+     -fstrict-volatile-bitfields.  Warn if that option is given explicitly
+     and prevent backends from defaulting to turning it on.  */
+  if (flag_isoc11 || cxx_dialect >= cxx11)
+    {
+      if (flag_strict_volatile_bitfields > 0)
+	warning (0, "-fstrict-volatile-bitfields conflicts with the "
+		    "C11 and C++11 memory model");
+      else
+	flag_strict_volatile_bitfields = 0;
+    }
+
   /* Excess precision other than "fast" requires front-end
      support.  */
   if (c_dialect_cxx ())
Index: gcc/doc/invoke.texi
===================================================================
--- gcc/doc/invoke.texi	(revision 199963)
+++ gcc/doc/invoke.texi	(working copy)
@@ -20899,4 +20899,12 @@ 
 AAPCS, @option{-fstrict-volatile-bitfields} is the default.
 
+Note that @option{-fstrict-volatile-bitfields} is incompatible with
+the bit-field access behavior required by the ISO C11 and C++11
+standards.  GCC warns if @option{-fstrict-volatile-bitfields} is given
+explicitly with an incompatible @option{-std=} option.  On targets
+that otherwise default to @option{-fstrict-volatile-bitfields},
+providing an incompatible @option{-std=} option implicitly disables
+@option{-fstrict-volatile-bitfields}.
+
 @item -fsync-libcalls
 @opindex fsync-libcalls