This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [AArch64] Fix ICEs in aarch64_print_operand
- From: Christophe Lyon <christophe dot lyon at linaro dot org>
- To: James Greenhalgh <james dot greenhalgh at arm dot com>
- Cc: Richard Sandiford <richard dot sandiford at linaro dot org>, "gcc-patches at gcc dot gnu dot org" <gcc-patches at gcc dot gnu dot org>, Richard Earnshaw <Richard dot Earnshaw at arm dot com>, Marcus Shawcroft <Marcus dot Shawcroft at arm dot com>, nd <nd at arm dot com>
- Date: Fri, 8 Dec 2017 14:40:38 +0100
- Subject: Re: [AArch64] Fix ICEs in aarch64_print_operand
- Authentication-results: sourceware.org; auth=none
- References: <87r2s9awha.fsf@linaro.org> <20171207093108.GA35729@arm.com>
Hi Richard,
On 7 December 2017 at 10:31, James Greenhalgh <james.greenhalgh@arm.com> wrote:
> On Tue, Dec 05, 2017 at 05:57:37PM +0000, Richard Sandiford wrote:
>> Three related regression fixes:
>>
>> - We can't use asserts like:
>>
>> gcc_assert (GET_MODE_SIZE (mode) == 16);
>>
>> in aarch64_print_operand because it could trigger for invalid user input.
>>
>> - The output_operand_lossage in aarch64_print_address_internal:
>>
>> output_operand_lossage ("invalid operand for '%%%c'", op);
>>
>> wasn't right because "op" is an rtx_code enum rather than the
>> prefix character.
>>
>> - aarch64_print_operand_address shouldn't call output_operand_lossage
>> (because it doesn't have a prefix code) but instead fall back to
>> output_addr_const.
>>
>> Tested on aarch64-linux-gnu. OK to install?
>
> OK.
>
> Thanks,
> James
>
>>
>> Thanks,
>> Richard
>>
>>
>> 2017-12-05 Richard Sandiford <richard.sandiford@linaro.org>
>>
>> gcc/
>> * config/aarch64/aarch64.c (aarch64_print_address_internal): Return
>> a bool success value. Don't call output_operand_lossage here.
>> (aarch64_print_ldpstp_address): Return a bool success value.
>> (aarch64_print_operand_address): Call output_addr_const if
>> aarch64_print_address_internal fails.
>> (aarch64_print_operand): Don't assert that the mode is 16 bytes for
>> 'y'; call output_operand_lossage instead. Call output_operand_lossage
>> if aarch64_print_ldpstp_address fails.
>>
>> gcc/testsuite/
>> * gcc.target/aarch64/asm-2.c: New test.
>> * gcc.target/aarch64/asm-3.c: Likewise.
>>
The new test gcc.target/aarch64/asm-2.c ICEs when compiled with -mabi=ilp32:
/gcc/testsuite/gcc.target/aarch64/asm-2.c: In function 'f':
/gcc/testsuite/gcc.target/aarch64/asm-2.c:10:1: internal compiler
error: in aarch64_print_address_internal, at
config/aarch64/aarch64.c:5636
0xf2afd3 aarch64_print_address_internal
/gcc/config/aarch64/aarch64.c:5636
0xf2affd aarch64_print_operand_address
/gcc/config/aarch64/aarch64.c:5733
0x7fdd43 output_address(machine_mode, rtx_def*)
/gcc/final.c:3913
0x801288 output_asm_insn(char const*, rtx_def**)
/gcc/final.c:3770
0x802437 final_scan_insn(rtx_insn*, _IO_FILE*, int, int, int*)
/gcc/final.c:2673
0x802a1a final(rtx_insn*, _IO_FILE*, int)
/gcc/final.c:2052
0x8035ab rest_of_handle_final
/gcc/final.c:4498
0x8035ab execute
/gcc/final.c:4572
Can you check?
Thanks,
Christophe
>> Index: gcc/config/aarch64/aarch64.c
>> ===================================================================
>> --- gcc/config/aarch64/aarch64.c 2017-12-05 14:24:52.477015238 +0000
>> +++ gcc/config/aarch64/aarch64.c 2017-12-05 17:54:56.466247227 +0000
>> @@ -150,7 +150,7 @@ static bool aarch64_builtin_support_vect
>> bool is_packed);
>> static machine_mode
>> aarch64_simd_container_mode (scalar_mode mode, unsigned width);
>> -static void aarch64_print_ldpstp_address (FILE *f, machine_mode mode, rtx x);
>> +static bool aarch64_print_ldpstp_address (FILE *, machine_mode, rtx);
>>
>> /* Major revision number of the ARM Architecture implemented by the target. */
>> unsigned aarch64_architecture_version;
>> @@ -5600,22 +5600,21 @@ #define buf_size 20
>> {
>> machine_mode mode = GET_MODE (x);
>>
>> - if (GET_CODE (x) != MEM)
>> + if (GET_CODE (x) != MEM
>> + || (code == 'y' && GET_MODE_SIZE (mode) != 16))
>> {
>> output_operand_lossage ("invalid operand for '%%%c'", code);
>> return;
>> }
>>
>> if (code == 'y')
>> - {
>> - /* LDP/STP which uses a single double-width memory operand.
>> - Adjust the mode to appear like a typical LDP/STP.
>> - Currently this is supported for 16-byte accesses only. */
>> - gcc_assert (GET_MODE_SIZE (mode) == 16);
>> - mode = DFmode;
>> - }
>> + /* LDP/STP which uses a single double-width memory operand.
>> + Adjust the mode to appear like a typical LDP/STP.
>> + Currently this is supported for 16-byte accesses only. */
>> + mode = DFmode;
>>
>> - aarch64_print_ldpstp_address (f, mode, XEXP (x, 0));
>> + if (!aarch64_print_ldpstp_address (f, mode, XEXP (x, 0)))
>> + output_operand_lossage ("invalid operand prefix '%%%c'", code);
>> }
>> break;
>>
>> @@ -5628,7 +5627,7 @@ #define buf_size 20
>> /* Print address 'x' of a memory access with mode 'mode'.
>> 'op' is the context required by aarch64_classify_address. It can either be
>> MEM for a normal memory access or PARALLEL for LDP/STP. */
>> -static void
>> +static bool
>> aarch64_print_address_internal (FILE *f, machine_mode mode, rtx x, RTX_CODE op)
>> {
>> struct aarch64_address_info addr;
>> @@ -5645,7 +5644,7 @@ aarch64_print_address_internal (FILE *f,
>> else
>> asm_fprintf (f, "[%s, %wd]", reg_names [REGNO (addr.base)],
>> INTVAL (addr.offset));
>> - return;
>> + return true;
>>
>> case ADDRESS_REG_REG:
>> if (addr.shift == 0)
>> @@ -5654,7 +5653,7 @@ aarch64_print_address_internal (FILE *f,
>> else
>> asm_fprintf (f, "[%s, %s, lsl %u]", reg_names [REGNO (addr.base)],
>> reg_names [REGNO (addr.offset)], addr.shift);
>> - return;
>> + return true;
>>
>> case ADDRESS_REG_UXTW:
>> if (addr.shift == 0)
>> @@ -5663,7 +5662,7 @@ aarch64_print_address_internal (FILE *f,
>> else
>> asm_fprintf (f, "[%s, w%d, uxtw %u]", reg_names [REGNO (addr.base)],
>> REGNO (addr.offset) - R0_REGNUM, addr.shift);
>> - return;
>> + return true;
>>
>> case ADDRESS_REG_SXTW:
>> if (addr.shift == 0)
>> @@ -5672,7 +5671,7 @@ aarch64_print_address_internal (FILE *f,
>> else
>> asm_fprintf (f, "[%s, w%d, sxtw %u]", reg_names [REGNO (addr.base)],
>> REGNO (addr.offset) - R0_REGNUM, addr.shift);
>> - return;
>> + return true;
>>
>> case ADDRESS_REG_WB:
>> switch (GET_CODE (x))
>> @@ -5680,27 +5679,27 @@ aarch64_print_address_internal (FILE *f,
>> case PRE_INC:
>> asm_fprintf (f, "[%s, %d]!", reg_names [REGNO (addr.base)],
>> GET_MODE_SIZE (mode));
>> - return;
>> + return true;
>> case POST_INC:
>> asm_fprintf (f, "[%s], %d", reg_names [REGNO (addr.base)],
>> GET_MODE_SIZE (mode));
>> - return;
>> + return true;
>> case PRE_DEC:
>> asm_fprintf (f, "[%s, -%d]!", reg_names [REGNO (addr.base)],
>> GET_MODE_SIZE (mode));
>> - return;
>> + return true;
>> case POST_DEC:
>> asm_fprintf (f, "[%s], -%d", reg_names [REGNO (addr.base)],
>> GET_MODE_SIZE (mode));
>> - return;
>> + return true;
>> case PRE_MODIFY:
>> asm_fprintf (f, "[%s, %wd]!", reg_names [REGNO (addr.base)],
>> INTVAL (addr.offset));
>> - return;
>> + return true;
>> case POST_MODIFY:
>> asm_fprintf (f, "[%s], %wd", reg_names [REGNO (addr.base)],
>> INTVAL (addr.offset));
>> - return;
>> + return true;
>> default:
>> break;
>> }
>> @@ -5710,28 +5709,29 @@ aarch64_print_address_internal (FILE *f,
>> asm_fprintf (f, "[%s, #:lo12:", reg_names [REGNO (addr.base)]);
>> output_addr_const (f, addr.offset);
>> asm_fprintf (f, "]");
>> - return;
>> + return true;
>>
>> case ADDRESS_SYMBOLIC:
>> output_addr_const (f, x);
>> - return;
>> + return true;
>> }
>>
>> - output_operand_lossage ("invalid operand for '%%%c'", op);
>> + return false;
>> }
>>
>> /* Print address 'x' of a LDP/STP with mode 'mode'. */
>> -static void
>> +static bool
>> aarch64_print_ldpstp_address (FILE *f, machine_mode mode, rtx x)
>> {
>> - aarch64_print_address_internal (f, mode, x, PARALLEL);
>> + return aarch64_print_address_internal (f, mode, x, PARALLEL);
>> }
>>
>> /* Print address 'x' of a memory access with mode 'mode'. */
>> static void
>> aarch64_print_operand_address (FILE *f, machine_mode mode, rtx x)
>> {
>> - aarch64_print_address_internal (f, mode, x, MEM);
>> + if (!aarch64_print_address_internal (f, mode, x, MEM))
>> + output_addr_const (f, x);
>> }
>>
>> bool
>> Index: gcc/testsuite/gcc.target/aarch64/asm-2.c
>> ===================================================================
>> --- /dev/null 2017-12-05 14:21:55.753572108 +0000
>> +++ gcc/testsuite/gcc.target/aarch64/asm-2.c 2017-12-05 17:54:56.466247227 +0000
>> @@ -0,0 +1,10 @@
>> +/* { dg-do compile } */
>> +/* { dg-options "-O2" } */
>> +
>> +int x;
>> +
>> +void
>> +f (void)
>> +{
>> + asm volatile ("%a0" :: "X" (&x));
>> +}
>> Index: gcc/testsuite/gcc.target/aarch64/asm-3.c
>> ===================================================================
>> --- /dev/null 2017-12-05 14:21:55.753572108 +0000
>> +++ gcc/testsuite/gcc.target/aarch64/asm-3.c 2017-12-05 17:54:56.467247189 +0000
>> @@ -0,0 +1,10 @@
>> +/* { dg-do compile } */
>> +/* { dg-options "-O2" } */
>> +
>> +int x;
>> +
>> +void
>> +f (void)
>> +{
>> + asm volatile ("%y0" :: "X" (x)); /* { dg-error "invalid" } */
>> +}