diff mbox series

Fix ICEs on invalid rs6000 asm (PR target/88188)

Message ID 20181126204347.GH12380@tucnak
State New
Headers show
Series Fix ICEs on invalid rs6000 asm (PR target/88188) | expand

Commit Message

Jakub Jelinek Nov. 26, 2018, 8:43 p.m. UTC 
Hi!

The following patch fixes a bunch of ICEs in rs6000 print_operand; we
shouldn't ICE on them when users write mess in their inline asm.

Bootstrapped/regtested on powerpc64{,le}-linux (including -m32 testing
on powerpc64-linux), ok for trunk?

2018-11-26  Jakub Jelinek  <jakub@redhat.com>

	PR target/88188
	* config/rs6000/rs6000.c (print_operand) <case 'D'>: Use
	output_operand_lossage instead of gcc_assert.
	<case 't'>: Likewise.
	<case 'z'>: Likewise.
	<case 'V'>: Use output_operand_lossage instead of gcc_unreachable.

	* gcc.target/powerpc/pr88188.c: New test.


	Jakub

Comments

Segher Boessenkool Nov. 26, 2018, 10:20 p.m. UTC | #1 
Hi Jakub,

On Mon, Nov 26, 2018 at 09:43:47PM +0100, Jakub Jelinek wrote:
> The following patch fixes a bunch of ICEs in rs6000 print_operand; we
> shouldn't ICE on them when users write mess in their inline asm.

> 2018-11-26  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR target/88188
> 	* config/rs6000/rs6000.c (print_operand) <case 'D'>: Use
> 	output_operand_lossage instead of gcc_assert.
> 	<case 't'>: Likewise.
> 	<case 'z'>: Likewise.
> 	<case 'V'>: Use output_operand_lossage instead of gcc_unreachable.
> 
> 	* gcc.target/powerpc/pr88188.c: New test.


