diff mbox series

gas: x86: ginsn: adjust ginsns for certain lea ops

Message ID 20240123093855.3617792-1-indu.bhagat@oracle.com
State Unresolved
Headers
Series gas: x86: ginsn: adjust ginsns for certain lea ops |

Checks

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

Commit Message

Indu Bhagat Jan. 23, 2024, 9:38 a.m. UTC 
  A review comment on the SCFI V4 series was to handle ginsn creation for
certain lea opcodes more precisely.

Specifically, we should preferably handle the following two cases of lea
opcodes similarly:
  - #1 lea with "index register and scale factor of 1, but no base
    register",
  - #2 lea with "no index register, but base register present".
Currently, a ginsn of type GINSN_TYPE_OTHER is generated for the
case of #1 above.  For #2, however, the lea insn is translated to either
a GINSN_TYPE_ADD or GINSN_TYPE_MOV depending on whether the immediate
for displacement is non-zero or not respectively.

Change the handling in x86_ginsn_lea so that both of the above lea
manifestations are handled similarly.

gas/
	* gas/config/tc-i386.c (x86_ginsn_lea): Handle selected lea with
	no base register similar to the case of no index register.

gas/testsuite/
	* gas/scfi/x86_64/ginsn-lea-1.l: New test.
	* gas/scfi/x86_64/ginsn-lea-1.s: Likewise.
	* gas/scfi/x86_64/scfi-x86-64.exp: Add new test.
---
 gas/config/tc-i386.c                          | 88 ++++++++++---------
 gas/testsuite/gas/scfi/x86_64/ginsn-lea-1.l   | 32 +++++++
 gas/testsuite/gas/scfi/x86_64/ginsn-lea-1.s   | 13 +++
 gas/testsuite/gas/scfi/x86_64/scfi-x86-64.exp |  1 +
 4 files changed, 93 insertions(+), 41 deletions(-)
 create mode 100644 gas/testsuite/gas/scfi/x86_64/ginsn-lea-1.l
 create mode 100644 gas/testsuite/gas/scfi/x86_64/ginsn-lea-1.s

Comments

