Patchwork [ARM,AArch64] Make aarch64-common.c files more robust.

login
register
mail settings
Submitter James Greenhalgh
Date Sept. 30, 2013, 8:52 a.m.
Message ID <1380531143-25769-1-git-send-email-james.greenhalgh@arm.com>
Download mbox | patch
Permalink /patch/278966/
State New
Headers show

Comments

James Greenhalgh - Sept. 30, 2013, 8:52 a.m.
Hi,

Recently I've found myself getting a number of segfaults from
code calling in to the arm_early_load/alu_dep functions in
aarch64-common.c. These functions expect a particular form
for the RTX patterns they work over, but some of them do
not validate this form.

This patch fixes that, removing segmentation faults I see
when tuning for Cortex-A15 and Cortex-A7.

Tested on aarch64-none-elf and arm-none-eabi with no regressions.

OK?

Thanks,
James

---
gcc/

2013-09-30  James Greenhalgh  <james.greenhalgh@arm.com>

	* config/arm/aarch-common.c
	(arm_early_load_addr_dep): Add sanity checking.
	(arm_no_early_alu_shift_dep): Likewise.
	(arm_no_early_alu_shift_value_dep): Likewise.
	(arm_no_early_mul_dep): Likewise.
	(arm_no_early_store_addr_dep): Likewise.
Marcus Shawcroft - Sept. 30, 2013, 10:05 a.m.
On 30 September 2013 09:52, James Greenhalgh <james.greenhalgh@arm.com> wrote:
>
> Hi,
>
> aarch64-common.c. These functions expect a particular form

You meant aarch-common.c here and in the title ;-)

This is fine by me, but as a config/arm/ change needs OK from Ramana or Richard.

/Marcus

> 2013-09-30  James Greenhalgh  <james.greenhalgh@arm.com>
>
>         * config/arm/aarch-common.c
>         (arm_early_load_addr_dep): Add sanity checking.
>         (arm_no_early_alu_shift_dep): Likewise.
>         (arm_no_early_alu_shift_value_dep): Likewise.
>         (arm_no_early_mul_dep): Likewise.
>         (arm_no_early_store_addr_dep): Likewise.
Richard Earnshaw - Oct. 1, 2013, 5:01 p.m.
On 30/09/13 09:52, James Greenhalgh wrote:
> 
> Hi,
> 
> Recently I've found myself getting a number of segfaults from
> code calling in to the arm_early_load/alu_dep functions in
> aarch64-common.c. These functions expect a particular form
> for the RTX patterns they work over, but some of them do
> not validate this form.
> 
> This patch fixes that, removing segmentation faults I see
> when tuning for Cortex-A15 and Cortex-A7.
> 
> Tested on aarch64-none-elf and arm-none-eabi with no regressions.
> 
> OK?
> 
> Thanks,
> James
> 
> ---
> gcc/
> 
> 2013-09-30  James Greenhalgh  <james.greenhalgh@arm.com>
> 
> 	* config/arm/aarch-common.c
> 	(arm_early_load_addr_dep): Add sanity checking.
> 	(arm_no_early_alu_shift_dep): Likewise.
> 	(arm_no_early_alu_shift_value_dep): Likewise.
> 	(arm_no_early_mul_dep): Likewise.
> 	(arm_no_early_store_addr_dep): Likewise.
> 
> 
> 0001-ARM-AArch64-Make-aarch64-common.c-files-more-robust.patch
> 
> 
> diff --git a/gcc/config/arm/aarch-common.c b/gcc/config/arm/aarch-common.c
> index 69366af..ea50848 100644
> --- a/gcc/config/arm/aarch-common.c
> +++ b/gcc/config/arm/aarch-common.c
> @@ -44,7 +44,12 @@ arm_early_load_addr_dep (rtx producer, rtx consumer)
>      value = COND_EXEC_CODE (value);
>    if (GET_CODE (value) == PARALLEL)
>      value = XVECEXP (value, 0, 0);
> +
> +  if (GET_CODE (value) != SET)
> +    return 0;
> +
>    value = XEXP (value, 0);

