public inbox for gcc@gcc.gnu.org
 help / color / mirror / Atom feed
* Still fails with strict-volatile-bitfields
@ 2014-01-09  8:26 Joey Ye
  2014-01-09 10:45 ` Bernd Edlinger
  0 siblings, 1 reply; 13+ messages in thread
From: Joey Ye @ 2014-01-09  8:26 UTC (permalink / raw)
  To: gcc, sandra, bernd.edlinger

Sandra, Bernd,

Can you take a look at
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59734

It seems a siimple case still doesn't work as expected. Did I miss anything?

Thanks,
Joey

^ permalink raw reply	[flat|nested] 13+ messages in thread

* RE: Still fails with strict-volatile-bitfields
  2014-01-09  8:26 Still fails with strict-volatile-bitfields Joey Ye
@ 2014-01-09 10:45 ` Bernd Edlinger
  2014-01-09 16:30   ` Richard Earnshaw
  0 siblings, 1 reply; 13+ messages in thread
From: Bernd Edlinger @ 2014-01-09 10:45 UTC (permalink / raw)
  To: Joey Ye, gcc, sandra

Hi,

On Thu, 9 Jan 2014 15:01:54, Yoey Ye wrote:
>
> Sandra, Bernd,
>
> Can you take a look at
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59734
>
> It seems a siimple case still doesn't work as expected. Did I miss anything?
>
> Thanks,
> Joey

Yes,

this is a major case where the C++ memory model is
in conflict with AAPCS.

You can get the expected code, by changing the struct
like this:

struct str {
  volatile unsigned f1: 8;
  unsigned dummy:24;
};

If it is written this way the C++ memory model allows
QImode, HImode, SImode. And -fstrict-volatile-bitfields
demands SImode, so the conflict is resolved. This dummy
member makes only a difference on the C level, and is
completely invisible in the generated code.

If -fstrict-volatile-bitfields is now given, we use SImode,
if -fno-strict-volatile-bitfields is given, we give GCC the
freedom to choose the access mode, maybe QImode if that is
faster.

In the _very_ difficult process to find an solution
that seems to be acceptable to all maintainers, we came to
the solution, that we need to adhere to the C++ memory
model by default. And we may not change the default
setting of -fstruct-volatile-bitfields at the same time!

As a future extension we discussed the possibility
to add a new option -fstrict-volatile-bitfields=aapcs
that explicitly allows us to break the C++ memory model.

But I did not yet try to implement this, as I feel that
would certainly not be accepted as we are in Phase3 now.

As another future extension there was the discussion
about the -Wportable-volatility warning, which I see now
as a warning that analyzes the structure layout and
warns about any structures that are not "well-formed",
in the sense, that a bit-field fails to define all
bits of the container.

Those people that do use bit-fields to access device-registers
do always define all bits, and of course in the same mode.

It would be good to have a warning, when some bits are missing.
They currently have to use great care to check their structures
manually.

I had a proposal for that warning but that concentrated
only on the volatile attribute, but I will have to re-write
that completely so that it can be done in stor-layout.c:

It should warn independent of optimization levels or actual
bitfield member references, thus, be implemented entirely at
the time we lay out the structure. The well-formed-ness of
a bit-field makes that possible.

But that will come too late for Phase3 as well.


Regards
Bernd. 		 	   		  

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: Still fails with strict-volatile-bitfields
  2014-01-09 10:45 ` Bernd Edlinger
@ 2014-01-09 16:30   ` Richard Earnshaw
  2014-01-10  8:56     ` Bernd Edlinger
  0 siblings, 1 reply; 13+ messages in thread
From: Richard Earnshaw @ 2014-01-09 16:30 UTC (permalink / raw)
  To: Bernd Edlinger; +Cc: Joey Ye, gcc, sandra

On 09/01/14 08:26, Bernd Edlinger wrote:
> Hi,
> 
> On Thu, 9 Jan 2014 15:01:54, Yoey Ye wrote:
>>
>> Sandra, Bernd,
>>
>> Can you take a look at
>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59734
>>
>> It seems a siimple case still doesn't work as expected. Did I miss anything?
>>
>> Thanks,
>> Joey
> 
> Yes,
> 
> this is a major case where the C++ memory model is
> in conflict with AAPCS.
> 

Does the compiler warn about this?  And if so, is the warning on by
default?  I think it's essential that we don't have quiet changes in
behaviour without such warnings.

R.

> You can get the expected code, by changing the struct
> like this:
> 
> struct str {
>   volatile unsigned f1: 8;
>   unsigned dummy:24;
> };
> 
> If it is written this way the C++ memory model allows
> QImode, HImode, SImode. And -fstrict-volatile-bitfields
> demands SImode, so the conflict is resolved. This dummy
> member makes only a difference on the C level, and is
> completely invisible in the generated code.
> 
> If -fstrict-volatile-bitfields is now given, we use SImode,
> if -fno-strict-volatile-bitfields is given, we give GCC the
> freedom to choose the access mode, maybe QImode if that is
> faster.
> 
> In the _very_ difficult process to find an solution
> that seems to be acceptable to all maintainers, we came to
> the solution, that we need to adhere to the C++ memory
> model by default. And we may not change the default
> setting of -fstruct-volatile-bitfields at the same time!
> 
> As a future extension we discussed the possibility
> to add a new option -fstrict-volatile-bitfields=aapcs
> that explicitly allows us to break the C++ memory model.
> 
> But I did not yet try to implement this, as I feel that
> would certainly not be accepted as we are in Phase3 now.
> 
> As another future extension there was the discussion
> about the -Wportable-volatility warning, which I see now
> as a warning that analyzes the structure layout and
> warns about any structures that are not "well-formed",
> in the sense, that a bit-field fails to define all
> bits of the container.
> 
> Those people that do use bit-fields to access device-registers
> do always define all bits, and of course in the same mode.
> 
> It would be good to have a warning, when some bits are missing.
> They currently have to use great care to check their structures
> manually.
> 
> I had a proposal for that warning but that concentrated
> only on the volatile attribute, but I will have to re-write
> that completely so that it can be done in stor-layout.c:
> 
> It should warn independent of optimization levels or actual
> bitfield member references, thus, be implemented entirely at
> the time we lay out the structure. The well-formed-ness of
> a bit-field makes that possible.
> 
> But that will come too late for Phase3 as well.
> 
> 
> Regards
> Bernd. 		 	   		  
> 
> 


^ permalink raw reply	[flat|nested] 13+ messages in thread

* RE: Still fails with strict-volatile-bitfields
  2014-01-09 16:30   ` Richard Earnshaw
@ 2014-01-10  8:56     ` Bernd Edlinger
  2014-01-10  9:02       ` Eric Botcazou
                         ` (2 more replies)
  0 siblings, 3 replies; 13+ messages in thread
From: Bernd Edlinger @ 2014-01-10  8:56 UTC (permalink / raw)
  To: Richard Earnshaw; +Cc: Joey Ye, gcc, Sandra Loosemore

