[PATCH] x86/Intel: SHLD/SHRD have dual meaning

[PATCH] x86/Intel: SHLD/SHRD have dual meaning

[Jan Beulich]

Since we uniformly permit D suffixes in Intel mode whenever in AT&T mode
an L suffix may be used, we need to be consistent with this.

Take the easy route, despite that still leading to an anomaly which is
also visible from the new testcase:

	shld	eax, ecx, 1
	shld	eax, ecx, cl

can mean two things with APX: SHL with a D suffix in NDD EVEX encoding,
or the traditional SHLD in legacy encoding.
---
The alternative, more intrusive and more risky (in terms of perceived or
even real regressions) route would be to mark the few insns which permit
suffixes even in Intel syntax, and reject suffix uses when that
indicator isn't set.

--- a/gas/config/tc-i386.c
+++ b/gas/config/tc-i386.c
@@ -5389,7 +5389,7 @@ static void init_globals (void)
 }
 
 /* Helper for md_assemble() to decide whether to prepare for a possible 2nd
-   parsing pass. Instead of introducing a rarely use new insn attribute this
+   parsing pass. Instead of introducing a rarely used new insn attribute this
    utilizes a common pattern between affected templates. It is deemed
    acceptable that this will lead to unnecessary pass 2 preparations in a
    limited set of cases.  */
@@ -5401,7 +5401,10 @@ static INLINE bool may_need_pass2 (const
 	 : (t->opcode_space == SPACE_0F
 	    && (t->base_opcode | 1) == 0xbf)
 	   || (t->opcode_space == SPACE_BASE
-	       && t->base_opcode == 0x63);
+	       && t->base_opcode == 0x63)
+	   || (intel_syntax /* shld / shrd may mean suffixed shl / shr.  */
+	       && t->opcode_space == SPACE_EVEXMAP4
+	       && (t->base_opcode | 8) == 0x2c);
 }
 
 #if defined (OBJ_MAYBE_ELF) || defined (OBJ_ELF)