Please change this to use SET_DEST.

> +
>    if (GET_CODE (addr) == COND_EXEC)
>      addr = COND_EXEC_CODE (addr);
>    if (GET_CODE (addr) == PARALLEL)
> @@ -54,6 +59,10 @@ arm_early_load_addr_dep (rtx producer, rtx consumer)
>        else
>          addr = XVECEXP (addr, 0, 0);
>      }
> +
> +  if (GET_CODE (addr) != SET)
> +    return 0;
> +
>    addr = XEXP (addr, 1);
>  

And this to use SET_SRC.

>    return reg_overlap_mentioned_p (value, addr);
> @@ -74,21 +83,41 @@ arm_no_early_alu_shift_dep (rtx producer, rtx consumer)
>      value = COND_EXEC_CODE (value);
>    if (GET_CODE (value) == PARALLEL)
>      value = XVECEXP (value, 0, 0);
> +
> +  if (GET_CODE (value) != SET)
> +    return 0;
> +
>    value = XEXP (value, 0);

SET_DEST.

> +
>    if (GET_CODE (op) == COND_EXEC)
>      op = COND_EXEC_CODE (op);
>    if (GET_CODE (op) == PARALLEL)
>      op = XVECEXP (op, 0, 0);
> +
> +  if (GET_CODE (op) != SET)
> +    return 0;
> +
>    op = XEXP (op, 1);

SET_SRC.

>  
> +  if (!INSN_P (op))
> +    return 0;

This looks wrong.  What are you really expecting here?  Surely not INSN,
since then ...
> +
>    early_op = XEXP (op, 0);

... this would always fault, since element 0 is an int