On Thu, 9 Jan 2014 16:22:33, Richard Earnshaw wrote:
>
> On 09/01/14 08:26, Bernd Edlinger wrote:
>> Hi,
>>
>> On Thu, 9 Jan 2014 15:01:54, Yoey Ye wrote:
>>>
>>> Sandra, Bernd,
>>>
>>> Can you take a look at
>>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59734
>>>
>>> It seems a siimple case still doesn't work as expected. Did I miss anything?
>>>
>>> Thanks,
>>> Joey
>>
>> Yes,
>>
>> this is a major case where the C++ memory model is
>> in conflict with AAPCS.
>>
>
> Does the compiler warn about this? And if so, is the warning on by
> default? I think it's essential that we don't have quiet changes in
> behaviour without such warnings.
>
> R.

No. This example was working in 4.6 and broken in 4.7 and 4.8.
Well, 4.7 should have warned about that.

4.9 does simply not change that, because it would violate the C++
memory model. Maybe that should go into release notes.

At the same time we had all kinds of invalid code generation,
starting at 4.6, especially with packed structures in C and Ada(!),
(writing not all bits, and using unaligned accesses)
and that is fixed in 4.9.


Bernd.

>
>> You can get the expected code, by changing the struct
>> like this:
>>
>> struct str {
>> volatile unsigned f1: 8;
>> unsigned dummy:24;
>> };
>>
>> If it is written this way the C++ memory model allows
>> QImode, HImode, SImode. And -fstrict-volatile-bitfields
>> demands SImode, so the conflict is resolved. This dummy
>> member makes only a difference on the C level, and is
>> completely invisible in the generated code.
>>
>> If -fstrict-volatile-bitfields is now given, we use SImode,
>> if -fno-strict-volatile-bitfields is given, we give GCC the
>> freedom to choose the access mode, maybe QImode if that is
>> faster.
>>
>> In the _very_ difficult process to find an solution
>> that seems to be acceptable to all maintainers, we came to
>> the solution, that we need to adhere to the C++ memory
>> model by default. And we may not change the default
>> setting of -fstruct-volatile-bitfields at the same time!
>>
>> As a future extension we discussed the possibility
>> to add a new option -fstrict-volatile-bitfields=aapcs
>> that explicitly allows us to break the C++ memory model.
>>
>> But I did not yet try to implement this, as I feel that
>> would certainly not be accepted as we are in Phase3 now.
>>
>> As another future extension there was the discussion
>> about the -Wportable-volatility warning, which I see now
>> as a warning that analyzes the structure layout and
>> warns about any structures that are not "well-formed",
>> in the sense, that a bit-field fails to define all
>> bits of the container.
>>
>> Those people that do use bit-fields to access device-registers
>> do always define all bits, and of course in the same mode.
>>
>> It would be good to have a warning, when some bits are missing.
>> They currently have to use great care to check their structures
>> manually.
>>
>> I had a proposal for that warning but that concentrated
>> only on the volatile attribute, but I will have to re-write
>> that completely so that it can be done in stor-layout.c:
>>
>> It should warn independent of optimization levels or actual
>> bitfield member references, thus, be implemented entirely at
>> the time we lay out the structure. The well-formed-ness of
>> a bit-field makes that possible.
>>
>> But that will come too late for Phase3 as well.
>>
>>
>> Regards
>> Bernd.
>>
>>
>
> 		 	   		  

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: Still fails with strict-volatile-bitfields
  2014-01-10  8:56     ` Bernd Edlinger
@ 2014-01-10  9:02       ` Eric Botcazou
  2014-01-10  9:11       ` Eric Botcazou
  2014-01-10 11:52       ` Richard Earnshaw
  2 siblings, 0 replies; 13+ messages in thread
From: Eric Botcazou @ 2014-01-10  9:02 UTC (permalink / raw)
  To: Bernd Edlinger; +Cc: gcc, Richard Earnshaw, Joey Ye, Sandra Loosemore

> No. This example was working in 4.6 and broken in 4.7 and 4.8.
> Well, 4.7 should have warned about that.

The 4.7 branch is not closed so it's not too late to add the warning there.

-- 
Eric Botcazou

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: Still fails with strict-volatile-bitfields
  2014-01-10  8:56     ` Bernd Edlinger
  2014-01-10  9:02       ` Eric Botcazou
@ 2014-01-10  9:11       ` Eric Botcazou
  2014-01-10 11:52       ` Richard Earnshaw
  2 siblings, 0 replies; 13+ messages in thread
From: Eric Botcazou @ 2014-01-10  9:11 UTC (permalink / raw)
  To: Bernd Edlinger; +Cc: Joey Ye, gcc, Richard Earnshaw, Sandra Loosemore

> No. This example was working in 4.6 and broken in 4.7 and 4.8.

Note that it probably broke in 4.7.1 because the DECL_BIT_FIELD_REPRESENTATIVE 
stuff was backported after the initial 4.7.0 release.

-- 
Eric Botcazou

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: Still fails with strict-volatile-bitfields
  2014-01-10  8:56     ` Bernd Edlinger
  2014-01-10  9:02       ` Eric Botcazou
  2014-01-10  9:11       ` Eric Botcazou
@ 2014-01-10 11:52       ` Richard Earnshaw
  2014-01-10 13:45         ` Bernd Edlinger
  2 siblings, 1 reply; 13+ messages in thread
From: Richard Earnshaw @ 2014-01-10 11:52 UTC (permalink / raw)
  To: Bernd Edlinger; +Cc: Joey Ye, gcc, Sandra Loosemore

On 10/01/14 08:49, Bernd Edlinger wrote:
> On Thu, 9 Jan 2014 16:22:33, Richard Earnshaw wrote:
>>
>> On 09/01/14 08:26, Bernd Edlinger wrote:
>>> Hi,
>>>
>>> On Thu, 9 Jan 2014 15:01:54, Yoey Ye wrote:
>>>>
>>>> Sandra, Bernd,
>>>>
>>>> Can you take a look at
>>>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59734
>>>>
>>>> It seems a siimple case still doesn't work as expected. Did I miss anything?
>>>>
>>>> Thanks,
>>>> Joey
>>>
>>> Yes,
>>>
>>> this is a major case where the C++ memory model is
>>> in conflict with AAPCS.
>>>
>>
>> Does the compiler warn about this? And if so, is the warning on by
>> default? I think it's essential that we don't have quiet changes in
>> behaviour without such warnings.
>>
>> R.
> 
> No. This example was working in 4.6 and broken in 4.7 and 4.8.
> Well, 4.7 should have warned about that.
> 
> 4.9 does simply not change that, because it would violate the C++
> memory model. Maybe that should go into release notes.

That's no excuse for not generating a warning at compile time when the
situation is encountered.  Users of this feature are experiencing a
quiet change of behaviour and that's unacceptable, even if the compiler
is doing what it was intended to do; that doesn't necessarily match the
user's expectations.  There should always be a way to rewrite the C11
intended semantics in a way that doesn't violate the strict volatile
bitfields semantics.

> 
> At the same time we had all kinds of invalid code generation,
> starting at 4.6, especially with packed structures in C and Ada(!),
> (writing not all bits, and using unaligned accesses)
> and that is fixed in 4.9.
> 

I'm not worried about packed structures.  You can't sensibly implement
the strict volatile bitfields rules when things aren't aligned.

R.

> 
> Bernd.
> 
>>
>>> You can get the expected code, by changing the struct
>>> like this:
>>>
>>> struct str {
>>> volatile unsigned f1: 8;
>>> unsigned dummy:24;
>>> };
>>>
>>> If it is written this way the C++ memory model allows
>>> QImode, HImode, SImode. And -fstrict-volatile-bitfields
>>> demands SImode, so the conflict is resolved. This dummy
>>> member makes only a difference on the C level, and is
>>> completely invisible in the generated code.
>>>
>>> If -fstrict-volatile-bitfields is now given, we use SImode,
>>> if -fno-strict-volatile-bitfields is given, we give GCC the
>>> freedom to choose the access mode, maybe QImode if that is
>>> faster.
>>>
>>> In the _very_ difficult process to find an solution
>>> that seems to be acceptable to all maintainers, we came to
>>> the solution, that we need to adhere to the C++ memory
>>> model by default. And we may not change the default
>>> setting of -fstruct-volatile-bitfields at the same time!
>>>
>>> As a future extension we discussed the possibility
>>> to add a new option -fstrict-volatile-bitfields=aapcs
>>> that explicitly allows us to break the C++ memory model.
>>>
>>> But I did not yet try to implement this, as I feel that
>>> would certainly not be accepted as we are in Phase3 now.
>>>
>>> As another future extension there was the discussion
>>> about the -Wportable-volatility warning, which I see now
>>> as a warning that analyzes the structure layout and
>>> warns about any structures that are not "well-formed",
>>> in the sense, that a bit-field fails to define all
>>> bits of the container.
>>>
>>> Those people that do use bit-fields to access device-registers
>>> do always define all bits, and of course in the same mode.
>>>
>>> It would be good to have a warning, when some bits are missing.
>>> They currently have to use great care to check their structures
>>> manually.
>>>
>>> I had a proposal for that warning but that concentrated
>>> only on the volatile attribute, but I will have to re-write
>>> that completely so that it can be done in stor-layout.c:
>>>
>>> It should warn independent of optimization levels or actual
>>> bitfield member references, thus, be implemented entirely at
>>> the time we lay out the structure. The well-formed-ness of
>>> a bit-field makes that possible.
>>>
>>> But that will come too late for Phase3 as well.
>>>
>>>
>>> Regards
>>> Bernd.
>>>
>>>
>>
>> 		 	   		  
> 


^ permalink raw reply	[flat|nested] 13+ messages in thread

* RE: Still fails with strict-volatile-bitfields
  2014-01-10 11:52       ` Richard Earnshaw
