Patchwork [ARM] Fix PR 43872, incorrectly aligned VLAs

login
register
mail settings
Submitter Chung-Lin Tang
Date Feb. 17, 2011, 10:01 a.m.
Message ID <4D5CF204.4080306@codesourcery.com>
Download mbox | patch
Permalink /patch/83438/
State New
Headers show

Comments

Chung-Lin Tang - Feb. 17, 2011, 10:01 a.m.
Hi,
this PR is a case where we have a leaf function with a zero-size frame,
that calls alloca() (here through a C99 VLA).

The ARM backend recognizes the leaf-and-no-frame opportunity to save an
unneeded stack alignment. But when calling alloca(), the allocated block
is of BIGGEST_ALIGNMENT, which is 8-bytes under current AAPCS. Thus a
stack align may still be needed to place the alloca() returned block
properly.

This patch adjusts the early return condition with !cfun->calls_alloca.
Tested without regressions on ARM-Linux, okay for trunk?

Also related, the BIGGEST_ALIGNMENT defined for ARM is currently
ABI-based; 64-bits for AAPCS targets, and 32 for old ABI.  Should this
also consider arch-level as well? i.e. anything >= ARMv5TE (with ldrd)
should have this set to 64.

Thanks,
Chung-Lin

2011-02-17  Chung-Lin Tang  <cltang@codesourcery.com>

	PR target/43872
	* config/arm/arm.c (arm_get_frame_offsets): Adjust early
	return condition with !cfun->calls_alloca.
Chung-Lin Tang - March 7, 2011, 2:10 p.m.
Ping.

On 2011/2/17 06:01 PM, Chung-Lin Tang wrote:
> Hi,
> this PR is a case where we have a leaf function with a zero-size frame,
> that calls alloca() (here through a C99 VLA).
> 
> The ARM backend recognizes the leaf-and-no-frame opportunity to save an
> unneeded stack alignment. But when calling alloca(), the allocated block
> is of BIGGEST_ALIGNMENT, which is 8-bytes under current AAPCS. Thus a
> stack align may still be needed to place the alloca() returned block
> properly.
> 
> This patch adjusts the early return condition with !cfun->calls_alloca.
> Tested without regressions on ARM-Linux, okay for trunk?
> 
> Also related, the BIGGEST_ALIGNMENT defined for ARM is currently
> ABI-based; 64-bits for AAPCS targets, and 32 for old ABI.  Should this
> also consider arch-level as well? i.e. anything >= ARMv5TE (with ldrd)
> should have this set to 64.
> 
> Thanks,
> Chung-Lin
> 
> 2011-02-17  Chung-Lin Tang  <cltang@codesourcery.com>
> 
> 	PR target/43872
> 	* config/arm/arm.c (arm_get_frame_offsets): Adjust early
> 	return condition with !cfun->calls_alloca.
Chung-Lin Tang - March 15, 2011, 1:38 a.m.
Ping.

On 2011/3/7 10:10 PM, Chung-Lin Tang wrote:
> Ping.
> 
> On 2011/2/17 06:01 PM, Chung-Lin Tang wrote:
>> Hi,
>> this PR is a case where we have a leaf function with a zero-size frame,
>> that calls alloca() (here through a C99 VLA).
>>
>> The ARM backend recognizes the leaf-and-no-frame opportunity to save an
>> unneeded stack alignment. But when calling alloca(), the allocated block
>> is of BIGGEST_ALIGNMENT, which is 8-bytes under current AAPCS. Thus a
>> stack align may still be needed to place the alloca() returned block
>> properly.
>>
>> This patch adjusts the early return condition with !cfun->calls_alloca.
>> Tested without regressions on ARM-Linux, okay for trunk?
>>
>> Also related, the BIGGEST_ALIGNMENT defined for ARM is currently
>> ABI-based; 64-bits for AAPCS targets, and 32 for old ABI.  Should this
>> also consider arch-level as well? i.e. anything >= ARMv5TE (with ldrd)
>> should have this set to 64.
>>
>> Thanks,
>> Chung-Lin
>>
>> 2011-02-17  Chung-Lin Tang  <cltang@codesourcery.com>
>>
>> 	PR target/43872
>> 	* config/arm/arm.c (arm_get_frame_offsets): Adjust early
>> 	return condition with !cfun->calls_alloca.
>
Ramana Radhakrishnan - March 15, 2011, 6:41 a.m.
On 17/02/11 10:01, Chung-Lin Tang wrote:
> Hi,
> this PR is a case where we have a leaf function with a zero-size frame,
> that calls alloca() (here through a C99 VLA).
>
> The ARM backend recognizes the leaf-and-no-frame opportunity to save an
> unneeded stack alignment. But when calling alloca(), the allocated block
> is of BIGGEST_ALIGNMENT, which is 8-bytes under current AAPCS. Thus a
> stack align may still be needed to place the alloca() returned block
> properly.
>
> This patch adjusts the early return condition with !cfun->calls_alloca.
> Tested without regressions on ARM-Linux, okay for trunk?

