On Tue, Oct 22, 2013 at 12:25 PM, Bernd Edlinger <bernd.edlin...@hotmail.de> wrote: > Hi, > >> On Tue, 8 Oct 2013 22:50:21, Eric Botcazou wrote: >>> >>>> I agree, that assigning a non-BLKmode to structures with zero-sized arrays >>>> should be considered a bug. >>> >>> Fine, then let's apply Martin's patch, on mainline at least. >>> >> >> That would definitely be a good move. Maybe someone should approve it? >> >>> But this testcase is invalid on STRICT_ALIGNMENT platforms: xx is pointer >>> to a >>> type with 4-byte alignment so its value must be a multiple of 4. >> >> Then you probably win. But I still have some doubts. >> >> I had to use this silly alignment/pack(4) to circumvent this statement >> in compute_record_mode: >> >> /* If structure's known alignment is less than what the scalar >> mode would need, and it matters, then stick with BLKmode. */ >> if (TYPE_MODE (type) != BLKmode >> && STRICT_ALIGNMENT >> && ! (TYPE_ALIGN (type)>= BIGGEST_ALIGNMENT >> || TYPE_ALIGN (type)>= GET_MODE_ALIGNMENT (TYPE_MODE (type)))) >> { >> /* If this is the only reason this type is BLKmode, then >> don't force containing types to be BLKmode. */ >> TYPE_NO_FORCE_BLK (type) = 1; >> SET_TYPE_MODE (type, BLKmode); >> } >> >> But there are at least two targets where STRICT_ALIGNMENT = 0 >> and SLOW_UNALIGNED_ACCESS != 0: rs6000 and alpha. >> >> This example with a byte-aligned structure will on one of these targets >> likely execute this code path in expand_expr_real_1/case MEM_REF: >> >> else if (SLOW_UNALIGNED_ACCESS (mode, align)) >> temp = extract_bit_field (temp, GET_MODE_BITSIZE (mode), >> 0, TYPE_UNSIGNED (TREE_TYPE (exp)), >> (modifier == EXPAND_STACK_PARM >> ? NULL_RTX : target), >> mode, mode); >> >> This looks wrong, but unfortunately I cannot test on these targets... >> > > Hmm, well, > > the condition that would be necessary to execute that code path > would be > > STRICT_ALIGNMENT = 0 > and SLOW_UNALIGNED_ACCESS != 0 for any integer mode. > > The only target that is close to hit this "bug" is rs6000: > > #define STRICT_ALIGNMENT 0 > > #define SLOW_UNALIGNED_ACCESS(MODE, ALIGN) \ > (STRICT_ALIGNMENT \ > || (((MODE) == SFmode || (MODE) == DFmode || (MODE) == TFmode \ > || (MODE) == SDmode || (MODE) == DDmode || (MODE) == TDmode) \ > && (ALIGN) < 32) \ > || (VECTOR_MODE_P ((MODE)) && (((int)(ALIGN)) < VECTOR_ALIGN (MODE)))) > > but, luckily this is 0 for all integer modes. > > So I am now convinced, there won't be any valid example with unions that > executes this code path. > > Therefore I updated Martin's previous patch, to meet Eric's request: > That is to only handle zero-sized arrays at the end of the structure.
That looks backwards. Why should struct { V i; V j[0]; } have a different mode than struct { V j[0]; V i; } ? And why should the same issue not exist for struct { V a[1]; } which is also "flexible enough" that we support accesses beyond it via a pointer? That struct also has V2DImode. And this is all because /* If this field is the whole struct, remember its mode so that, say, we can put a double in a class into a DF register instead of forcing it to live in the stack. */ if (simple_cst_equal (TYPE_SIZE (type), DECL_SIZE (field))) mode = DECL_MODE (field); which is IMHO ok. > Boot-strapped and regression-tested on x86_64-linux-gnu. > > Ok for trunk? Well, I'm not so sure. And if so then I'd prefer martins original patch, rejecting all zero-sized fields. But then only if you consider it a "real" field. Of course I blame those that added the zero-sized array extension for all this mess ... maybe we can reduce it by rejecting zero-sized arrays that are not at the end of a structure - which means we can demote them to flexible arrays with a NULL TYPE_SIZE which would completely side-step this issue in stor-layout.c. Richard. > Regards > Bernd.