@ 2014-01-10 13:45         ` Bernd Edlinger
  2014-01-10 14:12           ` Richard Biener
  0 siblings, 1 reply; 13+ messages in thread
From: Bernd Edlinger @ 2014-01-10 13:45 UTC (permalink / raw)
  To: Richard Earnshaw; +Cc: Joey Ye, gcc, Sandra Loosemore

On, Fri, 10 Jan 2014 09:41:06, Richard Earnshaw wrote:
>
> On 10/01/14 08:49, Bernd Edlinger wrote:
>> On Thu, 9 Jan 2014 16:22:33, Richard Earnshaw wrote:
>>>
>>> On 09/01/14 08:26, Bernd Edlinger wrote:
>>>> Hi,
>>>>
>>>> On Thu, 9 Jan 2014 15:01:54, Yoey Ye wrote:
>>>>>
>>>>> Sandra, Bernd,
>>>>>
>>>>> Can you take a look at
>>>>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59734
>>>>>
>>>>> It seems a siimple case still doesn't work as expected. Did I miss anything?
>>>>>
>>>>> Thanks,
>>>>> Joey
>>>>
>>>> Yes,
>>>>
>>>> this is a major case where the C++ memory model is
>>>> in conflict with AAPCS.
>>>>
>>>
>>> Does the compiler warn about this? And if so, is the warning on by
>>> default? I think it's essential that we don't have quiet changes in
>>> behaviour without such warnings.
>>>
>>> R.
>>
>> No. This example was working in 4.6 and broken in 4.7 and 4.8.
>> Well, 4.7 should have warned about that.
>>
>> 4.9 does simply not change that, because it would violate the C++
>> memory model. Maybe that should go into release notes.
>
> That's no excuse for not generating a warning at compile time when the
> situation is encountered. Users of this feature are experiencing a
> quiet change of behaviour and that's unacceptable, even if the compiler
> is doing what it was intended to do; that doesn't necessarily match the
> user's expectations. There should always be a way to rewrite the C11
> intended semantics in a way that doesn't violate the strict volatile
> bitfields semantics.
>

Hmm...
You mean we should have a (ugly) warning enabled by default in 4.9 (at expmed.c)
if a bit-field access would be perfectly aligned and so, but is in conflict with the
C++ memory model, and -fstrict-volatile-bitfields is in effect.
Maybe only once per compilation?


>>
>> At the same time we had all kinds of invalid code generation,
>> starting at 4.6, especially with packed structures in C and Ada(!),
>> (writing not all bits, and using unaligned accesses)
>> and that is fixed in 4.9.
>>
>
> I'm not worried about packed structures. You can't sensibly implement
> the strict volatile bitfields rules when things aren't aligned.
>
> R.
>
>>
>> Bernd.
>>
>>>
>>>> You can get the expected code, by changing the struct
>>>> like this:
>>>>
>>>> struct str {
>>>> volatile unsigned f1: 8;
>>>> unsigned dummy:24;
>>>> };
>>>>
>>>> If it is written this way the C++ memory model allows
>>>> QImode, HImode, SImode. And -fstrict-volatile-bitfields
>>>> demands SImode, so the conflict is resolved. This dummy
>>>> member makes only a difference on the C level, and is
>>>> completely invisible in the generated code.
>>>>
>>>> If -fstrict-volatile-bitfields is now given, we use SImode,
>>>> if -fno-strict-volatile-bitfields is given, we give GCC the
>>>> freedom to choose the access mode, maybe QImode if that is
>>>> faster.
>>>>
>>>> In the _very_ difficult process to find an solution
>>>> that seems to be acceptable to all maintainers, we came to
>>>> the solution, that we need to adhere to the C++ memory
>>>> model by default. And we may not change the default
>>>> setting of -fstruct-volatile-bitfields at the same time!
>>>>
>>>> As a future extension we discussed the possibility
>>>> to add a new option -fstrict-volatile-bitfields=aapcs
>>>> that explicitly allows us to break the C++ memory model.
>>>>
>>>> But I did not yet try to implement this, as I feel that
>>>> would certainly not be accepted as we are in Phase3 now.
>>>>
>>>> As another future extension there was the discussion
>>>> about the -Wportable-volatility warning, which I see now
>>>> as a warning that analyzes the structure layout and
>>>> warns about any structures that are not "well-formed",
>>>> in the sense, that a bit-field fails to define all
>>>> bits of the container.
>>>>
>>>> Those people that do use bit-fields to access device-registers
>>>> do always define all bits, and of course in the same mode.
>>>>
>>>> It would be good to have a warning, when some bits are missing.
>>>> They currently have to use great care to check their structures
>>>> manually.
>>>>
>>>> I had a proposal for that warning but that concentrated
>>>> only on the volatile attribute, but I will have to re-write
>>>> that completely so that it can be done in stor-layout.c:
>>>>
>>>> It should warn independent of optimization levels or actual
>>>> bitfield member references, thus, be implemented entirely at
>>>> the time we lay out the structure. The well-formed-ness of
>>>> a bit-field makes that possible.
>>>>
>>>> But that will come too late for Phase3 as well.
>>>>
>>>>
>>>> Regards
>>>> Bernd.
>>>>
>>>>
>>>
>>>
>>
>
> 		 	   		  

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: Still fails with strict-volatile-bitfields
  2014-01-10 13:45         ` Bernd Edlinger
@ 2014-01-10 14:12           ` Richard Biener
  2014-01-10 14:20             ` Richard Earnshaw
  2014-01-10 17:04             ` Bernd Edlinger
  0 siblings, 2 replies; 13+ messages in thread
