RE: PATCH: PR gas/3712: i386 assembler accepts invalid assembly code

["rajagopal, dwarak" <dwarak dot rajagopal at amd dot com>]

Hi,

> +	      switch (t->operands)
> +		{
> +		case 4:
> +		  overlap3 = i.types[3] & operand_types[3];

You also have to set overlap2 as it is not set. 
	overlap2 = i.types[2] & operand_types[2];
I have added this and attached the patch.


> +		  if (!MATCH (overlap3, i.types[3], operand_types[3])
> +		      || !CONSISTENT_REGISTER_MATCH (overlap2,
> +						     i.types[2],
> +						     operand_types[2],
> +						     overlap3,
> +						     i.types[3],
> +						     operand_types[3]))
> +		    continue;
> +		case 3:


The patch for the testsuite looks fine.

Thanks,
Dwarak


> -----Original Message-----
> From: H. J. Lu [mailto:hjl@lucon.org]
> Sent: Wednesday, December 13, 2006 8:37 AM
> To: rajagopal, dwarak; Meissner, Michael
> Cc: binutils@sources.redhat.com
> Subject: PATCH: PR gas/3712: i386 assembler accepts invalid assembly
code
> 
> On Tue, Dec 12, 2006 at 10:57:01PM -0800, H. J. Lu wrote:
> > Hi Dwarakanath, Michael,
> >
> > i386 assembler accepts invalid assembly code:
> >
> > http://sourceware.org/bugzilla/show_bug.cgi?id=3712
> >
> > Your change
> >
> > http://sourceware.org/ml/binutils/2006-07/msg00131.html
> >
> > added the 4th operand. But it doesn't check if the 4th operand is
> > valid. Can you take a look?
> >
> 
> Hi Dwarakanath, Michael,
> 
> Does this patch look OK to you?
> 
> Thanks.
> 
> 
> H.J.
> -----
> gas/
> 
> 2006-12-13  H.J. Lu  <hongjiu.lu@intel.com>
> 
> 	PR gas/3712
> 	* config/tc-i386.c (match_template): Use MAX_OPERANDS for the
> 	number of operands. Issue an error if MAX_OPERANDS >= 5. Add
> 	the 4th operand check.
> 
> gas/testsuite/
> 
> 2006-12-13  H.J. Lu  <hongjiu.lu@intel.com>
> 
> 	PR gas/3712
> 	* gas/i386/inval.s: Add invalid insertq.
> 	* gas/i386/x86-64-inval.s: Likewise.
> 
> 	* gas/i386/inval.l: Updated.
> 	* gas/i386/x86-64-inval.l: Likewise.
> 
> --- gas/config/tc-i386.c.op	2006-11-09 07:36:27.000000000 -0800
> +++ gas/config/tc-i386.c	2006-12-13 06:33:25.000000000 -0800
> @@ -2568,12 +2568,16 @@ match_template ()
>  {
>    /* Points to template once we've found it.  */
>    const template *t;
> -  unsigned int overlap0, overlap1, overlap2;
> +  unsigned int overlap0, overlap1, overlap2, overlap3;
>    unsigned int found_reverse_match;
>    int suffix_check;
> -  unsigned int operand_types [3];
> +  unsigned int operand_types [MAX_OPERANDS];
>    int addr_prefix_disp;
> 
> +#if MAX_OPERANDS >= 5
> +# error "MAX_OPERANDS must be less than 5."
> +#endif
> +
>  #define MATCH(overlap, given, template)
\
>    ((overlap & ~JumpAbsolute)					\
>     && (((given) & (BaseIndex | JumpAbsolute))
\
> @@ -2590,10 +2594,12 @@ match_template ()
>    overlap0 = 0;
>    overlap1 = 0;
>    overlap2 = 0;
> +  overlap3 = 0;
>    found_reverse_match = 0;
>    operand_types [0] = 0;
>    operand_types [1] = 0;
>    operand_types [2] = 0;
> +  operand_types [3] = 0;
>    addr_prefix_disp = -1;
>    suffix_check = (i.suffix == BYTE_MNEM_SUFFIX
>  		  ? No_bSuf
> @@ -2625,6 +2631,7 @@ match_template ()
>        operand_types [0] = t->operand_types [0];
>        operand_types [1] = t->operand_types [1];
>        operand_types [2] = t->operand_types [2];
> +      operand_types [3] = t->operand_types [3];
> 
>        /* In general, don't allow 64-bit operands in 32-bit mode.  */
>        if (i.suffix == QWORD_MNEM_SUFFIX
> @@ -2670,7 +2677,7 @@ match_template ()
>  	      break;
>  	    }
> 
> -	    for (j = 0; j < 3; j++)
> +	    for (j = 0; j < MAX_OPERANDS; j++)
>  	      {
>  		/* There should be only one Disp operand.  */
>  		if ((operand_types[j] & DispOff))
> @@ -2692,6 +2699,7 @@ match_template ()
>  	  break;
>  	case 2:
>  	case 3:
> +	case 4:
>  	  overlap1 = i.types[1] & operand_types[1];
>  	  if (!MATCH (overlap0, i.types[0], operand_types[0])
>  	      || !MATCH (overlap1, i.types[1], operand_types[1])
> @@ -2726,23 +2734,39 @@ match_template ()
>  		 we've found.  */
>  	      found_reverse_match = t->opcode_modifier & (D | FloatDR);
>  	    }
> -	  /* Found a forward 2 operand match here.  */
> -	  else if (t->operands == 3)
> +	  else
>  	    {
> -	      /* Here we make use of the fact that there are no
> -		 reverse match 3 operand instructions, and all 3
> -		 operand instructions only need to be checked for
> -		 register consistency between operands 2 and 3.  */
> -	      overlap2 = i.types[2] & operand_types[2];
> -	      if (!MATCH (overlap2, i.types[2], operand_types[2])
> -		  || !CONSISTENT_REGISTER_MATCH (overlap1, i.types[1],
> -						 operand_types[1],
> -						 overlap2, i.types[2],
> -						 operand_types[2]))
> -
> -		continue;
> +	      /* Found a forward 2 operand match here.  */
> +	      switch (t->operands)
> +		{
> +		case 4:
> +		  overlap3 = i.types[3] & operand_types[3];
> +		  if (!MATCH (overlap3, i.types[3], operand_types[3])
> +		      || !CONSISTENT_REGISTER_MATCH (overlap2,
> +						     i.types[2],
> +						     operand_types[2],
> +						     overlap3,
> +						     i.types[3],
> +						     operand_types[3]))
> +		    continue;
> +		case 3:
> +		  /* Here we make use of the fact that there are no
> +		     reverse match 3 operand instructions, and all 3
> +		     operand instructions only need to be checked for
> +		     register consistency between operands 2 and 3.  */
> +		  overlap2 = i.types[2] & operand_types[2];
> +		  if (!MATCH (overlap2, i.types[2], operand_types[2])
> +		      || !CONSISTENT_REGISTER_MATCH (overlap1,
> +						     i.types[1],
> +						     operand_types[1],
> +						     overlap2,
> +						     i.types[2],
> +						     operand_types[2]))
> +		    continue;
> +		  break;
> +		}
>  	    }
> -	  /* Found either forward/reverse 2 or 3 operand match here:
> +	  /* Found either forward/reverse 2, 3 or 4 operand match here:
>  	     slip through to break.  */
>  	}
>        if (t->cpu_flags & ~cpu_arch_flags)
> --- gas/testsuite/gas/i386/inval.l.op	2006-04-18 10:52:37.000000000 -
> 0700
> +++ gas/testsuite/gas/i386/inval.l	2006-12-13 06:28:07.000000000
-0800
> @@ -46,6 +46,7 @@
>  .*:47: Error: .*
>  .*:48: Error: .*
>  .*:49: Error: .*
> +.*:50: Error: .*
>  GAS LISTING .*
> 
> 
> @@ -98,3 +99,4 @@ GAS LISTING .*
>    47 [ 	]*	fcompll	28\(%ebp\)
>    48 [ 	]*	fldlw	\(%eax\)
>    49 [ 	]*	movl	\$%ebx,%eax
> +  50 [ 	]*	insertq	\$4,\$2,%xmm2,%ebx
> --- gas/testsuite/gas/i386/inval.s.op	2006-04-18 10:52:37.000000000 -
> 0700
> +++ gas/testsuite/gas/i386/inval.s	2006-12-13 06:27:10.000000000
-0800
> @@ -47,3 +47,4 @@ foo:	jaw	foo
>  	fcompll	28(%ebp)
>  	fldlw	(%eax)
>  	movl	$%ebx,%eax
> +	insertq	$4,$2,%xmm2,%ebx
> --- gas/testsuite/gas/i386/x86-64-inval.l.op	2004-11-25
> 00:42:54.000000000 -0800
> +++ gas/testsuite/gas/i386/x86-64-inval.l	2006-12-13
06:29:32.000000000 -
> 0800
> @@ -48,6 +48,7 @@
>  .*:49: Error: .*
>  .*:50: Error: .*
>  .*:51: Error: .*
> +.*:52: Error: .*
>  GAS LISTING .*
> 
> 
> @@ -102,3 +103,4 @@ GAS LISTING .*
>    49 [ 	]*pushfl		# can't have 32-bit stack
operands
>    50 [ 	]*popfl		# can't have 32-bit stack operands
>    51 [ 	]*retl		# can't have 32-bit stack operands
> +  52 [ 	]*insertq \$4,\$2,%xmm2,%ebx # The last operand must be
XMM
> register.
> --- gas/testsuite/gas/i386/x86-64-inval.s.op	2004-11-25
> 00:42:54.000000000 -0800
> +++ gas/testsuite/gas/i386/x86-64-inval.s	2006-12-13
06:29:42.000000000 -
> 0800
> @@ -49,3 +49,4 @@ foo:	jcxz foo	# No prefix exists to
sele
>          pushfl		# can't have 32-bit stack operands
>  	popfl		# can't have 32-bit stack operands
>  	retl		# can't have 32-bit stack operands
> +	insertq $4,$2,%xmm2,%ebx # The last operand must be XMM
register.
>

Attachment: bug-3712.patch
Description: bug-3712.patch