| Message ID | 20171211173304.GQ2353@tucnak |
|---|---|
| State | New |
| Headers | show |
| Series | [AArch64] Fix ICEs in aarch64_print_operand_internal (PR target/83335) | expand |
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
--- gcc/config/aarch64/aarch64.c.jj 2017-12-11 17:54:13.000000000 +0100 +++ gcc/config/aarch64/aarch64.c 2017-12-11 17:57:41.245261299 +0100 @@ -5633,7 +5633,8 @@ aarch64_print_address_internal (FILE *f, struct aarch64_address_info addr; /* Check all addresses are Pmode - including ILP32. */ - gcc_assert (GET_MODE (x) == Pmode); + if (GET_MODE (x) != Pmode) + return false; if (aarch64_classify_address (&addr, x, mode, op, true)) switch (addr.type)