From: Richard Biener @ 2014-01-10 14:12 UTC (permalink / raw)
  To: Bernd Edlinger; +Cc: Richard Earnshaw, Joey Ye, gcc, Sandra Loosemore

On Fri, Jan 10, 2014 at 2:30 PM, Bernd Edlinger
<bernd.edlinger@hotmail.de> wrote:
> On, Fri, 10 Jan 2014 09:41:06, Richard Earnshaw wrote:
>>
>> On 10/01/14 08:49, Bernd Edlinger wrote:
>>> On Thu, 9 Jan 2014 16:22:33, Richard Earnshaw wrote:
>>>>
>>>> On 09/01/14 08:26, Bernd Edlinger wrote:
>>>>> Hi,
>>>>>
>>>>> On Thu, 9 Jan 2014 15:01:54, Yoey Ye wrote:
>>>>>>
>>>>>> Sandra, Bernd,
>>>>>>
>>>>>> Can you take a look at
>>>>>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59734
>>>>>>
>>>>>> It seems a siimple case still doesn't work as expected. Did I miss anything?
>>>>>>
>>>>>> Thanks,
>>>>>> Joey
>>>>>
>>>>> Yes,
>>>>>
>>>>> this is a major case where the C++ memory model is
>>>>> in conflict with AAPCS.
>>>>>
>>>>
>>>> Does the compiler warn about this? And if so, is the warning on by
>>>> default? I think it's essential that we don't have quiet changes in
>>>> behaviour without such warnings.
>>>>
>>>> R.
>>>
>>> No. This example was working in 4.6 and broken in 4.7 and 4.8.
>>> Well, 4.7 should have warned about that.
>>>
>>> 4.9 does simply not change that, because it would violate the C++
>>> memory model. Maybe that should go into release notes.
>>
>> That's no excuse for not generating a warning at compile time when the
>> situation is encountered. Users of this feature are experiencing a
>> quiet change of behaviour and that's unacceptable, even if the compiler
>> is doing what it was intended to do; that doesn't necessarily match the
>> user's expectations. There should always be a way to rewrite the C11
>> intended semantics in a way that doesn't violate the strict volatile
>> bitfields semantics.
>>
>
> Hmm...
> You mean we should have a (ugly) warning enabled by default in 4.9 (at expmed.c)
> if a bit-field access would be perfectly aligned and so, but is in conflict with the
> C++ memory model, and -fstrict-volatile-bitfields is in effect.
> Maybe only once per compilation?

I'd say you want a warning for the structure declaration instead
"Accesses to XYZ will not follow AACPS due to C++ memory model
constraints".

Richard.