--- /dev/null
+++ b/gas/testsuite/gas/i386/intel-suffix.d
@@ -0,0 +1,34 @@
+#objdump: -dw
+#name: Intel syntax w/ suffixes
+
+.*: +file format .*
+
+Disassembly of section \.text:
+0+0 <.*>:
+[ 	]*[a-f0-9]+:	0f a4 c8 01[ 	]+shld   \$0x1,%ecx,%eax
+[ 	]*[a-f0-9]+:	0f a5 c8[ 	]+shld   %cl,%ecx,%eax
+[ 	]*[a-f0-9]+:	d1 e1[ 	]+shl    \$1,%ecx
+[ 	]*[a-f0-9]+:	d3 e1[ 	]+shl    %cl,%ecx
+[ 	]*[a-f0-9]+:	62 f4 7c 18 d1 e1[ 	]+shl    \$1,%ecx,%eax
+[ 	]*[a-f0-9]+:	62 f4 7c 18 d3 e1[ 	]+shl    %cl,%ecx,%eax
+[ 	]*[a-f0-9]+:	d1 e1[ 	]+shl    \$1,%ecx
+[ 	]*[a-f0-9]+:	d3 e1[ 	]+shl    %cl,%ecx
+[ 	]*[a-f0-9]+:	62 f4 7c 18 d1 c1[ 	]+rol    \$1,%ecx,%eax
+[ 	]*[a-f0-9]+:	62 f4 7c 18 d3 c1[ 	]+rol    %cl,%ecx,%eax
+[ 	]*[a-f0-9]+:	d1 c1[ 	]+rol    \$1,%ecx
+[ 	]*[a-f0-9]+:	d3 c1[ 	]+rol    %cl,%ecx
+
+0+[0-9a-f]+ <.*>:
+[ 	]*[a-f0-9]+:	0f ac c8 01[ 	]+shrd   \$0x1,%ecx,%eax
+[ 	]*[a-f0-9]+:	0f ad c8[ 	]+shrd   %cl,%ecx,%eax
+[ 	]*[a-f0-9]+:	d1 e9[ 	]+shr    \$1,%ecx
+[ 	]*[a-f0-9]+:	d3 e9[ 	]+shr    %cl,%ecx
+[ 	]*[a-f0-9]+:	62 f4 7c 18 d1 f9[ 	]+sar    \$1,%ecx,%eax
+[ 	]*[a-f0-9]+:	62 f4 7c 18 d3 f9[ 	]+sar    %cl,%ecx,%eax
+[ 	]*[a-f0-9]+:	d1 f9[ 	]+sar    \$1,%ecx
+[ 	]*[a-f0-9]+:	d3 f9[ 	]+sar    %cl,%ecx
+[ 	]*[a-f0-9]+:	62 f4 7c 18 d1 c9[ 	]+ror    \$1,%ecx,%eax
+[ 	]*[a-f0-9]+:	62 f4 7c 18 d3 c9[ 	]+ror    %cl,%ecx,%eax
+[ 	]*[a-f0-9]+:	d1 c9[ 	]+ror    \$1,%ecx
+[ 	]*[a-f0-9]+:	d3 c9[ 	]+ror    %cl,%ecx
+#pass
--- /dev/null
+++ b/gas/testsuite/gas/i386/intel-suffix.s
@@ -0,0 +1,39 @@
+	.intel_syntax noprefix
+	.text
+left:
+	shld	eax, ecx, 1
+	shld	eax, ecx, cl
+
+	shld	ecx, 1
+	shld	ecx, cl
+
+	sald	eax, ecx, 1
+	sald	eax, ecx, cl
+
+	sald	ecx, 1
+	sald	ecx, cl
+
+	rold	eax, ecx, 1
+	rold	eax, ecx, cl
+
+	rold	ecx, 1
+	rold	ecx, cl
+
+right:
+	shrd	eax, ecx, 1
+	shrd	eax, ecx, cl
+
+	shrd	ecx, 1
+	shrd	ecx, cl
+
+	sard	eax, ecx, 1
+	sard	eax, ecx, cl
+
+	sard	ecx, 1
+	sard	ecx, cl
+
+	rord	eax, ecx, 1
+	rord	eax, ecx, cl
+
+	rord	ecx, 1
+	rord	ecx, cl
--- a/gas/testsuite/gas/i386/x86-64.exp
+++ b/gas/testsuite/gas/i386/x86-64.exp
@@ -160,6 +160,7 @@ run_dump_test "x86-64-disp-intel"
 run_list_test "disp-imm-64"
 run_dump_test "intel-movs64"
 run_dump_test "intel-cmps64"
+run_dump_test "intel-suffix"
 run_dump_test "x86-64-disp32"
 run_dump_test "rexw"
 run_list_test "x86-64-specific-reg"

Re: [PATCH] x86/Intel: SHLD/SHRD have dual meaning

[Hongtao Liu]