> +
>    /* This is either an actual independent shift, or a shift applied to
>       the first operand of another operation.  We want the whole shift
>       operation.  */
>    if (REG_P (early_op))
>      early_op = op;
>  
> -  return !reg_overlap_mentioned_p (value, early_op);
> +  if (GET_CODE (op) == ASHIFT
> +      || GET_CODE (op) == ROTATE
> +      || GET_CODE (op) == ASHIFTRT
> +      || GET_CODE (op) == LSHIFTRT
> +      || GET_CODE (op) == ROTATERT)
> +    return !reg_overlap_mentioned_p (value, early_op);
> +  else
> +    return 0;
>  }
>  
>  /* Return nonzero if the CONSUMER instruction (an ALU op) does not
> @@ -106,13 +135,25 @@ arm_no_early_alu_shift_value_dep (rtx producer, rtx consumer)
>      value = COND_EXEC_CODE (value);
>    if (GET_CODE (value) == PARALLEL)
>      value = XVECEXP (value, 0, 0);
> +
> +  if (GET_CODE (value) != SET)
> +    return 0;
> +
>    value = XEXP (value, 0);

SET_DEST.

> +
>    if (GET_CODE (op) == COND_EXEC)
>      op = COND_EXEC_CODE (op);
>    if (GET_CODE (op) == PARALLEL)
>      op = XVECEXP (op, 0, 0);
> +
> +  if (GET_CODE (op) != SET)
> +    return 0;
> +
>    op = XEXP (op, 1);
>  
> +  if (!INSN_P (op))
> +    return 0;
> +

Same issue as above.

>    early_op = XEXP (op, 0);
>  
>    /* This is either an actual independent shift, or a shift applied to
> @@ -121,7 +162,14 @@ arm_no_early_alu_shift_value_dep (rtx producer, rtx consumer)
>    if (!REG_P (early_op))
>      early_op = XEXP (early_op, 0);
>  
> -  return !reg_overlap_mentioned_p (value, early_op);
> +  if (GET_CODE (op) == ASHIFT
> +      || GET_CODE (op) == ROTATE
> +      || GET_CODE (op) == ASHIFTRT
> +      || GET_CODE (op) == LSHIFTRT
> +      || GET_CODE (op) == ROTATERT)
> +    return !reg_overlap_mentioned_p (value, early_op);
> +  else
> +    return 0;
>  }
>  
>  /* Return nonzero if the CONSUMER (a mul or mac op) does not
> @@ -138,11 +186,20 @@ arm_no_early_mul_dep (rtx producer, rtx consumer)
>      value = COND_EXEC_CODE (value);
>    if (GET_CODE (value) == PARALLEL)
>      value = XVECEXP (value, 0, 0);
> +
> +  if (GET_CODE (value) != SET)
> +    return 0;
> +
>    value = XEXP (value, 0);

SET_DEST.

> +
>    if (GET_CODE (op) == COND_EXEC)
>      op = COND_EXEC_CODE (op);
>    if (GET_CODE (op) == PARALLEL)
>      op = XVECEXP (op, 0, 0);
> +
> +  if (GET_CODE (op) != SET)
> +    return 0;
> +
>    op = XEXP (op, 1);

SET_SRC.

>  
>    if (GET_CODE (op) == PLUS || GET_CODE (op) == MINUS)
> @@ -169,11 +226,20 @@ arm_no_early_store_addr_dep (rtx producer, rtx consumer)
>      value = COND_EXEC_CODE (value);
>    if (GET_CODE (value) == PARALLEL)
>      value = XVECEXP (value, 0, 0);
> +
> +  if (GET_CODE (value) != SET)
> +    return 0;
> +
>    value = XEXP (value, 0);
> +
>    if (GET_CODE (addr) == COND_EXEC)
>      addr = COND_EXEC_CODE (addr);
>    if (GET_CODE (addr) == PARALLEL)
>      addr = XVECEXP (addr, 0, 0);
> +
> +  if (GET_CODE (addr) != SET)
> +    return 0;
> +
>    addr = XEXP (addr, 0);

SET_DEST.

>  
>    return !reg_overlap_mentioned_p (value, addr);
> 

R.

Patch

diff --git a/gcc/config/arm/aarch-common.c b/gcc/config/arm/aarch-common.c
index 69366af..ea50848 100644
--- a/gcc/config/arm/aarch-common.c
+++ b/gcc/config/arm/aarch-common.c
@@ -44,7 +44,12 @@  arm_early_load_addr_dep (rtx producer, rtx consumer)
     value = COND_EXEC_CODE (value);
   if (GET_CODE (value) == PARALLEL)
     value = XVECEXP (value, 0, 0);
+
+  if (GET_CODE (value) != SET)
+    return 0;
+
   value = XEXP (value, 0);
+
   if (GET_CODE (addr) == COND_EXEC)
     addr = COND_EXEC_CODE (addr);
   if (GET_CODE (addr) == PARALLEL)
@@ -54,6 +59,10 @@  arm_early_load_addr_dep (rtx producer, rtx consumer)
       else
         addr = XVECEXP (addr, 0, 0);
     }
+
+  if (GET_CODE (addr) != SET)
+    return 0;
+
   addr = XEXP (addr, 1);
 
   return reg_overlap_mentioned_p (value, addr);
@@ -74,21 +83,41 @@  arm_no_early_alu_shift_dep (rtx producer, rtx consumer)
     value = COND_EXEC_CODE (value);
   if (GET_CODE (value) == PARALLEL)
     value = XVECEXP (value, 0, 0);
+
+  if (GET_CODE (value) != SET)
+    return 0;
+
   value = XEXP (value, 0);
+
   if (GET_CODE (op) == COND_EXEC)
     op = COND_EXEC_CODE (op);
   if (GET_CODE (op) == PARALLEL)
     op = XVECEXP (op, 0, 0);
+
+  if (GET_CODE (op) != SET)
+    return 0;
+
   op = XEXP (op, 1);
 
+  if (!INSN_P (op))
+    return 0;
+
   early_op = XEXP (op, 0);
+
   /* This is either an actual independent shift, or a shift applied to
      the first operand of another operation.  We want the whole shift
      operation.  */
   if (REG_P (early_op))
     early_op = op;
 
