| Message ID | CAFiYyc3Rr0emS0OeEu7+gYvtmO2-754CLjqqMUtrwgUE5U8Dqw@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
> Index: gcc/expr.c > =================================================================== > --- gcc/expr.c (revision 202778) > +++ gcc/expr.c (working copy) > @@ -9878,7 +9878,7 @@ > && modifier != EXPAND_STACK_PARM > ? target : NULL_RTX), > VOIDmode, > - modifier == EXPAND_SUM ? EXPAND_NORMAL : modifier); > + EXPAND_MEMORY); > > /* If the bitfield is volatile, we want to access it in the > field's mode, not the computed mode. > > context suggests that we may arrive with EXPAND_STACK_PARM here > which is a correctness modifier (see its docs). But I'm not too familiar > with the details of the various expand modifiers, Eric may be though. Yes, this change looks far too bold and is presumably papering over the underlying issue... > That said, I still believe that fixing the misalign path in > expand_assignment would be better than trying to avoid it. For this > testcase the issue is again that expand_assignment passes the wrong > mode/target to the > movmisalign optab. ...then let's just fix the movmisalign stuff.
Hi, On Tue, Sep 24, 2013 at 12:02:17PM +0200, Richard Biener wrote: > On Tue, Sep 24, 2013 at 4:52 AM, Bernd Edlinger > <bernd.edlinger@hotmail.de> wrote: > > Hi, > > > > with the attached patch the read-side of the out of bounds accesses are fixed. > > There is a single new test case pr57748-3.c that is derived from Martin's test case. > > The test case does only test the read access and does not depend on part 1 of the > > patch. > > > > This patch was boot-strapped and regression tested on x86_64-unknown-linux-gnu. > > > > Additionally I generated eCos and an eCos-application (on ARMv5 using packed > > structures) with an arm-eabi cross compiler, and looked for differences in the > > disassembled code with and without this patch, but there were none. > > > > OK for trunk? > > Index: gcc/expr.c > =================================================================== > --- gcc/expr.c (revision 202778) > +++ gcc/expr.c (working copy) > @@ -9878,7 +9878,7 @@ > && modifier != EXPAND_STACK_PARM > ? target : NULL_RTX), > VOIDmode, > - modifier == EXPAND_SUM ? EXPAND_NORMAL : modifier); > + EXPAND_MEMORY); > > /* If the bitfield is volatile, we want to access it in the > field's mode, not the computed mode. > > context suggests that we may arrive with EXPAND_STACK_PARM here > which is a correctness modifier (see its docs). But I'm not too familiar > with the details of the various expand modifiers, Eric may be though. > > That said, I still believe that fixing the misalign path in expand_assignment > would be better than trying to avoid it. For this testcase the issue is > again that expand_assignment passes the wrong mode/target to the > movmisalign optab. Perhaps I'm stating the obvious, but this is supposed to address a separate bug in the expansion of the RHS (as opposed to the first bug which was in the expansion of the LHS), and so we would have to make expand_assignment to also examine potential misalignments in the RHS, which it so far does not do, despite its complexity. Having said that, I also dislike the patch, but I am quite convinced that if we allow non-BLK structures with zero sized arrays, the fix will be ugly - but I'll be glad to be shown I've been wrong. Martin
On Tue, Sep 24, 2013 at 8:06 PM, Martin Jambor <mjambor@suse.cz> wrote: > Hi, > > On Tue, Sep 24, 2013 at 12:02:17PM +0200, Richard Biener wrote: >> On Tue, Sep 24, 2013 at 4:52 AM, Bernd Edlinger >> <bernd.edlinger@hotmail.de> wrote: >> > Hi, >> > >> > with the attached patch the read-side of the out of bounds accesses are fixed. >> > There is a single new test case pr57748-3.c that is derived from Martin's test case. >> > The test case does only test the read access and does not depend on part 1 of the >> > patch. >> > >> > This patch was boot-strapped and regression tested on x86_64-unknown-linux-gnu. >> > >> > Additionally I generated eCos and an eCos-application (on ARMv5 using packed >> > structures) with an arm-eabi cross compiler, and looked for differences in the >> > disassembled code with and without this patch, but there were none. >> > >> > OK for trunk? >> >> Index: gcc/expr.c >> =================================================================== >> --- gcc/expr.c (revision 202778) >> +++ gcc/expr.c (working copy) >> @@ -9878,7 +9878,7 @@ >> && modifier != EXPAND_STACK_PARM >> ? target : NULL_RTX), >> VOIDmode, >> - modifier == EXPAND_SUM ? EXPAND_NORMAL : modifier); >> + EXPAND_MEMORY); >> >> /* If the bitfield is volatile, we want to access it in the >> field's mode, not the computed mode. >> >> context suggests that we may arrive with EXPAND_STACK_PARM here >> which is a correctness modifier (see its docs). But I'm not too familiar >> with the details of the various expand modifiers, Eric may be though. >> >> That said, I still believe that fixing the misalign path in expand_assignment >> would be better than trying to avoid it. For this testcase the issue is >> again that expand_assignment passes the wrong mode/target to the >> movmisalign optab. > > Perhaps I'm stating the obvious, but this is supposed to address a > separate bug in the expansion of the RHS (as opposed to the first bug > which was in the expansion of the LHS), and so we would have to make > expand_assignment to also examine potential misalignments in the RHS, > which it so far does not do, despite its complexity. > > Having said that, I also dislike the patch, but I am quite convinced > that if we allow non-BLK structures with zero sized arrays, the fix > will be ugly - but I'll be glad to be shown I've been wrong. I don't think the issues have anything to do with zero sized arrays or non-BLK structures. The issue is just we are feeding movmisaling with the wrong mode (the mode of the base object) if we are expanding a base object which is unaligned and has non-BLK mode. So what we maybe need is another expand modifier that tells us to not use movmisalign when expanding the base object. Or we need to stop expanding the base object with VOIDmode and instead pass down the mode of the access (TYPE_MODE (TREE_TYPE (exp)) which will probably confuse other code. So eventually not handling the misaligned case by expansion of the base but inlined similar to how it was in expand_assignment would be needed here. Richard. > Martin
Hmm....., On Tue, 24 Sep 2013 20:06:51, Martin Jambor wrote: > Hi, > > On Tue, Sep 24, 2013 at 12:02:17PM +0200, Richard Biener wrote: >> On Tue, Sep 24, 2013 at 4:52 AM, Bernd Edlinger >> <bernd.edlinger@hotmail.de> wrote: >>> Hi, >>> >>> with the attached patch the read-side of the out of bounds accesses are fixed. >>> There is a single new test case pr57748-3.c that is derived from Martin's test case. >>> The test case does only test the read access and does not depend on part 1 of the >>> patch. >>> >>> This patch was boot-strapped and regression tested on x86_64-unknown-linux-gnu. >>> >>> Additionally I generated eCos and an eCos-application (on ARMv5 using packed >>> structures) with an arm-eabi cross compiler, and looked for differences in the >>> disassembled code with and without this patch, but there were none. >>> >>> OK for trunk? >> >> Index: gcc/expr.c >> =================================================================== >> --- gcc/expr.c (revision 202778) >> +++ gcc/expr.c (working copy) >> @@ -9878,7 +9878,7 @@ >> && modifier != EXPAND_STACK_PARM >> ? target : NULL_RTX), >> VOIDmode, >> - modifier == EXPAND_SUM ? EXPAND_NORMAL : modifier); >> + EXPAND_MEMORY); >> >> /* If the bitfield is volatile, we want to access it in the >> field's mode, not the computed mode. >> >> context suggests that we may arrive with EXPAND_STACK_PARM here >> which is a correctness modifier (see its docs). But I'm not too familiar >> with the details of the various expand modifiers, Eric may be though. " EXPAND_STACK_PARM is used when expanding to a TARGET on the stack for a call parameter. Such targets require special care as we haven't yet marked TARGET so that it's safe from being trashed by libcalls. We don't want to use TARGET for anything but the final result; Intermediate values must go elsewhere. Additionally, calls to emit_block_move will be flagged with BLOCK_OP_CALL_PARM." This should not be a problem: If modifier == EXPAND_STACK_PARM the target is NOT passed down, and thus not giving the modifier either should not cause incorrect code. But I might be wrong... On the other hand, in the case where tem is a unaligned MEM_REF and exp is not exactly the same object as tem, EXPAND_STACK_PARM may exactly do the wrong thing. What I can tell is, that this patch does not change a single bit in the complete GCC boot strap process, stage 2 and stage 3 are binary identical with or without this patch, and that all test cases pass, even the ADA test cases... >> That said, I still believe that fixing the misalign path in expand_assignment >> would be better than trying to avoid it. For this testcase the issue is >> again that expand_assignment passes the wrong mode/target to the >> movmisalign optab. > > Perhaps I'm stating the obvious, but this is supposed to address a > separate bug in the expansion of the RHS (as opposed to the first bug > which was in the expansion of the LHS), and so we would have to make > expand_assignment to also examine potential misalignments in the RHS, > which it so far does not do, despite its complexity. Thanks for pointing that out. This is true. > Having said that, I also dislike the patch, but I am quite convinced > that if we allow non-BLK structures with zero sized arrays, the fix > will be ugly - but I'll be glad to be shown I've been wrong. > > Martin I could of course look at the type of tem, and only mess with the expand modifier in case of a MEM_REF. But I am not sure what to do about the VIEW_CONVERT_EXPR, if the code at normal_inner_ref is wrong, the code at VIEW_CONVERT_EXPR can't possibly be any better, although I have not yet any idea how to write a test case for this: /* ??? We should work harder and deal with non-zero offsets. */ if (!offset && (bitpos % BITS_PER_UNIT) == 0 && bitsize>= 0 && compare_tree_int (TYPE_SIZE (type), bitsize) == 0) { /* See the normal_inner_ref case for the rationale. */ orig_op0 = expand_expr (tem, (TREE_CODE (TREE_TYPE (tem)) == UNION_TYPE && (TREE_CODE (TYPE_SIZE (TREE_TYPE (tem))) != INTEGER_CST) && modifier != EXPAND_STACK_PARM ? target : NULL_RTX), VOIDmode, modifier == EXPAND_SUM ? EXPAND_NORMAL : modifier); Especially for EXPAND_STACK_PARM? If this does really allocate something on a register, then the result will be thrown away, because it is not MEM_P? Right? Note the RHS-Bug can also affect ordinary unions on ARM and all STRICT_ALIGNMENT targets. Please check the attached test case. It may not be obvious at first sight, but the assembler code for check is definitely wrong, because it reads the whole structure, and uses only one byte of it. And that is, not OK because x was volatile... check: @ args = 0, pretend = 0, frame = 0 @ frame_needed = 0, uses_anonymous_args = 0 stmfd sp!, {r3, lr} mov r3, #-2147483648 ldr r3, [r3, #2] @ unaligned mov r3, r3, lsr #24 cmp r3, #3 ldmeqfd sp!, {r3, pc} bl abort Bernd. /* PR middle-end/57748 */ /* arm-eabi-gcc -mcpu=cortex-a9 -O3 */ #include <stdlib.h> union x { short a[2]; char x[4]; } __attribute__((packed, aligned(4))) ; typedef volatile union x *s; void __attribute__((noinline, noclone)) check (void) { s xx=(s)(0x80000002); if (xx->x[3] != 3) abort (); } void __attribute__((noinline, noclone)) foo (void) { s xx=(s)(0x80000002); xx->x[3] = 3; } int main () { foo (); check (); return 0; }
Hi, On Wed, Sep 25, 2013 at 11:05:21AM +0200, Richard Biener wrote: > On Tue, Sep 24, 2013 at 8:06 PM, Martin Jambor <mjambor@suse.cz> wrote: > > Hi, > > > > On Tue, Sep 24, 2013 at 12:02:17PM +0200, Richard Biener wrote: > >> On Tue, Sep 24, 2013 at 4:52 AM, Bernd Edlinger > >> <bernd.edlinger@hotmail.de> wrote: > >> > Hi, > >> > > >> > with the attached patch the read-side of the out of bounds accesses are fixed. > >> > There is a single new test case pr57748-3.c that is derived from Martin's test case. > >> > The test case does only test the read access and does not depend on part 1 of the > >> > patch. > >> > > >> > This patch was boot-strapped and regression tested on x86_64-unknown-linux-gnu. > >> > > >> > Additionally I generated eCos and an eCos-application (on ARMv5 using packed > >> > structures) with an arm-eabi cross compiler, and looked for differences in the > >> > disassembled code with and without this patch, but there were none. > >> > > >> > OK for trunk? > >> > >> Index: gcc/expr.c > >> =================================================================== > >> --- gcc/expr.c (revision 202778) > >> +++ gcc/expr.c (working copy) > >> @@ -9878,7 +9878,7 @@ > >> && modifier != EXPAND_STACK_PARM > >> ? target : NULL_RTX), > >> VOIDmode, > >> - modifier == EXPAND_SUM ? EXPAND_NORMAL : modifier); > >> + EXPAND_MEMORY); > >> > >> /* If the bitfield is volatile, we want to access it in the > >> field's mode, not the computed mode. > >> > >> context suggests that we may arrive with EXPAND_STACK_PARM here > >> which is a correctness modifier (see its docs). But I'm not too familiar > >> with the details of the various expand modifiers, Eric may be though. > >> > >> That said, I still believe that fixing the misalign path in expand_assignment > >> would be better than trying to avoid it. For this testcase the issue is > >> again that expand_assignment passes the wrong mode/target to the > >> movmisalign optab. > > > > Perhaps I'm stating the obvious, but this is supposed to address a > > separate bug in the expansion of the RHS (as opposed to the first bug > > which was in the expansion of the LHS), and so we would have to make > > expand_assignment to also examine potential misalignments in the RHS, > > which it so far does not do, despite its complexity. > > > > Having said that, I also dislike the patch, but I am quite convinced > > that if we allow non-BLK structures with zero sized arrays, the fix > > will be ugly - but I'll be glad to be shown I've been wrong. > > I don't think the issues have anything to do with zero sized arrays > or non-BLK structures. The issue is just we are feeding movmisaling > with the wrong mode (the mode of the base object) if we are expanding > a base object which is unaligned and has non-BLK mode. I disagree. Consider a slightly modified testcase: #include <stdlib.h> typedef long long V __attribute__ ((vector_size (2 * sizeof (long long)), may_alias)); #if 1 typedef struct S { V a; V b[0]; } P __attribute__((aligned (1))); struct __attribute__((packed)) T { char c; P s; }; #else typedef struct S { V a; V b[0]; } P; struct T { char c; P s; }; #endif void __attribute__((noinline, noclone)) good_value (V b) { if (b[0] != 3 || b[1] != 4) __builtin_abort (); } void __attribute__((noinline, noclone)) check (P *p) { good_value (p->b[0]); } int main () { struct T *t = (struct T *) calloc (128, 1); V a = { 3, 4 }; t->s.b[0] = a; check (&t->s); free (t); return 0; } The problem is the expansion of the memory load in function check. All involved modes, the one of the base structure and of the loaded component, are V2DI so even if we somehow mixed them up, in this example it should not matter, yet the unaligned load gives wrong results. I'm still convinced that the problem is that we have a V2DImode structure which is larger that the mode tells and we want to load the data from outside of its mode. That is only happening because zero sized arrays. > > So what we maybe need is another expand modifier that tells us > to not use movmisalign when expanding the base object. In that case we can just as well use EXPAND_MEMORY. If so, I'd do that only when there is a zero sized array involved in order not to avoid using movmisalign when we can. > Or we need to stop expanding the base object with VOIDmode and > instead pass down the mode of the access (TYPE_MODE (TREE_TYPE > (exp)) which will probably confuse other code. Well, because I think the problem is not (just) mixing up modes, I don't think this will help either. > So eventually not > handling the misaligned case by expansion of the base but inlined > similar to how it was in expand_assignment would be needed here. At least now I fail to see how this would be different from copying a large portion of expand_expr_real_1 (all of the MEM_REF part without the movmisalign bit) and calling that when we detect "this case" whatever we eventually agree it is. Martin
Hi, On Wed, 25 Sep 2013 16:01:02, Martin Jambor wrote: > Hi, > > On Wed, Sep 25, 2013 at 11:05:21AM +0200, Richard Biener wrote: >> On Tue, Sep 24, 2013 at 8:06 PM, Martin Jambor <mjambor@suse.cz> wrote: >>> Hi, >>> >>> On Tue, Sep 24, 2013 at 12:02:17PM +0200, Richard Biener wrote: >>>> On Tue, Sep 24, 2013 at 4:52 AM, Bernd Edlinger >>>> <bernd.edlinger@hotmail.de> wrote: >>>>> Hi, >>>>> >>>>> with the attached patch the read-side of the out of bounds accesses are fixed. >>>>> There is a single new test case pr57748-3.c that is derived from Martin's test case. >>>>> The test case does only test the read access and does not depend on part 1 of the >>>>> patch. >>>>> >>>>> This patch was boot-strapped and regression tested on x86_64-unknown-linux-gnu. >>>>> >>>>> Additionally I generated eCos and an eCos-application (on ARMv5 using packed >>>>> structures) with an arm-eabi cross compiler, and looked for differences in the >>>>> disassembled code with and without this patch, but there were none. >>>>> >>>>> OK for trunk? >>>> >>>> Index: gcc/expr.c >>>> =================================================================== >>>> --- gcc/expr.c (revision 202778) >>>> +++ gcc/expr.c (working copy) >>>> @@ -9878,7 +9878,7 @@ >>>> && modifier != EXPAND_STACK_PARM >>>> ? target : NULL_RTX), >>>> VOIDmode, >>>> - modifier == EXPAND_SUM ? EXPAND_NORMAL : modifier); >>>> + EXPAND_MEMORY); >>>> >>>> /* If the bitfield is volatile, we want to access it in the >>>> field's mode, not the computed mode. >>>> >>>> context suggests that we may arrive with EXPAND_STACK_PARM here >>>> which is a correctness modifier (see its docs). But I'm not too familiar >>>> with the details of the various expand modifiers, Eric may be though. >>>> >>>> That said, I still believe that fixing the misalign path in expand_assignment >>>> would be better than trying to avoid it. For this testcase the issue is >>>> again that expand_assignment passes the wrong mode/target to the >>>> movmisalign optab. >>> >>> Perhaps I'm stating the obvious, but this is supposed to address a >>> separate bug in the expansion of the RHS (as opposed to the first bug >>> which was in the expansion of the LHS), and so we would have to make >>> expand_assignment to also examine potential misalignments in the RHS, >>> which it so far does not do, despite its complexity. >>> >>> Having said that, I also dislike the patch, but I am quite convinced >>> that if we allow non-BLK structures with zero sized arrays, the fix >>> will be ugly - but I'll be glad to be shown I've been wrong. >> >> I don't think the issues have anything to do with zero sized arrays >> or non-BLK structures. The issue is just we are feeding movmisaling >> with the wrong mode (the mode of the base object) if we are expanding >> a base object which is unaligned and has non-BLK mode. > > I disagree. Consider a slightly modified testcase: > > #include <stdlib.h> > typedef long long V > __attribute__ ((vector_size (2 * sizeof (long long)), may_alias)); > > #if 1 > typedef struct S { V a; V b[0]; } P __attribute__((aligned (1))); > struct __attribute__((packed)) T { char c; P s; }; > #else > typedef struct S { V a; V b[0]; } P; > struct T { char c; P s; }; > #endif > > void __attribute__((noinline, noclone)) > good_value (V b) > { > if (b[0] != 3 || b[1] != 4) > __builtin_abort (); > } > > void __attribute__((noinline, noclone)) > check (P *p) > { > good_value (p->b[0]); > } > > int > main () > { > struct T *t = (struct T *) calloc (128, 1); > V a = { 3, 4 }; > t->s.b[0] = a; > check (&t->s); > free (t); > return 0; > } > > The problem is the expansion of the memory load in function check. > All involved modes, the one of the base structure and of the loaded > component, are V2DI so even if we somehow mixed them up, in this > example it should not matter, yet the unaligned load gives wrong > results. > > I'm still convinced that the problem is that we have a V2DImode > structure which is larger that the mode tells and we want to load the > data from outside of its mode. That is only happening because zero > sized arrays. Yes, this is another good example, and it passes vector values on the stack to a function. Again my patch produces working code, while the current trunk produces just completely _insane_ code. And again, this is not only a problem of structures with zero-sized arrays at the end. Remember my previous example code: On ARM (or anything with STRICT_ALIGNMENT) this union has the same problems: /* PR middle-end/57748 */ /* arm-eabi-gcc -mcpu=cortex-a9 -O3 */ #include <stdlib.h> union x { short a[2]; char x[4]; } __attribute__((packed, aligned(4))) ; typedef volatile union x *s; void __attribute__((noinline, noclone)) check (void) { s xx=(s)(0x80000002); /* although volatile xx->x[3] reads 4 bytes here */ if (xx->x[3] != 3) abort (); } void __attribute__((noinline, noclone)) foo (void) { s xx=(s)(0x80000002); xx->x[3] = 3; } int main () { foo (); check (); return 0; } >> >> So what we maybe need is another expand modifier that tells us >> to not use movmisalign when expanding the base object. > > In that case we can just as well use EXPAND_MEMORY. If so, I'd do > that only when there is a zero sized array involved in order not to > avoid using movmisalign when we can. > >> Or we need to stop expanding the base object with VOIDmode and >> instead pass down the mode of the access (TYPE_MODE (TREE_TYPE >> (exp)) which will probably confuse other code. > > Well, because I think the problem is not (just) mixing up modes, I > don't think this will help either. > >> So eventually not >> handling the misaligned case by expansion of the base but inlined >> similar to how it was in expand_assignment would be needed here. > > At least now I fail to see how this would be different from copying a > large portion of expand_expr_real_1 (all of the MEM_REF part without > the movmisalign bit) and calling that when we detect "this case" > whatever we eventually agree it is. > > Martin So I still think my patch does the right thing. The rationale is: = expand_expr (tem, (TREE_CODE (TREE_TYPE (tem)) == UNION_TYPE && COMPLETE_TYPE_P (TREE_TYPE (tem)) && (TREE_CODE (TYPE_SIZE (TREE_TYPE (tem))) != INTEGER_CST) && modifier != EXPAND_STACK_PARM ? target : NULL_RTX), VOIDmode, EXPAND_MEMORY); returns the address of the structure in question, we can add offset, bitoffset, and access the memory in the right mode and alignment information is passed to the backend via MEM_ALIGN (op0). Passing EXPAND_STACK_PARM would explicitly do the wrong thing, as this is not the final result but only an intermediate value. As long as MEM_ALIGN(op0) is right it does not matter if we use mov_optab or movmisalign_optab. If _that_ is a bug, then the back-ends worked around it already for years, and I don't see any reason for changing that now. Bernd.
> So I still think my patch does the right thing. > > The rationale is: > > = expand_expr (tem, > (TREE_CODE (TREE_TYPE (tem)) == UNION_TYPE > && COMPLETE_TYPE_P (TREE_TYPE (tem)) > && (TREE_CODE (TYPE_SIZE (TREE_TYPE (tem))) > != INTEGER_CST) > && modifier != EXPAND_STACK_PARM > ? target : NULL_RTX), > VOIDmode, > EXPAND_MEMORY); > > returns the address of the structure in question, > we can add offset, bitoffset, and access the memory > in the right mode and alignment information is > passed to the backend via MEM_ALIGN (op0). But there are conceptually no reasons to require a MEM here. Look at all the code just below the block. Given how hard it is to eliminate spills to memory in RTL once they are generated, this shouldn't be taken lightly.
Hi, On Thu, 26 Sep 2013 11:34:02, Eric Botcazou wrote: > >> So I still think my patch does the right thing. >> >> The rationale is: >> >> = expand_expr (tem, >> (TREE_CODE (TREE_TYPE (tem)) == UNION_TYPE >> && COMPLETE_TYPE_P (TREE_TYPE (tem)) >> && (TREE_CODE (TYPE_SIZE (TREE_TYPE (tem))) >> != INTEGER_CST) >> && modifier != EXPAND_STACK_PARM >> ? target : NULL_RTX), >> VOIDmode, >> EXPAND_MEMORY); >> >> returns the address of the structure in question, >> we can add offset, bitoffset, and access the memory >> in the right mode and alignment information is >> passed to the backend via MEM_ALIGN (op0). > > But there are conceptually no reasons to require a MEM here. Look at all the > code just below the block. Given how hard it is to eliminate spills to memory > in RTL once they are generated, this shouldn't be taken lightly. > > -- > Eric Botcazou Sure, but the modifier is not meant to force something into memory, especially when it is already in an register. Remember, we are only talking of structures here, and we only want to access one member. It is more the other way round: It says: "You do not have to load the value in a register, if it is already in memory I'm happy" At least in my understanding. Bernd.
> Sure, but the modifier is not meant to force something into memory, > especially when it is already in an register. Remember, we are only > talking of structures here, and we only want to access one member. > > It is more the other way round: > It says: "You do not have to load the value in a register, if it is already > in memory I'm happy" EXPAND_MEMORY means we are interested in a memory result, even if the memory is constant and we could have propagated a constant value. */ We definitely want to propagate constant values here, look at the code below. And it already lists explicit cases where we really need to splill to memory.
Hi Eric, On Fri, 27 Sep 2013 10:36:57, Eric Botcazou wrote: > >> Sure, but the modifier is not meant to force something into memory, >> especially when it is already in an register. Remember, we are only >> talking of structures here, and we only want to access one member. >> >> It is more the other way round: >> It says: "You do not have to load the value in a register, if it is already >> in memory I'm happy" > > EXPAND_MEMORY means we are interested in a memory result, even if > the memory is constant and we could have propagated a constant value. */ > > We definitely want to propagate constant values here, look at the code below. > And it already lists explicit cases where we really need to splill to memory. > > -- > Eric Botcazou I'd like to continue this discussion by proposing this updated patch. I have now added a new value "EXPAND_REFERENCE" to the expand_modifier enumeration. It is almost like EXPAND_MEMORY but it does not interfere with constant values. I have done the same modification to VIEW_CONVERT_EXPR too, because this is a possible inner reference, itself. It is however inherently hard to test around this code. To understand this patch it is good to know what type of object the return value "tem" of get_inner_reference can be. From the program logic at get_inner_reference it is clear that the return value may *not* be BIT_FIELD_REF, COMPONENT_REF, ARRAY_REF, ARRAY_RANGE_REF, REALPART_EXPR, IMAGPART_EXPR. The result may be VIEW_CONVERT_EXPR only on a STRICT_ALIGNMENT target. This is probably further restricted because exp is gimplified. Usually the result will be a MEM_REF or a SSA_NAME of the memory where the structure is to be found. When you look at where EXPAND_MEMORY is handled you see it is special-cased in TARGET_MEM_REF, MEM_REF, ARRAY_REF, COMPONENT_REF, BIT_FIELD_REF, ARRAY_RANGE_REF. At TARGET_MEM_REF, MEM_REF, VIEW_CONVERT_EXPR, it should be the same if EXPAND_MEMORY, EXPAND_WRITE or EXPAND_REFERENCE is given: If it is an unaligned memory, we just return the unaligned reference. This was missing for VIEW_CONVERT_EXPR, and unfortunately I have no test case, because it is only a problem for STRICT_ALIGNMENT targets, and even there it will certainly be really hard to find test cases that exercise this code. In ARRAY_REF, COMPONENT_REF, BIT_FIELD_REF, ARRAY_RANGE_REF we do not have to touch the handling of the outer modifier. However we pass EXPAND_REFERENCE to the inner object, which should not be a recursive use of any ARRAY_REF, COMPONENT_REF, BIT_FIELD_REF, ARRAY_RANGE_REF. TARGET_MEM_REF, MEM_REF and VIEW_CONVERT_EXPR know how to handle EXPAND_REFERENCE, anything else handles it like EXPAND_NORMAL. OK, what do you think of it now? Thanks Bernd. 2013-10-03 Bernd Edlinger <bernd.edlinger@hotmail.de> PR middle-end/57748 * expr.h (expand_modifier): New enum value EXPAND_REFERENCE. * expr.c (expand_expr_real_1): Use EXAND_REFERENCE on base object. testsuite/ PR middle-end/57748 * gcc.dg/torture/pr57748-3.c: New test.
> OK, what do you think of it now?
My take on this is that the Proper Fix(tm) has been posted by Martin:
http://gcc.gnu.org/ml/gcc-patches/2013-08/msg00082.html
IMO it's a no-brainer, modulo the ABI concern. Everything else is more or
less clever stuff to paper over this original compute_record_mode bug.
We are lying to the expander by pretending that the object has V2DImode since
it's larger and thus cannot be manipulated in this mode; everything is clearly
downhill from there. If we don't want to properly fix the bug then let's put
a hack in the expander, possibly using EXPAND_MEMORY, but it should trigger
_only_ for structures with zero-sized arrays and non-BLKmode and be preceded
by a big ??? comment explaining why it is deemed necessary.
Hi, On Tue, 8 Oct 2013 10:01:37, Eric Botcazou wrote: > >> OK, what do you think of it now? > > My take on this is that the Proper Fix(tm) has been posted by Martin: > http://gcc.gnu.org/ml/gcc-patches/2013-08/msg00082.html > IMO it's a no-brainer, modulo the ABI concern. Everything else is more or > less clever stuff to paper over this original compute_record_mode bug. > > We are lying to the expander by pretending that the object has V2DImode since > it's larger and thus cannot be manipulated in this mode; everything is clearly > downhill from there. If we don't want to properly fix the bug then let's put > a hack in the expander, possibly using EXPAND_MEMORY, but it should trigger > _only_ for structures with zero-sized arrays and non-BLKmode and be preceded > by a big ??? comment explaining why it is deemed necessary. > > -- > Eric Botcazou I agree, that assigning a non-BLKmode to structures with zero-sized arrays should be considered a bug. And it should be checked that there is only ONE zero-sized array. And it should be checked that it is only allowed at the end of the structure. Otherwise we have something like a union instead of a structure, which will break the code in tree-ssa-alias.c ! And again, this is not only a problem of structures with zero-sized arrays at the end. Remember my previous example code: On ARM (or anything with STRICT_ALIGNMENT) this union has the same problems: /* PR middle-end/57748 */ /* arm-eabi-gcc -mcpu=cortex-a9 -O3 */ #include <stdlib.h> union x { short a[2]; char x[4]; } __attribute__((packed, aligned(4))) ; typedef volatile union x *s; void __attribute__((noinline, noclone)) check (void) { s xx=(s)(0x80000002); /* although volatile xx->x[3] reads 4 bytes here */ if (xx->x[3] != 3) abort (); } void __attribute__((noinline, noclone)) foo (void) { s xx=(s)(0x80000002); xx->x[3] = 3; } int main () { foo (); check (); return 0; } This union has a UINT32 mode, look at compute_record_mode! Because of this example I still think that the expander should know what we intend to do with the base object. Regards Bernd.
> 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. > And again, this is not only a problem of structures with zero-sized > arrays at the end. Remember my previous example code: > On ARM (or anything with STRICT_ALIGNMENT) this union has the > same problems: > > /* PR middle-end/57748 */ > /* arm-eabi-gcc -mcpu=cortex-a9 -O3 */ > #include <stdlib.h> > > union x > { > short a[2]; > char x[4]; > } __attribute__((packed, aligned(4))) ; > typedef volatile union x *s; > > void __attribute__((noinline, noclone)) > check (void) > { > s xx=(s)(0x80000002); > /* although volatile xx->x[3] reads 4 bytes here */ > if (xx->x[3] != 3) > abort (); > } > > void __attribute__((noinline, noclone)) > foo (void) > { > s xx=(s)(0x80000002); > xx->x[3] = 3; > } > > int > main () > { > foo (); > check (); > return 0; > } 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.
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... Regards Bernd.
> That would definitely be a good move. Maybe someone should approve it?
Yes, but I agree that we ought to restrict it to trailing zero-sized arrays.
That would help to allay Jakub's concerns about possible ABI fallouts.
Hi, >> That would definitely be a good move. Maybe someone should approve it? > > Yes, but I agree that we ought to restrict it to trailing zero-sized arrays. > That would help to allay Jakub's concerns about possible ABI fallouts. > > -- > Eric Botcazou Other uses of zero-sized arrays in structures and unions are at least questionable, and should be rejected. While this construct produces an error: struct s { int a[]; float b; }; error: flexible array member not at end of struct This does not even produce a waning: struct s { int a[0]; float b; }; Would you agree that this "error: flexible array member" should also be emitted for a zero-sized array member, maybe as "error: zero-sized array member not at end of struct"? Regards Bernd.
> Would you agree that this "error: flexible array member" > should also be emitted for a zero-sized array member, > maybe as "error: zero-sized array member not at end of struct"? I would have answered yes when zero-sized arrays where introduced, but it's far less clear a couple of decades later IMO.
Hi Eric, >> Would you agree that this "error: flexible array member" >> should also be emitted for a zero-sized array member, >> maybe as "error: zero-sized array member not at end of struct"? > > I would have answered yes when zero-sized arrays where introduced, but it's > far less clear a couple of decades later IMO. But if zero-sized arrays everywhere in a structure is valid C, then the attached test case is a valid test case. And it will break your patch for PR58570 at -O1 and above, because you can no longer assume that different array members are not an alias. Regards Bernd. int printf (const char *, ...); struct S { char a[0]; short b[1]; }; int e = 1, i; static struct S d; int main () { if (e) { d.b[i]=!d.b[i]; //0->1 d.a[i]=!d.a[i]; //1->0 d.b[i]=!d.b[i]; //0->1 } printf ("%d\n", d.b[0]); return 0; }
> But if zero-sized arrays everywhere in a structure is valid C, > then the attached test case is a valid test case. Not necessarily, you can write the declaration but you cannot index the array, i.e. this is undefined behavior. And there is nothing new, distinct fields have been disambiguated in alias.c for ages.
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. Boot-strapped and regression-tested on x86_64-linux-gnu. Ok for trunk? Regards Bernd. 2013-10-22 Martin Jambor <mjambor@suse.cz> Bernd Edlinger <bernd.edlinger@hotmail.de> PR middle-end/57748 * stor-layout.c (compute_record_mode): Treat trailing zero-sized array fields like incomplete types. testsuite: 2013-10-22 Bernd Edlinger <bernd.edlinger@hotmail.de> PR middle-end/57748 * gcc.dg/torture/pr57748-3.c: New test.
On Tue, Oct 22, 2013 at 12:25 PM, Bernd Edlinger <bernd.edlinger@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.
Hi, On Wed, Oct 23, 2013 at 06:00:43PM +0200, Richard Biener wrote: > ... > ? And why should the same issue not exist for > > struct { V a[1]; } > indeed, it does and it's simple to trigger (on x86_64): ---------------------------------------- /* { dg-do run } */ #include <stdlib.h> typedef long long V __attribute__ ((vector_size (2 * sizeof (long long)), may_alias)); #if 1 typedef struct S {V b[1]; } P __attribute__((aligned (1))); struct __attribute__((packed)) T { char c; P s; }; #else typedef struct S {V b[1]; } P; struct T { char c; P s; }; #endif void __attribute__((noinline, noclone)) good_value (V b) { if (b[0] != 3 || b[1] != 4) __builtin_abort (); } void __attribute__((noinline, noclone)) check (P *p) { good_value (p->b[1]); } int main () { struct T *t = (struct T *) calloc (128, 1); V a = { 3, 4 }; t->s.b[1] = a; check (&t->s); free (t); return 0; } ---------------------------------------- While I was willing to discount the zero sized array as an insufficiently specified oddity, this seems to be much more serious and potentially common. It seems we really need to be able to detect these out-of-bounds situations and tell lower levels of expander that "whatever mode you think you are expanding, it is actually BLK mode." It's uglier than I thought. Martin > 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.
> That looks backwards. Why should > > struct { V i; V j[0]; } > > have a different mode than > > struct { V j[0]; V i; } > > ? Because we support out-of-bounds access for the array in the former case and not in the latter case. > 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. It's OK *only* if TYPE_SIZE (type) is really the size of all the objects of the type; if it isn't, i.e. if we allow access past TYPE_SIZE (type), then we cannot use the field's mode. So we need to decide where to draw the line.
> While I was willing to discount the zero sized array as an > insufficiently specified oddity, this seems to be much more serious > and potentially common. It seems we really need to be able to detect > these out-of-bounds situations and tell lower levels of expander that > "whatever mode you think you are expanding, it is actually BLK mode." Please let's not enter this hazardous business. We just need to draw a line somewhere and clearly state what we support in terms of out-of-bounds access.
Hi Martin, On Wed, 23 Oct 2013 19:11:06, Martin Jambor wrote: > On Wed, Oct 23, 2013 at 06:00:43PM +0200, Richard Biener wrote: >> > > ... > >> ? And why should the same issue not exist for >> >> struct { V a[1]; } >> > > indeed, it does and it's simple to trigger (on x86_64): > > ---------------------------------------- > /* { dg-do run } */ > > #include <stdlib.h> > > typedef long long V > __attribute__ ((vector_size (2 * sizeof (long long)), may_alias)); > > #if 1 > typedef struct S {V b[1]; } P __attribute__((aligned (1))); > struct __attribute__((packed)) T { char c; P s; }; > #else > typedef struct S {V b[1]; } P; > struct T { char c; P s; }; > #endif > > void __attribute__((noinline, noclone)) > good_value (V b) > { > if (b[0] != 3 || b[1] != 4) > __builtin_abort (); > } > > void __attribute__((noinline, noclone)) > check (P *p) > { > good_value (p->b[1]); > } > > int > main () > { > struct T *t = (struct T *) calloc (128, 1); > V a = { 3, 4 }; > t->s.b[1] = a; > check (&t->s); > free (t); > return 0; > } > ---------------------------------------- > > While I was willing to discount the zero sized array as an > insufficiently specified oddity, this seems to be much more serious > and potentially common. It seems we really need to be able to detect > these out-of-bounds situations and tell lower levels of expander that > "whatever mode you think you are expanding, it is actually BLK mode." > > It's uglier than I thought. > > Martin > Deja-vu... Thanks for this test case. This definitely destroys the idea to fix this in stor-layout.c I think that is common practice to write array[1]; at the end of the structure, and use it as a flexible array. The compute_record_mode has no way to decide if that is going to be a flexible array or not. Sorry Eric, but now I have no other choice than to go back to my previous version of part 2: http://gcc.gnu.org/ml/gcc-patches/2013-10/msg00222.html I'd just add Martin's new test case as 57748-4.c. Bernd.
> I think that is common practice to write array[1]; at the end of the > structure, and use it as a flexible array. The compute_record_mode has no > way to decide if that is going to be a flexible array or not. > > Sorry Eric, but now I have no other choice than to go back to my previous > version of part 2: > > http://gcc.gnu.org/ml/gcc-patches/2013-10/msg00222.html Why? Just set BLKmode in this case as well.
>> I think that is common practice to write array[1]; at the end of the >> structure, and use it as a flexible array. The compute_record_mode has no >> way to decide if that is going to be a flexible array or not. >> >> Sorry Eric, but now I have no other choice than to go back to my previous >> version of part 2: >> >> http://gcc.gnu.org/ml/gcc-patches/2013-10/msg00222.html > > Why? Just set BLKmode in this case as well. Because of the ABI-change? > > -- > Eric Botcazou
> Because of the ABI-change?
While concerns about accidental ABI changes are real, they shouldn't be
overstated either. We have been saying for longer than a decade that the
back-ends must not use the type mode to implement calling conventions and
other external interfaces.
On Thu, Oct 24, 2013 at 10:48 AM, Eric Botcazou <ebotcazou@adacore.com> wrote: >> I think that is common practice to write array[1]; at the end of the >> structure, and use it as a flexible array. The compute_record_mode has no >> way to decide if that is going to be a flexible array or not. >> >> Sorry Eric, but now I have no other choice than to go back to my previous >> version of part 2: >> >> http://gcc.gnu.org/ml/gcc-patches/2013-10/msg00222.html > > Why? Just set BLKmode in this case as well. Works for me if it works ABI-wise (which you say it should unless the backend is buggy). Note that means mode_for_array should unconditionally return BLKmode. Richard. > -- > Eric Botcazou
On Thu, Oct 24, 2013 at 9:15 AM, Bernd Edlinger <bernd.edlinger@hotmail.de> wrote: > Hi Martin, > > On Wed, 23 Oct 2013 19:11:06, Martin Jambor wrote: >> On Wed, Oct 23, 2013 at 06:00:43PM +0200, Richard Biener wrote: >>> >> >> ... >> >>> ? And why should the same issue not exist for >>> >>> struct { V a[1]; } >>> >> >> indeed, it does and it's simple to trigger (on x86_64): >> >> ---------------------------------------- >> /* { dg-do run } */ >> >> #include <stdlib.h> >> >> typedef long long V >> __attribute__ ((vector_size (2 * sizeof (long long)), may_alias)); >> >> #if 1 >> typedef struct S {V b[1]; } P __attribute__((aligned (1))); >> struct __attribute__((packed)) T { char c; P s; }; >> #else >> typedef struct S {V b[1]; } P; >> struct T { char c; P s; }; >> #endif >> >> void __attribute__((noinline, noclone)) >> good_value (V b) >> { >> if (b[0] != 3 || b[1] != 4) >> __builtin_abort (); >> } >> >> void __attribute__((noinline, noclone)) >> check (P *p) >> { >> good_value (p->b[1]); >> } >> >> int >> main () >> { >> struct T *t = (struct T *) calloc (128, 1); >> V a = { 3, 4 }; >> t->s.b[1] = a; >> check (&t->s); >> free (t); >> return 0; >> } >> ---------------------------------------- >> >> While I was willing to discount the zero sized array as an >> insufficiently specified oddity, this seems to be much more serious >> and potentially common. It seems we really need to be able to detect >> these out-of-bounds situations and tell lower levels of expander that >> "whatever mode you think you are expanding, it is actually BLK mode." >> >> It's uglier than I thought. >> >> Martin >> > > Deja-vu... > > Thanks for this test case. This definitely destroys the idea to fix this in stor-layout.c > > I think that is common practice to write array[1]; at the end of the structure, > and use it as a flexible array. Same for stuff like struct X { char c[4]; }; that currently gets SImode. In alias handling we treat all trailing arrays as flexible, even if they happen to be nested in sub-structs like for struct X { int i; struct Y { char c[4]; } }; too much code out in the wild to disallow this. > The compute_record_mode has no way to > decide if that is going to be a flexible array or not. But it could assign BLKmode to _all_ array types. Richard. > Sorry Eric, but now I have no other choice than to go back to my previous version > of part 2: > > http://gcc.gnu.org/ml/gcc-patches/2013-10/msg00222.html > > I'd just add Martin's new test case as 57748-4.c. > > > Bernd.
On Thu, Oct 24, 2013 at 12:22 PM, Richard Biener <richard.guenther@gmail.com> wrote: > On Thu, Oct 24, 2013 at 9:15 AM, Bernd Edlinger > <bernd.edlinger@hotmail.de> wrote: >> Hi Martin, >> >> On Wed, 23 Oct 2013 19:11:06, Martin Jambor wrote: >>> On Wed, Oct 23, 2013 at 06:00:43PM +0200, Richard Biener wrote: >>>> >>> >>> ... >>> >>>> ? And why should the same issue not exist for >>>> >>>> struct { V a[1]; } >>>> >>> >>> indeed, it does and it's simple to trigger (on x86_64): >>> >>> ---------------------------------------- >>> /* { dg-do run } */ >>> >>> #include <stdlib.h> >>> >>> typedef long long V >>> __attribute__ ((vector_size (2 * sizeof (long long)), may_alias)); >>> >>> #if 1 >>> typedef struct S {V b[1]; } P __attribute__((aligned (1))); >>> struct __attribute__((packed)) T { char c; P s; }; >>> #else >>> typedef struct S {V b[1]; } P; >>> struct T { char c; P s; }; >>> #endif >>> >>> void __attribute__((noinline, noclone)) >>> good_value (V b) >>> { >>> if (b[0] != 3 || b[1] != 4) >>> __builtin_abort (); >>> } >>> >>> void __attribute__((noinline, noclone)) >>> check (P *p) >>> { >>> good_value (p->b[1]); >>> } >>> >>> int >>> main () >>> { >>> struct T *t = (struct T *) calloc (128, 1); >>> V a = { 3, 4 }; >>> t->s.b[1] = a; >>> check (&t->s); >>> free (t); >>> return 0; >>> } >>> ---------------------------------------- >>> >>> While I was willing to discount the zero sized array as an >>> insufficiently specified oddity, this seems to be much more serious >>> and potentially common. It seems we really need to be able to detect >>> these out-of-bounds situations and tell lower levels of expander that >>> "whatever mode you think you are expanding, it is actually BLK mode." >>> >>> It's uglier than I thought. >>> >>> Martin >>> >> >> Deja-vu... >> >> Thanks for this test case. This definitely destroys the idea to fix this in stor-layout.c >> >> I think that is common practice to write array[1]; at the end of the structure, >> and use it as a flexible array. > > Same for stuff like > > struct X { char c[4]; }; > > that currently gets SImode. In alias handling we treat all trailing > arrays as flexible, even if they happen to be nested in sub-structs like for > > struct X { int i; struct Y { char c[4]; } }; > > too much code out in the wild to disallow this. > >> The compute_record_mode has no way to >> decide if that is going to be a flexible array or not. > > But it could assign BLKmode to _all_ array types. And if it is to prevent ICEs then even struct Y { char c[1]; char c2; } which gets HImode might be used as ((struct Y *)p)->c[1] - invalid but still shouldn't ICE in any way. Richard. > Richard. > >> Sorry Eric, but now I have no other choice than to go back to my previous version >> of part 2: >> >> http://gcc.gnu.org/ml/gcc-patches/2013-10/msg00222.html >> >> I'd just add Martin's new test case as 57748-4.c. >> >> >> Bernd.
Index: gcc/expr.c =================================================================== --- gcc/expr.c (revision 202778) +++ gcc/expr.c (working copy) @@ -9878,7 +9878,7 @@ && modifier != EXPAND_STACK_PARM ? target : NULL_RTX), VOIDmode, - modifier == EXPAND_SUM ? EXPAND_NORMAL : modifier); + EXPAND_MEMORY); /* If the bitfield is volatile, we want to access it in the field's mode, not the computed mode.