On Fri, Apr 19, 2024 at 5:29 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> Since we uniformly permit D suffixes in Intel mode whenever in AT&T mode
> an L suffix may be used, we need to be consistent with this.
I think we need to forbid the D suffix for APX NDD SHL/SHR under Intel
mode to avoid ambiguity.
Neither SHL (always SAL), nor SHR with a D suffix (Intel mode) is
generated by GCC.
>
> Take the easy route, despite that still leading to an anomaly which is
> also visible from the new testcase:
>
>         shld    eax, ecx, 1
>         shld    eax, ecx, cl
>
> can mean two things with APX: SHL with a D suffix in NDD EVEX encoding,
> or the traditional SHLD in legacy encoding.
> ---
> The alternative, more intrusive and more risky (in terms of perceived or
> even real regressions) route would be to mark the few insns which permit
> suffixes even in Intel syntax, and reject suffix uses when that
> indicator isn't set.
>
> --- a/gas/config/tc-i386.c
> +++ b/gas/config/tc-i386.c
> @@ -5389,7 +5389,7 @@ static void init_globals (void)
>  }
>
>  /* Helper for md_assemble() to decide whether to prepare for a possible 2nd
> -   parsing pass. Instead of introducing a rarely use new insn attribute this
> +   parsing pass. Instead of introducing a rarely used new insn attribute this
>     utilizes a common pattern between affected templates. It is deemed
>     acceptable that this will lead to unnecessary pass 2 preparations in a
>     limited set of cases.  */
> @@ -5401,7 +5401,10 @@ static INLINE bool may_need_pass2 (const
>          : (t->opcode_space == SPACE_0F
>             && (t->base_opcode | 1) == 0xbf)
>            || (t->opcode_space == SPACE_BASE
> -              && t->base_opcode == 0x63);
> +              && t->base_opcode == 0x63)
> +          || (intel_syntax /* shld / shrd may mean suffixed shl / shr.  */
> +              && t->opcode_space == SPACE_EVEXMAP4
> +              && (t->base_opcode | 8) == 0x2c);
>  }
>
>  #if defined (OBJ_MAYBE_ELF) || defined (OBJ_ELF)
> --- /dev/null
> +++ b/gas/testsuite/gas/i386/intel-suffix.d
> @@ -0,0 +1,34 @@
> +#objdump: -dw
> +#name: Intel syntax w/ suffixes
> +
> +.*: +file format .*
> +
> +Disassembly of section \.text:
> +0+0 <.*>:
> +[      ]*[a-f0-9]+:    0f a4 c8 01[    ]+shld   \$0x1,%ecx,%eax
> +[      ]*[a-f0-9]+:    0f a5 c8[       ]+shld   %cl,%ecx,%eax
> +[      ]*[a-f0-9]+:    d1 e1[  ]+shl    \$1,%ecx
> +[      ]*[a-f0-9]+:    d3 e1[  ]+shl    %cl,%ecx
> +[      ]*[a-f0-9]+:    62 f4 7c 18 d1 e1[      ]+shl    \$1,%ecx,%eax
> +[      ]*[a-f0-9]+:    62 f4 7c 18 d3 e1[      ]+shl    %cl,%ecx,%eax
> +[      ]*[a-f0-9]+:    d1 e1[  ]+shl    \$1,%ecx
> +[      ]*[a-f0-9]+:    d3 e1[  ]+shl    %cl,%ecx
> +[      ]*[a-f0-9]+:    62 f4 7c 18 d1 c1[      ]+rol    \$1,%ecx,%eax
> +[      ]*[a-f0-9]+:    62 f4 7c 18 d3 c1[      ]+rol    %cl,%ecx,%eax
> +[      ]*[a-f0-9]+:    d1 c1[  ]+rol    \$1,%ecx
> +[      ]*[a-f0-9]+:    d3 c1[  ]+rol    %cl,%ecx
> +
> +0+[0-9a-f]+ <.*>:
> +[      ]*[a-f0-9]+:    0f ac c8 01[    ]+shrd   \$0x1,%ecx,%eax
> +[      ]*[a-f0-9]+:    0f ad c8[       ]+shrd   %cl,%ecx,%eax
> +[      ]*[a-f0-9]+:    d1 e9[  ]+shr    \$1,%ecx
> +[      ]*[a-f0-9]+:    d3 e9[  ]+shr    %cl,%ecx
> +[      ]*[a-f0-9]+:    62 f4 7c 18 d1 f9[      ]+sar    \$1,%ecx,%eax
> +[      ]*[a-f0-9]+:    62 f4 7c 18 d3 f9[      ]+sar    %cl,%ecx,%eax
> +[      ]*[a-f0-9]+:    d1 f9[  ]+sar    \$1,%ecx
> +[      ]*[a-f0-9]+:    d3 f9[  ]+sar    %cl,%ecx
> +[      ]*[a-f0-9]+:    62 f4 7c 18 d1 c9[      ]+ror    \$1,%ecx,%eax
> +[      ]*[a-f0-9]+:    62 f4 7c 18 d3 c9[      ]+ror    %cl,%ecx,%eax
> +[      ]*[a-f0-9]+:    d1 c9[  ]+ror    \$1,%ecx
> +[      ]*[a-f0-9]+:    d3 c9[  ]+ror    %cl,%ecx
> +#pass
> --- /dev/null
> +++ b/gas/testsuite/gas/i386/intel-suffix.s
> @@ -0,0 +1,39 @@
> +       .intel_syntax noprefix
> +       .text
> +left:
> +       shld    eax, ecx, 1
> +       shld    eax, ecx, cl
> +
> +       shld    ecx, 1
> +       shld    ecx, cl
> +
> +       sald    eax, ecx, 1
> +       sald    eax, ecx, cl
> +
> +       sald    ecx, 1
> +       sald    ecx, cl
> +
> +       rold    eax, ecx, 1
> +       rold    eax, ecx, cl
> +
> +       rold    ecx, 1
> +       rold    ecx, cl
> +
> +right:
> +       shrd    eax, ecx, 1
> +       shrd    eax, ecx, cl
> +
> +       shrd    ecx, 1
> +       shrd    ecx, cl
> +
> +       sard    eax, ecx, 1
> +       sard    eax, ecx, cl
> +
> +       sard    ecx, 1
> +       sard    ecx, cl
> +
> +       rord    eax, ecx, 1
> +       rord    eax, ecx, cl
> +
> +       rord    ecx, 1
> +       rord    ecx, cl
> --- a/gas/testsuite/gas/i386/x86-64.exp
> +++ b/gas/testsuite/gas/i386/x86-64.exp
> @@ -160,6 +160,7 @@ run_dump_test "x86-64-disp-intel"
>  run_list_test "disp-imm-64"
>  run_dump_test "intel-movs64"
>  run_dump_test "intel-cmps64"
> +run_dump_test "intel-suffix"
>  run_dump_test "x86-64-disp32"
>  run_dump_test "rexw"
>  run_list_test "x86-64-specific-reg"