Jan Beulich Jan. 23, 2024, 10:15 a.m. UTC | #1 
On 23.01.2024 10:38, Indu Bhagat wrote:
> @@ -5678,47 +5680,7 @@ x86_ginsn_lea (const symbolS *insn_end_sym)
>  			     GINSN_SRC_IMM, 0xf /* arbitrary const.  */, 0,
>  			     GINSN_DST_REG, dst_reg, 0);
>      }
> -  else if (i.base_reg && !i.index_reg)
> -    {
> -      /* lea    -0x2(%base),%dst.  */
> -      base_reg = ginsn_dw2_regnum (i.base_reg);
> -      dst_reg = ginsn_dw2_regnum (i.op[1].regs);
> -
> -      if (i.disp_operands)
> -	src_disp = i.op[0].disps->X_add_number;
> -
> -      if (src_disp)
> -	/* Generate an ADD ginsn.  */
> -	ginsn = ginsn_new_add (insn_end_sym, true,
> -			       GINSN_SRC_REG, base_reg, 0,
> -			       GINSN_SRC_IMM, 0, src_disp,
> -			       GINSN_DST_REG, dst_reg, 0);
> -      else
> -	/* Generate a MOV ginsn.  */
> -	ginsn = ginsn_new_mov (insn_end_sym, true,
> -			       GINSN_SRC_REG, base_reg, 0,
> -			       GINSN_DST_REG, dst_reg, 0);
> -    }
> -  else if (!i.base_reg && i.index_reg)
> -    {
> -      /* lea (,%index,imm), %dst.  */
> -      /* TBD_GINSN_INFO_LOSS - There is no explicit ginsn multiply operation,
> -	 instead use GINSN_TYPE_OTHER.  Also, note that info about displacement
> -	 is not carried forward either.  But this is fine because
> -	 GINSN_TYPE_OTHER will cause SCFI pass to bail out any which way if
> -	 dest reg is interesting.  */
> -      index_scale = i.log2_scale_factor;
> -      index_reg = ginsn_dw2_regnum (i.index_reg);
> -      dst_reg = ginsn_dw2_regnum (i.op[1].regs);
> -      ginsn = ginsn_new_other (insn_end_sym, true,
> -			       GINSN_SRC_REG, index_reg,
> -			       GINSN_SRC_IMM, index_scale,
> -			       GINSN_DST_REG, dst_reg);
> -      /* FIXME - It seems to make sense to represent a scale factor of 1
> -	 correctly here (i.e. not as "other", but rather similar to the
> -	 base-without- index case above)?  */
> -    }
> -  else
> +  else if (i.index_reg && i.base_reg)
>      {
>        /* lea disp(%base,%index,imm) %dst.  */
>        /* TBD_GINSN_INFO_LOSS - Skip adding information about the disp and imm
> @@ -5732,6 +5694,50 @@ x86_ginsn_lea (const symbolS *insn_end_sym)
>  			       GINSN_SRC_REG, index_reg,
>  			       GINSN_DST_REG, dst_reg);
>      }
> +  else
> +    {
> +      /* lea disp(%base) %dst    or    lea disp(,%index,imm) %dst.  */
> +      gas_assert ((i.base_reg && !i.index_reg)
> +		  || (!i.base_reg && i.index_reg));
> +
> +      index_scale = i.log2_scale_factor;
> +      src1 = (i.base_reg) ? i.base_reg : i.index_reg;
> +      src1_reg = ginsn_dw2_regnum (src1);
> +      dst_reg = ginsn_dw2_regnum (i.op[1].regs);
> +      /* It makes sense to represent a scale factor of 1 correctly here
> +	 (i.e., not using GINSN_TYPE_OTHER, but rather similar to the
> +	 base-without-index case).  */
> +      if (!index_scale)
> +	{
> +	  if (i.disp_operands)
> +	    src_disp = i.op[0].disps->X_add_number;
> +
> +	  if (src_disp)
> +	    /* Generate an ADD ginsn.  */
> +	    ginsn = ginsn_new_add (insn_end_sym, true,
> +				   GINSN_SRC_REG, src1_reg, 0,
> +				   GINSN_SRC_IMM, 0, src_disp,
> +				   GINSN_DST_REG, dst_reg, 0);
> +	  else
> +	    /* Generate a MOV ginsn.  */
> +	    ginsn = ginsn_new_mov (insn_end_sym, true,
> +				   GINSN_SRC_REG, src1_reg, 0,
> +				   GINSN_DST_REG, dst_reg, 0);

You're still losing symbol information if "disp" involves one. Perhaps
worth a comment.

> +	}
> +      /* TBD_GINSN_INFO_LOSS - There is no explicit ginsn multiply operation,
> +	 instead use GINSN_TYPE_OTHER.  Also, note that info about displacement
> +	 is not carried forward either.  But this is fine because
> +	 GINSN_TYPE_OTHER will cause SCFI pass to bail out any which way if
> +	 dest reg is interesting.  */

This comment would better move ...

> +      else
> +	{

... here. I also have to admit that I have trouble parsing the last sentence.

> +	  gas_assert (i.index_reg);
> +	  ginsn = ginsn_new_other (insn_end_sym, true,
> +				   GINSN_SRC_REG, src1_reg,
> +				   GINSN_SRC_IMM, index_scale,
> +				   GINSN_DST_REG, dst_reg);
> +	}
> +    }
>  
>    ginsn_set_where (ginsn);
>  

Since overall this is an improvement: Okay with the comment adjustments.

