On Fri, Nov 15, 2013 at 10:28 AM, Bernd Edlinger
<bernd.edlin...@hotmail.de> wrote:
> Hi,
>
> On Thu, 14 Nov 2013 16:31:10, Richard Biener wrote:
>>
>> On Thu, Nov 14, 2013 at 11:16 AM, Bernd Edlinger
>> <bernd.edlin...@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
>>>> <san...@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.
But that's not conditional on -fstrict-volatile-bitfields and frankly it doesn't
make sense to me. Why is the recursion not there for
-fno-strict-volatile-bitfields?
Richard.
>
> Bernd.
>
>
>>
>>> Bernd.
>>>
>>>> Richard.
>>>>
>>>>> -Sandra
