diff mbox

Still fails with strict-volatile-bitfields

Message ID DUB129-W37D4ED8E0F6242C57A8FCCE4AB0@phx.gbl
State New
Headers show

Commit Message

Bernd Edlinger Feb. 3, 2014, 9:08 p.m. UTC 
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:


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.
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>>
diff mbox

Patch

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.