Jan
Indu Bhagat Jan. 24, 2024, 6:22 a.m. UTC | #2 
On 1/23/24 02:15, Jan Beulich wrote:
> On 23.01.2024 10:38, Indu Bhagat wrote:
>> @@ -5678,47 +5680,7 @@ x86_ginsn_lea (const symbolS *insn_end_sym)
>>   			     GINSN_SRC_IMM, 0xf /* arbitrary const.  */, 0,
>>   			     GINSN_DST_REG, dst_reg, 0);
>>       }
>> -  else if (i.base_reg && !i.index_reg)
>> -    {
>> -      /* lea    -0x2(%base),%dst.  */
>> -      base_reg = ginsn_dw2_regnum (i.base_reg);
>> -      dst_reg = ginsn_dw2_regnum (i.op[1].regs);
>> -
>> -      if (i.disp_operands)
>> -	src_disp = i.op[0].disps->X_add_number;
>> -
>> -      if (src_disp)
>> -	/* Generate an ADD ginsn.  */
>> -	ginsn = ginsn_new_add (insn_end_sym, true,
>> -			       GINSN_SRC_REG, base_reg, 0,
>> -			       GINSN_SRC_IMM, 0, src_disp,
>> -			       GINSN_DST_REG, dst_reg, 0);
>> -      else
>> -	/* Generate a MOV ginsn.  */
>> -	ginsn = ginsn_new_mov (insn_end_sym, true,
>> -			       GINSN_SRC_REG, base_reg, 0,
>> -			       GINSN_DST_REG, dst_reg, 0);
>> -    }
>> -  else if (!i.base_reg && i.index_reg)
>> -    {
>> -      /* lea (,%index,imm), %dst.  */
>> -      /* TBD_GINSN_INFO_LOSS - There is no explicit ginsn multiply operation,
>> -	 instead use GINSN_TYPE_OTHER.  Also, note that info about displacement
>> -	 is not carried forward either.  But this is fine because
>> -	 GINSN_TYPE_OTHER will cause SCFI pass to bail out any which way if
>> -	 dest reg is interesting.  */
>> -      index_scale = i.log2_scale_factor;
>> -      index_reg = ginsn_dw2_regnum (i.index_reg);
>> -      dst_reg = ginsn_dw2_regnum (i.op[1].regs);
>> -      ginsn = ginsn_new_other (insn_end_sym, true,
>> -			       GINSN_SRC_REG, index_reg,
>> -			       GINSN_SRC_IMM, index_scale,
>> -			       GINSN_DST_REG, dst_reg);
>> -      /* FIXME - It seems to make sense to represent a scale factor of 1
>> -	 correctly here (i.e. not as "other", but rather similar to the
>> -	 base-without- index case above)?  */
>> -    }
>> -  else
>> +  else if (i.index_reg && i.base_reg)
>>       {
>>         /* lea disp(%base,%index,imm) %dst.  */
>>         /* TBD_GINSN_INFO_LOSS - Skip adding information about the disp and imm
>> @@ -5732,6 +5694,50 @@ x86_ginsn_lea (const symbolS *insn_end_sym)
>>   			       GINSN_SRC_REG, index_reg,
>>   			       GINSN_DST_REG, dst_reg);
>>       }
>> +  else
>> +    {
>> +      /* lea disp(%base) %dst    or    lea disp(,%index,imm) %dst.  */
>> +      gas_assert ((i.base_reg && !i.index_reg)
>> +		  || (!i.base_reg && i.index_reg));
>> +
>> +      index_scale = i.log2_scale_factor;
>> +      src1 = (i.base_reg) ? i.base_reg : i.index_reg;
>> +      src1_reg = ginsn_dw2_regnum (src1);
>> +      dst_reg = ginsn_dw2_regnum (i.op[1].regs);
>> +      /* It makes sense to represent a scale factor of 1 correctly here
>> +	 (i.e., not using GINSN_TYPE_OTHER, but rather similar to the
>> +	 base-without-index case).  */
>> +      if (!index_scale)
>> +	{
>> +	  if (i.disp_operands)
>> +	    src_disp = i.op[0].disps->X_add_number;
>> +
>> +	  if (src_disp)
>> +	    /* Generate an ADD ginsn.  */
>> +	    ginsn = ginsn_new_add (insn_end_sym, true,
>> +				   GINSN_SRC_REG, src1_reg, 0,
>> +				   GINSN_SRC_IMM, 0, src_disp,
>> +				   GINSN_DST_REG, dst_reg, 0);
>> +	  else
>> +	    /* Generate a MOV ginsn.  */
>> +	    ginsn = ginsn_new_mov (insn_end_sym, true,
>> +				   GINSN_SRC_REG, src1_reg, 0,
>> +				   GINSN_DST_REG, dst_reg, 0);
> 
> You're still losing symbol information if "disp" involves one. Perhaps
> worth a comment.
> 
>> +	}
>> +      /* TBD_GINSN_INFO_LOSS - There is no explicit ginsn multiply operation,
>> +	 instead use GINSN_TYPE_OTHER.  Also, note that info about displacement
>> +	 is not carried forward either.  But this is fine because
>> +	 GINSN_TYPE_OTHER will cause SCFI pass to bail out any which way if
>> +	 dest reg is interesting.  */
> 
> This comment would better move ...
> 
>> +      else
>> +	{
> 
> ... here. I also have to admit that I have trouble parsing the last sentence.
> 

When trying to reword this, I realized that it is not correct to 
translate lea %symbol, %rN to a 'GINSN_TYPE_MOV fake_const %rN' (when 
the destination is REG_SP/REG_FP a fake_const may then appear in the 
synthesized CFI).

Fixing that to generate GINSN_TYPE_OTHER then instead of GINSN_TYPE_MOV, 
led me to conclude that its time I take another of your review feedback: 
do not create ops now, only to say they are unnecessary later.

I have now removed code paths creating GINSN_TYPE_OTHER from this 
function.  We will let x86_ginsn_unhandled () code path create 
GINSN_TYPE_OTHER if necessary.

Sending V2 shortly.

Thanks

>> +	  gas_assert (i.index_reg);
>> +	  ginsn = ginsn_new_other (insn_end_sym, true,
>> +				   GINSN_SRC_REG, src1_reg,
>> +				   GINSN_SRC_IMM, index_scale,
>> +				   GINSN_DST_REG, dst_reg);
>> +	}
>> +    }
>>   
>>     ginsn_set_where (ginsn);
>>   
> 
> Since overall this is an improvement: Okay with the comment adjustments.
>
diff mbox series

