[4/5] x86-64: special case LEA when determining signedness of displacement

Message ID b87025a8-4628-0c23-cc7b-4328257191a7@suse.com
State New
Headers show
Series
  • x86: assorted relocation handling related adjustment (part I)
Related show

Commit Message

H.J. Lu via Binutils April 22, 2021, 8:40 a.m.
LEA behavior without a 64-bit destination is independent of address size
- in particular LEA with 32-bit addressing and 64-bit destination is the
same as LEA with 64-bit addressing and 32-bit destination. IOW checking
merely i.prefix[ADDR_PREFIX] is insufficient. This also means wrong
relocation types (R_X86_64_32S when R_X86_64_32 is needed) were used so
far in such cases.

Note that in one case in build_modrm_byte() the 64-bit check came too
early altogether, and hence gets dropped in favor of the one included in
the new helper. This is benign to non-64-bit code from all I can tell,
but the failure to clear disp16 could have been a latent problem.

gas/
2021-04-XX  Jan Beulich  <jbeulich@suse.com>

	* config/tc-i386.c (want_disp32): New.
	(md_assemble): Use it.
	(optimize_disp): Likewise.
	(build_modrm_byte): Likewise.

Comments

H.J. Lu via Binutils April 22, 2021, 11:54 a.m. | #1
On Thu, Apr 22, 2021 at 1:40 AM Jan Beulich <jbeulich@suse.com> wrote:
>

> LEA behavior without a 64-bit destination is independent of address size

> - in particular LEA with 32-bit addressing and 64-bit destination is the

> same as LEA with 64-bit addressing and 32-bit destination. IOW checking

> merely i.prefix[ADDR_PREFIX] is insufficient. This also means wrong

> relocation types (R_X86_64_32S when R_X86_64_32 is needed) were used so

> far in such cases.


Do you have a testcase for the wrong relocation type?

> Note that in one case in build_modrm_byte() the 64-bit check came too

> early altogether, and hence gets dropped in favor of the one included in

> the new helper. This is benign to non-64-bit code from all I can tell,

> but the failure to clear disp16 could have been a latent problem.

>

> gas/

> 2021-04-XX  Jan Beulich  <jbeulich@suse.com>

>

>         * config/tc-i386.c (want_disp32): New.

>         (md_assemble): Use it.

>         (optimize_disp): Likewise.

>         (build_modrm_byte): Likewise.

>

> --- a/gas/config/tc-i386.c

> +++ b/gas/config/tc-i386.c

> @@ -3555,6 +3555,16 @@ tc_i386_fix_adjustable (fixS *fixP ATTRI

>    return 1;

>  }

>

> +static INLINE bool

> +want_disp32 (const insn_template *t)

> +{

> +  return flag_code != CODE_64BIT

> +        || i.prefix[ADDR_PREFIX]

> +        || (t->base_opcode == 0x8d

> +            && t->opcode_modifier.opcodespace == SPACE_BASE

> +            && !i.types[1].bitfield.qword);

> +}

> +

>  static int

>  intel_float_operand (const char *mnemonic)

