This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [ARM, AArch64] Make aarch64-common.c files more robust.
- From: Richard Earnshaw <rearnsha at arm dot com>
- To: James Greenhalgh <James dot Greenhalgh at arm dot com>
- Cc: "gcc-patches at gcc dot gnu dot org" <gcc-patches at gcc dot gnu dot org>, Marcus Shawcroft <Marcus dot Shawcroft at arm dot com>, Ramana Radhakrishnan <Ramana dot Radhakrishnan at arm dot com>
- Date: Tue, 01 Oct 2013 18:01:13 +0100
- Subject: Re: [ARM, AArch64] Make aarch64-common.c files more robust.
- Authentication-results: sourceware.org; auth=none
- References: <1380531143-25769-1-git-send-email-james dot greenhalgh at arm dot com>
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.