[02/12] x86: correct VCVT{,U}SI2SD rounding mode handling

Message ID a6b399dc-e35f-bf03-51f2-a15cb4d57b2a@suse.com
State New
Headers show
Series
  • x86: disassembler fixes and some consolidation
Related show

Commit Message

Libor Bukata via Binutils July 21, 2021, 10:19 a.m.
With EVEX.W clear the instruction doesn't ignore the rounding mode, but
(like for other insns without rounding semantics) EVEX.b set causes #UD.
Hence the handling of EVEX.W needs to be done when processing
evex_rounding_64_mode, not at the decode stages.

Derive a new 64-bit testcase from the 32-bit one to cover the different
EVEX.W treatment in both cases.
---
This demonstrates a broader problem: Instructions not permitting
rounding control at all (which #UD if such was specified in the
encoding) get displayed without any hint to the badness, merely by there
not being any respective operand at all. While OP_E_memory() handles
EVEX.b (broadcast) wrongly being set (in an unhelpful way, in that not
all of the opcode bytes get consumed), there's nowhere that EVEX.b
(rounding) would be checked except for the three EXxEVexR, EXxEVexR64,
and EXxEVexS ones.

Comments

Libor Bukata via Binutils July 22, 2021, 11:18 a.m. | #1
On 21.07.2021 12:19, Jan Beulich via Binutils wrote:
> With EVEX.W clear the instruction doesn't ignore the rounding mode, but

> (like for other insns without rounding semantics) EVEX.b set causes #UD.

> Hence the handling of EVEX.W needs to be done when processing

> evex_rounding_64_mode, not at the decode stages.

> 

> Derive a new 64-bit testcase from the 32-bit one to cover the different

> EVEX.W treatment in both cases.


I've committed this and the other parts of this series, but I wonder ...

> ---

> This demonstrates a broader problem: Instructions not permitting

> rounding control at all (which #UD if such was specified in the

> encoding) get displayed without any hint to the badness, merely by there

> not being any respective operand at all. While OP_E_memory() handles

> EVEX.b (broadcast) wrongly being set (in an unhelpful way, in that not

> all of the opcode bytes get consumed), there's nowhere that EVEX.b

> (rounding) would be checked except for the three EXxEVexR, EXxEVexR64,

> and EXxEVexS ones.


... whether you have an opinion here. We could follow the model of
marking decoded bits used, but I'd like to avoid calling BadOp() from
the top level handler (I'd really like to get many of its uses dropped,
as it screws up disassembly of subsequent insns). Hence my preference
would be to express this as a pseudo-operand, e.g. {rn-bad} (to
parallel {rn-sae} and thus making visible what the two L'L bits are
set to in the encoding).

Jan
Libor Bukata via Binutils July 22, 2021, 11:31 a.m. | #2
On Thu, Jul 22, 2021 at 4:18 AM Jan Beulich <jbeulich@suse.com> wrote:
>

> On 21.07.2021 12:19, Jan Beulich via Binutils wrote:

> > With EVEX.W clear the instruction doesn't ignore the rounding mode, but

> > (like for other insns without rounding semantics) EVEX.b set causes #UD.

> > Hence the handling of EVEX.W needs to be done when processing

> > evex_rounding_64_mode, not at the decode stages.

> >

> > Derive a new 64-bit testcase from the 32-bit one to cover the different

> > EVEX.W treatment in both cases.

>

> I've committed this and the other parts of this series, but I wonder ...

>

> > ---

> > This demonstrates a broader problem: Instructions not permitting

> > rounding control at all (which #UD if such was specified in the

> > encoding) get displayed without any hint to the badness, merely by there

> > not being any respective operand at all. While OP_E_memory() handles

> > EVEX.b (broadcast) wrongly being set (in an unhelpful way, in that not

> > all of the opcode bytes get consumed), there's nowhere that EVEX.b

> > (rounding) would be checked except for the three EXxEVexR, EXxEVexR64,

> > and EXxEVexS ones.

>

> ... whether you have an opinion here. We could follow the model of

> marking decoded bits used, but I'd like to avoid calling BadOp() from

> the top level handler (I'd really like to get many of its uses dropped,

> as it screws up disassembly of subsequent insns). Hence my preference

> would be to express this as a pseudo-operand, e.g. {rn-bad} (to

> parallel {rn-sae} and thus making visible what the two L'L bits are

> set to in the encoding).


{rn-bad} sounds good to me.

Thanks.

-- 
H.J.

