diff mbox series

[2/2] x86: don't allow pseudo-prefixes to be overridden by legacy suffixes

Message ID f8725897-acbb-2376-2ac7-2401177bf55b@suse.com
State Unresolved
Headers
Series x86: pseudo-prefix handling |

Checks

Context Check Description
snail/binutils-gdb-check warning Git am fail log

Commit Message

Jan Beulich Nov. 6, 2023, 2:22 p.m. UTC 
  Deprecated functionality would better not win over its modern
counterparts.
---
We could be more strict, in disallowing legacy prefixes when any pseudo-
prefix was used.

I further wonder about us accepting .d32 even when a pre-386 CPU was
selected.

Comments

Cui, Lili Nov. 7, 2023, 8:47 a.m. UTC | #1 
> Subject: [PATCH 2/2] x86: don't allow pseudo-prefixes to be overridden by
> legacy suffixes
> 
> Deprecated functionality would better not win over its modern counterparts.
> ---
> We could be more strict, in disallowing legacy prefixes when any pseudo-
> prefix was used.
> 
> I further wonder about us accepting .d32 even when a pre-386 CPU was
> selected.
> 
> --- a/gas/config/tc-i386.c
> +++ b/gas/config/tc-i386.c
> @@ -5899,16 +5899,32 @@ parse_insn (const char *line, char *mnem
>  	 Check if we should swap operand or force 32bit displacement in
>  	 encoding.  */
>        if (mnem_p - 2 == dot_p && dot_p[1] == 's')
> -	i.dir_encoding = dir_encoding_swap;
> +	{
> +	  if (i.dir_encoding == dir_encoding_default)
> +	    i.dir_encoding = dir_encoding_swap;
> +	  else
> +	    as_warn (_("ignoring `.s' suffix due to earlier `{%s}'"),
> +		     i.dir_encoding == dir_encoding_load ? "load" : "store");
> +	}
>        else if (mnem_p - 3 == dot_p
>  	       && dot_p[1] == 'd'
>  	       && dot_p[2] == '8')
> -	i.disp_encoding = disp_encoding_8bit;
> +	{
> +	  if (i.disp_encoding == disp_encoding_default)
> +	    i.disp_encoding = disp_encoding_8bit;
> +	  else if (i.disp_encoding != disp_encoding_8bit)
> +	    as_warn (_("ignoring `.d8' suffix due to earlier `{disp<N>}'"));
> +	}
>        else if (mnem_p - 4 == dot_p
>  	       && dot_p[1] == 'd'
>  	       && dot_p[2] == '3'
>  	       && dot_p[3] == '2')
> -	i.disp_encoding = disp_encoding_32bit;
> +	{
> +	  if (i.disp_encoding == disp_encoding_default)
> +	    i.disp_encoding = disp_encoding_32bit;
> +	  else if (i.disp_encoding != disp_encoding_32bit)
> +	    as_warn (_("ignoring `.d32' suffix due to earlier `{disp<N>}'"));
> +	}
>        else
>  	goto check_suffix;
>        mnem_p = dot_p;

Do we use disp like this "{disp8}{disp32}" now? Do we need to add invalid test cases for it?

Lili.
Jan Beulich Nov. 7, 2023, 10:07 a.m. UTC | #2 
On 07.11.2023 09:47, Cui, Lili wrote:
>> --- a/gas/config/tc-i386.c
>> +++ b/gas/config/tc-i386.c
>> @@ -5899,16 +5899,32 @@ parse_insn (const char *line, char *mnem
>>  	 Check if we should swap operand or force 32bit displacement in
>>  	 encoding.  */
>>        if (mnem_p - 2 == dot_p && dot_p[1] == 's')
>> -	i.dir_encoding = dir_encoding_swap;
>> +	{
>> +	  if (i.dir_encoding == dir_encoding_default)
>> +	    i.dir_encoding = dir_encoding_swap;
>> +	  else
>> +	    as_warn (_("ignoring `.s' suffix due to earlier `{%s}'"),
>> +		     i.dir_encoding == dir_encoding_load ? "load" : "store");
>> +	}
>>        else if (mnem_p - 3 == dot_p
>>  	       && dot_p[1] == 'd'
>>  	       && dot_p[2] == '8')
>> -	i.disp_encoding = disp_encoding_8bit;
>> +	{
>> +	  if (i.disp_encoding == disp_encoding_default)
>> +	    i.disp_encoding = disp_encoding_8bit;
>> +	  else if (i.disp_encoding != disp_encoding_8bit)
>> +	    as_warn (_("ignoring `.d8' suffix due to earlier `{disp<N>}'"));
>> +	}
>>        else if (mnem_p - 4 == dot_p
>>  	       && dot_p[1] == 'd'
>>  	       && dot_p[2] == '3'
>>  	       && dot_p[3] == '2')
>> -	i.disp_encoding = disp_encoding_32bit;
>> +	{
>> +	  if (i.disp_encoding == disp_encoding_default)
>> +	    i.disp_encoding = disp_encoding_32bit;
>> +	  else if (i.disp_encoding != disp_encoding_32bit)
>> +	    as_warn (_("ignoring `.d32' suffix due to earlier `{disp<N>}'"));
>> +	}
>>        else
>>  	goto check_suffix;
>>        mnem_p = dot_p;
> 
> Do we use disp like this "{disp8}{disp32}" now?

I think this is permitted, and the last one wins. No need to disallow imo.

Jan
Cui, Lili Nov. 7, 2023, 2:07 p.m. UTC | #3 
> Subject: Re: [PATCH 2/2] x86: don't allow pseudo-prefixes to be overridden by
> legacy suffixes
> 
> On 07.11.2023 09:47, Cui, Lili wrote:
> >> --- a/gas/config/tc-i386.c
> >> +++ b/gas/config/tc-i386.c
> >> @@ -5899,16 +5899,32 @@ parse_insn (const char *line, char *mnem
> >>  	 Check if we should swap operand or force 32bit displacement in
> >>  	 encoding.  */
> >>        if (mnem_p - 2 == dot_p && dot_p[1] == 's')
> >> -	i.dir_encoding = dir_encoding_swap;
> >> +	{
> >> +	  if (i.dir_encoding == dir_encoding_default)
> >> +	    i.dir_encoding = dir_encoding_swap;
> >> +	  else
> >> +	    as_warn (_("ignoring `.s' suffix due to earlier `{%s}'"),
> >> +		     i.dir_encoding == dir_encoding_load ? "load" : "store");
> >> +	}
> >>        else if (mnem_p - 3 == dot_p
> >>  	       && dot_p[1] == 'd'
> >>  	       && dot_p[2] == '8')
> >> -	i.disp_encoding = disp_encoding_8bit;
> >> +	{
> >> +	  if (i.disp_encoding == disp_encoding_default)
> >> +	    i.disp_encoding = disp_encoding_8bit;
> >> +	  else if (i.disp_encoding != disp_encoding_8bit)
> >> +	    as_warn (_("ignoring `.d8' suffix due to earlier `{disp<N>}'"));
> >> +	}
> >>        else if (mnem_p - 4 == dot_p
> >>  	       && dot_p[1] == 'd'
> >>  	       && dot_p[2] == '3'
> >>  	       && dot_p[3] == '2')
> >> -	i.disp_encoding = disp_encoding_32bit;
> >> +	{
> >> +	  if (i.disp_encoding == disp_encoding_default)
> >> +	    i.disp_encoding = disp_encoding_32bit;
> >> +	  else if (i.disp_encoding != disp_encoding_32bit)
> >> +	    as_warn (_("ignoring `.d32' suffix due to earlier `{disp<N>}'"));
> >> +	}
> >>        else
> >>  	goto check_suffix;
> >>        mnem_p = dot_p;
> >
> > Do we use disp like this "{disp8}{disp32}" now?
> 
> I think this is permitted, and the last one wins. No need to disallow imo.
> 
This expression should be a typo, since the first expression neither constrains the following expression nor actually generates the bits. Maybe a warning is enough.
Jan Beulich Nov. 7, 2023, 3:11 p.m. UTC | #4 
On 07.11.2023 15:07, Cui, Lili wrote:
>> Subject: Re: [PATCH 2/2] x86: don't allow pseudo-prefixes to be overridden by
>> legacy suffixes
>>
>> On 07.11.2023 09:47, Cui, Lili wrote:
>>>> --- a/gas/config/tc-i386.c
>>>> +++ b/gas/config/tc-i386.c
>>>> @@ -5899,16 +5899,32 @@ parse_insn (const char *line, char *mnem
>>>>  	 Check if we should swap operand or force 32bit displacement in
>>>>  	 encoding.  */
>>>>        if (mnem_p - 2 == dot_p && dot_p[1] == 's')
>>>> -	i.dir_encoding = dir_encoding_swap;
>>>> +	{
>>>> +	  if (i.dir_encoding == dir_encoding_default)
>>>> +	    i.dir_encoding = dir_encoding_swap;
>>>> +	  else
>>>> +	    as_warn (_("ignoring `.s' suffix due to earlier `{%s}'"),
>>>> +		     i.dir_encoding == dir_encoding_load ? "load" : "store");
>>>> +	}
>>>>        else if (mnem_p - 3 == dot_p
>>>>  	       && dot_p[1] == 'd'
>>>>  	       && dot_p[2] == '8')
>>>> -	i.disp_encoding = disp_encoding_8bit;
>>>> +	{
>>>> +	  if (i.disp_encoding == disp_encoding_default)
>>>> +	    i.disp_encoding = disp_encoding_8bit;
>>>> +	  else if (i.disp_encoding != disp_encoding_8bit)
>>>> +	    as_warn (_("ignoring `.d8' suffix due to earlier `{disp<N>}'"));
>>>> +	}
>>>>        else if (mnem_p - 4 == dot_p
>>>>  	       && dot_p[1] == 'd'
>>>>  	       && dot_p[2] == '3'
>>>>  	       && dot_p[3] == '2')
>>>> -	i.disp_encoding = disp_encoding_32bit;
>>>> +	{
>>>> +	  if (i.disp_encoding == disp_encoding_default)
>>>> +	    i.disp_encoding = disp_encoding_32bit;
>>>> +	  else if (i.disp_encoding != disp_encoding_32bit)
>>>> +	    as_warn (_("ignoring `.d32' suffix due to earlier `{disp<N>}'"));
>>>> +	}
>>>>        else
>>>>  	goto check_suffix;
>>>>        mnem_p = dot_p;
>>>
>>> Do we use disp like this "{disp8}{disp32}" now?
>>
>> I think this is permitted, and the last one wins. No need to disallow imo.
>>
> This expression should be a typo, since the first expression neither constrains the following expression nor actually generates the bits. Maybe a warning is enough.

Well, perhaps. But then also for e.g. {vex}{evex} or {load}{store}. Yet
another patch, if so desired, but nothing to deal with right here.

Jan
diff mbox series

Patch

--- a/gas/config/tc-i386.c
+++ b/gas/config/tc-i386.c
@@ -5899,16 +5899,32 @@  parse_insn (const char *line, char *mnem
 	 Check if we should swap operand or force 32bit displacement in
 	 encoding.  */
       if (mnem_p - 2 == dot_p && dot_p[1] == 's')
-	i.dir_encoding = dir_encoding_swap;
+	{
+	  if (i.dir_encoding == dir_encoding_default)
+	    i.dir_encoding = dir_encoding_swap;
+	  else
+	    as_warn (_("ignoring `.s' suffix due to earlier `{%s}'"),
+		     i.dir_encoding == dir_encoding_load ? "load" : "store");
+	}
       else if (mnem_p - 3 == dot_p
 	       && dot_p[1] == 'd'
 	       && dot_p[2] == '8')
-	i.disp_encoding = disp_encoding_8bit;
+	{
+	  if (i.disp_encoding == disp_encoding_default)
+	    i.disp_encoding = disp_encoding_8bit;
+	  else if (i.disp_encoding != disp_encoding_8bit)
+	    as_warn (_("ignoring `.d8' suffix due to earlier `{disp<N>}'"));
+	}
       else if (mnem_p - 4 == dot_p
 	       && dot_p[1] == 'd'
 	       && dot_p[2] == '3'
 	       && dot_p[3] == '2')
-	i.disp_encoding = disp_encoding_32bit;
+	{
+	  if (i.disp_encoding == disp_encoding_default)
+	    i.disp_encoding = disp_encoding_32bit;
+	  else if (i.disp_encoding != disp_encoding_32bit)
+	    as_warn (_("ignoring `.d32' suffix due to earlier `{disp<N>}'"));
+	}
       else
 	goto check_suffix;
       mnem_p = dot_p;