> --- gcc/config/rs6000/rs6000.c.jj	2018-11-16 21:35:42.497982764 +0100
> +++ gcc/config/rs6000/rs6000.c	2018-11-26 10:56:28.079516458 +0100
> @@ -20644,7 +20644,11 @@ print_operand (FILE *file, rtx x, int co
>  
>      case 'D':
>        /* Like 'J' but get to the GT bit only.  */
> -      gcc_assert (REG_P (x));
> +      if (!REG_P (x))
> +	{
> +	  output_operand_lossage ("invalid %%D value");
> +	  return;
> +	}
>  
>        /* Bit 1 is GT bit.  */
>        i = 4 * (REGNO (x) - CR0_REGNO) + 1;

'D' just blows up if you pass it something not a CR field (well, it compiles
into something that doesn't assemble).

'J' uses ccr_bit.  This actually explodes (a gcc_assert, an ICE).

> @@ -20900,7 +20904,11 @@ print_operand (FILE *file, rtx x, int co
>  
>      case 't':
>        /* Like 'J' but get to the OVERFLOW/UNORDERED bit.  */
> -      gcc_assert (REG_P (x) && GET_MODE (x) == CCmode);
> +      if (!REG_P (x) || GET_MODE (x) != CCmode)
> +	{
> +	  output_operand_lossage ("invalid %%t value");
> +	  return;
> +	}
>  
>        /* Bit 3 is OV bit.  */
>        i = 4 * (REGNO (x) - CR0_REGNO) + 3;

't' does not seem easy to blow up, heh.

Both 'D' and 't', as well as ccr_bit, should use cc_reg_operand I think?
That allows some pseudos as well, but that doesn't matter.

> @@ -21059,7 +21067,11 @@ print_operand (FILE *file, rtx x, int co
>  	 names.  If we are configured for System V (or the embedded ABI) on
>  	 the PowerPC, do not emit the period, since those systems do not use
>  	 TOCs and the like.  */
> -      gcc_assert (GET_CODE (x) == SYMBOL_REF);
> +      if (GET_CODE (x) != SYMBOL_REF)

SYMBOL_REF_P please.

Okay for trunk.  Thanks!


Segher
Jakub Jelinek Nov. 26, 2018, 11:34 p.m. UTC | #2 
On Mon, Nov 26, 2018 at 04:20:18PM -0600, Segher Boessenkool wrote:
> > --- gcc/config/rs6000/rs6000.c.jj	2018-11-16 21:35:42.497982764 +0100
> > +++ gcc/config/rs6000/rs6000.c	2018-11-26 10:56:28.079516458 +0100
> > @@ -20644,7 +20644,11 @@ print_operand (FILE *file, rtx x, int co
> >  
> >      case 'D':
> >        /* Like 'J' but get to the GT bit only.  */
> > -      gcc_assert (REG_P (x));
> > +      if (!REG_P (x))
> > +	{
> > +	  output_operand_lossage ("invalid %%D value");
> > +	  return;
> > +	}
> >  
> >        /* Bit 1 is GT bit.  */
> >        i = 4 * (REGNO (x) - CR0_REGNO) + 1;
> 
> 'D' just blows up if you pass it something not a CR field (well, it compiles
> into something that doesn't assemble).

I can add || !CR_REGNO_P (REGNO (x)) .

> 'J' uses ccr_bit.  This actually explodes (a gcc_assert, an ICE).

Oops, I thought ccr_bit will just return -1.
So, shall we change all the gcc_assert/gcc_unreachable in ccr_bit to
(conditional) return -1; and perhaps add gcc_assert in the non-print_operand
callers?

> > @@ -20900,7 +20904,11 @@ print_operand (FILE *file, rtx x, int co
> >  
> >      case 't':
> >        /* Like 'J' but get to the OVERFLOW/UNORDERED bit.  */
> > -      gcc_assert (REG_P (x) && GET_MODE (x) == CCmode);
> > +      if (!REG_P (x) || GET_MODE (x) != CCmode)
> > +	{
> > +	  output_operand_lossage ("invalid %%t value");
> > +	  return;
> > +	}
> >  
> >        /* Bit 3 is OV bit.  */
> >        i = 4 * (REGNO (x) - CR0_REGNO) + 3;
> 
> 't' does not seem easy to blow up, heh.
> 
> Both 'D' and 't', as well as ccr_bit, should use cc_reg_operand I think?
> That allows some pseudos as well, but that doesn't matter.

It also allows SUBREGs and the code will crash badly if they appear.
Testing just REG_P (x) && CR_REGNO_P (REGNO (x)) seems better to me.

> > @@ -21059,7 +21067,11 @@ print_operand (FILE *file, rtx x, int co
> >  	 names.  If we are configured for System V (or the embedded ABI) on
> >  	 the PowerPC, do not emit the period, since those systems do not use
> >  	 TOCs and the like.  */
> > -      gcc_assert (GET_CODE (x) == SYMBOL_REF);
> > +      if (GET_CODE (x) != SYMBOL_REF)
> 
> SYMBOL_REF_P please.

Ok.

	Jakub
diff mbox series

Patch

--- gcc/config/rs6000/rs6000.c.jj	2018-11-16 21:35:42.497982764 +0100
+++ gcc/config/rs6000/rs6000.c	2018-11-26 10:56:28.079516458 +0100
@@ -20644,7 +20644,11 @@  print_operand (FILE *file, rtx x, int co
 
     case 'D':
       /* Like 'J' but get to the GT bit only.  */
-      gcc_assert (REG_P (x));
+      if (!REG_P (x))
+	{
+	  output_operand_lossage ("invalid %%D value");
+	  return;
+	}
 
       /* Bit 1 is GT bit.  */
       i = 4 * (REGNO (x) - CR0_REGNO) + 1;
@@ -20900,7 +20904,11 @@  print_operand (FILE *file, rtx x, int co
 
     case 't':
       /* Like 'J' but get to the OVERFLOW/UNORDERED bit.  */
-      gcc_assert (REG_P (x) && GET_MODE (x) == CCmode);
+      if (!REG_P (x) || GET_MODE (x) != CCmode)
+	{
+	  output_operand_lossage ("invalid %%t value");
+	  return;
+	}
 
       /* Bit 3 is OV bit.  */
       i = 4 * (REGNO (x) - CR0_REGNO) + 3;
@@ -20989,7 +20997,7 @@  print_operand (FILE *file, rtx x, int co
 	  fputs ("lge", file);  /* 5 */
 	  break;
 	default:
-	  gcc_unreachable ();
+	  output_operand_lossage ("invalid %%V value");
 	}
       break;
 
@@ -21059,7 +21067,11 @@  print_operand (FILE *file, rtx x, int co
 	 names.  If we are configured for System V (or the embedded ABI) on
 	 the PowerPC, do not emit the period, since those systems do not use
 	 TOCs and the like.  */
-      gcc_assert (GET_CODE (x) == SYMBOL_REF);
+      if (GET_CODE (x) != SYMBOL_REF)
+	{
+	  output_operand_lossage ("invalid %%z value");
+	  return;
+	}
 
       /* For macho, check to see if we need a stub.  */
       if (TARGET_MACHO)
--- gcc/testsuite/gcc.target/powerpc/pr88188.c.jj	2018-11-26 11:02:56.077086682 +0100
+++ gcc/testsuite/gcc.target/powerpc/pr88188.c	2018-11-26 11:02:13.621786846 +0100
@@ -0,0 +1,13 @@ 
+/* PR target/88188 */
+/* { dg-do compile } */
+
+int m;
+
+void
+foo (void)
+{
+  __asm volatile ("%D0" : : "m" (m));	/* { dg-error "invalid %D value" } */
+  __asm volatile ("%t0" : : "m" (m));	/* { dg-error "invalid %t value" } */
+  __asm volatile ("%V0" : : "r" (0));	/* { dg-error "invalid %V value" } */
+  __asm volatile ("%z0" : : "r" (0));	/* { dg-error "invalid %z value" } */
+}