Patch

--- a/gas/testsuite/gas/i386/evex.d
+++ b/gas/testsuite/gas/i386/evex.d
@@ -1,5 +1,5 @@ 
 #objdump: -dw -Msuffix
-#name: i386 EVX insns
+#name: i386 EVEX insns
 
 .*: +file format .*
 
@@ -8,9 +8,12 @@  Disassembly of section .text:
 
 0+ <_start>:
  +[a-f0-9]+:	62 f1 d6 38 2a f0    	vcvtsi2ssl %eax,\{rd-sae\},%xmm5,%xmm6
+ +[a-f0-9]+:	62 f1 57 38 2a f0    	vcvtsi2sdl %eax,\(bad\),%xmm5,%xmm6
  +[a-f0-9]+:	62 f1 d7 38 2a f0    	vcvtsi2sdl %eax,\(bad\),%xmm5,%xmm6
  +[a-f0-9]+:	62 f1 d6 08 7b f0    	vcvtusi2ssl %eax,%xmm5,%xmm6
+ +[a-f0-9]+:	62 f1 57 08 7b f0    	vcvtusi2sdl %eax,%xmm5,%xmm6
  +[a-f0-9]+:	62 f1 d7 08 7b f0    	vcvtusi2sdl %eax,%xmm5,%xmm6
  +[a-f0-9]+:	62 f1 d6 38 7b f0    	vcvtusi2ssl %eax,\{rd-sae\},%xmm5,%xmm6
+ +[a-f0-9]+:	62 f1 57 38 7b f0    	vcvtusi2sdl %eax,\(bad\),%xmm5,%xmm6
  +[a-f0-9]+:	62 f1 d7 38 7b f0    	vcvtusi2sdl %eax,\(bad\),%xmm5,%xmm6
 #pass
--- a/gas/testsuite/gas/i386/evex.s
+++ b/gas/testsuite/gas/i386/evex.s
@@ -4,8 +4,11 @@ 
 	.text
 _start:
 	.byte 0x62, 0xf1, 0xd6, 0x38, 0x2a, 0xf0
+	.byte 0x62, 0xf1, 0x57, 0x38, 0x2a, 0xf0
 	.byte 0x62, 0xf1, 0xd7, 0x38, 0x2a, 0xf0
 	.byte 0x62, 0xf1, 0xd6, 0x08, 0x7b, 0xf0
+	.byte 0x62, 0xf1, 0x57, 0x08, 0x7b, 0xf0
 	.byte 0x62, 0xf1, 0xd7, 0x08, 0x7b, 0xf0
 	.byte 0x62, 0xf1, 0xd6, 0x38, 0x7b, 0xf0
+	.byte 0x62, 0xf1, 0x57, 0x38, 0x7b, 0xf0
 	.byte 0x62, 0xf1, 0xd7, 0x38, 0x7b, 0xf0