-- 
BR,
Hongtao

Re: [PATCH] x86/Intel: SHLD/SHRD have dual meaning

[Jan Beulich]

On 22.04.2024 06:09, Hongtao Liu wrote:
> On Fri, Apr 19, 2024 at 5:29 PM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> Since we uniformly permit D suffixes in Intel mode whenever in AT&T mode
>> an L suffix may be used, we need to be consistent with this.
> I think we need to forbid the D suffix for APX NDD SHL/SHR under Intel
> mode to avoid ambiguity.

Hmm. Special casing just two insns is out of question imo (in fact that's
what is - unintentionally - partly happening prior to the change here).

> Neither SHL (always SAL), nor SHR with a D suffix (Intel mode) is
> generated by GCC.

While this may be deemed helpful, I actually view it as a mistake, even if
only from a cosmetic perspective: SHL is the main insn; SAL is merely an
alias (questionably using extension opcode 4 rather than 6 in gas).

Still extending what you suggest - limiting the restriction to APX - may
be an option, albeit ...

>> ---
>> The alternative, more intrusive and more risky (in terms of perceived or
>> even real regressions) route would be to mark the few insns which permit
>> suffixes even in Intel syntax, and reject suffix uses when that
>> indicator isn't set.

... as implied here I'd still consider this inconsistent.

Furthermore the point in time when suffixes are processed off of the
incoming mnemonic is too early to know whether an insn is having an APX
representation. Hence doing said extension of what you suggest would likely
end up quite hacky.

Jan

RE: [PATCH] x86/Intel: SHLD/SHRD have dual meaning

[Cui, Lili]

