Re: Still fails with strict-volatile-bitfields

[Joey Ye <joey dot ye dot cc at gmail dot com>]

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