--- a/gas/testsuite/gas/i386/i386.exp
+++ b/gas/testsuite/gas/i386/i386.exp
@@ -929,6 +929,7 @@  if [gas_64_check] then {
     run_dump_test "x86-64-avx512er-intel"
     run_dump_test "x86-64-avx512pf"
     run_dump_test "x86-64-avx512pf-intel"
+    run_dump_test "x86-64-evex"
     run_dump_test "x86-64-evex-lig256"
     run_dump_test "x86-64-evex-lig512"
     run_dump_test "x86-64-evex-lig256-intel"
--- /dev/null
+++ b/gas/testsuite/gas/i386/x86-64-evex.d
@@ -0,0 +1,20 @@ 
+#objdump: -dw
+#name: x86-64 EVEX insns
+#source: evex.s
+
+.*: +file format .*
+
+
+Disassembly of section .text:
+
+0+ <_start>:
+ +[a-f0-9]+:	62 f1 d6 38 2a f0    	vcvtsi2ss %rax,\{rd-sae\},%xmm5,%xmm6
+ +[a-f0-9]+:	62 f1 57 38 2a f0    	vcvtsi2sd %eax,\(bad\),%xmm5,%xmm6
+ +[a-f0-9]+:	62 f1 d7 38 2a f0    	vcvtsi2sd %rax,\{rd-sae\},%xmm5,%xmm6
+ +[a-f0-9]+:	62 f1 d6 08 7b f0    	vcvtusi2ss %rax,%xmm5,%xmm6
+ +[a-f0-9]+:	62 f1 57 08 7b f0    	vcvtusi2sd %eax,%xmm5,%xmm6
+ +[a-f0-9]+:	62 f1 d7 08 7b f0    	vcvtusi2sd %rax,%xmm5,%xmm6
+ +[a-f0-9]+:	62 f1 d6 38 7b f0    	vcvtusi2ss %rax,\{rd-sae\},%xmm5,%xmm6
+ +[a-f0-9]+:	62 f1 57 38 7b f0    	vcvtusi2sd %eax,\(bad\),%xmm5,%xmm6
+ +[a-f0-9]+:	62 f1 d7 38 7b f0    	vcvtusi2sd %rax,\{rd-sae\},%xmm5,%xmm6
+#pass
--- a/opcodes/i386-dis-evex-prefix.h
+++ b/opcodes/i386-dis-evex-prefix.h
@@ -30,7 +30,7 @@ 
     { Bad_Opcode },
     { "vcvtsi2ss{%LQ|}",	{ XMScalar, VexScalar, EXxEVexR, Edq }, 0 },
     { Bad_Opcode },
-    { VEX_W_TABLE (EVEX_W_0F2A_P_3) },
+    { "vcvtsi2sd{%LQ|}",	{ XMScalar, VexScalar, EXxEVexR64, Edq }, 0 },
   },
   /* PREFIX_EVEX_0F51 */
   {
@@ -134,7 +134,7 @@ 
     { Bad_Opcode },
     { "vcvtusi2ss{%LQ|}",	{ XMScalar, VexScalar, EXxEVexR, Edq }, 0 },
     { VEX_W_TABLE (EVEX_W_0F7B_P_2) },
-    { VEX_W_TABLE (EVEX_W_0F7B_P_3) },
+    { "vcvtusi2sd{%LQ|}",	{ XMScalar, VexScalar, EXxEVexR64, Edq }, 0 },
   },
   /* PREFIX_EVEX_0F7E */
   {
--- a/opcodes/i386-dis-evex-w.h
+++ b/opcodes/i386-dis-evex-w.h
@@ -37,11 +37,6 @@ 
   {
     { "vmovshdup",	{ XM, EXx }, 0 },
   },
-  /* EVEX_W_0F2A_P_3 */
-  {
-    { "vcvtsi2sd{%LQ|}",	{ XMScalar, VexScalar, Ed }, 0 },
-    { "vcvtsi2sd{%LQ|}",	{ XMScalar, VexScalar, EXxEVexR64, Edq }, 0 },
-  },
   /* EVEX_W_0F51_P_1 */
   {
     { "vsqrtss",	{ XMScalar, VexScalar, EXxmm_md, EXxEVexR }, 0 },
@@ -243,11 +238,6 @@ 
     { "vcvtps2qq",	{ XM, EXEvexHalfBcstXmmq, EXxEVexR }, 0 },
     { "vcvtpd2qq",	{ XM, EXx, EXxEVexR }, 0 },
   },
-  /* EVEX_W_0F7B_P_3 */
-  {
-    { "vcvtusi2sd{%LQ|}",	{ XMScalar, VexScalar, Ed }, 0 },
-    { "vcvtusi2sd{%LQ|}",	{ XMScalar, VexScalar, EXxEVexR64, Edq }, 0 },
-  },
   /* EVEX_W_0F7E_P_1 */
   {
     { Bad_Opcode },
--- a/opcodes/i386-dis.c
+++ b/opcodes/i386-dis.c
@@ -1476,7 +1476,6 @@  enum
   EVEX_W_0F12_P_3,
   EVEX_W_0F16_P_0_M_1,
   EVEX_W_0F16_P_1,
-  EVEX_W_0F2A_P_3,
   EVEX_W_0F51_P_1,
   EVEX_W_0F51_P_3,
   EVEX_W_0F58_P_1,
@@ -1521,7 +1520,6 @@  enum
   EVEX_W_0F7A_P_2,
   EVEX_W_0F7A_P_3,
   EVEX_W_0F7B_P_2,
-  EVEX_W_0F7B_P_3,
   EVEX_W_0F7E_P_1,
   EVEX_W_0F7F_P_1,
   EVEX_W_0F7F_P_2,
@@ -13724,7 +13722,7 @@  OP_Rounding (int bytemode, int sizeflag
     switch (bytemode)
       {
       case evex_rounding_64_mode:
-	if (address_mode != mode_64bit)
+	if (address_mode != mode_64bit || !vex.w)
 	  {
 	    oappend ("(bad)");
 	    break;