[PATCH][AArch64] Fix PR63293

[PATCH][AArch64] Fix PR63293

[Jiong Wang]

[-- Attachment #1: Type: text/plain, Size: 665 bytes --]

when generating instructions to access local variable, for example a local array,

if the array size very big, then we need a temp reg to keep the intermediate index,
then use that temp reg as base reg, so that ldr is capable of indexing the element.

while this will cause trouble, because the introduce of temp reg break the dependence
between the stack variable access and stack adjustment instructions which is unsafe
when signal trampoline executed.

this patch add barrier before stack adjustment in epilogue.

ok for trunk?

thanks.

gcc/
   * gcc/config/aarch64.c (aarch64_expand_epilogue): Add barrier before stack adjustment.

-- Jiong

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: e.patch --]
[-- Type: text/x-patch; name=e.patch, Size: 444 bytes --]

diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index 023f9fd..8eed9cf 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -2431,6 +2431,8 @@ aarch64_expand_epilogue (bool for_sibcall)

   if (frame_size > 0)
     {
+      emit_insn (gen_stack_tie (stack_pointer_rtx, stack_pointer_rtx));
+
       if (frame_size >= 0x1000000)
 	{
 	  rtx op0 = gen_rtx_REG (Pmode, IP0_REGNUM);

RE: [PATCH][AArch64] Fix PR63293

[Wilco Dijkstra]

> Jiong Wang wrote:
> 
> when generating instructions to access local variable, for example a local array,
> 
> if the array size very big, then we need a temp reg to keep the intermediate index,
> then use that temp reg as base reg, so that ldr is capable of indexing the element.
> 
> while this will cause trouble, because the introduce of temp reg break the dependence
> between the stack variable access and stack adjustment instructions which is unsafe
> when signal trampoline executed.
> 
> this patch add barrier before stack adjustment in epilogue.
> 
> ok for trunk?

I believe you need more barriers. Ie. for all SP modifying instructions (including ldp
with writeback) except for ones that just remove the outgoing arguments. You can avoid
emitting barriers if alloca is not used and there are no locals in the frame (common case).

Basically without that any memory access that may alias with the locals could be
scheduled incorrectly. It seems odd that the scheduler does not understand this by 
default.

Wilco

Re: [PATCH][AArch64] Fix PR63293

[Jiong Wang]

[-- Attachment #1: Type: text/plain, Size: 1398 bytes --]


On 19/09/14 15:35, Wilco Dijkstra wrote:
>> Jiong Wang wrote:
>>
>> when generating instructions to access local variable, for example a local array,
>>
>> if the array size very big, then we need a temp reg to keep the intermediate index,
>> then use that temp reg as base reg, so that ldr is capable of indexing the element.
>>
>> while this will cause trouble, because the introduce of temp reg break the dependence
>> between the stack variable access and stack adjustment instructions which is unsafe
>> when signal trampoline executed.
>>
>> this patch add barrier before stack adjustment in epilogue.
>>
>> ok for trunk?
> I believe you need more barriers. Ie. for all SP modifying instructions (including ldp
> with writeback) except for ones that just remove the outgoing arguments. You can avoid
> emitting barriers if alloca is not used and there are no locals in the frame (common case).
>
> Basically without that any memory access that may alias with the locals could be
> scheduled incorrectly. It seems odd that the scheduler does not understand this by
> default.

thanks for pointing this out.

patch updated, please review.


2014-09-25  Jiong Wang  <jiong.wang@arm.com>
2014-09-25  Wilco Dijkstra  <wilco.dijkstra@arm.com>

gcc/
   PR target/63293
   * config/aarch64/aarch64.c (aarch64_expand_epiloue): Add barriers before stack adjustment.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: e.patch --]
[-- Type: text/x-patch; name=e.patch, Size: 1601 bytes --]

diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index 023f9fd..d258425 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -2331,6 +2331,9 @@ aarch64_expand_epilogue (bool for_sibcall)
   HOST_WIDE_INT fp_offset;
   HOST_WIDE_INT hard_fp_offset;
   rtx_insn *insn;
+  /* We need to add memory barrier to prevent read from deallocated stack.  */
+  bool need_barrier_p = (get_frame_size () != 0
+			 || cfun->machine->frame.saved_varargs_size);
 
   aarch64_layout_frame ();
 
@@ -2365,6 +2368,9 @@ aarch64_expand_epilogue (bool for_sibcall)
   if (frame_pointer_needed
       && (crtl->outgoing_args_size || cfun->calls_alloca))
     {
+      if (cfun->calls_alloca)
+	emit_insn (gen_stack_tie (stack_pointer_rtx, stack_pointer_rtx));
+
       insn = emit_insn (gen_add3_insn (stack_pointer_rtx,
 				       hard_frame_pointer_rtx,
 				       GEN_INT (0)));
@@ -2391,6 +2397,9 @@ aarch64_expand_epilogue (bool for_sibcall)
       aarch64_restore_callee_saves (DFmode, fp_offset, V0_REGNUM, V31_REGNUM,
 				    skip_wb, &cfi_ops);
 
+      if (need_barrier_p)
+	emit_insn (gen_stack_tie (stack_pointer_rtx, stack_pointer_rtx));
+
       if (skip_wb)
 	{
 	  enum machine_mode mode1 = (reg1 <= R30_REGNUM) ? DImode : DFmode;
@@ -2431,6 +2440,9 @@ aarch64_expand_epilogue (bool for_sibcall)
 
   if (frame_size > 0)
     {
+      if (need_barrier_p)
+	emit_insn (gen_stack_tie (stack_pointer_rtx, stack_pointer_rtx));
+
       if (frame_size >= 0x1000000)
 	{
 	  rtx op0 = gen_rtx_REG (Pmode, IP0_REGNUM);

Re: [PATCH][AArch64] Fix PR63293

[Marcus Shawcroft]

On 25 September 2014 17:32, Jiong Wang <jiong.wang@arm.com> wrote:
>

> patch updated, please review.
>
>
> 2014-09-25  Jiong Wang  <jiong.wang@arm.com>
> 2014-09-25  Wilco Dijkstra  <wilco.dijkstra@arm.com>
>
> gcc/
>   PR target/63293
>   * config/aarch64/aarch64.c (aarch64_expand_epiloue): Add barriers before
> stack adjustment.

OK /Marcus