[1/2] Add check for 8-bit old registers in EVEX format
Checks
| Context |
Check |
Description |
| linaro-tcwg-bot/tcwg_binutils_build--master-arm |
success
|
Testing passed
|
| linaro-tcwg-bot/tcwg_binutils_build--master-aarch64 |
success
|
Testing passed
|
| linaro-tcwg-bot/tcwg_binutils_check--master-aarch64 |
success
|
Testing passed
|
| linaro-tcwg-bot/tcwg_binutils_check--master-arm |
success
|
Testing passed
|
Commit Message
gas/ChangeLog:
* config/tc-i386.c (md_assemble): Add invalid check for old byte
registers in EVEX/VEX format.
* testsuite/gas/i386/x86-64-apx-inval.l: Add new test.
* testsuite/gas/i386/x86-64-apx-inval.s: Ditto.
---
gas/config/tc-i386.c | 12 ++++++++++++
gas/testsuite/gas/i386/x86-64-apx-inval.l | 3 +++
gas/testsuite/gas/i386/x86-64-apx-inval.s | 2 ++
3 files changed, 17 insertions(+)
Comments
On 15.05.2024 08:31, Cui, Lili wrote:
> --- a/gas/config/tc-i386.c
> +++ b/gas/config/tc-i386.c
> @@ -7029,6 +7029,18 @@ md_assemble (char *line)
> as_bad (_("{rex2} prefix invalid with `%s'"), insn_name (&i.tm));
> return;
> }
> + /* Check for 8 bit operand that uses old registers. */
> + for (unsigned int op = 0; op < i.operands; op++)
> + {
> + if (i.types[op].bitfield.class == Reg
> + && i.types[op].bitfield.byte
> + && !(i.op[op].regs->reg_flags & RegRex64)
> + && i.op[op].regs->reg_num > 3)
> +
> + as_bad (_("can't encode register '%s' in an "
> + " EVEX/VEX prefix instruction"),
> + i.op[op].regs->reg_name);
> + }
I'd like us to avoid duplicating the conditional used here, matching what
establish_rex() does. establish_rex() is called unconditionally, so there
are two questions (both of which could have been addressed if there wasn't,
once again, an entirely empty patch description here): Why does the
checking there not cover this case? Is it not possible to amend that logic,
rather than introducing another instance in an entirely distinct place?
Also, nit: If this can't be folded with the existing checking, please
don't separate the as_bad() invocation by a blank line from its controlling
if(). This makes the two appear as if they don't belong together.
> --- a/gas/testsuite/gas/i386/x86-64-apx-inval.l
> +++ b/gas/testsuite/gas/i386/x86-64-apx-inval.l
> @@ -12,3 +12,6 @@
> .*:13: Error: \{nf\} unsupported for `mulx'
> .*:14: Error: \{nf\} cannot be combined with \{vex\}/\{vex3\}
> .*:15: Error: \{nf\} cannot be combined with \{vex\}/\{vex3\}
> +.*:16: Error: can't encode register 'ah' in an EVEX/VEX prefix instruction
> +.*:17: Error: can't encode register 'ah' in an EVEX/VEX prefix instruction
Nit: You did observe the odd double blank in the diagnostic, didn't you?
Plus: Why the mention of VEX? There isn't any VEX-encoded insn permitting
8- or 16-bit register operands, is there? (Otherwise I don't see why XOP
wouldn't need mentioning, too.)
Plus: The register wants presenting differently - see establish_rex().
> --- a/gas/testsuite/gas/i386/x86-64-apx-inval.s
> +++ b/gas/testsuite/gas/i386/x86-64-apx-inval.s
> @@ -13,3 +13,5 @@
> {nf} mulx %r15,%r15,%r11
> {nf} {vex} bextr %ecx, %edx, %r10d
> {vex} {nf} bextr %ecx, %edx, %r10d
> + {nf} add %dl,%ah
> + {evex} adc %dl,%ah
Nit: Inconsistent indentation.
Jan
> On 15.05.2024 08:31, Cui, Lili wrote:
> > --- a/gas/config/tc-i386.c
> > +++ b/gas/config/tc-i386.c
> > @@ -7029,6 +7029,18 @@ md_assemble (char *line)
> > as_bad (_("{rex2} prefix invalid with `%s'"), insn_name (&i.tm));
> > return;
> > }
> > + /* Check for 8 bit operand that uses old registers. */
> > + for (unsigned int op = 0; op < i.operands; op++)
> > + {
> > + if (i.types[op].bitfield.class == Reg
> > + && i.types[op].bitfield.byte
> > + && !(i.op[op].regs->reg_flags & RegRex64)
> > + && i.op[op].regs->reg_num > 3)
> > +
> > + as_bad (_("can't encode register '%s' in an "
> > + " EVEX/VEX prefix instruction"),
> > + i.op[op].regs->reg_name);
> > + }
>
> I'd like us to avoid duplicating the conditional used here, matching what
> establish_rex() does. establish_rex() is called unconditionally, so there are two
> questions (both of which could have been addressed if there wasn't, once
> again, an entirely empty patch description here): Why does the checking there
> not cover this case? Is it not possible to amend that logic, rather than
> introducing another instance in an entirely distinct place?
>
Agreed, when I added this patch, I first tried adding it with rex/rex2. But since that judgment of establish_rex() is tied to "i.rex", as you also suggested, I created a patch to detach the old register check from that branch and then extended it to check the EVEX prefixed instructions .
> > --- a/gas/testsuite/gas/i386/x86-64-apx-inval.l
> > +++ b/gas/testsuite/gas/i386/x86-64-apx-inval.l
> > @@ -12,3 +12,6 @@
> > .*:13: Error: \{nf\} unsupported for `mulx'
> > .*:14: Error: \{nf\} cannot be combined with \{vex\}/\{vex3\}
> > .*:15: Error: \{nf\} cannot be combined with \{vex\}/\{vex3\}
> > +.*:16: Error: can't encode register 'ah' in an EVEX/VEX prefix
> > +instruction
> > +.*:17: Error: can't encode register 'ah' in an EVEX/VEX prefix
> > +instruction
>
> Nit: You did observe the odd double blank in the diagnostic, didn't you?
>
Changed.
> Plus: Why the mention of VEX? There isn't any VEX-encoded insn permitting
> 8- or 16-bit register operands, is there? (Otherwise I don't see why XOP
> wouldn't need mentioning, too.)
>
Dropped VEX.
> > --- a/gas/testsuite/gas/i386/x86-64-apx-inval.s
> > +++ b/gas/testsuite/gas/i386/x86-64-apx-inval.s
> > @@ -13,3 +13,5 @@
> > {nf} mulx %r15,%r15,%r11
> > {nf} {vex} bextr %ecx, %edx, %r10d
> > {vex} {nf} bextr %ecx, %edx, %r10d
> > + {nf} add %dl,%ah
> > + {evex} adc %dl,%ah
>
> Nit: Inconsistent indentation.
>
Done.
Thanks,
Lili.
On 17.05.2024 03:45, Cui, Lili wrote:
>> On 15.05.2024 08:31, Cui, Lili wrote:
>>> --- a/gas/config/tc-i386.c
>>> +++ b/gas/config/tc-i386.c
>>> @@ -7029,6 +7029,18 @@ md_assemble (char *line)
>>> as_bad (_("{rex2} prefix invalid with `%s'"), insn_name (&i.tm));
>>> return;
>>> }
>>> + /* Check for 8 bit operand that uses old registers. */
>>> + for (unsigned int op = 0; op < i.operands; op++)
>>> + {
>>> + if (i.types[op].bitfield.class == Reg
>>> + && i.types[op].bitfield.byte
>>> + && !(i.op[op].regs->reg_flags & RegRex64)
>>> + && i.op[op].regs->reg_num > 3)
>>> +
>>> + as_bad (_("can't encode register '%s' in an "
>>> + " EVEX/VEX prefix instruction"),
>>> + i.op[op].regs->reg_name);
>>> + }
>>
>> I'd like us to avoid duplicating the conditional used here, matching what
>> establish_rex() does. establish_rex() is called unconditionally, so there are two
>> questions (both of which could have been addressed if there wasn't, once
>> again, an entirely empty patch description here): Why does the checking there
>> not cover this case? Is it not possible to amend that logic, rather than
>> introducing another instance in an entirely distinct place?
>>
>
> Agreed, when I added this patch, I first tried adding it with rex/rex2. But since that judgment of establish_rex() is tied to "i.rex", as you also suggested, I created a patch to detach the old register check from that branch and then extended it to check the EVEX prefixed instructions .
Yet that tying to "i.rex" could be amended, I think.
Jan
> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Friday, May 17, 2024 3:14 PM
> To: Cui, Lili <lili.cui@intel.com>
> Cc: hjl.tools@gmail.com; binutils@sourceware.org
> Subject: Re: [PATCH 1/2] Add check for 8-bit old registers in EVEX format
>
> On 17.05.2024 03:45, Cui, Lili wrote:
> >> On 15.05.2024 08:31, Cui, Lili wrote:
> >>> --- a/gas/config/tc-i386.c
> >>> +++ b/gas/config/tc-i386.c
> >>> @@ -7029,6 +7029,18 @@ md_assemble (char *line)
> >>> as_bad (_("{rex2} prefix invalid with `%s'"), insn_name (&i.tm));
> >>> return;
> >>> }
> >>> + /* Check for 8 bit operand that uses old registers. */
> >>> + for (unsigned int op = 0; op < i.operands; op++)
> >>> + {
> >>> + if (i.types[op].bitfield.class == Reg
> >>> + && i.types[op].bitfield.byte
> >>> + && !(i.op[op].regs->reg_flags & RegRex64)
> >>> + && i.op[op].regs->reg_num > 3)
> >>> +
> >>> + as_bad (_("can't encode register '%s' in an "
> >>> + " EVEX/VEX prefix instruction"),
> >>> + i.op[op].regs->reg_name);
> >>> + }
> >>
> >> I'd like us to avoid duplicating the conditional used here, matching
> >> what
> >> establish_rex() does. establish_rex() is called unconditionally, so
> >> there are two questions (both of which could have been addressed if
> >> there wasn't, once again, an entirely empty patch description here):
> >> Why does the checking there not cover this case? Is it not possible
> >> to amend that logic, rather than introducing another instance in an entirely
> distinct place?
> >>
> >
> > Agreed, when I added this patch, I first tried adding it with rex/rex2. But
> since that judgment of establish_rex() is tied to "i.rex", as you also suggested, I
> created a patch to detach the old register check from that branch and then
> extended it to check the EVEX prefixed instructions .
>
> Yet that tying to "i.rex" could be amended, I think.
>
Yes, I will send out patches after you review APX ZU.
Thanks,
Lili.
@@ -7029,6 +7029,18 @@ md_assemble (char *line)
as_bad (_("{rex2} prefix invalid with `%s'"), insn_name (&i.tm));
return;
}
+ /* Check for 8 bit operand that uses old registers. */
+ for (unsigned int op = 0; op < i.operands; op++)
+ {
+ if (i.types[op].bitfield.class == Reg
+ && i.types[op].bitfield.byte
+ && !(i.op[op].regs->reg_flags & RegRex64)
+ && i.op[op].regs->reg_num > 3)
+
+ as_bad (_("can't encode register '%s' in an "
+ " EVEX/VEX prefix instruction"),
+ i.op[op].regs->reg_name);
+ }
if (is_apx_evex_encoding ())
build_apx_evex_prefix ();
@@ -12,3 +12,6 @@
.*:13: Error: \{nf\} unsupported for `mulx'
.*:14: Error: \{nf\} cannot be combined with \{vex\}/\{vex3\}
.*:15: Error: \{nf\} cannot be combined with \{vex\}/\{vex3\}
+.*:16: Error: can't encode register 'ah' in an EVEX/VEX prefix instruction
+.*:17: Error: can't encode register 'ah' in an EVEX/VEX prefix instruction
+#pass
@@ -13,3 +13,5 @@
{nf} mulx %r15,%r15,%r11
{nf} {vex} bextr %ecx, %edx, %r10d
{vex} {nf} bextr %ecx, %edx, %r10d
+ {nf} add %dl,%ah
+ {evex} adc %dl,%ah