[PATCH] Strict volatile bit-fields clean-up

[Richard Biener]

On Tue, Dec 3, 2013 at 10:59 AM, Richard Biener
<richard.guenther@gmail.com> wrote:
> On Tue, Dec 3, 2013 at 10:47 AM, Bernd Edlinger
> <bernd.edlinger@hotmail.de> wrote:
>> On Mon, 2 Dec 2013 15:42:48, Richard Biener wrote:
>>>
>>> On Wed, Nov 20, 2013 at 11:48 AM, Bernd Edlinger
>>> <bernd.edlinger@hotmail.de> wrote:
>>>> Hello Richard,
>>>>
>>>> as a follow-up patch to the bit-fields patch(es), I wanted to remove the dependencies on
>>>> the variable flag_strict_volatile_bitfields from expand_assignment and expand_expr_real_1.
>>>> Additionally I want the access mode of the field to be selected in the memory context,
>>>> instead of the structure's mode.
>>>>
>>>> Boot-strapped and regression-tested on x86_64-linux-gnu.
>>>>
>>>> OK for trunk?
>>>
>>> Ok.
>>>
>>> Thanks,
>>> Richard.
>>>
>>
>>
>> Oops....
>>
>> Sorry, tonight this patch caused an Ada regression, in pack17.adb and misaligned_nest.adb
>>
>> So I'll have to put that on hold at the moment.
>>
>> This ICE is very similar to PR59134.
>> It is again the recursion between store_fixed_bit_field and store_split_bit_field.
>>
>> The trigger is a latent problem in the ada gcc_interface.
>>
>> That is we have a bit-field of exactly 8 bit size, which is not byte-aligned,
>> but DECL_MODE=QImode, DECL_BIT_FIELD=false which looks quite strange,
>
> It's not required that DECL_BIT_FIELD is set I think, and the mode
> is that of the type if it's not a bitfield ...

Btw, I see DECL_BIT_FIELD and DECL_BIT_FIELD_TYPE set (but QImode
used) for 'v' on x86_64 and the testcase "works".

>> and is totally different from how C structures look like. I should mention that there
>> are even some places in the target back-ends, where the attribute DECL_BIT_FIELD is
>> used for whatever.
>>
>> Now, due to this hunk in the cleanup-patch we get the QImode selected in the memory
>> context:
>>
>>        if (MEM_P (to_rtx))
>>         {
>> -         if (volatilep && flag_strict_volatile_bitfields> 0)
>> +         if (mode1 != VOIDmode)
>>             to_rtx = adjust_address (to_rtx, mode1, 0);
>
> Which I think is correct - the memory access is in QImode - that's what
> get_inner_reference said.
>
> So whatever issue we run into downstream points to the bug ...
>
> OTOH Eric may know better what the middle-end can expect for
> bit-aligned Ada records (apart from "all bets are off" which isn't
> really a good solution ;))
>
> Richard.
>
>> However even without that patch, I can arrange for "volatilep && flag_strict_volatile_bitfields> 0"
>> to be true in Ada (even on X86_64, or whatever platform you like):
>>
>> -- { dg-do run }
>> -- { dg-options "-gnatp -fstrict-volatile-bitfields" }
>>
>> procedure Misaligned_Volatile is
>>
>>    type Byte is mod 2**8;
>>
>>    type Block is record
>>       B : Boolean;
>>       V : Byte;
>>    end record;
>>    pragma Volatile (Block);
>>    pragma Pack (Block);
>>    for Block'Alignment use 1;
>>
>>    type Pair is array (1 .. 2) of Block;
>>
>>    P : Pair;
>> begin
>>    for K in P'Range loop
>>       P(K).V := 237;
>>    end loop;
>>    for K in P'Range loop
>>       if P(K).V /= 237 then
>>          raise Program_error;
>>       end if;
>>    end loop;
>> end;
>>
>>
>> This Ada test case causes either wrong code generation or an ICE at compile time,
>> if the -fstrict-volatile-bitfields option is either given by the user,
>> or by the target-specific default as it is on ARM for instance  (which is completely
>> pointless on Ada, I know!)...
>>
>> Now I am preparing a new bitfields-update-patch which fixes this above test case and the
>> latent recursion problem.
>>
>>
>> Thanks  ...  for you patience :-(
>> Bernd.