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