>  {

> @@ -4740,7 +4750,7 @@ md_assemble (char *line)

>    if (i.imm_operands)

>      optimize_imm ();

>

> -  if (i.disp_operands && flag_code == CODE_64BIT && !i.prefix[ADDR_PREFIX])

> +  if (i.disp_operands && !want_disp32 (current_templates->start))

>      {

>        for (j = 0; j < i.operands; ++j)

>         {

> @@ -5748,7 +5758,7 @@ optimize_disp (void)

>  #ifdef BFD64

>             else if (flag_code == CODE_64BIT)

>               {

> -               if (i.prefix[ADDR_PREFIX]

> +               if (want_disp32 (current_templates->start)

>                     && fits_in_unsigned_long (op_disp))

>                   i.types[op].bitfield.disp32 = 1;

>

> @@ -8108,7 +8118,7 @@ build_modrm_byte (void)

>                   i.types[op].bitfield.disp8 = 0;

>                   i.types[op].bitfield.disp16 = 0;

>                   i.types[op].bitfield.disp64 = 0;

> -                 if (flag_code != CODE_64BIT || i.prefix[ADDR_PREFIX])

> +                 if (want_disp32 (&i.tm))

>                     {

>                       /* Must be 32 bit */

>                       i.types[op].bitfield.disp32 = 1;

> @@ -8159,7 +8169,7 @@ build_modrm_byte (void)

>                       i.rm.regmem = ESCAPE_TO_TWO_BYTE_ADDRESSING;

>                       i.sib.base = NO_BASE_REGISTER;

>                       i.sib.index = NO_INDEX_REGISTER;

> -                     newdisp = (!i.prefix[ADDR_PREFIX] ? disp32s : disp32);

> +                     newdisp = (want_disp32(&i.tm) ? disp32 : disp32s);

>                     }

>                   else if ((flag_code == CODE_16BIT)

>                            ^ (i.prefix[ADDR_PREFIX] != 0))

> @@ -8188,7 +8198,7 @@ build_modrm_byte (void)

>                   i.types[op].bitfield.disp8 = 0;

>                   i.types[op].bitfield.disp16 = 0;

>                   i.types[op].bitfield.disp64 = 0;

> -                 if (flag_code != CODE_64BIT || i.prefix[ADDR_PREFIX])

> +                 if (want_disp32 (&i.tm))

>                     {

>                       /* Must be 32 bit */

>                       i.types[op].bitfield.disp32 = 1;

> @@ -8263,12 +8273,11 @@ build_modrm_byte (void)

>             }

>           else /* i.base_reg and 32/64 bit mode  */

>             {

> -             if (flag_code == CODE_64BIT

> -                 && operand_type_check (i.types[op], disp))

> +             if (operand_type_check (i.types[op], disp))

>                 {

>                   i.types[op].bitfield.disp16 = 0;

>                   i.types[op].bitfield.disp64 = 0;

> -                 if (i.prefix[ADDR_PREFIX] == 0)

> +                 if (!want_disp32 (&i.tm))

>                     {

>                       i.types[op].bitfield.disp32 = 0;

>                       i.types[op].bitfield.disp32s = 1;

>



-- 
H.J.
H.J. Lu via Binutils April 22, 2021, 12:07 p.m. | #2
On 22.04.2021 13:54, H.J. Lu wrote:
> On Thu, Apr 22, 2021 at 1:40 AM Jan Beulich <jbeulich@suse.com> wrote:

>>

>> LEA behavior without a 64-bit destination is independent of address size

>> - in particular LEA with 32-bit addressing and 64-bit destination is the

>> same as LEA with 64-bit addressing and 32-bit destination. IOW checking

>> merely i.prefix[ADDR_PREFIX] is insufficient. This also means wrong

>> relocation types (R_X86_64_32S when R_X86_64_32 is needed) were used so

>> far in such cases.

> 

> Do you have a testcase for the wrong relocation type?


That'll be covered by a later change (with a more extensive test case).

Jan

>> Note that in one case in build_modrm_byte() the 64-bit check came too

>> early altogether, and hence gets dropped in favor of the one included in

>> the new helper. This is benign to non-64-bit code from all I can tell,

>> but the failure to clear disp16 could have been a latent problem.

>>

>> gas/

>> 2021-04-XX  Jan Beulich  <jbeulich@suse.com>

>>

>>         * config/tc-i386.c (want_disp32): New.

>>         (md_assemble): Use it.

>>         (optimize_disp): Likewise.

>>         (build_modrm_byte): Likewise.

>>

>> --- a/gas/config/tc-i386.c

>> +++ b/gas/config/tc-i386.c

>> @@ -3555,6 +3555,16 @@ tc_i386_fix_adjustable (fixS *fixP ATTRI

>>    return 1;

>>  }

>>

>> +static INLINE bool

>> +want_disp32 (const insn_template *t)

>> +{

>> +  return flag_code != CODE_64BIT

>> +        || i.prefix[ADDR_PREFIX]

>> +        || (t->base_opcode == 0x8d

>> +            && t->opcode_modifier.opcodespace == SPACE_BASE

>> +            && !i.types[1].bitfield.qword);

>> +}

>> +

>>  static int

>>  intel_float_operand (const char *mnemonic)

>>  {

>> @@ -4740,7 +4750,7 @@ md_assemble (char *line)

>>    if (i.imm_operands)

>>      optimize_imm ();

>>

>> -  if (i.disp_operands && flag_code == CODE_64BIT && !i.prefix[ADDR_PREFIX])

>> +  if (i.disp_operands && !want_disp32 (current_templates->start))

>>      {

>>        for (j = 0; j < i.operands; ++j)

>>         {

>> @@ -5748,7 +5758,7 @@ optimize_disp (void)

>>  #ifdef BFD64

>>             else if (flag_code == CODE_64BIT)

>>               {

>> -               if (i.prefix[ADDR_PREFIX]

>> +               if (want_disp32 (current_templates->start)

>>                     && fits_in_unsigned_long (op_disp))

>>                   i.types[op].bitfield.disp32 = 1;

>>

>> @@ -8108,7 +8118,7 @@ build_modrm_byte (void)

>>                   i.types[op].bitfield.disp8 = 0;

>>                   i.types[op].bitfield.disp16 = 0;

>>                   i.types[op].bitfield.disp64 = 0;

>> -                 if (flag_code != CODE_64BIT || i.prefix[ADDR_PREFIX])

>> +                 if (want_disp32 (&i.tm))

>>                     {

>>                       /* Must be 32 bit */

>>                       i.types[op].bitfield.disp32 = 1;

>> @@ -8159,7 +8169,7 @@ build_modrm_byte (void)

>>                       i.rm.regmem = ESCAPE_TO_TWO_BYTE_ADDRESSING;

>>                       i.sib.base = NO_BASE_REGISTER;

>>                       i.sib.index = NO_INDEX_REGISTER;

>> -                     newdisp = (!i.prefix[ADDR_PREFIX] ? disp32s : disp32);

>> +                     newdisp = (want_disp32(&i.tm) ? disp32 : disp32s);

>>                     }

>>                   else if ((flag_code == CODE_16BIT)

>>                            ^ (i.prefix[ADDR_PREFIX] != 0))

>> @@ -8188,7 +8198,7 @@ build_modrm_byte (void)

>>                   i.types[op].bitfield.disp8 = 0;

>>                   i.types[op].bitfield.disp16 = 0;

>>                   i.types[op].bitfield.disp64 = 0;

>> -                 if (flag_code != CODE_64BIT || i.prefix[ADDR_PREFIX])

>> +                 if (want_disp32 (&i.tm))

>>                     {

>>                       /* Must be 32 bit */

>>                       i.types[op].bitfield.disp32 = 1;

>> @@ -8263,12 +8273,11 @@ build_modrm_byte (void)

>>             }

>>           else /* i.base_reg and 32/64 bit mode  */

>>             {

>> -             if (flag_code == CODE_64BIT

>> -                 && operand_type_check (i.types[op], disp))

>> +             if (operand_type_check (i.types[op], disp))

>>                 {

>>                   i.types[op].bitfield.disp16 = 0;

>>                   i.types[op].bitfield.disp64 = 0;

>> -                 if (i.prefix[ADDR_PREFIX] == 0)

>> +                 if (!want_disp32 (&i.tm))

>>                     {

>>                       i.types[op].bitfield.disp32 = 0;

>>                       i.types[op].bitfield.disp32s = 1;

>>

> 

>
H.J. Lu via Binutils April 22, 2021, 1:13 p.m. | #3
On Thu, Apr 22, 2021 at 5:07 AM Jan Beulich <jbeulich@suse.com> wrote:
>

> On 22.04.2021 13:54, H.J. Lu wrote:

> > On Thu, Apr 22, 2021 at 1:40 AM Jan Beulich <jbeulich@suse.com> wrote:

> >>

> >> LEA behavior without a 64-bit destination is independent of address size

> >> - in particular LEA with 32-bit addressing and 64-bit destination is the

> >> same as LEA with 64-bit addressing and 32-bit destination. IOW checking

> >> merely i.prefix[ADDR_PREFIX] is insufficient. This also means wrong

> >> relocation types (R_X86_64_32S when R_X86_64_32 is needed) were used so

> >> far in such cases.

> >

> > Do you have a testcase for the wrong relocation type?

>

> That'll be covered by a later change (with a more extensive test case).


OK.

Thanks.

> Jan

>

> >> Note that in one case in build_modrm_byte() the 64-bit check came too

> >> early altogether, and hence gets dropped in favor of the one included in

> >> the new helper. This is benign to non-64-bit code from all I can tell,

> >> but the failure to clear disp16 could have been a latent problem.

> >>

> >> gas/

> >> 2021-04-XX  Jan Beulich  <jbeulich@suse.com>

> >>

> >>         * config/tc-i386.c (want_disp32): New.

> >>         (md_assemble): Use it.

> >>         (optimize_disp): Likewise.

> >>         (build_modrm_byte): Likewise.

> >>

> >> --- a/gas/config/tc-i386.c

> >> +++ b/gas/config/tc-i386.c

> >> @@ -3555,6 +3555,16 @@ tc_i386_fix_adjustable (fixS *fixP ATTRI

> >>    return 1;

> >>  }

> >>

> >> +static INLINE bool

> >> +want_disp32 (const insn_template *t)

> >> +{

> >> +  return flag_code != CODE_64BIT

> >> +        || i.prefix[ADDR_PREFIX]

> >> +        || (t->base_opcode == 0x8d

> >> +            && t->opcode_modifier.opcodespace == SPACE_BASE

> >> +            && !i.types[1].bitfield.qword);

> >> +}

> >> +

> >>  static int

> >>  intel_float_operand (const char *mnemonic)

> >>  {

> >> @@ -4740,7 +4750,7 @@ md_assemble (char *line)

> >>    if (i.imm_operands)

> >>      optimize_imm ();

> >>

> >> -  if (i.disp_operands && flag_code == CODE_64BIT && !i.prefix[ADDR_PREFIX])

> >> +  if (i.disp_operands && !want_disp32 (current_templates->start))

> >>      {

> >>        for (j = 0; j < i.operands; ++j)

> >>         {

> >> @@ -5748,7 +5758,7 @@ optimize_disp (void)

> >>  #ifdef BFD64

> >>             else if (flag_code == CODE_64BIT)

> >>               {

> >> -               if (i.prefix[ADDR_PREFIX]

> >> +               if (want_disp32 (current_templates->start)

> >>                     && fits_in_unsigned_long (op_disp))

> >>                   i.types[op].bitfield.disp32 = 1;

> >>

> >> @@ -8108,7 +8118,7 @@ build_modrm_byte (void)

> >>                   i.types[op].bitfield.disp8 = 0;

> >>                   i.types[op].bitfield.disp16 = 0;

> >>                   i.types[op].bitfield.disp64 = 0;

> >> -                 if (flag_code != CODE_64BIT || i.prefix[ADDR_PREFIX])

> >> +                 if (want_disp32 (&i.tm))

> >>                     {

> >>                       /* Must be 32 bit */

> >>                       i.types[op].bitfield.disp32 = 1;

> >> @@ -8159,7 +8169,7 @@ build_modrm_byte (void)

> >>                       i.rm.regmem = ESCAPE_TO_TWO_BYTE_ADDRESSING;

> >>                       i.sib.base = NO_BASE_REGISTER;

> >>                       i.sib.index = NO_INDEX_REGISTER;

> >> -                     newdisp = (!i.prefix[ADDR_PREFIX] ? disp32s : disp32);

> >> +                     newdisp = (want_disp32(&i.tm) ? disp32 : disp32s);

> >>                     }

> >>                   else if ((flag_code == CODE_16BIT)

> >>                            ^ (i.prefix[ADDR_PREFIX] != 0))

> >> @@ -8188,7 +8198,7 @@ build_modrm_byte (void)

> >>                   i.types[op].bitfield.disp8 = 0;

> >>                   i.types[op].bitfield.disp16 = 0;

> >>                   i.types[op].bitfield.disp64 = 0;

> >> -                 if (flag_code != CODE_64BIT || i.prefix[ADDR_PREFIX])

> >> +                 if (want_disp32 (&i.tm))

> >>                     {

> >>                       /* Must be 32 bit */

> >>                       i.types[op].bitfield.disp32 = 1;

> >> @@ -8263,12 +8273,11 @@ build_modrm_byte (void)

> >>             }

> >>           else /* i.base_reg and 32/64 bit mode  */

> >>             {

> >> -             if (flag_code == CODE_64BIT

> >> -                 && operand_type_check (i.types[op], disp))

> >> +             if (operand_type_check (i.types[op], disp))

> >>                 {

> >>                   i.types[op].bitfield.disp16 = 0;

> >>                   i.types[op].bitfield.disp64 = 0;

> >> -                 if (i.prefix[ADDR_PREFIX] == 0)

> >> +                 if (!want_disp32 (&i.tm))

> >>                     {

> >>                       i.types[op].bitfield.disp32 = 0;

> >>                       i.types[op].bitfield.disp32s = 1;

> >>

> >

> >

>



-- 
H.J.

Patch

--- a/gas/config/tc-i386.c
+++ b/gas/config/tc-i386.c
@@ -3555,6 +3555,16 @@  tc_i386_fix_adjustable (fixS *fixP ATTRI
   return 1;
 }
 
+static INLINE bool
+want_disp32 (const insn_template *t)
+{
+  return flag_code != CODE_64BIT
+	 || i.prefix[ADDR_PREFIX]
+	 || (t->base_opcode == 0x8d
+	     && t->opcode_modifier.opcodespace == SPACE_BASE
+	     && !i.types[1].bitfield.qword);
+}
+
 static int
 intel_float_operand (const char *mnemonic)
 {
@@ -4740,7 +4750,7 @@  md_assemble (char *line)
   if (i.imm_operands)
     optimize_imm ();
 
-  if (i.disp_operands && flag_code == CODE_64BIT && !i.prefix[ADDR_PREFIX])
+  if (i.disp_operands && !want_disp32 (current_templates->start))
     {
       for (j = 0; j < i.operands; ++j)
 	{
@@ -5748,7 +5758,7 @@  optimize_disp (void)
 #ifdef BFD64
 	    else if (flag_code == CODE_64BIT)
 	      {
-		if (i.prefix[ADDR_PREFIX]
+		if (want_disp32 (current_templates->start)
 		    && fits_in_unsigned_long (op_disp))
 		  i.types[op].bitfield.disp32 = 1;
 
@@ -8108,7 +8118,7 @@  build_modrm_byte (void)
 		  i.types[op].bitfield.disp8 = 0;
 		  i.types[op].bitfield.disp16 = 0;
 		  i.types[op].bitfield.disp64 = 0;
-		  if (flag_code != CODE_64BIT || i.prefix[ADDR_PREFIX])
+		  if (want_disp32 (&i.tm))
 		    {
 		      /* Must be 32 bit */
 		      i.types[op].bitfield.disp32 = 1;
@@ -8159,7 +8169,7 @@  build_modrm_byte (void)
 		      i.rm.regmem = ESCAPE_TO_TWO_BYTE_ADDRESSING;
 		      i.sib.base = NO_BASE_REGISTER;
 		      i.sib.index = NO_INDEX_REGISTER;
-		      newdisp = (!i.prefix[ADDR_PREFIX] ? disp32s : disp32);
+		      newdisp = (want_disp32(&i.tm) ? disp32 : disp32s);
 		    }
 		  else if ((flag_code == CODE_16BIT)
 			   ^ (i.prefix[ADDR_PREFIX] != 0))
@@ -8188,7 +8198,7 @@  build_modrm_byte (void)
 		  i.types[op].bitfield.disp8 = 0;
 		  i.types[op].bitfield.disp16 = 0;
 		  i.types[op].bitfield.disp64 = 0;
-		  if (flag_code != CODE_64BIT || i.prefix[ADDR_PREFIX])
+		  if (want_disp32 (&i.tm))
 		    {
 		      /* Must be 32 bit */
 		      i.types[op].bitfield.disp32 = 1;
@@ -8263,12 +8273,11 @@  build_modrm_byte (void)
 	    }
 	  else /* i.base_reg and 32/64 bit mode  */
 	    {
-	      if (flag_code == CODE_64BIT
-		  && operand_type_check (i.types[op], disp))
+	      if (operand_type_check (i.types[op], disp))
 		{
 		  i.types[op].bitfield.disp16 = 0;
 		  i.types[op].bitfield.disp64 = 0;
-		  if (i.prefix[ADDR_PREFIX] == 0)
+		  if (!want_disp32 (&i.tm))
 		    {
 		      i.types[op].bitfield.disp32 = 0;
 		      i.types[op].bitfield.disp32s = 1;