>
>>>
>>> At the same time we had all kinds of invalid code generation,
>>> starting at 4.6, especially with packed structures in C and Ada(!),
>>> (writing not all bits, and using unaligned accesses)
>>> and that is fixed in 4.9.
>>>
>>
>> I'm not worried about packed structures. You can't sensibly implement
>> the strict volatile bitfields rules when things aren't aligned.
>>
>> R.
>>
>>>
>>> Bernd.
>>>
>>>>
>>>>> You can get the expected code, by changing the struct
>>>>> like this:
>>>>>
>>>>> struct str {
>>>>> volatile unsigned f1: 8;
>>>>> unsigned dummy:24;
>>>>> };
>>>>>
>>>>> If it is written this way the C++ memory model allows
>>>>> QImode, HImode, SImode. And -fstrict-volatile-bitfields
>>>>> demands SImode, so the conflict is resolved. This dummy
>>>>> member makes only a difference on the C level, and is
>>>>> completely invisible in the generated code.
>>>>>
>>>>> If -fstrict-volatile-bitfields is now given, we use SImode,
>>>>> if -fno-strict-volatile-bitfields is given, we give GCC the
>>>>> freedom to choose the access mode, maybe QImode if that is
>>>>> faster.
>>>>>
>>>>> In the _very_ difficult process to find an solution
>>>>> that seems to be acceptable to all maintainers, we came to
>>>>> the solution, that we need to adhere to the C++ memory
>>>>> model by default. And we may not change the default
>>>>> setting of -fstruct-volatile-bitfields at the same time!
>>>>>
>>>>> As a future extension we discussed the possibility
>>>>> to add a new option -fstrict-volatile-bitfields=aapcs
>>>>> that explicitly allows us to break the C++ memory model.
>>>>>
>>>>> But I did not yet try to implement this, as I feel that
>>>>> would certainly not be accepted as we are in Phase3 now.
>>>>>
>>>>> As another future extension there was the discussion
>>>>> about the -Wportable-volatility warning, which I see now
>>>>> as a warning that analyzes the structure layout and
>>>>> warns about any structures that are not "well-formed",
>>>>> in the sense, that a bit-field fails to define all
>>>>> bits of the container.
>>>>>
>>>>> Those people that do use bit-fields to access device-registers
>>>>> do always define all bits, and of course in the same mode.
>>>>>
>>>>> It would be good to have a warning, when some bits are missing.
>>>>> They currently have to use great care to check their structures
>>>>> manually.
>>>>>
>>>>> I had a proposal for that warning but that concentrated
>>>>> only on the volatile attribute, but I will have to re-write
>>>>> that completely so that it can be done in stor-layout.c:
>>>>>
>>>>> It should warn independent of optimization levels or actual
>>>>> bitfield member references, thus, be implemented entirely at
>>>>> the time we lay out the structure. The well-formed-ness of
>>>>> a bit-field makes that possible.
>>>>>
>>>>> But that will come too late for Phase3 as well.
>>>>>
>>>>>
>>>>> Regards
>>>>> Bernd.
>>>>>
>>>>>
>>>>
>>>>
>>>
>>
>>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: Still fails with strict-volatile-bitfields
  2014-01-10 14:12           ` Richard Biener
@ 2014-01-10 14:20             ` Richard Earnshaw
  2014-01-10 17:04             ` Bernd Edlinger
  1 sibling, 0 replies; 13+ messages in thread
From: Richard Earnshaw @ 2014-01-10 14:20 UTC (permalink / raw)
  To: Richard Biener; +Cc: Bernd Edlinger, Joey Ye, gcc, Sandra Loosemore

On 10/01/14 13:45, Richard Biener wrote:
> On Fri, Jan 10, 2014 at 2:30 PM, Bernd Edlinger
> <bernd.edlinger@hotmail.de> wrote:
>> On, Fri, 10 Jan 2014 09:41:06, Richard Earnshaw wrote:
>>>
>>> On 10/01/14 08:49, Bernd Edlinger wrote:
>>>> On Thu, 9 Jan 2014 16:22:33, Richard Earnshaw wrote:
>>>>>
>>>>> On 09/01/14 08:26, Bernd Edlinger wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On Thu, 9 Jan 2014 15:01:54, Yoey Ye wrote:
>>>>>>>
>>>>>>> Sandra, Bernd,
>>>>>>>
>>>>>>> Can you take a look at
>>>>>>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59734
>>>>>>>
>>>>>>> It seems a siimple case still doesn't work as expected. Did I miss anything?
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Joey
>>>>>>
>>>>>> Yes,
>>>>>>
>>>>>> this is a major case where the C++ memory model is
>>>>>> in conflict with AAPCS.
>>>>>>
>>>>>
>>>>> Does the compiler warn about this? And if so, is the warning on by
>>>>> default? I think it's essential that we don't have quiet changes in
>>>>> behaviour without such warnings.
>>>>>
>>>>> R.
>>>>
>>>> No. This example was working in 4.6 and broken in 4.7 and 4.8.
>>>> Well, 4.7 should have warned about that.
>>>>
>>>> 4.9 does simply not change that, because it would violate the C++
>>>> memory model. Maybe that should go into release notes.
>>>
>>> That's no excuse for not generating a warning at compile time when the
>>> situation is encountered. Users of this feature are experiencing a
>>> quiet change of behaviour and that's unacceptable, even if the compiler
>>> is doing what it was intended to do; that doesn't necessarily match the
>>> user's expectations. There should always be a way to rewrite the C11
>>> intended semantics in a way that doesn't violate the strict volatile
>>> bitfields semantics.
>>>
>>
>> Hmm...
>> You mean we should have a (ugly) warning enabled by default in 4.9 (at expmed.c)
>> if a bit-field access would be perfectly aligned and so, but is in conflict with the
>> C++ memory model, and -fstrict-volatile-bitfields is in effect.
>> Maybe only once per compilation?
> 
> I'd say you want a warning for the structure declaration instead
> "Accesses to XYZ will not follow AACPS due to C++ memory model
> constraints".
> 

Well AAPCS (note ordering of letters ;-) is target-specific.  But
'strict volatile bitfields' would be fine.

Watch out, though, for

struct a
{
  // bitfield decls
  ...
};

volatile struct a *p;  // Need the warning here

R.

> Richard.
> 
>>
>>>>
>>>> At the same time we had all kinds of invalid code generation,
>>>> starting at 4.6, especially with packed structures in C and Ada(!),
>>>> (writing not all bits, and using unaligned accesses)
>>>> and that is fixed in 4.9.
>>>>
>>>
>>> I'm not worried about packed structures. You can't sensibly implement
>>> the strict volatile bitfields rules when things aren't aligned.
>>>
>>> R.
>>>
>>>>
>>>> Bernd.
>>>>
>>>>>
>>>>>> You can get the expected code, by changing the struct
>>>>>> like this:
>>>>>>
>>>>>> struct str {
>>>>>> volatile unsigned f1: 8;
>>>>>> unsigned dummy:24;
>>>>>> };
>>>>>>
>>>>>> If it is written this way the C++ memory model allows
>>>>>> QImode, HImode, SImode. And -fstrict-volatile-bitfields
>>>>>> demands SImode, so the conflict is resolved. This dummy
>>>>>> member makes only a difference on the C level, and is
>>>>>> completely invisible in the generated code.
>>>>>>
>>>>>> If -fstrict-volatile-bitfields is now given, we use SImode,
>>>>>> if -fno-strict-volatile-bitfields is given, we give GCC the
>>>>>> freedom to choose the access mode, maybe QImode if that is
>>>>>> faster.
>>>>>>
>>>>>> In the _very_ difficult process to find an solution
>>>>>> that seems to be acceptable to all maintainers, we came to
>>>>>> the solution, that we need to adhere to the C++ memory
>>>>>> model by default. And we may not change the default
>>>>>> setting of -fstruct-volatile-bitfields at the same time!
>>>>>>
>>>>>> As a future extension we discussed the possibility
>>>>>> to add a new option -fstrict-volatile-bitfields=aapcs
>>>>>> that explicitly allows us to break the C++ memory model.
>>>>>>
>>>>>> But I did not yet try to implement this, as I feel that
>>>>>> would certainly not be accepted as we are in Phase3 now.
>>>>>>
>>>>>> As another future extension there was the discussion
>>>>>> about the -Wportable-volatility warning, which I see now
>>>>>> as a warning that analyzes the structure layout and
>>>>>> warns about any structures that are not "well-formed",
>>>>>> in the sense, that a bit-field fails to define all
>>>>>> bits of the container.
>>>>>>
>>>>>> Those people that do use bit-fields to access device-registers
>>>>>> do always define all bits, and of course in the same mode.
>>>>>>
>>>>>> It would be good to have a warning, when some bits are missing.
>>>>>> They currently have to use great care to check their structures
>>>>>> manually.
>>>>>>
>>>>>> I had a proposal for that warning but that concentrated
>>>>>> only on the volatile attribute, but I will have to re-write
>>>>>> that completely so that it can be done in stor-layout.c:
>>>>>>
>>>>>> It should warn independent of optimization levels or actual
>>>>>> bitfield member references, thus, be implemented entirely at
>>>>>> the time we lay out the structure. The well-formed-ness of
>>>>>> a bit-field makes that possible.
>>>>>>
>>>>>> But that will come too late for Phase3 as well.
>>>>>>
>>>>>>
>>>>>> Regards
>>>>>> Bernd.
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>>
> 


^ permalink raw reply	[flat|nested] 13+ messages in thread

* RE: Still fails with strict-volatile-bitfields
  2014-01-10 14:12           ` Richard Biener
  2014-01-10 14:20             ` Richard Earnshaw
@ 2014-01-10 17:04             ` Bernd Edlinger
  2014-01-13  6:40               ` Joey Ye
  1 sibling, 1 reply; 13+ messages in thread
From: Bernd Edlinger @ 2014-01-10 17:04 UTC (permalink / raw)
  To: Richard Biener; +Cc: Richard Earnshaw, Joey Ye, gcc, Sandra Loosemore

On Fri, 10 Jan 2014 14:45:12, Richard Biener wrote:
>
> On Fri, Jan 10, 2014 at 2:30 PM, Bernd Edlinger
> <bernd.edlinger@hotmail.de> wrote:
>> On, Fri, 10 Jan 2014 09:41:06, Richard Earnshaw wrote:
>>>
>>> On 10/01/14 08:49, Bernd Edlinger wrote:
>>>> On Thu, 9 Jan 2014 16:22:33, Richard Earnshaw wrote:
>>>>>
>>>>> On 09/01/14 08:26, Bernd Edlinger wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On Thu, 9 Jan 2014 15:01:54, Yoey Ye wrote:
>>>>>>>
>>>>>>> Sandra, Bernd,
>>>>>>>
>>>>>>> Can you take a look at
>>>>>>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59734
>>>>>>>
>>>>>>> It seems a siimple case still doesn't work as expected. Did I miss anything?
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Joey
>>>>>>
>>>>>> Yes,
>>>>>>
>>>>>> this is a major case where the C++ memory model is
>>>>>> in conflict with AAPCS.
>>>>>>
>>>>>
>>>>> Does the compiler warn about this? And if so, is the warning on by
>>>>> default? I think it's essential that we don't have quiet changes in
>>>>> behaviour without such warnings.
>>>>>
>>>>> R.
>>>>
>>>> No. This example was working in 4.6 and broken in 4.7 and 4.8.
>>>> Well, 4.7 should have warned about that.
>>>>
>>>> 4.9 does simply not change that, because it would violate the C++
>>>> memory model. Maybe that should go into release notes.
>>>
>>> That's no excuse for not generating a warning at compile time when the
>>> situation is encountered. Users of this feature are experiencing a
>>> quiet change of behaviour and that's unacceptable, even if the compiler
>>> is doing what it was intended to do; that doesn't necessarily match the
>>> user's expectations. There should always be a way to rewrite the C11
>>> intended semantics in a way that doesn't violate the strict volatile
>>> bitfields semantics.
>>>
>>
>> Hmm...
>> You mean we should have a (ugly) warning enabled by default in 4.9 (at expmed.c)
>> if a bit-field access would be perfectly aligned and so, but is in conflict with the
>> C++ memory model, and -fstrict-volatile-bitfields is in effect.
>> Maybe only once per compilation?
>
> I'd say you want a warning for the structure declaration instead
> "Accesses to XYZ will not follow AACPS due to C++ memory model
> constraints".
>
> Richard.
>

Yes, that would be the way how we wanted to implement the
-Wportable-volatility warning, except that it would be on by default
if -fstrict-volatile-bitfields is in effect.
And it would not only warn if the member is declared volatile,
because the structure can be declared volatile later.

Except that I did not even start to implement it that way, that
would be quite late for 4.9 now?

Bernd.
>>
>>>>
>>>> At the same time we had all kinds of invalid code generation,
>>>> starting at 4.6, especially with packed structures in C and Ada(!),
>>>> (writing not all bits, and using unaligned accesses)
>>>> and that is fixed in 4.9.
>>>>
>>>
>>> I'm not worried about packed structures. You can't sensibly implement
>>> the strict volatile bitfields rules when things aren't aligned.
>>>
>>> R.
>>>
>>>>
>>>> Bernd.
>>>>
>>>>>
>>>>>> You can get the expected code, by changing the struct
>>>>>> like this:
>>>>>>
>>>>>> struct str {
>>>>>> volatile unsigned f1: 8;
>>>>>> unsigned dummy:24;
>>>>>> };
>>>>>>
>>>>>> If it is written this way the C++ memory model allows
>>>>>> QImode, HImode, SImode. And -fstrict-volatile-bitfields
>>>>>> demands SImode, so the conflict is resolved. This dummy
>>>>>> member makes only a difference on the C level, and is
>>>>>> completely invisible in the generated code.
>>>>>>
>>>>>> If -fstrict-volatile-bitfields is now given, we use SImode,
>>>>>> if -fno-strict-volatile-bitfields is given, we give GCC the
>>>>>> freedom to choose the access mode, maybe QImode if that is
>>>>>> faster.
>>>>>>
>>>>>> In the _very_ difficult process to find an solution
>>>>>> that seems to be acceptable to all maintainers, we came to
>>>>>> the solution, that we need to adhere to the C++ memory
>>>>>> model by default. And we may not change the default
>>>>>> setting of -fstruct-volatile-bitfields at the same time!
>>>>>>
>>>>>> As a future extension we discussed the possibility
>>>>>> to add a new option -fstrict-volatile-bitfields=aapcs
>>>>>> that explicitly allows us to break the C++ memory model.
>>>>>>
>>>>>> But I did not yet try to implement this, as I feel that
>>>>>> would certainly not be accepted as we are in Phase3 now.
>>>>>>
>>>>>> As another future extension there was the discussion
>>>>>> about the -Wportable-volatility warning, which I see now
>>>>>> as a warning that analyzes the structure layout and
>>>>>> warns about any structures that are not "well-formed",
>>>>>> in the sense, that a bit-field fails to define all
>>>>>> bits of the container.
>>>>>>
>>>>>> Those people that do use bit-fields to access device-registers
>>>>>> do always define all bits, and of course in the same mode.
>>>>>>
>>>>>> It would be good to have a warning, when some bits are missing.
>>>>>> They currently have to use great care to check their structures
>>>>>> manually.
>>>>>>
>>>>>> I had a proposal for that warning but that concentrated
>>>>>> only on the volatile attribute, but I will have to re-write
>>>>>> that completely so that it can be done in stor-layout.c:
>>>>>>
>>>>>> It should warn independent of optimization levels or actual
>>>>>> bitfield member references, thus, be implemented entirely at
>>>>>> the time we lay out the structure. The well-formed-ness of
>>>>>> a bit-field makes that possible.
>>>>>>
>>>>>> But that will come too late for Phase3 as well.
>>>>>>
>>>>>>
>>>>>> Regards
>>>>>> Bernd.
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>> 		 	   		  

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: Still fails with strict-volatile-bitfields
  2014-01-10 17:04             ` Bernd Edlinger