> On 22.04.2024 06:09, Hongtao Liu wrote:
> > On Fri, Apr 19, 2024 at 5:29 PM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> Since we uniformly permit D suffixes in Intel mode whenever in AT&T
> >> mode an L suffix may be used, we need to be consistent with this.
> > I think we need to forbid the D suffix for APX NDD SHL/SHR under Intel
> > mode to avoid ambiguity.
> 
> Hmm. Special casing just two insns is out of question imo (in fact that's what
> is - unintentionally - partly happening prior to the change here).
> 
> > Neither SHL (always SAL), nor SHR with a D suffix (Intel mode) is
> > generated by GCC.
> 
> While this may be deemed helpful, I actually view it as a mistake, even if only
> from a cosmetic perspective: SHL is the main insn; SAL is merely an alias
> (questionably using extension opcode 4 rather than 6 in gas).
> 
> Still extending what you suggest - limiting the restriction to APX - may be an
> option, albeit ...
> 
> >> ---
> >> The alternative, more intrusive and more risky (in terms of perceived
> >> or even real regressions) route would be to mark the few insns which
> >> permit suffixes even in Intel syntax, and reject suffix uses when
> >> that indicator isn't set.
> 
> ... as implied here I'd still consider this inconsistent.
> 
> Furthermore the point in time when suffixes are processed off of the
> incoming mnemonic is too early to know whether an insn is having an APX
> representation. Hence doing said extension of what you suggest would likely
> end up quite hacky.
> 

If the user messes them up, we can't figure out the original intention because the input is exactly the same, maybe we can find a suitable place to inform the user that shl + apx ndd does not support suffix?

Lili.

Re: [PATCH] x86/Intel: SHLD/SHRD have dual meaning

[Hongtao Liu]

On Mon, Apr 22, 2024 at 3:44 PM Cui, Lili <lili.cui@intel.com> wrote:
>
> > On 22.04.2024 06:09, Hongtao Liu wrote:
> > > On Fri, Apr 19, 2024 at 5:29 PM Jan Beulich <jbeulich@suse.com> wrote:
> > >>
> > >> Since we uniformly permit D suffixes in Intel mode whenever in AT&T
> > >> mode an L suffix may be used, we need to be consistent with this.
> > > I think we need to forbid the D suffix for APX NDD SHL/SHR under Intel
> > > mode to avoid ambiguity.
> >
> > Hmm. Special casing just two insns is out of question imo (in fact that's what
> > is - unintentionally - partly happening prior to the change here).
> >
I know  the suffix L is needed in AT&T mode to specify the operand
size to avoid ambiguity.
But why do we need the suffix D for Intel mode?



-- 
BR,
Hongtao

Re: [PATCH] x86/Intel: SHLD/SHRD have dual meaning

[Jan Beulich]

On 22.04.2024 09:47, Hongtao Liu wrote:
> On Mon, Apr 22, 2024 at 3:44 PM Cui, Lili <lili.cui@intel.com> wrote:
>>> On 22.04.2024 06:09, Hongtao Liu wrote:
>>>> On Fri, Apr 19, 2024 at 5:29 PM Jan Beulich <jbeulich@suse.com> wrote:
>>>>>
>>>>> Since we uniformly permit D suffixes in Intel mode whenever in AT&T
>>>>> mode an L suffix may be used, we need to be consistent with this.
>>>> I think we need to forbid the D suffix for APX NDD SHL/SHR under Intel
>>>> mode to avoid ambiguity.
>>>
>>> Hmm. Special casing just two insns is out of question imo (in fact that's what
>>> is - unintentionally - partly happening prior to the change here).
>>>
> I know  the suffix L is needed in AT&T mode to specify the operand
> size to avoid ambiguity.
> But why do we need the suffix D for Intel mode?

We don't need it. But that's not just here; the vast majority of insns should
never have permitted suffixes in Intel syntax. Yet here we are. And the
question is what to do about it. What I've been considering in the meantime
is to disallow suffixes where not needed by default, but have a command line
option to restore original behavior (perhaps right away with a deprecation
warning), just to allow people to get their (bogus) code to build again (or,
if need be, to work around issues we might introduce, i.e. mistakenly
refusing use of a suffix where one might in fact be needed). Thoughts?

Jan

