| Message ID | 5271A851.6000808@codesourcery.com |
|---|---|
| State | New |
| Headers | show |
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.
Hi,
This modified test case exposes a bug in the already approved part of the strict-volatile-bitfields patch:
#include <stdlib.h>
typedef struct {
char pad;
int arr[0];
} __attribute__((packed)) str;
str *
foo (int* src)
{
str *s = malloc (sizeof (str) + sizeof (int));
s->arr[0] = 0x12345678;
asm volatile("":::"memory");
*src = s->arr[0];
return s;
}
As we know this test case triggered a recursion in the store_bit_field on ARM and on PowerPC,
which is no longer reproducible after this patch is applied: http://gcc.gnu.org/ml/gcc-patches/2013-11/msg02025.html
Additionally it triggered a recursion on extract_bit_field, but _only_ on my local copy of the trunk.
I had this patch installed, but did not expect it to change anything unless the values are volatile.
That was cased by this hunk in the strict-volatile-bitfields v4 patch:
@@ -1691,45 +1736,19 @@ extract_fixed_bit_field (enum machine_mo
includes the entire field. If such a mode would be larger than
a word, we won't be doing the extraction the normal way. */
- if (MEM_VOLATILE_P (op0)
- && flag_strict_volatile_bitfields> 0)
- {
- if (GET_MODE_BITSIZE (GET_MODE (op0))> 0)
- mode = GET_MODE (op0);
- else if (target && GET_MODE_BITSIZE (GET_MODE (target))> 0)
- mode = GET_MODE (target);
- else
- mode = tmode;
- }
- else
- mode = get_best_mode (bitsize, bitnum, 0, 0,
- MEM_ALIGN (op0), word_mode, MEM_VOLATILE_P (op0));
+ 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, 0, 0,
+ MEM_ALIGN (op0), mode, MEM_VOLATILE_P (op0));
if (mode == VOIDmode)
/* The only way this should occur is if the field spans word
boundaries. */
return extract_split_bit_field (op0, bitsize, bitnum, unsignedp);
So the problem started, because initially this function did not look at GET_MODE(op0)
and always used word_mode. That was changed, but now also affected non-volatile data.
Now, if we solve this differently and install the C++ memory model patch,
we can avoid to introduce the recursion in the extract path,
and remove these two hunks in the update patch at the same time:
+ 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);
Attached you'll find a new version of the bitfields-update patch,
it is again relative to the already approved version of the volatile-bitfields patch v4, part 1/2.
Boot-strapped and regression-tested on X86_64-linux-gnu.
additionally tested with an ARM cross-compiler.
OK for trunk?
Thanks
Bernd.
2013-11-18 Bernd Edlinger <bernd.edlinger@hotmail.de>
Sandra Loosemore <sandra@codesourcery.com>
PR middle-end/23623
PR middle-end/48784
PR middle-end/56341
PR middle-end/56997
* expmed.c (strict_volatile_bitfield_p): Add bitregion_start
and bitregion_end parameters. Test for compliance with C++
memory model.
(store_bit_field): Adjust call to strict_volatile_bitfield_p.
Add fallback logic for cases where -fstrict-volatile-bitfields
is supposed to apply, but cannot.
(extract_bit_field): Likewise. Use narrow_bit_field_mem and
extract_fixed_bit_field_1 to do the extraction.
(extract_fixed_bit_field): Revert to previous mode selection algorithm.
Call extract_fixed_bit_field_1 to do the real work.
(extract_fixed_bit_field_1): New function.
testsuite:
2013-11-18 Bernd Edlinger <bernd.edlinger@hotmail.de>
Sandra Loosemore <sandra@codesourcery.com>
* gcc.dg/pr23623.c: Update to test interaction with C++
memory model.
On Mon, Nov 18, 2013 at 1:11 PM, Bernd Edlinger <bernd.edlinger@hotmail.de> wrote: > Hi, > > > This modified test case exposes a bug in the already approved part of the strict-volatile-bitfields patch: > > #include <stdlib.h> > > typedef struct { > char pad; > int arr[0]; > } __attribute__((packed)) str; > > str * > foo (int* src) > { > str *s = malloc (sizeof (str) + sizeof (int)); > s->arr[0] = 0x12345678; > asm volatile("":::"memory"); > *src = s->arr[0]; > return s; > } > > > As we know this test case triggered a recursion in the store_bit_field on ARM and on PowerPC, > which is no longer reproducible after this patch is applied: http://gcc.gnu.org/ml/gcc-patches/2013-11/msg02025.html > > Additionally it triggered a recursion on extract_bit_field, but _only_ on my local copy of the trunk. > I had this patch installed, but did not expect it to change anything unless the values are volatile. > That was cased by this hunk in the strict-volatile-bitfields v4 patch: > > > @@ -1691,45 +1736,19 @@ extract_fixed_bit_field (enum machine_mo > includes the entire field. If such a mode would be larger than > a word, we won't be doing the extraction the normal way. */ > > - if (MEM_VOLATILE_P (op0) > - && flag_strict_volatile_bitfields> 0) > - { > - if (GET_MODE_BITSIZE (GET_MODE (op0))> 0) > - mode = GET_MODE (op0); > - else if (target && GET_MODE_BITSIZE (GET_MODE (target))> 0) > - mode = GET_MODE (target); > - else > - mode = tmode; > - } > - else > - mode = get_best_mode (bitsize, bitnum, 0, 0, > - MEM_ALIGN (op0), word_mode, MEM_VOLATILE_P (op0)); > + 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, 0, 0, > + MEM_ALIGN (op0), mode, MEM_VOLATILE_P (op0)); > > if (mode == VOIDmode) > /* The only way this should occur is if the field spans word > boundaries. */ > return extract_split_bit_field (op0, bitsize, bitnum, unsignedp); > > So the problem started, because initially this function did not look at GET_MODE(op0) > and always used word_mode. That was changed, but now also affected non-volatile data. > > > Now, if we solve this differently and install the C++ memory model patch, > we can avoid to introduce the recursion in the extract path, > and remove these two hunks in the update patch at the same time: > > + 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); > > > > Attached you'll find a new version of the bitfields-update patch, > it is again relative to the already approved version of the volatile-bitfields patch v4, part 1/2. > > Boot-strapped and regression-tested on X86_64-linux-gnu. > additionally tested with an ARM cross-compiler. > > > OK for trunk? Ok. Thanks, Richard. > > Thanks > Bernd.
diff -u gcc/doc/invoke.texi gcc/doc/invoke.texi --- gcc/doc/invoke.texi (working copy) +++ gcc/doc/invoke.texi (working copy) @@ -21659,7 +21659,8 @@ In some cases, such as when the @code{packed} attribute is applied to a structure field, it may not be possible to access the field with a single read or write that is correctly aligned for the target machine. In this -case GCC falls back to generating multiple accesses rather than code that +case GCC falls back to generating either multiple accesses +or an access in a larger mode, rather than code that will fault or truncate the result at run time. The default value of this option is determined by the application binary diff -u gcc/testsuite/gcc.dg/pr23623.c gcc/testsuite/gcc.dg/pr23623.c --- gcc/testsuite/gcc.dg/pr23623.c (revision 0) +++ gcc/testsuite/gcc.dg/pr23623.c (revision 0) @@ -8,16 +8,19 @@ extern struct { unsigned int b : 1; + unsigned int : 31; } bf1; extern volatile struct { unsigned int b : 1; + unsigned int : 31; } bf2; extern struct { volatile unsigned int b : 1; + volatile unsigned int : 31; } bf3; void writeb(void) diff -u gcc/expmed.c gcc/expmed.c --- gcc/expmed.c (working copy) +++ gcc/expmed.c (working copy) @@ -416,12 +416,17 @@ } /* Return true if -fstrict-volatile-bitfields applies an access of OP0 - containing BITSIZE bits starting at BITNUM, with field mode FIELDMODE. */ + containing BITSIZE bits starting at BITNUM, with field mode FIELDMODE. + Return false if the access would touch memory outside the range + BITREGION_START to BITREGION_END for conformance to the C++ memory + model. */ static bool strict_volatile_bitfield_p (rtx op0, unsigned HOST_WIDE_INT bitsize, unsigned HOST_WIDE_INT bitnum, - enum machine_mode fieldmode) + enum machine_mode fieldmode, + unsigned HOST_WIDE_INT bitregion_start, + unsigned HOST_WIDE_INT bitregion_end) { unsigned HOST_WIDE_INT modesize = GET_MODE_BITSIZE (fieldmode); @@ -448,6 +453,12 @@ && bitnum % GET_MODE_ALIGNMENT (fieldmode) + bitsize > modesize)) return false; + /* Check for cases where the C++ memory model applies. */ + if (bitregion_end != 0 + && (bitnum - bitnum % modesize < bitregion_start + || bitnum - bitnum % modesize + modesize > bitregion_end)) + return false; + return true; } @@ -904,7 +915,8 @@ rtx value) { /* Handle -fstrict-volatile-bitfields in the cases where it applies. */ - if (strict_volatile_bitfield_p (str_rtx, bitsize, bitnum, fieldmode)) + if (strict_volatile_bitfield_p (str_rtx, bitsize, bitnum, fieldmode, + bitregion_start, bitregion_end)) { /* Storing any naturally aligned field can be done with a simple @@ -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 @@ -1695,7 +1715,7 @@ else mode1 = tmode; - if (strict_volatile_bitfield_p (str_rtx, bitsize, bitnum, mode1)) + if (strict_volatile_bitfield_p (str_rtx, bitsize, bitnum, mode1, 0, 0)) { rtx result; @@ -1709,6 +1729,14 @@ target, unsignedp); return convert_extracted_bit_field (result, mode, tmode, unsignedp); } + 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); return extract_bit_field_1 (str_rtx, bitsize, bitnum, unsignedp, target, mode, tmode, true);