@ 2014-01-13  6:40               ` Joey Ye
  2014-02-03 21:08                 ` Bernd Edlinger
  0 siblings, 1 reply; 13+ messages in thread
From: Joey Ye @ 2014-01-13  6:40 UTC (permalink / raw)
  To: Bernd Edlinger; +Cc: Richard Biener, Richard Earnshaw, gcc, Sandra Loosemore

Bernd,

If that's the case, can you please firstly fix invoke.texi where the
behavior of strict-volatile-bitfields is described? At least my
interpretation of current doc doesn't explain the behavior of the case
we are discussing. Also it should be a generic definition rather than
target specific one.

A related issue is that how would we expect users to build their
original code with volatile bitfields usage? From the approach I
understand they have to explicitly add -fstrict-volatile-bitfields for
4.9 (by default it is disabled now), and probably
-fstrict-volatile-bitfields=aapcs (to allow C++ memory model conflict)
later. Neither of them are obvious to users. How about a configure
option to set default?

Thanks,
Joey

On Fri, Jan 10, 2014 at 10:20 PM, Bernd Edlinger
<bernd.edlinger@hotmail.de> wrote:
> On Fri, 10 Jan 2014 14:45:12, Richard Biener wrote:
>>
>> On Fri, Jan 10, 2014 at 2:30 PM, Bernd Edlinger
>> <bernd.edlinger@hotmail.de> wrote:
>>> On, Fri, 10 Jan 2014 09:41:06, Richard Earnshaw wrote:
>>>>
>>>> On 10/01/14 08:49, Bernd Edlinger wrote:
>>>>> On Thu, 9 Jan 2014 16:22:33, Richard Earnshaw wrote:
>>>>>>
>>>>>> On 09/01/14 08:26, Bernd Edlinger wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> On Thu, 9 Jan 2014 15:01:54, Yoey Ye wrote:
>>>>>>>>
>>>>>>>> Sandra, Bernd,
>>>>>>>>
>>>>>>>> Can you take a look at
>>>>>>>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59734
>>>>>>>>
>>>>>>>> It seems a siimple case still doesn't work as expected. Did I miss anything?
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Joey
>>>>>>>
>>>>>>> Yes,
>>>>>>>
>>>>>>> this is a major case where the C++ memory model is
>>>>>>> in conflict with AAPCS.
>>>>>>>
>>>>>>
>>>>>> Does the compiler warn about this? And if so, is the warning on by
>>>>>> default? I think it's essential that we don't have quiet changes in
>>>>>> behaviour without such warnings.
>>>>>>
>>>>>> R.
>>>>>
>>>>> No. This example was working in 4.6 and broken in 4.7 and 4.8.
>>>>> Well, 4.7 should have warned about that.
>>>>>
>>>>> 4.9 does simply not change that, because it would violate the C++
>>>>> memory model. Maybe that should go into release notes.
>>>>
>>>> That's no excuse for not generating a warning at compile time when the
>>>> situation is encountered. Users of this feature are experiencing a
>>>> quiet change of behaviour and that's unacceptable, even if the compiler
>>>> is doing what it was intended to do; that doesn't necessarily match the
>>>> user's expectations. There should always be a way to rewrite the C11
>>>> intended semantics in a way that doesn't violate the strict volatile
>>>> bitfields semantics.
>>>>
>>>
>>> Hmm...
>>> You mean we should have a (ugly) warning enabled by default in 4.9 (at expmed.c)
>>> if a bit-field access would be perfectly aligned and so, but is in conflict with the
>>> C++ memory model, and -fstrict-volatile-bitfields is in effect.
>>> Maybe only once per compilation?
>>
>> I'd say you want a warning for the structure declaration instead
>> "Accesses to XYZ will not follow AACPS due to C++ memory model
>> constraints".
>>
>> Richard.
>>
>
> Yes, that would be the way how we wanted to implement the
> -Wportable-volatility warning, except that it would be on by default
> if -fstrict-volatile-bitfields is in effect.
> And it would not only warn if the member is declared volatile,
> because the structure can be declared volatile later.
>
> Except that I did not even start to implement it that way, that
> would be quite late for 4.9 now?
>
> Bernd.
>>>
>>>>>
>>>>> At the same time we had all kinds of invalid code generation,
>>>>> starting at 4.6, especially with packed structures in C and Ada(!),
>>>>> (writing not all bits, and using unaligned accesses)
>>>>> and that is fixed in 4.9.
>>>>>
>>>>
>>>> I'm not worried about packed structures. You can't sensibly implement
>>>> the strict volatile bitfields rules when things aren't aligned.
>>>>
>>>> R.
>>>>
>>>>>
>>>>> Bernd.
>>>>>
>>>>>>
>>>>>>> You can get the expected code, by changing the struct
>>>>>>> like this:
>>>>>>>
>>>>>>> struct str {
>>>>>>> volatile unsigned f1: 8;
>>>>>>> unsigned dummy:24;
>>>>>>> };
>>>>>>>
>>>>>>> If it is written this way the C++ memory model allows
>>>>>>> QImode, HImode, SImode. And -fstrict-volatile-bitfields
>>>>>>> demands SImode, so the conflict is resolved. This dummy
>>>>>>> member makes only a difference on the C level, and is
>>>>>>> completely invisible in the generated code.
>>>>>>>
>>>>>>> If -fstrict-volatile-bitfields is now given, we use SImode,
>>>>>>> if -fno-strict-volatile-bitfields is given, we give GCC the
>>>>>>> freedom to choose the access mode, maybe QImode if that is
>>>>>>> faster.
>>>>>>>
>>>>>>> In the _very_ difficult process to find an solution
>>>>>>> that seems to be acceptable to all maintainers, we came to
>>>>>>> the solution, that we need to adhere to the C++ memory
>>>>>>> model by default. And we may not change the default
>>>>>>> setting of -fstruct-volatile-bitfields at the same time!
>>>>>>>
>>>>>>> As a future extension we discussed the possibility
>>>>>>> to add a new option -fstrict-volatile-bitfields=aapcs
>>>>>>> that explicitly allows us to break the C++ memory model.
>>>>>>>
>>>>>>> But I did not yet try to implement this, as I feel that
>>>>>>> would certainly not be accepted as we are in Phase3 now.
>>>>>>>
>>>>>>> As another future extension there was the discussion
>>>>>>> about the -Wportable-volatility warning, which I see now
>>>>>>> as a warning that analyzes the structure layout and
>>>>>>> warns about any structures that are not "well-formed",
>>>>>>> in the sense, that a bit-field fails to define all
>>>>>>> bits of the container.
>>>>>>>
>>>>>>> Those people that do use bit-fields to access device-registers
>>>>>>> do always define all bits, and of course in the same mode.
>>>>>>>
>>>>>>> It would be good to have a warning, when some bits are missing.
>>>>>>> They currently have to use great care to check their structures
>>>>>>> manually.
>>>>>>>
>>>>>>> I had a proposal for that warning but that concentrated
>>>>>>> only on the volatile attribute, but I will have to re-write
>>>>>>> that completely so that it can be done in stor-layout.c:
>>>>>>>
>>>>>>> It should warn independent of optimization levels or actual
>>>>>>> bitfield member references, thus, be implemented entirely at
>>>>>>> the time we lay out the structure. The well-formed-ness of
>>>>>>> a bit-field makes that possible.
>>>>>>>
>>>>>>> But that will come too late for Phase3 as well.
>>>>>>>
>>>>>>>
>>>>>>> Regards
>>>>>>> Bernd.
>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* RE: Still fails with strict-volatile-bitfields
  2014-01-13  6:40               ` Joey Ye
@ 2014-02-03 21:08                 ` Bernd Edlinger
  0 siblings, 0 replies; 13+ messages in thread