-  return !reg_overlap_mentioned_p (value, early_op);
+  if (GET_CODE (op) == ASHIFT
+      || GET_CODE (op) == ROTATE
+      || GET_CODE (op) == ASHIFTRT
+      || GET_CODE (op) == LSHIFTRT
+      || GET_CODE (op) == ROTATERT)
+    return !reg_overlap_mentioned_p (value, early_op);
+  else
+    return 0;
 }
 
 /* Return nonzero if the CONSUMER instruction (an ALU op) does not
@@ -106,13 +135,25 @@  arm_no_early_alu_shift_value_dep (rtx producer, rtx consumer)
     value = COND_EXEC_CODE (value);
   if (GET_CODE (value) == PARALLEL)
     value = XVECEXP (value, 0, 0);
+
+  if (GET_CODE (value) != SET)
+    return 0;
+
   value = XEXP (value, 0);
+
   if (GET_CODE (op) == COND_EXEC)
     op = COND_EXEC_CODE (op);
   if (GET_CODE (op) == PARALLEL)
     op = XVECEXP (op, 0, 0);
+
+  if (GET_CODE (op) != SET)
+    return 0;
+
   op = XEXP (op, 1);
 
+  if (!INSN_P (op))
+    return 0;
+
   early_op = XEXP (op, 0);
 
   /* This is either an actual independent shift, or a shift applied to
@@ -121,7 +162,14 @@  arm_no_early_alu_shift_value_dep (rtx producer, rtx consumer)
   if (!REG_P (early_op))
     early_op = XEXP (early_op, 0);
 
-  return !reg_overlap_mentioned_p (value, early_op);
+  if (GET_CODE (op) == ASHIFT
+      || GET_CODE (op) == ROTATE
+      || GET_CODE (op) == ASHIFTRT
+      || GET_CODE (op) == LSHIFTRT
+      || GET_CODE (op) == ROTATERT)
+    return !reg_overlap_mentioned_p (value, early_op);
+  else
+    return 0;
 }
 
 /* Return nonzero if the CONSUMER (a mul or mac op) does not
@@ -138,11 +186,20 @@  arm_no_early_mul_dep (rtx producer, rtx consumer)
     value = COND_EXEC_CODE (value);
   if (GET_CODE (value) == PARALLEL)
     value = XVECEXP (value, 0, 0);
+
+  if (GET_CODE (value) != SET)
+    return 0;
+
   value = XEXP (value, 0);
+
   if (GET_CODE (op) == COND_EXEC)
     op = COND_EXEC_CODE (op);
   if (GET_CODE (op) == PARALLEL)
     op = XVECEXP (op, 0, 0);
+
+  if (GET_CODE (op) != SET)
+    return 0;
+
   op = XEXP (op, 1);
 
   if (GET_CODE (op) == PLUS || GET_CODE (op) == MINUS)
@@ -169,11 +226,20 @@  arm_no_early_store_addr_dep (rtx producer, rtx consumer)
     value = COND_EXEC_CODE (value);
   if (GET_CODE (value) == PARALLEL)
     value = XVECEXP (value, 0, 0);
+
+  if (GET_CODE (value) != SET)
+    return 0;
+
   value = XEXP (value, 0);
+
   if (GET_CODE (addr) == COND_EXEC)
     addr = COND_EXEC_CODE (addr);
   if (GET_CODE (addr) == PARALLEL)
     addr = XVECEXP (addr, 0, 0);
+
+  if (GET_CODE (addr) != SET)
+    return 0;
+
   addr = XEXP (addr, 0);
 
   return !reg_overlap_mentioned_p (value, addr);