This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: PATCH: PR gas/3712: i386 assembler accepts invalid assembly code
- From: "H. J. Lu" <hjl at lucon dot org>
- To: "rajagopal, dwarak" <dwarak dot rajagopal at amd dot com>
- Cc: "Meissner, Michael" <michael dot meissner at amd dot com>, binutils at sources dot redhat dot com
- Date: Wed, 13 Dec 2006 09:10:15 -0800
- Subject: Re: PATCH: PR gas/3712: i386 assembler accepts invalid assembly code
- References: <06FDA0246543E443ABBB36B1FCD5CC067AB813@SAUSEXMB2.amd.com>
I can't apply your patch. I got
patching file tc-i386.c
patch: **** malformed patch at line 96:
On Wed, Dec 13, 2006 at 10:45:25AM -0600, rajagopal, dwarak wrote:
> 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.
> >
>