From: Bernd Edlinger @ 2014-02-03 21:08 UTC (permalink / raw)
  To: Joey Ye
  Cc: Richard Biener, Richard Earnshaw, gcc, Sandra Loosemore, gcc-patches

Hi,


On Mon, 13 Jan 2014 10:26:03, Joey Ye wrote:
>
> Bernd,
>
> If that's the case, can you please firstly fix invoke.texi where the
> behavior of strict-volatile-bitfields is described? At least my
> interpretation of current doc doesn't explain the behavior of the case
> we are discussing. Also it should be a generic definition rather than
> target specific one.
>

Yes,
if nobody objects I'd add a note like this to the documentation
regarding the -fstrict-volatile-bitfields:

Index: invoke.texi
===================================================================
--- invoke.texi (revision 207223)
+++ invoke.texi (working copy)
@@ -22456,6 +22456,10 @@
 case GCC falls back to generating multiple accesses rather than code that
 will fault or truncate the result at run time.

+Note:  Due to restrictions of the C/C++11 memory model, write accesses are
+not allowed to touch non bit-field members.  It is therefore recommended
+to define all bits of the field's type as bit-field members.
+
 The default value of this option is determined by the application binary
 interface for the target processor.


Thanks
Bernd.

> A related issue is that how would we expect users to build their
> original code with volatile bitfields usage? From the approach I
> understand they have to explicitly add -fstrict-volatile-bitfields for
> 4.9 (by default it is disabled now), and probably
> -fstrict-volatile-bitfields=aapcs (to allow C++ memory model conflict)
> later. Neither of them are obvious to users. How about a configure
> option to set default?
>
> Thanks,
> Joey
>
> On Fri, Jan 10, 2014 at 10:20 PM, Bernd Edlinger
> <bernd.edlinger@hotmail.de> wrote:
>> On Fri, 10 Jan 2014 14:45:12, Richard Biener wrote:
>>>
>>> On Fri, Jan 10, 2014 at 2:30 PM, Bernd Edlinger
>>> <bernd.edlinger@hotmail.de> wrote:
>>>> On, Fri, 10 Jan 2014 09:41:06, Richard Earnshaw wrote:
>>>>>
>>>>> On 10/01/14 08:49, Bernd Edlinger wrote:
>>>>>> On Thu, 9 Jan 2014 16:22:33, Richard Earnshaw wrote:
>>>>>>>
>>>>>>> On 09/01/14 08:26, Bernd Edlinger wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> On Thu, 9 Jan 2014 15:01:54, Yoey Ye wrote:
>>>>>>>>>
>>>>>>>>> Sandra, Bernd,
>>>>>>>>>
>>>>>>>>> Can you take a look at
>>>>>>>>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59734
>>>>>>>>>
>>>>>>>>> It seems a siimple case still doesn't work as expected. Did I miss anything?
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Joey
>>>>>>>>
>>>>>>>> Yes,
>>>>>>>>
>>>>>>>> this is a major case where the C++ memory model is
>>>>>>>> in conflict with AAPCS.
>>>>>>>>
>>>>>>>
>>>>>>> Does the compiler warn about this? And if so, is the warning on by
>>>>>>> default? I think it's essential that we don't have quiet changes in
>>>>>>> behaviour without such warnings.
>>>>>>>
>>>>>>> R.
>>>>>>
>>>>>> No. This example was working in 4.6 and broken in 4.7 and 4.8.
>>>>>> Well, 4.7 should have warned about that.
>>>>>>
>>>>>> 4.9 does simply not change that, because it would violate the C++
>>>>>> memory model. Maybe that should go into release notes.
>>>>>
>>>>> That's no excuse for not generating a warning at compile time when the
>>>>> situation is encountered. Users of this feature are experiencing a
>>>>> quiet change of behaviour and that's unacceptable, even if the compiler
>>>>> is doing what it was intended to do; that doesn't necessarily match the
>>>>> user's expectations. There should always be a way to rewrite the C11
>>>>> intended semantics in a way that doesn't violate the strict volatile
>>>>> bitfields semantics.
>>>>>
>>>>
>>>> Hmm...
>>>> You mean we should have a (ugly) warning enabled by default in 4.9 (at expmed.c)
>>>> if a bit-field access would be perfectly aligned and so, but is in conflict with the
>>>> C++ memory model, and -fstrict-volatile-bitfields is in effect.
>>>> Maybe only once per compilation?
>>>
>>> I'd say you want a warning for the structure declaration instead
>>> "Accesses to XYZ will not follow AACPS due to C++ memory model
>>> constraints".
>>>
>>> Richard.
>>>
>>
>> Yes, that would be the way how we wanted to implement the
>> -Wportable-volatility warning, except that it would be on by default
>> if -fstrict-volatile-bitfields is in effect.
>> And it would not only warn if the member is declared volatile,
>> because the structure can be declared volatile later.
>>
>> Except that I did not even start to implement it that way, that
>> would be quite late for 4.9 now?
>>
>> Bernd.
>>>>
>>>>>>
>>>>>> At the same time we had all kinds of invalid code generation,
>>>>>> starting at 4.6, especially with packed structures in C and Ada(!),
>>>>>> (writing not all bits, and using unaligned accesses)
>>>>>> and that is fixed in 4.9.
>>>>>>
>>>>>
>>>>> I'm not worried about packed structures. You can't sensibly implement
>>>>> the strict volatile bitfields rules when things aren't aligned.
>>>>>
>>>>> R.
>>>>>
>>>>>>
>>>>>> Bernd.
>>>>>>
>>>>>>>
>>>>>>>> You can get the expected code, by changing the struct
>>>>>>>> like this:
>>>>>>>>
>>>>>>>> struct str {
>>>>>>>> volatile unsigned f1: 8;
>>>>>>>> unsigned dummy:24;
>>>>>>>> };
>>>>>>>>
>>>>>>>> If it is written this way the C++ memory model allows
>>>>>>>> QImode, HImode, SImode. And -fstrict-volatile-bitfields
>>>>>>>> demands SImode, so the conflict is resolved. This dummy
>>>>>>>> member makes only a difference on the C level, and is
>>>>>>>> completely invisible in the generated code.
>>>>>>>>
>>>>>>>> If -fstrict-volatile-bitfields is now given, we use SImode,
>>>>>>>> if -fno-strict-volatile-bitfields is given, we give GCC the
>>>>>>>> freedom to choose the access mode, maybe QImode if that is
>>>>>>>> faster.
>>>>>>>>
>>>>>>>> In the _very_ difficult process to find an solution
>>>>>>>> that seems to be acceptable to all maintainers, we came to
>>>>>>>> the solution, that we need to adhere to the C++ memory
>>>>>>>> model by default. And we may not change the default
>>>>>>>> setting of -fstruct-volatile-bitfields at the same time!
>>>>>>>>
>>>>>>>> As a future extension we discussed the possibility
>>>>>>>> to add a new option -fstrict-volatile-bitfields=aapcs
>>>>>>>> that explicitly allows us to break the C++ memory model.
>>>>>>>>
>>>>>>>> But I did not yet try to implement this, as I feel that
>>>>>>>> would certainly not be accepted as we are in Phase3 now.
>>>>>>>>
>>>>>>>> As another future extension there was the discussion
>>>>>>>> about the -Wportable-volatility warning, which I see now
>>>>>>>> as a warning that analyzes the structure layout and
>>>>>>>> warns about any structures that are not "well-formed",
>>>>>>>> in the sense, that a bit-field fails to define all
>>>>>>>> bits of the container.
>>>>>>>>
>>>>>>>> Those people that do use bit-fields to access device-registers
>>>>>>>> do always define all bits, and of course in the same mode.
>>>>>>>>
>>>>>>>> It would be good to have a warning, when some bits are missing.
>>>>>>>> They currently have to use great care to check their structures
>>>>>>>> manually.
>>>>>>>>
>>>>>>>> I had a proposal for that warning but that concentrated
>>>>>>>> only on the volatile attribute, but I will have to re-write
>>>>>>>> that completely so that it can be done in stor-layout.c:
>>>>>>>>
>>>>>>>> It should warn independent of optimization levels or actual
>>>>>>>> bitfield member references, thus, be implemented entirely at
>>>>>>>> the time we lay out the structure. The well-formed-ness of
>>>>>>>> a bit-field makes that possible.
>>>>>>>>
>>>>>>>> But that will come too late for Phase3 as well.
>>>>>>>>
>>>>>>>>
>>>>>>>> Regards
>>>>>>>> Bernd.
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>> 		 	   		  

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2014-02-03 21:08 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-01-09  8:26 Still fails with strict-volatile-bitfields Joey Ye
2014-01-09 10:45 ` Bernd Edlinger
2014-01-09 16:30   ` Richard Earnshaw
2014-01-10  8:56     ` Bernd Edlinger
2014-01-10  9:02       ` Eric Botcazou
2014-01-10  9:11       ` Eric Botcazou
2014-01-10 11:52       ` Richard Earnshaw
2014-01-10 13:45         ` Bernd Edlinger
2014-01-10 14:12           ` Richard Biener
2014-01-10 14:20             ` Richard Earnshaw
2014-01-10 17:04             ` Bernd Edlinger
2014-01-13  6:40               ` Joey Ye
2014-02-03 21:08                 ` Bernd Edlinger

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).