Comments

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?
-Sandra

Can someone please review this patch?
http://gcc.gnu.org/ml/gcc-patches/2013-10/msg02637.html
I would like to commit the already-approved -fstrict-volatile-bitfields
patch once we also have an approved fix for the infinite recursion
problem I discovered while testing a backport of the patch series to a
local copy of GCC 4.8.
-Sandra

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?
Richard.
> -Sandra

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 !
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 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)))
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?
Bernd.
> Richard.>>> -Sandra

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.
> Bernd.>>> Richard.>>>>> -Sandra

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

On Fri, Nov 15, 2013 at 10:28 AM, Bernd Edlinger
<bernd.edlinger@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.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.
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

>>>> 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?>
the problem here is different, than we thought. Both recursions have been
observed initially only with volatile accesses and -fstrict-volatile-bitfields.
That's why it looked like the right thing to do. But PR59134 shows clearly
that both recursions have nothing to do with strict-volatile-bitfields. They
happen just because the hardware is not always able to access unaligned data
on one instruction. And the mode in str_rtx is sometimes too restrictive.
Now in this test case, we have s, a packed structure, but malloc retruns a
BIGGEST_ALIGNMENT. And at -O2 the back-end sees the larger alignment.
But the mode of str_rtx is QImode, because that is the mode of the struct str.
This mode is only good for accessing pad, but not for arr[0].
It does never make sense to access 32 bits in QImode, especially if the memory
is un-aligned. That is how this hunk resolves the issue.
> Richard.>>>>> Bernd.>>>>>>>>>>> Bernd.>>>>>>>>> Richard.>>>>>>>>>>> -Sandra

On Fri, Nov 15, 2013 at 1:08 PM, Bernd Edlinger
<bernd.edlinger@hotmail.de> wrote:
>>>>>> 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?>>>> the problem here is different, than we thought. Both recursions have been> observed initially only with volatile accesses and -fstrict-volatile-bitfields.> That's why it looked like the right thing to do. But PR59134 shows clearly> that both recursions have nothing to do with strict-volatile-bitfields. They> happen just because the hardware is not always able to access unaligned data> on one instruction. And the mode in str_rtx is sometimes too restrictive.>> Now in this test case, we have s, a packed structure, but malloc retruns a> BIGGEST_ALIGNMENT. And at -O2 the back-end sees the larger alignment.> But the mode of str_rtx is QImode, because that is the mode of the struct str.> This mode is only good for accessing pad, but not for arr[0].> It does never make sense to access 32 bits in QImode, especially if the memory> is un-aligned. That is how this hunk resolves the issue.
But then why is the mode QImode in the first place? The access is
definitely of SImode.
Richard.
>> Richard.>>>>>>>> Bernd.>>>>>>>>>>>>>>> Bernd.>>>>>>>>>>> Richard.>>>>>>>>>>>>> -Sandra

>> But then why is the mode QImode in the first place? The access is> definitely of SImode.>
that's in the test case:
s->arr[0] = 0x12345678;
it is QImode from that in expand_assignment:
to_rtx = expand_expr (tem, NULL_RTX, VOIDmode, EXPAND_WRITE);
tem is "s", a MEM_REF, of QImode, perfectly aligned. the mode is only
OK to access *s or s->pad. It is wrong for s->srr[0].
in store_bit_field the mode is used in store_fixed_bit_field:
mode = GET_MODE (op0);
if (GET_MODE_BITSIZE (mode) == 0
|| GET_MODE_BITSIZE (mode)> GET_MODE_BITSIZE (word_mode))
mode = word_mode;
mode = get_best_mode (bitsize, bitnum, bitregion_start, bitregion_end,
MEM_ALIGN (op0), mode, MEM_VOLATILE_P (op0));
if (mode == VOIDmode)
goto boom;
so this restricts the possible access mode. word_mode, means no restriction.
Everything would be OK if MEM_ALIGN(op0) was byte-aligned, but we have
a problem if MEM_ALIGN(op0)>=WORD_MODE.
Do you understand?
Bernd.

On Fri, Nov 15, 2013 at 2:24 PM, Bernd Edlinger
<bernd.edlinger@hotmail.de> wrote:
>>>> But then why is the mode QImode in the first place? The access is>> definitely of SImode.>>>> that's in the test case:>> s->arr[0] = 0x12345678;>>> it is QImode from that in expand_assignment:>> to_rtx = expand_expr (tem, NULL_RTX, VOIDmode, EXPAND_WRITE);>> tem is "s", a MEM_REF, of QImode, perfectly aligned. the mode is only> OK to access *s or s->pad. It is wrong for s->srr[0].
I undestand that, but why isn't that immediately adjusted to use
TYPE_MODE (TREE_TYPE (to))? The above expands *s, not s->arr[0].
Using the mode of *s if it is not equal to that of the destination doesn't
make any sense (usually it will be BLKmode anyway). This seems to
be the source of multiple problems.
> in store_bit_field the mode is used in store_fixed_bit_field:>> mode = GET_MODE (op0);> if (GET_MODE_BITSIZE (mode) == 0> || GET_MODE_BITSIZE (mode)> GET_MODE_BITSIZE (word_mode))> mode = word_mode;> mode = get_best_mode (bitsize, bitnum, bitregion_start, bitregion_end,> MEM_ALIGN (op0), mode, MEM_VOLATILE_P (op0));>> if (mode == VOIDmode)> goto boom;>> so this restricts the possible access mode. word_mode, means no restriction.> Everything would be OK if MEM_ALIGN(op0) was byte-aligned, but we have> a problem if MEM_ALIGN(op0)>=WORD_MODE.>> Do you understand?>> Bernd.