OK.

>
> Also related, the BIGGEST_ALIGNMENT defined for ARM is currently
> ABI-based; 64-bits for AAPCS targets, and 32 for old ABI.  Should this
> also consider arch-level as well? i.e. anything>= ARMv5TE (with ldrd)
> should have this set to 64.

I don't think it's that straightforward. The ABI specifies that an 8 
byte quantity must be aligned at 64 bit boundaries and this should be 
independent of architecture revisions. Think about pointers to 64 bit 
quantities passed from !(TARGET_LDRD) to TARGET_LDRD code or indeed 
creating structure layout issues between architecture revisions if you 
change this.

cheers
Ramana


> 2011-02-17  Chung-Lin Tang<cltang@codesourcery.com>
>
> 	PR target/43872
> 	* config/arm/arm.c (arm_get_frame_offsets): Adjust early
> 	return condition with !cfun->calls_alloca.
Chung-Lin Tang - March 15, 2011, 11:30 a.m.
On 2011/3/15 02:41 PM, Ramana Radhakrishnan wrote:
> On 17/02/11 10:01, Chung-Lin Tang wrote:
>> Hi,
>> this PR is a case where we have a leaf function with a zero-size frame,
>> that calls alloca() (here through a C99 VLA).
>>
>> The ARM backend recognizes the leaf-and-no-frame opportunity to save an
>> unneeded stack alignment. But when calling alloca(), the allocated block
>> is of BIGGEST_ALIGNMENT, which is 8-bytes under current AAPCS. Thus a
>> stack align may still be needed to place the alloca() returned block
>> properly.
>>
>> This patch adjusts the early return condition with !cfun->calls_alloca.
>> Tested without regressions on ARM-Linux, okay for trunk?
> 
> OK.

Thanks.

>>
>> Also related, the BIGGEST_ALIGNMENT defined for ARM is currently
>> ABI-based; 64-bits for AAPCS targets, and 32 for old ABI.  Should this
>> also consider arch-level as well? i.e. anything>= ARMv5TE (with ldrd)
>> should have this set to 64.
> 
> I don't think it's that straightforward. The ABI specifies that an 8
> byte quantity must be aligned at 64 bit boundaries and this should be
> independent of architecture revisions. Think about pointers to 64 bit
> quantities passed from !(TARGET_LDRD) to TARGET_LDRD code or indeed
> creating structure layout issues between architecture revisions if you
> change this.

Oh okay, I see.  I checked again, originally thought that structure
layout was controlled by other alignment related macros...

Patch

Index: config/arm/arm.c
===================================================================
--- config/arm/arm.c	(revision 170243)
+++ config/arm/arm.c	(working copy)
@@ -15415,7 +15415,10 @@ 
   offsets->soft_frame = offsets->saved_regs + CALLER_INTERWORKING_SLOT_SIZE;
   /* A leaf function does not need any stack alignment if it has nothing
      on the stack.  */
-  if (leaf && frame_size == 0)
+  if (leaf && frame_size == 0
+      /* However if it calls alloca(), we have a dynamically allocated
+	 block of BIGGEST_ALIGNMENT on stack, so still do stack alignment.  */
+      && ! cfun->calls_alloca)
     {
       offsets->outgoing_args = offsets->soft_frame;
       offsets->locals_base = offsets->soft_frame;