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

[Bernd Edlinger <bernd dot edlinger at hotmail dot de>]

Hi,

On Thu, 14 Nov 2013 16:31:10, Richard Biener wrote:
>
> On Thu, Nov 14, 2013 at 11:16 AM, Bernd Edlinger
> <bernd.edlinger@hotmail.de> wrote:
>> Hi,
>>
>> sorry, for the delay.
>> Sandra seems to be even more busy than me...
>>
>> Attached is a combined patch of the original part 1, and the update,
>> in diff -up format.
>>
>> On Mon, 11 Nov 2013 13:10:45, Richard Biener wrote:
>>>
>>> On Thu, Oct 31, 2013 at 1:46 AM, Sandra Loosemore
>>> <sandra@codesourcery.com> wrote:
>>>> On 10/29/2013 02:51 AM, Bernd Edlinger wrote:
>>>>>
>>>>>
>>>>> On Mon, 28 Oct 2013 21:29:24, Sandra Loosemore wrote:
>>>>>>
>>>>>> On 10/28/2013 03:20 AM, Bernd Edlinger wrote:
>>>>>>>
>>>>>>> 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.
>>>>
>>>>
>>>> Here's a new version of the update patch.
>>>>
>>>>
>>>>>> 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.
>>>>
>>>>
>>>> I decided that this approach was more expedient, after all.
>>>>
>>>> I've tested this patch (in conjunction with my already-approved but
>>>> not-yet-applied patch) on mainline for arm-none-eabi, x86_64-linux-gnu, and
>>>> mips-linux gnu. I also backported the entire series to GCC 4.8 and tested
>>>> there on arm-none-eabi and x86_64-linux-gnu. OK to apply?
>>>
>>> Hm, I can't seem to find the context for
>>>
>>> @@ -923,6 +935,14 @@
>>> store_fixed_bit_field (str_rtx, bitsize, bitnum, 0, 0, value);
>>> return;
>>> }
>>> + else if (MEM_P (str_rtx)
>>> + && MEM_VOLATILE_P (str_rtx)
>>> + && flag_strict_volatile_bitfields> 0)
>>> + /* This is a case where -fstrict-volatile-bitfields doesn't apply
>>> + because we can't do a single access in the declared mode of the field.
>>> + Since the incoming STR_RTX has already been adjusted to that mode,
>>> + fall back to word mode for subsequent logic. */
>>> + str_rtx = adjust_address (str_rtx, word_mode, 0);
>>>
>>> /* Under the C++0x memory model, we must not touch bits outside the
>>> bit region. Adjust the address to start at the beginning of the
>>>
>>> and the other similar hunk. I suppose they apply to earlier patches
>>> in the series? I suppose the above applies to store_bit_field (diff -p
>>> really helps!). Why would using word_mode be any good as
>>> fallback? That is, why is "Since the incoming STR_RTX has already
>>> been adjusted to that mode" not the thing to fix?
>>>
>>
>> Well, this hunk does not force the access to be in word_mode.
>>
>> Instead it allows get_best_mode to choose the access to be in any mode from
>> QI to word_mode.
>>
>> It is there to revert the effect of this weird code in expr.c (expand_assigment):
>>
>> if (volatilep && flag_strict_volatile_bitfields> 0)
>> to_rtx = adjust_address (to_rtx, mode1, 0);
>>
>> Note that this does not even check if the access is on a bit-field !
>
> Then why not remove that ...
>
>> The problem with the strict_volatile_bitfields is that it is used already
>> before the code reaches store_bit_field or extract_bit_field.
>>
>> It starts in get_inner_reference, (which is not only used in expand_assignment
>> and expand_expr_real_1)
>>
>> Then this,
>>
>> if (volatilep && flag_strict_volatile_bitfields> 0)
>> op0 = adjust_address (op0, mode1, 0);
>
> ... and this ...
>
>> and then this,
>>
>> /* If the field is volatile, we always want an aligned
>> access. Do this in following two situations:
>> 1. the access is not already naturally
>> aligned, otherwise "normal" (non-bitfield) volatile fields
>> become non-addressable.
>> 2. the bitsize is narrower than the access size. Need
>> to extract bitfields from the access. */
>> || (volatilep && flag_strict_volatile_bitfields> 0
>> && (bitpos % GET_MODE_ALIGNMENT (mode) != 0
>> || (mode1 != BLKmode
>> && bitsize < GET_MODE_SIZE (mode1) * BITS_PER_UNIT)))
>
> ... or this ...
>
>> As a result, a read access to an unaligned volatile data member does
>> not even reach the expand_bit_field if flag_strict_volatile_bitfields <= 0,
>> and instead goes through convert_move (target, op0, unsignedp).
>>
>> I still believe the proposed patch is guaranteed to not change anything if
>> -fno-strict-volatile-bitfields is used, and even if we can not guarantee
>> that it creates exactly the same code for cases where the strict-volatile-bitfields
>> does not apply, it certainly generates valid code, where we had invalid code,
>> or ICEs without the patch.
>>
>> OK for trunk?
>
> Again, most of the patch is ok (and nice), the
> store_bit_field/extract_bit_field changes
> point to the above issues which we should rather fix than trying
> to revert them after the fact.
>
> Why is that not possible?
>
> That said,
>
> + else if (MEM_P (str_rtx)
> + && MEM_VOLATILE_P (str_rtx)
> + && flag_strict_volatile_bitfields> 0)
> + /* This is a case where -fstrict-volatile-bitfields doesn't apply
> + because we can't do a single access in the declared mode of the field.
> + Since the incoming STR_RTX has already been adjusted to that mode,
> + fall back to word mode for subsequent logic. */
> + str_rtx = adjust_address (str_rtx, word_mode, 0);
>
> we are looking at an access with bitsize / bitregion properties so plainly
> choosing word_mode here looks wrong to me. Yes, only
> -fstrict-volatile-bitfields is affected but still ...
>
> The patch is ok if you look at the above as followup.
>
> Thanks,
> Richard.
>
>

hmm...
the above change is just not aggressive enough.


with a slightly changed test case I have now got a 
recursion on the extract_fixed_bit_fields on ARM but
interestingly not on powerpc:

#include <stdlib.h>

typedef struct {
  char pad;
  int arr[0];
} __attribute__((packed)) str;

str *
foo (int* src)
{
  str *s = malloc (sizeof (str) + sizeof (int));
  *src = s->arr[0];
  s->arr[0] = 0x12345678;
  return s;
}

Now I think about reverting that hunk back to what I had in mind initially:

  else if (MEM_P (str_rtx)
           && 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)))
    /* If the mode of str_rtx is too small or unaligned
       fall back to word mode for subsequent logic.  */
    str_rtx = adjust_address (str_rtx, word_mode, 0);

this fixes the recursion on the read side too. And it is limited to cases
where the mode does not match the bitnum and bitsize parameters.


Bernd.


>
>> Bernd.
>>
>>> Richard.
>>>
>>>> -Sandra