Patch

diff --git a/gas/config/tc-i386.c b/gas/config/tc-i386.c
index 51166ef3f02..7810e1de40d 100644
--- a/gas/config/tc-i386.c
+++ b/gas/config/tc-i386.c
@@ -5666,6 +5666,8 @@  x86_ginsn_lea (const symbolS *insn_end_sym)
   ginsnS *ginsn = NULL;
   unsigned int base_reg;
   unsigned int index_reg;
+  unsigned int src1_reg;
+  const reg_entry *src1;
   offsetT index_scale;
   unsigned int dst_reg;
 
@@ -5678,47 +5680,7 @@  x86_ginsn_lea (const symbolS *insn_end_sym)
 			     GINSN_SRC_IMM, 0xf /* arbitrary const.  */, 0,
 			     GINSN_DST_REG, dst_reg, 0);
     }
-  else if (i.base_reg && !i.index_reg)
-    {
-      /* lea    -0x2(%base),%dst.  */
-      base_reg = ginsn_dw2_regnum (i.base_reg);
-      dst_reg = ginsn_dw2_regnum (i.op[1].regs);
-
-      if (i.disp_operands)
-	src_disp = i.op[0].disps->X_add_number;
-
-      if (src_disp)
-	/* Generate an ADD ginsn.  */
-	ginsn = ginsn_new_add (insn_end_sym, true,
-			       GINSN_SRC_REG, base_reg, 0,
-			       GINSN_SRC_IMM, 0, src_disp,
-			       GINSN_DST_REG, dst_reg, 0);
-      else
-	/* Generate a MOV ginsn.  */
-	ginsn = ginsn_new_mov (insn_end_sym, true,
-			       GINSN_SRC_REG, base_reg, 0,
-			       GINSN_DST_REG, dst_reg, 0);
-    }
-  else if (!i.base_reg && i.index_reg)
-    {
-      /* lea (,%index,imm), %dst.  */
-      /* TBD_GINSN_INFO_LOSS - There is no explicit ginsn multiply operation,
-	 instead use GINSN_TYPE_OTHER.  Also, note that info about displacement
-	 is not carried forward either.  But this is fine because
-	 GINSN_TYPE_OTHER will cause SCFI pass to bail out any which way if
-	 dest reg is interesting.  */
-      index_scale = i.log2_scale_factor;
-      index_reg = ginsn_dw2_regnum (i.index_reg);
-      dst_reg = ginsn_dw2_regnum (i.op[1].regs);
-      ginsn = ginsn_new_other (insn_end_sym, true,
-			       GINSN_SRC_REG, index_reg,
-			       GINSN_SRC_IMM, index_scale,
-			       GINSN_DST_REG, dst_reg);
-      /* FIXME - It seems to make sense to represent a scale factor of 1
-	 correctly here (i.e. not as "other", but rather similar to the
-	 base-without- index case above)?  */
-    }
-  else
+  else if (i.index_reg && i.base_reg)
     {
       /* lea disp(%base,%index,imm) %dst.  */
       /* TBD_GINSN_INFO_LOSS - Skip adding information about the disp and imm
@@ -5732,6 +5694,50 @@  x86_ginsn_lea (const symbolS *insn_end_sym)
 			       GINSN_SRC_REG, index_reg,
 			       GINSN_DST_REG, dst_reg);
     }
+  else
+    {
+      /* lea disp(%base) %dst    or    lea disp(,%index,imm) %dst.  */
+      gas_assert ((i.base_reg && !i.index_reg)
+		  || (!i.base_reg && i.index_reg));
+
+      index_scale = i.log2_scale_factor;
+      src1 = (i.base_reg) ? i.base_reg : i.index_reg;
+      src1_reg = ginsn_dw2_regnum (src1);
+      dst_reg = ginsn_dw2_regnum (i.op[1].regs);
+      /* It makes sense to represent a scale factor of 1 correctly here
+	 (i.e., not using GINSN_TYPE_OTHER, but rather similar to the
+	 base-without-index case).  */
+      if (!index_scale)
+	{
+	  if (i.disp_operands)
+	    src_disp = i.op[0].disps->X_add_number;
+
+	  if (src_disp)
+	    /* Generate an ADD ginsn.  */
+	    ginsn = ginsn_new_add (insn_end_sym, true,
+				   GINSN_SRC_REG, src1_reg, 0,
+				   GINSN_SRC_IMM, 0, src_disp,
+				   GINSN_DST_REG, dst_reg, 0);
+	  else
+	    /* Generate a MOV ginsn.  */
+	    ginsn = ginsn_new_mov (insn_end_sym, true,
+				   GINSN_SRC_REG, src1_reg, 0,
+				   GINSN_DST_REG, dst_reg, 0);
+	}
+      /* TBD_GINSN_INFO_LOSS - There is no explicit ginsn multiply operation,
+	 instead use GINSN_TYPE_OTHER.  Also, note that info about displacement
+	 is not carried forward either.  But this is fine because
+	 GINSN_TYPE_OTHER will cause SCFI pass to bail out any which way if
+	 dest reg is interesting.  */
+      else
+	{
+	  gas_assert (i.index_reg);
+	  ginsn = ginsn_new_other (insn_end_sym, true,
+				   GINSN_SRC_REG, src1_reg,
+				   GINSN_SRC_IMM, index_scale,
+				   GINSN_DST_REG, dst_reg);
+	}
+    }
 
   ginsn_set_where (ginsn);
 
