[PATCH] reimplement -fstrict-volatile-bitfields v4, part 1/2

[Bernd Edlinger]

Hi,

On Mon, 28 Oct 2013 21:29:24, Sandra Loosemore wrote:
>
> On 10/28/2013 03:20 AM, Bernd Edlinger wrote:
>>
>> On Sun, 20 Oct 2013 20:23:49, Sandra Loosemore wrote:
>>>
>>> I tried a backport to GCC 4.8 and tested on arm-none-eabi. On the new
>>> pr56997-1.c testcase, it got stuck in infinite recursion between
>>> store_split_bit_field/store_fixed_bit_field and/or
>>> extract_split_bit_field/extract_fixed_bit_field. This didn't show up in
>>> my previous mainline testing.
>>>
>>> [snip]
>>
>> I have attached an update to your patch, that should
>> a) fix the recursion problem.
>> b) restrict the -fstrict-volatile-bitfields to not violate the C++ memory model.
>>
>
> Thanks for picking up the ball on this -- I've been busy with other
> tasks for several days.
>
> I again tried backporting the patch series along with your fix to GCC
> 4.8 and tested on arm-none-eabi. I found that it was still getting
> stuck in infinite recursion unless the test from this patch hunk
>

hmmm....
Actually I used your path on a clean 4.8.2 and built for --target=arm-eabi.
I have seen the recursion on the extract_bit_field, but not on store_bit_field
so far, maybe you could give me a hint which test case exposes the other
flavour of this recursion problem.

>> @@ -1699,6 +1711,14 @@ extract_bit_field (rtx str_rtx, unsigned
>> {
>> enum machine_mode mode1;
>>
>> + /* Handle extraction of unaligned fields,
>> + this can happen in -fstrict-volatile-bitfields. */
>> + if (GET_MODE_BITSIZE (GET_MODE (str_rtx)) != 0
>> + && GET_MODE_BITSIZE (GET_MODE (str_rtx)) < GET_MODE_BITSIZE (word_mode)
>> + && bitnum % GET_MODE_BITSIZE (GET_MODE (str_rtx)) + bitsize
>> +> GET_MODE_BITSIZE (GET_MODE (str_rtx)) )
>> + str_rtx = adjust_address (str_rtx, word_mode, 0);
>> +
>> /* Handle -fstrict-volatile-bitfields in the cases where it applies. */
>> if (GET_MODE_BITSIZE (GET_MODE (str_rtx))> 0)
>> mode1 = GET_MODE (str_rtx);
>
> was also inserted into store_bit_field.
>
> I also think that, minimally, this needs to be rewritten as something like
>
> if (MEM_P (str_rtx)
> && STRICT_ALIGNMENT
> && GET_MODE_BITSIZE (GET_MODE (str_rtx)) != 0
> && GET_MODE_BITSIZE (GET_MODE (str_rtx)) < GET_MODE_BITSIZE
> (word_mode)
> && (bitnum % MEM_ALIGN (str_rtx) + bitsize
>> GET_MODE_BITSIZE (GET_MODE (str_rtx))))
> str_rtx = adjust_address (str_rtx, word_mode, 0);
>

Yes, that looks fine. 
 

> Otherwise, we'll end up using word_mode instead of the field mode on
> targets that support unaligned accesses. I tested that (again, 4.8 and
> arm-none-eabi) and results looked good, but of course ARM isn't one of
> the targets that supports unaligned accesses. :-S
>
> I'm still not convinced this is the right fix, though. It seems to me
> that callers of store_bit_field and extract_bit_field in expr.c ought
> not to be messing with the mode of the rtx when
> flag_strict_volatile_bitfields> 0, because that is losing information
> about the original mode that we are trying to patch up here by forcing
> it to word_mode. Instead, I think the callers ought to be passing the
> declared field mode into store_bit_field and extract_bit_field, and
> those functions ought to change the mode of the incoming rtx to match
> the field mode only if strict_volatile_bitfield_p assures us that the
> insertion/extraction can be done in that mode.
>

The problem starts likely at expr.c when this is done:

if (volatilep && flag_strict_volatile_bitfields> 0)
     to_rtx = adjust_address (to_rtx, mode1, 0);

this restricts the possible access mode not only for bit-fields
but for all possible volatile members. But -fstrict-volatile-bitfields
is supposed to affect bit-fields only.

> Alternatively, if strict_volatile_bitfield_p returns false but
> flag_strict_volatile_bitfields> 0, then always force to word_mode and
> change the -fstrict-volatile-bitfields documentation to indicate that's
> the fallback if the insertion/extraction cannot be done in the declared
> mode, rather than claiming that it tries to do the same thing as if
> -fstrict-volatile-bitfields were not enabled at all.
>
> Either way, still needs more work, and more thorough testing (more
> targets, and obviously trunk as well as the 4.8 backport) before I'd
> consider this ready to commit. I might or might not be able to find
> some more time to work on this in the next week....
>

Yes. 

And it would be good to reach Richard's Nov-21 deadline for GCC-4.9

Bernd.

> -Sandra
>