| Message ID | 87r2s9awha.fsf@linaro.org |
|---|---|
| State | Accepted |
| Commit | c348cab062adf3946fdadff884b7f43774d72d8f |
| Headers | show |
| Series | [AArch64] Fix ICEs in aarch64_print_operand | expand |
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. > > 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" } */ > +}
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" } */ >> +}
Christophe Lyon <christophe.lyon@linaro.org> writes: > 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? I think that's a separate preexisting problem. Could you file a PR? Personally I'd just remove the assert, but I'm guessing that wouldn't be acceptable... Thanks, Richard
On 8 December 2017 at 17:05, Richard Sandiford <richard.sandiford@linaro.org> wrote: > Christophe Lyon <christophe.lyon@linaro.org> writes: >> 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? > > I think that's a separate preexisting problem. Could you file a PR? > Sure, I filed: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83335 > Personally I'd just remove the assert, but I'm guessing that wouldn't > be acceptable... > > Thanks, > Richard
Jakub Jelinek <jakub@redhat.com> writes: > Hi! > > On Fri, Dec 08, 2017 at 08:10:08PM +0100, Christophe Lyon wrote: >> >> Can you check? >> > >> > I think that's a separate preexisting problem. Could you file a PR? >> > >> >> Sure, I filed: >> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83335 >> >> > Personally I'd just remove the assert, but I'm guessing that wouldn't >> > be acceptable... > > So, I think either we can return false instead of dying on the assertion, > but then it will emit output_addr_const and often just silently emit it > without diagnosing it (first patch), or just call output_operand_lossage > there, which will ICE except for inline asm, where it will error. > It is true there is no code provided, but output_addr_const wouldn't > provide that either: > default: > if (targetm.asm_out.output_addr_const_extra (file, x)) > break; > > output_operand_lossage ("invalid expression as operand"); > in final.c. I think the testcase is valid even for ILP32, so the first sounds better to me. (It was because I thought the test was valid that I was leaving the fix to someone more familiar with ILP32 -- sorry that you've had to pick it up instead.) Thanks, Richard
On Tue, Dec 12, 2017 at 06:21:37AM +0000, Richard Sandiford wrote: > Jakub Jelinek <jakub@redhat.com> writes: > > Hi! > > > > On Fri, Dec 08, 2017 at 08:10:08PM +0100, Christophe Lyon wrote: > >> >> Can you check? > >> > > >> > I think that's a separate preexisting problem. Could you file a PR? > >> > > >> > >> Sure, I filed: > >> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83335 > >> > >> > Personally I'd just remove the assert, but I'm guessing that wouldn't > >> > be acceptable... > > > > So, I think either we can return false instead of dying on the assertion, > > but then it will emit output_addr_const and often just silently emit it > > without diagnosing it (first patch), or just call output_operand_lossage > > there, which will ICE except for inline asm, where it will error. > > It is true there is no code provided, but output_addr_const wouldn't > > provide that either: > > default: > > if (targetm.asm_out.output_addr_const_extra (file, x)) > > break; > > > > output_operand_lossage ("invalid expression as operand"); > > in final.c. > > I think the testcase is valid even for ILP32, so the first sounds better > to me. I think it is not valid to print "X" constraint operand in any way, because "X" operand can be anything. All we need to make sure is that we don't ICE on it if in inline asm. The purpose of "X" is for operands that aren't needed at all, or are needed just to denote they are read or written through other means. If you look at the testsuite, no other test but aarch64/asm-{2,3}.c attempts to print "X" operads. Jakub
Jakub Jelinek <jakub@redhat.com> writes: > On Tue, Dec 12, 2017 at 06:21:37AM +0000, Richard Sandiford wrote: >> Jakub Jelinek <jakub@redhat.com> writes: >> > Hi! >> > >> > On Fri, Dec 08, 2017 at 08:10:08PM +0100, Christophe Lyon wrote: >> >> >> Can you check? >> >> > >> >> > I think that's a separate preexisting problem. Could you file a PR? >> >> > >> >> >> >> Sure, I filed: >> >> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83335 >> >> >> >> > Personally I'd just remove the assert, but I'm guessing that wouldn't >> >> > be acceptable... >> > >> > So, I think either we can return false instead of dying on the assertion, >> > but then it will emit output_addr_const and often just silently emit it >> > without diagnosing it (first patch), or just call output_operand_lossage >> > there, which will ICE except for inline asm, where it will error. >> > It is true there is no code provided, but output_addr_const wouldn't >> > provide that either: >> > default: >> > if (targetm.asm_out.output_addr_const_extra (file, x)) >> > break; >> > >> > output_operand_lossage ("invalid expression as operand"); >> > in final.c. >> >> I think the testcase is valid even for ILP32, so the first sounds better >> to me. > > I think it is not valid to print "X" constraint operand in any way, because > "X" operand can be anything. All we need to make sure is that we don't ICE > on it if in inline asm. The purpose of "X" is for operands that aren't > needed at all, or are needed just to denote they are read or written through > other means. If you look at the testsuite, no other test but > aarch64/asm-{2,3}.c attempts to print "X" operads. The ICE triggers for "i" as well as "X" though: asm volatile ("%a0" :: "i" (&x)); Thanks, Richard
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" } */ +}
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? 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.