diff --git a/gas/testsuite/gas/scfi/x86_64/ginsn-lea-1.l b/gas/testsuite/gas/scfi/x86_64/ginsn-lea-1.l
new file mode 100644
index 00000000000..f4629f37fe1
--- /dev/null
+++ b/gas/testsuite/gas/scfi/x86_64/ginsn-lea-1.l
@@ -0,0 +1,32 @@ 
+GAS LISTING .*
+
+
+   1              	## Testcase with a variety of lea.
+   2              		.text
+   3              		.globl  foo
+   4              		.type   foo, @function
+   4              	ginsn: SYM FUNC_BEGIN
+   5              	foo:
+   5              	ginsn: SYM foo
+   6 0000 488D05FE 		lea  -0x2\(%rip\), %rax
+   6      FFFFFF
+   6              	ginsn: ADD %r4, -2, %r0
+   7 0007 678D4C18 		lea  -0x1\(%eax,%ebx\), %ecx
+   7      FF
+   7              	ginsn: OTH %r0, %r3, %r2
+   8 000c 678D4C58 		lea  0x55\(%eax,%ebx,2\), %ecx
+   8      55
+   8              	ginsn: OTH %r0, %r3, %r2
+   9 0011 678D0C1D 		lea  -0x3\(,%ebx,1\), %ecx
+   9      FDFFFFFF 
+   9              	ginsn: ADD %r3, -3, %r2
+  10 0019 678D0C1D 		lea  -0x3\(,%ebx,\), %ecx
+  10      FDFFFFFF 
+  10              	ginsn: ADD %r3, -3, %r2
+  11 0021 678D0C5D 		lea  -0x3\(,%ebx,2\), %ecx
+  11      FDFFFFFF 
+  11              	ginsn: OTH %r3, 1, %r2
+  12              	.LFE0:
+  12              	ginsn: SYM .LFE0
+  13              		.size   foo, .-foo
+  13              	ginsn: SYM FUNC_END
diff --git a/gas/testsuite/gas/scfi/x86_64/ginsn-lea-1.s b/gas/testsuite/gas/scfi/x86_64/ginsn-lea-1.s
new file mode 100644
index 00000000000..1861d069f39
--- /dev/null
+++ b/gas/testsuite/gas/scfi/x86_64/ginsn-lea-1.s
@@ -0,0 +1,13 @@ 
+## Testcase with a variety of lea.
+	.text
+	.globl  foo
+	.type   foo, @function
+foo:
+	lea  -0x2(%rip), %rax
+	lea  -0x1(%eax,%ebx), %ecx
+	lea  0x55(%eax,%ebx,2), %ecx
+	lea  -0x3(,%ebx,1), %ecx
+	lea  -0x3(,%ebx,), %ecx
+	lea  -0x3(,%ebx,2), %ecx
+.LFE0:
+	.size   foo, .-foo
diff --git a/gas/testsuite/gas/scfi/x86_64/scfi-x86-64.exp b/gas/testsuite/gas/scfi/x86_64/scfi-x86-64.exp
index 2b291800b65..d32cb290d92 100644
--- a/gas/testsuite/gas/scfi/x86_64/scfi-x86-64.exp
+++ b/gas/testsuite/gas/scfi/x86_64/scfi-x86-64.exp
@@ -26,6 +26,7 @@  if  { ([istarget "x86_64-*-*"] && ![istarget "x86_64-*-linux*-gnux32"]) } then {
 
     run_list_test "ginsn-dw2-regnum-1" "--scfi=experimental -ali"
     run_list_test "ginsn-add-1" "--scfi=experimental -ali"
+    run_list_test "ginsn-lea-1" "--scfi=experimental -ali"
     run_list_test "ginsn-pop-1" "--scfi=experimental -ali"
     run_list_test "ginsn-push-1" "--scfi=experimental -ali"