Patchwork [AArch64,ILP32] 3/5 Minor change in function.c:assign_parm_find_data_types()

login
register
mail settings
Submitter Yufeng Zhang
Date June 26, 2013, 10:39 p.m.
Message ID <51CB6D86.9020304@arm.com>
Download mbox | patch
Permalink /patch/254908/
State New
Headers show

Comments

Yufeng Zhang - June 26, 2013, 10:39 p.m.
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.
Andrew Pinski - June 26, 2013, 11:04 p.m.
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.
Yufeng Zhang - June 26, 2013, 11:41 p.m.
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
Andrew Pinski - June 26, 2013, 11:51 p.m.
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
Andrew Pinski - June 26, 2013, 11:57 p.m.
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
Yufeng Zhang - June 27, 2013, 12:37 a.m.
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
Yufeng Zhang - June 27, 2013, 12:51 a.m.
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
Yufeng Zhang - July 2, 2013, 10:44 p.m.
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.
Yufeng Zhang - July 8, 2013, 10:11 a.m.
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.
>
>
>
Yufeng Zhang - July 18, 2013, 10:28 a.m.
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.
>>
>>
>>
>
>
>
Marcus Shawcroft - July 19, 2013, 9:16 a.m.
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
Yufeng Zhang - Aug. 15, 2013, 6:21 p.m.
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.
>>>
>>>
>>>
>>
>>
>>
>
>
>
Andrew Pinski - Aug. 24, 2013, 7:48 p.m.
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.
>>>>
>>>>
>>>>
>>>>
>>>
>>>
>>>
>>
>>
>>
>
>
Richard Henderson - Aug. 26, 2013, 4:10 p.m.
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~

Patch

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.  */