| Message ID | 51CB6D86.9020304@arm.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Jun 26, 2013 at 3:39 PM, Yufeng Zhang <Yufeng.Zhang@arm.com> wrote: > This patch updates assign_parm_find_data_types to assign passed_mode and > nominal_mode with the mode of the built pointer type instead of the > hard-coded Pmode in the case of pass-by-reference. This is in line with the > assignment to passed_mode and nominal_mode in other cases inside the > function. > > assign_parm_find_data_types generally uses TYPE_MODE to calculate > passed_mode and nominal_mode: > > /* Find mode of arg as it is passed, and mode of arg as it should be > during execution of this function. */ > passed_mode = TYPE_MODE (passed_type); > nominal_mode = TYPE_MODE (nominal_type); > > this includes the case when the passed argument is a pointer by itself. > > However there is a discrepancy when it deals with argument passed by > invisible reference; it builds the argument's corresponding pointer type, > but sets passed_mode and nominal_mode with Pmode directly. > > This is OK for targets where Pmode == ptr_mode, but on AArch64 with ILP32 > they are different with Pmode as DImode and ptr_mode as SImode. When such a > reference is passed on stack, the reference is prepared by the caller in the > lower 4 bytes of an 8-byte slot but is fetched by the callee as an 8-byte > datum, of which the higher 4 bytes may contain junk. It is probably the > combination of Pmode != ptr_mode and the particular ABI specification that > make the AArch64 ILP32 the first target on which the issue manifests itself. > > Bootstrapped on x86_64-none-linux-gnu. > > OK for the trunk? IA64-hpux also uses Pmode != ptr_mode, can you provide the testcase which fails without this change? I used a powerpc64 target where Pmode != ptr_mode which did not hit this bug either. Thanks, Andrew > > Thanks, > Yufeng > > > gcc/ > * function.c (assign_parm_find_data_types): Set passed_mode and > nominal_mode to the TYPE_MODE of nominal_type for the built > pointer type in case of the struct-pass-by-reference.
On 06/27/13 00:04, Andrew Pinski wrote: > On Wed, Jun 26, 2013 at 3:39 PM, Yufeng Zhang<Yufeng.Zhang@arm.com> wrote: >> This patch updates assign_parm_find_data_types to assign passed_mode and >> nominal_mode with the mode of the built pointer type instead of the >> hard-coded Pmode in the case of pass-by-reference. This is in line with the >> assignment to passed_mode and nominal_mode in other cases inside the >> function. >> >> assign_parm_find_data_types generally uses TYPE_MODE to calculate >> passed_mode and nominal_mode: >> >> /* Find mode of arg as it is passed, and mode of arg as it should be >> during execution of this function. */ >> passed_mode = TYPE_MODE (passed_type); >> nominal_mode = TYPE_MODE (nominal_type); >> >> this includes the case when the passed argument is a pointer by itself. >> >> However there is a discrepancy when it deals with argument passed by >> invisible reference; it builds the argument's corresponding pointer type, >> but sets passed_mode and nominal_mode with Pmode directly. >> >> This is OK for targets where Pmode == ptr_mode, but on AArch64 with ILP32 >> they are different with Pmode as DImode and ptr_mode as SImode. When such a >> reference is passed on stack, the reference is prepared by the caller in the >> lower 4 bytes of an 8-byte slot but is fetched by the callee as an 8-byte >> datum, of which the higher 4 bytes may contain junk. It is probably the >> combination of Pmode != ptr_mode and the particular ABI specification that >> make the AArch64 ILP32 the first target on which the issue manifests itself. >> >> Bootstrapped on x86_64-none-linux-gnu. >> >> OK for the trunk? > > > IA64-hpux also uses Pmode != ptr_mode, can you provide the testcase > which fails without this change? > I used a powerpc64 target where Pmode != ptr_mode which did not hit > this bug either. The issue was firstly observed in one of the compat tests which passes a large number of non-small structures. The following is a trimmed-down reproducible code snippet (although not runnable but shall be easy to be make runnable): struct s5 { double a; double b; double c; double d; double e; } gS; double foo (struct s5 p1, struct s5 p2,struct s5 p3,struct s5 p4,struct s5 p5,struct s5 p6,struct s5 p7,struct s5 p8, struct s5 p9) { return p9.c; } --------------- CUT --------------- The code-gen (-O2) without the patch is: .text .align 2 .global foo .type foo, %function foo: ldr x0, [sp] <<=== here! ldr d0, [x0,16] ret .size foo, .-foo Where the arrow points is the load of the pointer to 'p9' that is passed on stack. The instruction really should be ldr w0, [sp], i.e. the pointer mode is SImode rather than DImode. It needs a number of conditions for the issue to manifest: 1. pass-by-reference; on aarch64 one example is a struct that is larger than 16 bytes. 2. the reference is passed on stack; on aarch64, this usually only happens after registers x0 - x7 are used. 3. the size of stack slot for passing pointer is larger than the pointer size; on aarch64, it is 8-byte vs. 4-byte 4. the unused part of the stack slot is not zeroed out, i.e. undefined by the ABI 5. in the runtime, the unused part of such a stack slot contains junk. The runtime segmentation fault may only be generated when all the above conditions are met. I'm not familiar with IA64-hpux or powerpc64 procedure call ABIs, but I guess those targets are just being lucky? Thanks, Yufeng
On Wed, Jun 26, 2013 at 4:41 PM, Yufeng Zhang <Yufeng.Zhang@arm.com> wrote: > On 06/27/13 00:04, Andrew Pinski wrote: >> >> On Wed, Jun 26, 2013 at 3:39 PM, Yufeng Zhang<Yufeng.Zhang@arm.com> >> wrote: >>> >>> This patch updates assign_parm_find_data_types to assign passed_mode and >>> nominal_mode with the mode of the built pointer type instead of the >>> hard-coded Pmode in the case of pass-by-reference. This is in line with >>> the >>> assignment to passed_mode and nominal_mode in other cases inside the >>> function. >>> >>> assign_parm_find_data_types generally uses TYPE_MODE to calculate >>> passed_mode and nominal_mode: >>> >>> /* Find mode of arg as it is passed, and mode of arg as it should be >>> during execution of this function. */ >>> passed_mode = TYPE_MODE (passed_type); >>> nominal_mode = TYPE_MODE (nominal_type); >>> >>> this includes the case when the passed argument is a pointer by itself. >>> >>> However there is a discrepancy when it deals with argument passed by >>> invisible reference; it builds the argument's corresponding pointer type, >>> but sets passed_mode and nominal_mode with Pmode directly. >>> >>> This is OK for targets where Pmode == ptr_mode, but on AArch64 with ILP32 >>> they are different with Pmode as DImode and ptr_mode as SImode. When such >>> a >>> reference is passed on stack, the reference is prepared by the caller in >>> the >>> lower 4 bytes of an 8-byte slot but is fetched by the callee as an 8-byte >>> datum, of which the higher 4 bytes may contain junk. It is probably the >>> combination of Pmode != ptr_mode and the particular ABI specification >>> that >>> make the AArch64 ILP32 the first target on which the issue manifests >>> itself. >>> >>> Bootstrapped on x86_64-none-linux-gnu. >>> >>> OK for the trunk? >> >> >> >> IA64-hpux also uses Pmode != ptr_mode, can you provide the testcase >> which fails without this change? >> I used a powerpc64 target where Pmode != ptr_mode which did not hit >> this bug either. > > > The issue was firstly observed in one of the compat tests which passes a > large number of non-small structures. The following is a trimmed-down > reproducible code snippet (although not runnable but shall be easy to be > make runnable): > > struct s5 > { > double a; > double b; > double c; > double d; > double e; > } gS; > > double foo (struct s5 p1, struct s5 p2,struct s5 p3,struct s5 p4,struct s5 > p5,struct s5 p6,struct s5 p7,struct s5 p8, struct s5 p9) > { > return p9.c; > } > --------------- CUT --------------- > > The code-gen (-O2) without the patch is: > > .text > .align 2 > .global foo > .type foo, %function > foo: > ldr x0, [sp] <<=== here! > ldr d0, [x0,16] > ret > .size foo, .-foo > > Where the arrow points is the load of the pointer to 'p9' that is passed on > stack. The instruction really should be ldr w0, [sp], i.e. the pointer mode > is SImode rather than DImode. > > It needs a number of conditions for the issue to manifest: > > 1. pass-by-reference; on aarch64 one example is a struct that is larger than > 16 bytes. > 2. the reference is passed on stack; on aarch64, this usually only happens > after registers x0 - x7 are used. > 3. the size of stack slot for passing pointer is larger than the pointer > size; on aarch64, it is 8-byte vs. 4-byte > 4. the unused part of the stack slot is not zeroed out, i.e. undefined by > the ABI This is the real issue. I think it is better if we change the ABI to say they are zero'd. It really makes things like this a mess. > 5. in the runtime, the unused part of such a stack slot contains junk. > > The runtime segmentation fault may only be generated when all the above > conditions are met. I'm not familiar with IA64-hpux or powerpc64 procedure > call ABIs, but I guess those targets are just being lucky? Or rather their ABIs all say are zero or sign extended for values less than 8 byte wide. Thanks, Andrew Pinski
On Wed, Jun 26, 2013 at 4:51 PM, Andrew Pinski <pinskia@gmail.com> wrote: > On Wed, Jun 26, 2013 at 4:41 PM, Yufeng Zhang <Yufeng.Zhang@arm.com> wrote: >> On 06/27/13 00:04, Andrew Pinski wrote: >>> >>> On Wed, Jun 26, 2013 at 3:39 PM, Yufeng Zhang<Yufeng.Zhang@arm.com> >>> wrote: >>>> >>>> This patch updates assign_parm_find_data_types to assign passed_mode and >>>> nominal_mode with the mode of the built pointer type instead of the >>>> hard-coded Pmode in the case of pass-by-reference. This is in line with >>>> the >>>> assignment to passed_mode and nominal_mode in other cases inside the >>>> function. >>>> >>>> assign_parm_find_data_types generally uses TYPE_MODE to calculate >>>> passed_mode and nominal_mode: >>>> >>>> /* Find mode of arg as it is passed, and mode of arg as it should be >>>> during execution of this function. */ >>>> passed_mode = TYPE_MODE (passed_type); >>>> nominal_mode = TYPE_MODE (nominal_type); >>>> >>>> this includes the case when the passed argument is a pointer by itself. >>>> >>>> However there is a discrepancy when it deals with argument passed by >>>> invisible reference; it builds the argument's corresponding pointer type, >>>> but sets passed_mode and nominal_mode with Pmode directly. >>>> >>>> This is OK for targets where Pmode == ptr_mode, but on AArch64 with ILP32 >>>> they are different with Pmode as DImode and ptr_mode as SImode. When such >>>> a >>>> reference is passed on stack, the reference is prepared by the caller in >>>> the >>>> lower 4 bytes of an 8-byte slot but is fetched by the callee as an 8-byte >>>> datum, of which the higher 4 bytes may contain junk. It is probably the >>>> combination of Pmode != ptr_mode and the particular ABI specification >>>> that >>>> make the AArch64 ILP32 the first target on which the issue manifests >>>> itself. >>>> >>>> Bootstrapped on x86_64-none-linux-gnu. >>>> >>>> OK for the trunk? >>> >>> >>> >>> IA64-hpux also uses Pmode != ptr_mode, can you provide the testcase >>> which fails without this change? >>> I used a powerpc64 target where Pmode != ptr_mode which did not hit >>> this bug either. >> >> >> The issue was firstly observed in one of the compat tests which passes a >> large number of non-small structures. The following is a trimmed-down >> reproducible code snippet (although not runnable but shall be easy to be >> make runnable): >> >> struct s5 >> { >> double a; >> double b; >> double c; >> double d; >> double e; >> } gS; >> >> double foo (struct s5 p1, struct s5 p2,struct s5 p3,struct s5 p4,struct s5 >> p5,struct s5 p6,struct s5 p7,struct s5 p8, struct s5 p9) >> { >> return p9.c; >> } >> --------------- CUT --------------- >> >> The code-gen (-O2) without the patch is: >> >> .text >> .align 2 >> .global foo >> .type foo, %function >> foo: >> ldr x0, [sp] <<=== here! >> ldr d0, [x0,16] >> ret >> .size foo, .-foo >> >> Where the arrow points is the load of the pointer to 'p9' that is passed on >> stack. The instruction really should be ldr w0, [sp], i.e. the pointer mode >> is SImode rather than DImode. >> >> It needs a number of conditions for the issue to manifest: >> >> 1. pass-by-reference; on aarch64 one example is a struct that is larger than >> 16 bytes. >> 2. the reference is passed on stack; on aarch64, this usually only happens >> after registers x0 - x7 are used. >> 3. the size of stack slot for passing pointer is larger than the pointer >> size; on aarch64, it is 8-byte vs. 4-byte >> 4. the unused part of the stack slot is not zeroed out, i.e. undefined by >> the ABI > > This is the real issue. I think it is better if we change the ABI to > say they are zero'd. It really makes things like this a mess. > >> 5. in the runtime, the unused part of such a stack slot contains junk. >> >> The runtime segmentation fault may only be generated when all the above >> conditions are met. I'm not familiar with IA64-hpux or powerpc64 procedure >> call ABIs, but I guess those targets are just being lucky? > > Or rather their ABIs all say are zero or sign extended for values less > than 8 byte wide. One more thing, it looks like your change will not work correctly for big-endian ILP32 AARCH64 either as the least significant word is offsetted by 4. Did you test big-endian ILP32 AARCH64? Thanks, Andrew Pinski
On 06/27/13 00:51, Andrew Pinski wrote: > On Wed, Jun 26, 2013 at 4:41 PM, Yufeng Zhang<Yufeng.Zhang@arm.com> wrote: >> On 06/27/13 00:04, Andrew Pinski wrote: >>> >>> On Wed, Jun 26, 2013 at 3:39 PM, Yufeng Zhang<Yufeng.Zhang@arm.com> >>> wrote: >>>> >>>> This patch updates assign_parm_find_data_types to assign passed_mode and >>>> nominal_mode with the mode of the built pointer type instead of the >>>> hard-coded Pmode in the case of pass-by-reference. This is in line with >>>> the >>>> assignment to passed_mode and nominal_mode in other cases inside the >>>> function. >>>> >>>> assign_parm_find_data_types generally uses TYPE_MODE to calculate >>>> passed_mode and nominal_mode: >>>> >>>> /* Find mode of arg as it is passed, and mode of arg as it should be >>>> during execution of this function. */ >>>> passed_mode = TYPE_MODE (passed_type); >>>> nominal_mode = TYPE_MODE (nominal_type); >>>> >>>> this includes the case when the passed argument is a pointer by itself. >>>> >>>> However there is a discrepancy when it deals with argument passed by >>>> invisible reference; it builds the argument's corresponding pointer type, >>>> but sets passed_mode and nominal_mode with Pmode directly. >>>> >>>> This is OK for targets where Pmode == ptr_mode, but on AArch64 with ILP32 >>>> they are different with Pmode as DImode and ptr_mode as SImode. When such >>>> a >>>> reference is passed on stack, the reference is prepared by the caller in >>>> the >>>> lower 4 bytes of an 8-byte slot but is fetched by the callee as an 8-byte >>>> datum, of which the higher 4 bytes may contain junk. It is probably the >>>> combination of Pmode != ptr_mode and the particular ABI specification >>>> that >>>> make the AArch64 ILP32 the first target on which the issue manifests >>>> itself. >>>> >>>> Bootstrapped on x86_64-none-linux-gnu. >>>> >>>> OK for the trunk? >>> >>> >>> >>> IA64-hpux also uses Pmode != ptr_mode, can you provide the testcase >>> which fails without this change? >>> I used a powerpc64 target where Pmode != ptr_mode which did not hit >>> this bug either. >> >> >> The issue was firstly observed in one of the compat tests which passes a >> large number of non-small structures. The following is a trimmed-down >> reproducible code snippet (although not runnable but shall be easy to be >> make runnable): >> >> struct s5 >> { >> double a; >> double b; >> double c; >> double d; >> double e; >> } gS; >> >> double foo (struct s5 p1, struct s5 p2,struct s5 p3,struct s5 p4,struct s5 >> p5,struct s5 p6,struct s5 p7,struct s5 p8, struct s5 p9) >> { >> return p9.c; >> } >> --------------- CUT --------------- >> >> The code-gen (-O2) without the patch is: >> >> .text >> .align 2 >> .global foo >> .type foo, %function >> foo: >> ldr x0, [sp]<<=== here! >> ldr d0, [x0,16] >> ret >> .size foo, .-foo >> >> Where the arrow points is the load of the pointer to 'p9' that is passed on >> stack. The instruction really should be ldr w0, [sp], i.e. the pointer mode >> is SImode rather than DImode. >> >> It needs a number of conditions for the issue to manifest: >> >> 1. pass-by-reference; on aarch64 one example is a struct that is larger than >> 16 bytes. >> 2. the reference is passed on stack; on aarch64, this usually only happens >> after registers x0 - x7 are used. >> 3. the size of stack slot for passing pointer is larger than the pointer >> size; on aarch64, it is 8-byte vs. 4-byte >> 4. the unused part of the stack slot is not zeroed out, i.e. undefined by >> the ABI > > This is the real issue. I think it is better if we change the ABI to > say they are zero'd. It really makes things like this a mess. I don't agree on this. There is nothing wrong with the unused bits filled with unspecified values; there are sufficient number of load/store instruction variants on AArch64 that are able to load/store smaller-sized datum from/to memory; zeroing-unused bits on the stack may require extra instructions which add cost. Nevertheless, assign_parm_find_data_types() shall not generate different modes for pass-by-reference argument and straight-forward pointer one. For instance in the following code snippet, the passed_mode and nominal_mode for '&p9' in foo and 'p9' in bar shall be the same; but '&p9' in foo gets DImode and 'p3' in bar get SImode, which is really wrong (by typing '&p9', I mean the pass-by-reference). struct s5 { double a; double b; double c; double d; double e; } gS; double foo (struct s5 p1, struct s5 p2,struct s5 p3,struct s5 p4,struct s5 p5,struct s5 p6,struct s5 p7,struct s5 p8, struct s5 p9) { return p9.c; } double bar (struct s5 *p1, struct s5 *p2,struct s5 *p3,struct s5 *p4,struct s5 *p5,struct s5 *p6,struct s5 *p7,struct s5 *p8, struct s5 *p9) { return p9->c; } Hope I have demonstrated the issue clearly. Thanks, Yufeng
On 06/27/13 00:57, Andrew Pinski wrote: > On Wed, Jun 26, 2013 at 4:51 PM, Andrew Pinski<pinskia@gmail.com> wrote: >> On Wed, Jun 26, 2013 at 4:41 PM, Yufeng Zhang<Yufeng.Zhang@arm.com> wrote: >>> On 06/27/13 00:04, Andrew Pinski wrote: >>>> >>>> On Wed, Jun 26, 2013 at 3:39 PM, Yufeng Zhang<Yufeng.Zhang@arm.com> >>>> wrote: >>>>> >>>>> This patch updates assign_parm_find_data_types to assign passed_mode and >>>>> nominal_mode with the mode of the built pointer type instead of the >>>>> hard-coded Pmode in the case of pass-by-reference. This is in line with >>>>> the >>>>> assignment to passed_mode and nominal_mode in other cases inside the >>>>> function. >>>>> >>>>> assign_parm_find_data_types generally uses TYPE_MODE to calculate >>>>> passed_mode and nominal_mode: >>>>> >>>>> /* Find mode of arg as it is passed, and mode of arg as it should be >>>>> during execution of this function. */ >>>>> passed_mode = TYPE_MODE (passed_type); >>>>> nominal_mode = TYPE_MODE (nominal_type); >>>>> >>>>> this includes the case when the passed argument is a pointer by itself. >>>>> >>>>> However there is a discrepancy when it deals with argument passed by >>>>> invisible reference; it builds the argument's corresponding pointer type, >>>>> but sets passed_mode and nominal_mode with Pmode directly. >>>>> >>>>> This is OK for targets where Pmode == ptr_mode, but on AArch64 with ILP32 >>>>> they are different with Pmode as DImode and ptr_mode as SImode. When such >>>>> a >>>>> reference is passed on stack, the reference is prepared by the caller in >>>>> the >>>>> lower 4 bytes of an 8-byte slot but is fetched by the callee as an 8-byte >>>>> datum, of which the higher 4 bytes may contain junk. It is probably the >>>>> combination of Pmode != ptr_mode and the particular ABI specification >>>>> that >>>>> make the AArch64 ILP32 the first target on which the issue manifests >>>>> itself. >>>>> >>>>> Bootstrapped on x86_64-none-linux-gnu. >>>>> >>>>> OK for the trunk? >>>> >>>> >>>> >>>> IA64-hpux also uses Pmode != ptr_mode, can you provide the testcase >>>> which fails without this change? >>>> I used a powerpc64 target where Pmode != ptr_mode which did not hit >>>> this bug either. >>> >>> >>> The issue was firstly observed in one of the compat tests which passes a >>> large number of non-small structures. The following is a trimmed-down >>> reproducible code snippet (although not runnable but shall be easy to be >>> make runnable): >>> >>> struct s5 >>> { >>> double a; >>> double b; >>> double c; >>> double d; >>> double e; >>> } gS; >>> >>> double foo (struct s5 p1, struct s5 p2,struct s5 p3,struct s5 p4,struct s5 >>> p5,struct s5 p6,struct s5 p7,struct s5 p8, struct s5 p9) >>> { >>> return p9.c; >>> } >>> --------------- CUT --------------- >>> >>> The code-gen (-O2) without the patch is: >>> >>> .text >>> .align 2 >>> .global foo >>> .type foo, %function >>> foo: >>> ldr x0, [sp]<<=== here! >>> ldr d0, [x0,16] >>> ret >>> .size foo, .-foo >>> >>> Where the arrow points is the load of the pointer to 'p9' that is passed on >>> stack. The instruction really should be ldr w0, [sp], i.e. the pointer mode >>> is SImode rather than DImode. >>> >>> It needs a number of conditions for the issue to manifest: >>> >>> 1. pass-by-reference; on aarch64 one example is a struct that is larger than >>> 16 bytes. >>> 2. the reference is passed on stack; on aarch64, this usually only happens >>> after registers x0 - x7 are used. >>> 3. the size of stack slot for passing pointer is larger than the pointer >>> size; on aarch64, it is 8-byte vs. 4-byte >>> 4. the unused part of the stack slot is not zeroed out, i.e. undefined by >>> the ABI >> >> This is the real issue. I think it is better if we change the ABI to >> say they are zero'd. It really makes things like this a mess. >> >>> 5. in the runtime, the unused part of such a stack slot contains junk. >>> >>> The runtime segmentation fault may only be generated when all the above >>> conditions are met. I'm not familiar with IA64-hpux or powerpc64 procedure >>> call ABIs, but I guess those targets are just being lucky? >> >> Or rather their ABIs all say are zero or sign extended for values less >> than 8 byte wide. > > One more thing, it looks like your change will not work correctly for > big-endian ILP32 AARCH64 either as the least significant word is > offsetted by 4. Ah, this is definitely a bug and I can confirm that it only happens on the 32-bit pointer-type parameter passed on stack. I'll bring up a patch today to fix it. > Did you test big-endian ILP32 AARCH64? I started the big-endian testing fairly recently, so there is limited testing done so far but I am still working on it and will prepare patches if more issues are found. Thanks, Yufeng
Ping~ Can I get an OK please if there is no objection? Regards, Yufeng On 06/26/13 23:39, Yufeng Zhang wrote: > This patch updates assign_parm_find_data_types to assign passed_mode and > nominal_mode with the mode of the built pointer type instead of the > hard-coded Pmode in the case of pass-by-reference. This is in line with > the assignment to passed_mode and nominal_mode in other cases inside the > function. > > assign_parm_find_data_types generally uses TYPE_MODE to calculate > passed_mode and nominal_mode: > > /* Find mode of arg as it is passed, and mode of arg as it should be > during execution of this function. */ > passed_mode = TYPE_MODE (passed_type); > nominal_mode = TYPE_MODE (nominal_type); > > this includes the case when the passed argument is a pointer by itself. > > However there is a discrepancy when it deals with argument passed by > invisible reference; it builds the argument's corresponding pointer > type, but sets passed_mode and nominal_mode with Pmode directly. > > This is OK for targets where Pmode == ptr_mode, but on AArch64 with > ILP32 they are different with Pmode as DImode and ptr_mode as SImode. > When such a reference is passed on stack, the reference is prepared by > the caller in the lower 4 bytes of an 8-byte slot but is fetched by the > callee as an 8-byte datum, of which the higher 4 bytes may contain junk. > It is probably the combination of Pmode != ptr_mode and the particular > ABI specification that make the AArch64 ILP32 the first target on which > the issue manifests itself. > > Bootstrapped on x86_64-none-linux-gnu. > > OK for the trunk? > > Thanks, > Yufeng > > > gcc/ > * function.c (assign_parm_find_data_types): Set passed_mode and > nominal_mode to the TYPE_MODE of nominal_type for the built > pointer type in case of the struct-pass-by-reference.
Ping^2~ Thanks, Yufeng On 07/02/13 23:44, Yufeng Zhang wrote: > Ping~ > > Can I get an OK please if there is no objection? > > Regards, > Yufeng > > On 06/26/13 23:39, Yufeng Zhang wrote: >> This patch updates assign_parm_find_data_types to assign passed_mode and >> nominal_mode with the mode of the built pointer type instead of the >> hard-coded Pmode in the case of pass-by-reference. This is in line with >> the assignment to passed_mode and nominal_mode in other cases inside the >> function. >> >> assign_parm_find_data_types generally uses TYPE_MODE to calculate >> passed_mode and nominal_mode: >> >> /* Find mode of arg as it is passed, and mode of arg as it should be >> during execution of this function. */ >> passed_mode = TYPE_MODE (passed_type); >> nominal_mode = TYPE_MODE (nominal_type); >> >> this includes the case when the passed argument is a pointer by itself. >> >> However there is a discrepancy when it deals with argument passed by >> invisible reference; it builds the argument's corresponding pointer >> type, but sets passed_mode and nominal_mode with Pmode directly. >> >> This is OK for targets where Pmode == ptr_mode, but on AArch64 with >> ILP32 they are different with Pmode as DImode and ptr_mode as SImode. >> When such a reference is passed on stack, the reference is prepared by >> the caller in the lower 4 bytes of an 8-byte slot but is fetched by the >> callee as an 8-byte datum, of which the higher 4 bytes may contain junk. >> It is probably the combination of Pmode != ptr_mode and the particular >> ABI specification that make the AArch64 ILP32 the first target on which >> the issue manifests itself. >> >> Bootstrapped on x86_64-none-linux-gnu. >> >> OK for the trunk? >> >> Thanks, >> Yufeng >> >> >> gcc/ >> * function.c (assign_parm_find_data_types): Set passed_mode and >> nominal_mode to the TYPE_MODE of nominal_type for the built >> pointer type in case of the struct-pass-by-reference. > > >
Ping^3~ Thanks, Yufeng On 07/08/13 11:11, Yufeng Zhang wrote: > Ping^2~ > > Thanks, > Yufeng > > > On 07/02/13 23:44, Yufeng Zhang wrote: >> Ping~ >> >> Can I get an OK please if there is no objection? >> >> Regards, >> Yufeng >> >> On 06/26/13 23:39, Yufeng Zhang wrote: >>> This patch updates assign_parm_find_data_types to assign passed_mode and >>> nominal_mode with the mode of the built pointer type instead of the >>> hard-coded Pmode in the case of pass-by-reference. This is in line with >>> the assignment to passed_mode and nominal_mode in other cases inside the >>> function. >>> >>> assign_parm_find_data_types generally uses TYPE_MODE to calculate >>> passed_mode and nominal_mode: >>> >>> /* Find mode of arg as it is passed, and mode of arg as it should be >>> during execution of this function. */ >>> passed_mode = TYPE_MODE (passed_type); >>> nominal_mode = TYPE_MODE (nominal_type); >>> >>> this includes the case when the passed argument is a pointer by itself. >>> >>> However there is a discrepancy when it deals with argument passed by >>> invisible reference; it builds the argument's corresponding pointer >>> type, but sets passed_mode and nominal_mode with Pmode directly. >>> >>> This is OK for targets where Pmode == ptr_mode, but on AArch64 with >>> ILP32 they are different with Pmode as DImode and ptr_mode as SImode. >>> When such a reference is passed on stack, the reference is prepared by >>> the caller in the lower 4 bytes of an 8-byte slot but is fetched by the >>> callee as an 8-byte datum, of which the higher 4 bytes may contain junk. >>> It is probably the combination of Pmode != ptr_mode and the particular >>> ABI specification that make the AArch64 ILP32 the first target on which >>> the issue manifests itself. >>> >>> Bootstrapped on x86_64-none-linux-gnu. >>> >>> OK for the trunk? >>> >>> Thanks, >>> Yufeng >>> >>> >>> gcc/ >>> * function.c (assign_parm_find_data_types): Set passed_mode and >>> nominal_mode to the TYPE_MODE of nominal_type for the built >>> pointer type in case of the struct-pass-by-reference. >> >> >> > > >
On 26 June 2013 23:39, Yufeng Zhang <Yufeng.Zhang@arm.com> wrote: > This patch updates assign_parm_find_data_types to assign passed_mode and > nominal_mode with the mode of the built pointer type instead of the > hard-coded Pmode in the case of pass-by-reference. This is in line with the > assignment to passed_mode and nominal_mode in other cases inside the > function. > > assign_parm_find_data_types generally uses TYPE_MODE to calculate > passed_mode and nominal_mode: > > /* Find mode of arg as it is passed, and mode of arg as it should be > during execution of this function. */ > passed_mode = TYPE_MODE (passed_type); > nominal_mode = TYPE_MODE (nominal_type); > > this includes the case when the passed argument is a pointer by itself. > > However there is a discrepancy when it deals with argument passed by > invisible reference; it builds the argument's corresponding pointer type, > but sets passed_mode and nominal_mode with Pmode directly. > > This is OK for targets where Pmode == ptr_mode, but on AArch64 with ILP32 > they are different with Pmode as DImode and ptr_mode as SImode. When such a > reference is passed on stack, the reference is prepared by the caller in the > lower 4 bytes of an 8-byte slot but is fetched by the callee as an 8-byte > datum, of which the higher 4 bytes may contain junk. It is probably the > combination of Pmode != ptr_mode and the particular ABI specification that > make the AArch64 ILP32 the first target on which the issue manifests itself. > > Bootstrapped on x86_64-none-linux-gnu. > > OK for the trunk? Yufeng, this change makes sense to me, however I can't approve such a change. /Marcus
Ping^4~ I am aware that it is currently holiday season, but it would be really nice if this tiny patch can get some further comments even if it is not an approval. The original RFA email is here: http://gcc.gnu.org/ml/gcc-patches/2013-06/msg01485.html Regards, Yufeng On 07/18/13 11:28, Yufeng Zhang wrote: > Ping^3~ > > Thanks, > Yufeng > > On 07/08/13 11:11, Yufeng Zhang wrote: >> Ping^2~ >> >> Thanks, >> Yufeng >> >> >> On 07/02/13 23:44, Yufeng Zhang wrote: >>> Ping~ >>> >>> Can I get an OK please if there is no objection? >>> >>> Regards, >>> Yufeng >>> >>> On 06/26/13 23:39, Yufeng Zhang wrote: >>>> This patch updates assign_parm_find_data_types to assign passed_mode and >>>> nominal_mode with the mode of the built pointer type instead of the >>>> hard-coded Pmode in the case of pass-by-reference. This is in line with >>>> the assignment to passed_mode and nominal_mode in other cases inside the >>>> function. >>>> >>>> assign_parm_find_data_types generally uses TYPE_MODE to calculate >>>> passed_mode and nominal_mode: >>>> >>>> /* Find mode of arg as it is passed, and mode of arg as it should be >>>> during execution of this function. */ >>>> passed_mode = TYPE_MODE (passed_type); >>>> nominal_mode = TYPE_MODE (nominal_type); >>>> >>>> this includes the case when the passed argument is a pointer by itself. >>>> >>>> However there is a discrepancy when it deals with argument passed by >>>> invisible reference; it builds the argument's corresponding pointer >>>> type, but sets passed_mode and nominal_mode with Pmode directly. >>>> >>>> This is OK for targets where Pmode == ptr_mode, but on AArch64 with >>>> ILP32 they are different with Pmode as DImode and ptr_mode as SImode. >>>> When such a reference is passed on stack, the reference is prepared by >>>> the caller in the lower 4 bytes of an 8-byte slot but is fetched by the >>>> callee as an 8-byte datum, of which the higher 4 bytes may contain junk. >>>> It is probably the combination of Pmode != ptr_mode and the particular >>>> ABI specification that make the AArch64 ILP32 the first target on which >>>> the issue manifests itself. >>>> >>>> Bootstrapped on x86_64-none-linux-gnu. >>>> >>>> OK for the trunk? >>>> >>>> Thanks, >>>> Yufeng >>>> >>>> >>>> gcc/ >>>> * function.c (assign_parm_find_data_types): Set passed_mode and >>>> nominal_mode to the TYPE_MODE of nominal_type for the built >>>> pointer type in case of the struct-pass-by-reference. >>> >>> >>> >> >> >> > > >
On Thu, Aug 15, 2013 at 11:21 AM, Yufeng Zhang <Yufeng.Zhang@arm.com> wrote: > Ping^4~ > > I am aware that it is currently holiday season, but it would be really nice > if this tiny patch can get some further comments even if it is not an > approval. > > The original RFA email is here: > http://gcc.gnu.org/ml/gcc-patches/2013-06/msg01485.html From my point of view it is correct after understanding the ABI better though I cannot approve it. Thanks, Andrew Pinski > > Regards, > Yufeng > > > On 07/18/13 11:28, Yufeng Zhang wrote: >> >> Ping^3~ >> >> Thanks, >> Yufeng >> >> On 07/08/13 11:11, Yufeng Zhang wrote: >>> >>> Ping^2~ >>> >>> Thanks, >>> Yufeng >>> >>> >>> On 07/02/13 23:44, Yufeng Zhang wrote: >>>> >>>> Ping~ >>>> >>>> Can I get an OK please if there is no objection? >>>> >>>> Regards, >>>> Yufeng >>>> >>>> On 06/26/13 23:39, Yufeng Zhang wrote: >>>>> >>>>> This patch updates assign_parm_find_data_types to assign passed_mode >>>>> and >>>>> nominal_mode with the mode of the built pointer type instead of the >>>>> hard-coded Pmode in the case of pass-by-reference. This is in line >>>>> with >>>>> the assignment to passed_mode and nominal_mode in other cases inside >>>>> the >>>>> function. >>>>> >>>>> assign_parm_find_data_types generally uses TYPE_MODE to calculate >>>>> passed_mode and nominal_mode: >>>>> >>>>> /* Find mode of arg as it is passed, and mode of arg as it >>>>> should be >>>>> during execution of this function. */ >>>>> passed_mode = TYPE_MODE (passed_type); >>>>> nominal_mode = TYPE_MODE (nominal_type); >>>>> >>>>> this includes the case when the passed argument is a pointer by itself. >>>>> >>>>> However there is a discrepancy when it deals with argument passed by >>>>> invisible reference; it builds the argument's corresponding pointer >>>>> type, but sets passed_mode and nominal_mode with Pmode directly. >>>>> >>>>> This is OK for targets where Pmode == ptr_mode, but on AArch64 with >>>>> ILP32 they are different with Pmode as DImode and ptr_mode as SImode. >>>>> When such a reference is passed on stack, the reference is prepared by >>>>> the caller in the lower 4 bytes of an 8-byte slot but is fetched by the >>>>> callee as an 8-byte datum, of which the higher 4 bytes may contain >>>>> junk. >>>>> It is probably the combination of Pmode != ptr_mode and the >>>>> particular >>>>> ABI specification that make the AArch64 ILP32 the first target on which >>>>> the issue manifests itself. >>>>> >>>>> Bootstrapped on x86_64-none-linux-gnu. >>>>> >>>>> OK for the trunk? >>>>> >>>>> Thanks, >>>>> Yufeng >>>>> >>>>> >>>>> gcc/ >>>>> * function.c (assign_parm_find_data_types): Set passed_mode and >>>>> nominal_mode to the TYPE_MODE of nominal_type for the built >>>>> pointer type in case of the struct-pass-by-reference. >>>> >>>> >>>> >>>> >>> >>> >>> >> >> >> > >
On 08/15/2013 11:21 AM, Yufeng Zhang wrote: > Ping^4~ > > I am aware that it is currently holiday season, but it would be really nice if > this tiny patch can get some further comments even if it is not an approval. > > The original RFA email is here: > http://gcc.gnu.org/ml/gcc-patches/2013-06/msg01485.html > Ok. It probably would have helped to have included the link or the patch itself in the various pings. It would have also been helpful to note that this makes the callee side match up with the caller side in calls.c:1247, initialize_argument_information. r~
diff --git a/gcc/function.c b/gcc/function.c index 3e33fc7..6a0aaaf 100644 --- a/gcc/function.c +++ b/gcc/function.c @@ -2369,7 +2369,7 @@ assign_parm_find_data_types (struct assign_parm_data_all *all, tree parm, { passed_type = nominal_type = build_pointer_type (passed_type); data->passed_pointer = true; - passed_mode = nominal_mode = Pmode; + passed_mode = nominal_mode = TYPE_MODE (nominal_type); } /* Find mode as it is passed by the ABI. */
This patch updates assign_parm_find_data_types to assign passed_mode and nominal_mode with the mode of the built pointer type instead of the hard-coded Pmode in the case of pass-by-reference. This is in line with the assignment to passed_mode and nominal_mode in other cases inside the function. assign_parm_find_data_types generally uses TYPE_MODE to calculate passed_mode and nominal_mode: /* Find mode of arg as it is passed, and mode of arg as it should be during execution of this function. */ passed_mode = TYPE_MODE (passed_type); nominal_mode = TYPE_MODE (nominal_type); this includes the case when the passed argument is a pointer by itself. However there is a discrepancy when it deals with argument passed by invisible reference; it builds the argument's corresponding pointer type, but sets passed_mode and nominal_mode with Pmode directly. This is OK for targets where Pmode == ptr_mode, but on AArch64 with ILP32 they are different with Pmode as DImode and ptr_mode as SImode. When such a reference is passed on stack, the reference is prepared by the caller in the lower 4 bytes of an 8-byte slot but is fetched by the callee as an 8-byte datum, of which the higher 4 bytes may contain junk. It is probably the combination of Pmode != ptr_mode and the particular ABI specification that make the AArch64 ILP32 the first target on which the issue manifests itself. Bootstrapped on x86_64-none-linux-gnu. OK for the trunk? Thanks, Yufeng gcc/ * function.c (assign_parm_find_data_types): Set passed_mode and nominal_mode to the TYPE_MODE of nominal_type for the built pointer type in case of the struct-pass-by-reference.