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