[AArch64] Fix ICEs in aarch64_print_operand_internal (PR target/83335)

Message ID 20171211173304.GQ2353@tucnak
State New
Headers show
Series
  • [AArch64] Fix ICEs in aarch64_print_operand_internal (PR target/83335)
Related show

Commit Message

Jakub Jelinek Dec. 11, 2017, 5:33 p.m.
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.

	Jakub
2017-12-11  Jakub Jelinek  <jakub@redhat.com>

	PR target/83335
	* config/aarch64/aarch64.c (aarch64_print_address_internal):
	If x doesn't have Pmode, return false to let.
2017-12-11  Jakub Jelinek  <jakub@redhat.com>

	PR target/83335
	* config/aarch64/aarch64.c (aarch64_print_address_internal):
	If x doesn't have Pmode, call output_operand_lossage and return true.

	* gcc.target/aarch64/asm-2.c: Expect error if ilp32.

--- gcc/config/aarch64/aarch64.c.jj	2017-12-11 17:54:13.000000000 +0100
+++ gcc/config/aarch64/aarch64.c	2017-12-11 18:23:25.847181675 +0100
@@ -5633,7 +5633,11 @@ 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)
+    {
+      output_operand_lossage ("invalid expression as operand");
+      return true;
+    }
 
   if (aarch64_classify_address (&addr, x, mode, op, true))
     switch (addr.type)
--- gcc/testsuite/gcc.target/aarch64/asm-2.c.jj	2017-12-08 00:50:23.000000000 +0100
+++ gcc/testsuite/gcc.target/aarch64/asm-2.c	2017-12-11 18:24:44.192215734 +0100
@@ -6,5 +6,5 @@ int x;
 void
 f (void)
 {
-  asm volatile ("%a0" :: "X" (&x));
+  asm volatile ("%a0" :: "X" (&x));	/* { dg-error "invalid" "" { target ilp32 } } */
 }

Comments

Richard Sandiford Dec. 12, 2017, 6:21 a.m. | #1
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
Jakub Jelinek Dec. 12, 2017, 7:50 a.m. | #2
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
Richard Sandiford Dec. 12, 2017, 12:29 p.m. | #3
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

Patch

--- 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)