Re: [PATCH] x86/Intel: SHLD/SHRD have dual meaning

[Jan Beulich]

On 22.04.2024 09:43, Cui, Lili wrote:
>> On 22.04.2024 06:09, Hongtao Liu wrote:
>>> On Fri, Apr 19, 2024 at 5:29 PM Jan Beulich <jbeulich@suse.com> wrote:
>>>>
>>>> Since we uniformly permit D suffixes in Intel mode whenever in AT&T
>>>> mode an L suffix may be used, we need to be consistent with this.
>>> I think we need to forbid the D suffix for APX NDD SHL/SHR under Intel
>>> mode to avoid ambiguity.
>>
>> Hmm. Special casing just two insns is out of question imo (in fact that's what
>> is - unintentionally - partly happening prior to the change here).
>>
>>> Neither SHL (always SAL), nor SHR with a D suffix (Intel mode) is
>>> generated by GCC.
>>
>> While this may be deemed helpful, I actually view it as a mistake, even if only
>> from a cosmetic perspective: SHL is the main insn; SAL is merely an alias
>> (questionably using extension opcode 4 rather than 6 in gas).
>>
>> Still extending what you suggest - limiting the restriction to APX - may be an
>> option, albeit ...
>>
>>>> ---
>>>> The alternative, more intrusive and more risky (in terms of perceived
>>>> or even real regressions) route would be to mark the few insns which
>>>> permit suffixes even in Intel syntax, and reject suffix uses when
>>>> that indicator isn't set.
>>
>> ... as implied here I'd still consider this inconsistent.
>>
>> Furthermore the point in time when suffixes are processed off of the
>> incoming mnemonic is too early to know whether an insn is having an APX
>> representation. Hence doing said extension of what you suggest would likely
>> end up quite hacky.
>>
> 
> If the user messes them up, we can't figure out the original intention because the input is exactly the same, maybe we can find a suitable place to inform the user that shl + apx ndd does not support suffix?

See my other reply to Hongtao. Plus, as said before, I'd much prefer if we
wouldn't start special-casing any specific insns in this regard. (Which
isn't to say that may not end up being necessary here.)

Jan

Re: [PATCH] x86/Intel: SHLD/SHRD have dual meaning

[Hongtao Liu]

On Mon, Apr 22, 2024 at 4:42 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 22.04.2024 09:47, Hongtao Liu wrote:
> > On Mon, Apr 22, 2024 at 3:44 PM Cui, Lili <lili.cui@intel.com> wrote:
> >>> On 22.04.2024 06:09, Hongtao Liu wrote:
> >>>> On Fri, Apr 19, 2024 at 5:29 PM Jan Beulich <jbeulich@suse.com> wrote:
> >>>>>
> >>>>> Since we uniformly permit D suffixes in Intel mode whenever in AT&T
> >>>>> mode an L suffix may be used, we need to be consistent with this.
> >>>> I think we need to forbid the D suffix for APX NDD SHL/SHR under Intel
> >>>> mode to avoid ambiguity.
> >>>
> >>> Hmm. Special casing just two insns is out of question imo (in fact that's what
> >>> is - unintentionally - partly happening prior to the change here).
> >>>
> > I know  the suffix L is needed in AT&T mode to specify the operand
> > size to avoid ambiguity.
> > But why do we need the suffix D for Intel mode?
>
> We don't need it. But that's not just here; the vast majority of insns should
> never have permitted suffixes in Intel syntax. Yet here we are. And the
> question is what to do about it. What I've been considering in the meantime
> is to disallow suffixes where not needed by default, but have a command line
> option to restore original behavior (perhaps right away with a deprecation
> warning), just to allow people to get their (bogus) code to build again (or,
> if need be, to work around issues we might introduce, i.e. mistakenly
> refusing use of a suffix where one might in fact be needed). Thoughts?
I prefer to issue a deprecation warning and drop the support in some
future release.
Easy maintenance.
>
> Jan



-- 
BR,
Hongtao