[PATCH,V2] gas: x86: ginsn: adjust ginsns for certain lea ops

[PATCH,V2] gas: x86: ginsn: adjust ginsns for certain lea ops

[Indu Bhagat]

[Changes from V1]
  - Remove code paths creating GINSN_TYPE_OTHER explicitly.  Let the
    existing x86_ginsn_unhandled code path deal with this, like it does
    for other ops.
  - Adjust code comments.
  - Updated testcase with few new ops.
[End of changes from V1]

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.

While at it, remove the code paths creating GINSN_TYPE_OTHER altogether
from the function.  It makes sense to piggy back on the
x86_ginsn_unhandled code path to create GINSN_TYPE_OTHER if the
destination register is interesting.  This was also suggested in one of
the previous review rounds;  the other functions already follow that
model, so this keeps functions symmetrical looking.

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

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                          | 112 ++++++++----------
 gas/testsuite/gas/scfi/x86_64/ginsn-lea-1.l   |  37 ++++++
 gas/testsuite/gas/scfi/x86_64/ginsn-lea-1.s   |  15 +++
 gas/testsuite/gas/scfi/x86_64/scfi-x86-64.exp |   1 +
 4 files changed, 104 insertions(+), 61 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

diff --git a/gas/config/tc-i386.c b/gas/config/tc-i386.c
index 51166ef3f02..9c849f89190 100644
--- a/gas/config/tc-i386.c
+++ b/gas/config/tc-i386.c
@@ -5655,8 +5655,10 @@ x86_ginsn_move (const symbolS *insn_end_sym)
 }
 
 /* Generate appropriate ginsn for lea.
-   Sub-cases marked with TBD_GINSN_INFO_LOSS indicate some loss of information
-   in the ginsn.  But these are fine for now for GINSN_GEN_SCFI generation
+
+   Unhandled sub-cases (marked with TBD_GINSN_GEN_NOT_SCFI) also suffer with
+   some loss of information in the final ginsn chosen eventually (type
+   GINSN_TYPE_OTHER).  But this is fine for now for GINSN_GEN_SCFI generation
    mode.  */
 
 static ginsnS *
@@ -5664,76 +5666,64 @@ x86_ginsn_lea (const symbolS *insn_end_sym)
 {
   offsetT src_disp = 0;
   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;
 
   if (!i.index_reg && !i.base_reg)
     {
-      /* lea symbol, %rN.  */
-      dst_reg = ginsn_dw2_regnum (i.op[1].regs);
-      /* TBD_GINSN_INFO_LOSS - Skip encoding information about the symbol.  */
-      ginsn = ginsn_new_mov (insn_end_sym, false,
-			     GINSN_SRC_IMM, 0xf /* arbitrary const.  */, 0,
-			     GINSN_DST_REG, dst_reg, 0);
+      if (i.disp_operands && i.op[0].disps->X_op == O_constant)
+	{
+	  /* lea const, %rN.  */
+	  src_disp = i.op[0].disps->X_add_number;
+	  dst_reg = ginsn_dw2_regnum (i.op[1].regs);
+	  ginsn = ginsn_new_mov (insn_end_sym, false,
+				 GINSN_SRC_IMM, 0, src_disp,
+				 GINSN_DST_REG, dst_reg, 0);
+	  ginsn_set_where (ginsn);
+	}
+      /* Skip handling lea symbol, %rN here.  Deal with it in the
+	 x86_ginsn_unhandled code path.  TBD_GINSN_GEN_NOT_SCFI.  */
     }
-  else if (i.base_reg && !i.index_reg)
+  else if ((i.base_reg && !i.index_reg)
+	   || (!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);
+      /* lea disp(%base) %dst    or    lea disp(,%index,imm) %dst.  */
 
-      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);
+      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);
-      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
-    {
-      /* lea disp(%base,%index,imm) %dst.  */
-      /* TBD_GINSN_INFO_LOSS - Skip adding information about the disp and imm
-	 for index reg.  */
-      base_reg = ginsn_dw2_regnum (i.base_reg);
-      index_reg = ginsn_dw2_regnum (i.index_reg);
-      dst_reg = ginsn_dw2_regnum (i.op[1].regs);
-      /* Generate an GINSN_TYPE_OTHER ginsn.  */
-      ginsn = ginsn_new_other (insn_end_sym, true,
-			       GINSN_SRC_REG, base_reg,
-			       GINSN_SRC_REG, index_reg,
-			       GINSN_DST_REG, dst_reg);
-    }
+      /* It makes sense to represent a scale factor of 1 precisely here
+	 (i.e., not using GINSN_TYPE_OTHER, but rather similar to the
+	 base-without-index case).  Ignore the case when disp has a symbol
+	 instead.  */
+      if (!index_scale
+	  && (!i.disp_operands
+	      || (i.disp_operands && i.op[0].disps->X_op == O_constant)))
+	{
+	  if (i.disp_operands)
+	    src_disp = i.op[0].disps->X_add_number;
 
-  ginsn_set_where (ginsn);
+	  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);
+
+	  ginsn_set_where (ginsn);
+	}
+    }
+  /* Skip handling other cases here, e.g., when (i.index_reg && i.base_reg) is
+     true.  Deal with it in the x86_ginsn_unhandled code path by generating
+     GINSN_TYPE_OTHER when necessary.  TBD_GINSN_GEN_NOT_SCFI.  */
 
   return 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..925153ce58e
--- /dev/null
+++ b/gas/testsuite/gas/scfi/x86_64/ginsn-lea-1.l
@@ -0,0 +1,37 @@
+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 488D2C25 		lea  symbol, %rbp
+   6      00000000 
+   6              	ginsn: OTH 0, 0, %r6
+   7 0008 488D2425 		lea  0x9090, %rsp
+   7      90900000 
+   7              	ginsn: MOV 37008, %r7
+   8 0010 488D05FE 		lea  -0x2\(%rip\), %rax
+   8      FFFFFF
+   8              	ginsn: ADD %r4, -2, %r0
+   9 0017 678D6C18 		lea  -0x1\(%eax,%ebx\), %ebp
+   9      FF
+   9              	ginsn: OTH 0, 0, %r6
+  10 001c 678D6C58 		lea  0x55\(%eax,%ebx,2\), %ebp
+  10      55
+  10              	ginsn: OTH 0, 0, %r6
+  11 0021 678D0C1D 		lea  -0x3\(,%ebx,1\), %ecx
+  11      FDFFFFFF 
+  11              	ginsn: ADD %r3, -3, %r2
+  12 0029 678D0C1D 		lea  -0x3\(,%ebx,\), %ecx
+  12      FDFFFFFF 
+  12              	ginsn: ADD %r3, -3, %r2
+  13 0031 678D0C5D 		lea  -0x3\(,%ebx,2\), %ecx
+  13      FDFFFFFF 
+  14              	.LFE0:
+  14              	ginsn: SYM .LFE0
+  15              		.size   foo, .-foo
+  15              	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..ae8601b302e
--- /dev/null
+++ b/gas/testsuite/gas/scfi/x86_64/ginsn-lea-1.s
@@ -0,0 +1,15 @@
+## Testcase with a variety of lea.
+	.text
+	.globl  foo
+	.type   foo, @function
+foo:
+	lea  symbol, %rbp
+	lea  0x9090, %rsp
+	lea  -0x2(%rip), %rax
+	lea  -0x1(%eax,%ebx), %ebp
+	lea  0x55(%eax,%ebx,2), %ebp
+	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"
 
-- 
2.43.0

Re: [PATCH,V2] gas: x86: ginsn: adjust ginsns for certain lea ops

[Jan Beulich]

On 24.01.2024 07:40, Indu Bhagat wrote:
> @@ -5664,76 +5666,64 @@ x86_ginsn_lea (const symbolS *insn_end_sym)
>  {
>    offsetT src_disp = 0;
>    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;
>  
>    if (!i.index_reg && !i.base_reg)
>      {
> -      /* lea symbol, %rN.  */
> -      dst_reg = ginsn_dw2_regnum (i.op[1].regs);
> -      /* TBD_GINSN_INFO_LOSS - Skip encoding information about the symbol.  */
> -      ginsn = ginsn_new_mov (insn_end_sym, false,
> -			     GINSN_SRC_IMM, 0xf /* arbitrary const.  */, 0,
> -			     GINSN_DST_REG, dst_reg, 0);
> +      if (i.disp_operands && i.op[0].disps->X_op == O_constant)
> +	{
> +	  /* lea const, %rN.  */
> +	  src_disp = i.op[0].disps->X_add_number;
> +	  dst_reg = ginsn_dw2_regnum (i.op[1].regs);
> +	  ginsn = ginsn_new_mov (insn_end_sym, false,
> +				 GINSN_SRC_IMM, 0, src_disp,
> +				 GINSN_DST_REG, dst_reg, 0);
> +	  ginsn_set_where (ginsn);
> +	}

Since earlier on you've been mentioning that you primarily target insn
forms actually in use in existing code, I wonder whether you've ever
seen any use of this. The same is better (shorter) expressed by MOV,
and hence I'd expect people to prefer that form. IOW the question here
is: Is there much value in having this code, rather than simply
penalizing people bogusly using such by having this case end at the
common x86_ginsn_unhandled path as well.

> +      /* Skip handling lea symbol, %rN here.  Deal with it in the
> +	 x86_ginsn_unhandled code path.  TBD_GINSN_GEN_NOT_SCFI.  */
>      }
> -  else if (i.base_reg && !i.index_reg)
> +  else if ((i.base_reg && !i.index_reg)
> +	   || (!i.base_reg && i.index_reg))

I frequently see conditionals like this written this way. Maybe it's
indeed clearer to a majority; personally I'd prefer the shorter

  else if (!i.base_reg != !i.index_reg)

However, considering the earlier if() this is an else-if to, even

  else if (!i.base_reg || !i.index_reg)

would also suffice (but as per above that if() may want to go away).

>      {
> -      /* lea    -0x2(%base),%dst.  */
> -      base_reg = ginsn_dw2_regnum (i.base_reg);
> -      dst_reg = ginsn_dw2_regnum (i.op[1].regs);
> +      /* lea disp(%base) %dst    or    lea disp(,%index,imm) %dst.  */

Would be nice if the missing commas were added here.

> -      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);
> +      src1 = (i.base_reg) ? i.base_reg : i.index_reg;
> +      src1_reg = ginsn_dw2_regnum (src1);

Since I can't spot any other use of src1, why not simply

      src1_reg = ginsn_dw2_regnum (i.base_reg ? i.base_reg : i.index_reg);

? Otherwise at the very least please omit the pointless parentheses in
the conditional expression.

>        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
> -    {
> -      /* lea disp(%base,%index,imm) %dst.  */
> -      /* TBD_GINSN_INFO_LOSS - Skip adding information about the disp and imm
> -	 for index reg.  */
> -      base_reg = ginsn_dw2_regnum (i.base_reg);
> -      index_reg = ginsn_dw2_regnum (i.index_reg);
> -      dst_reg = ginsn_dw2_regnum (i.op[1].regs);
> -      /* Generate an GINSN_TYPE_OTHER ginsn.  */
> -      ginsn = ginsn_new_other (insn_end_sym, true,
> -			       GINSN_SRC_REG, base_reg,
> -			       GINSN_SRC_REG, index_reg,
> -			       GINSN_DST_REG, dst_reg);
> -    }
> +      /* It makes sense to represent a scale factor of 1 precisely here
> +	 (i.e., not using GINSN_TYPE_OTHER, but rather similar to the
> +	 base-without-index case).  Ignore the case when disp has a symbol
> +	 instead.  */
> +      if (!index_scale
> +	  && (!i.disp_operands
> +	      || (i.disp_operands && i.op[0].disps->X_op == O_constant)))

This is functionally identical to the shorter

      if (!index_scale
	  && (!i.disp_operands || i.op[0].disps->X_op == O_constant))

But: What about any of

	lea	(%rax,%riz),%rbp
	lea	(%rax,4),%rbp
	lea	(%rax,%riz,4),%rbp

?

Jan

Re: [PATCH,V2] gas: x86: ginsn: adjust ginsns for certain lea ops

[Indu Bhagat]

On 1/23/24 23:51, Jan Beulich wrote:
> On 24.01.2024 07:40, Indu Bhagat wrote:
>> @@ -5664,76 +5666,64 @@ x86_ginsn_lea (const symbolS *insn_end_sym)
>>   {
>>     offsetT src_disp = 0;
>>     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;
>>   
>>     if (!i.index_reg && !i.base_reg)
>>       {
>> -      /* lea symbol, %rN.  */
>> -      dst_reg = ginsn_dw2_regnum (i.op[1].regs);
>> -      /* TBD_GINSN_INFO_LOSS - Skip encoding information about the symbol.  */
>> -      ginsn = ginsn_new_mov (insn_end_sym, false,
>> -			     GINSN_SRC_IMM, 0xf /* arbitrary const.  */, 0,
>> -			     GINSN_DST_REG, dst_reg, 0);
>> +      if (i.disp_operands && i.op[0].disps->X_op == O_constant)
>> +	{
>> +	  /* lea const, %rN.  */
>> +	  src_disp = i.op[0].disps->X_add_number;
>> +	  dst_reg = ginsn_dw2_regnum (i.op[1].regs);
>> +	  ginsn = ginsn_new_mov (insn_end_sym, false,
>> +				 GINSN_SRC_IMM, 0, src_disp,
>> +				 GINSN_DST_REG, dst_reg, 0);
>> +	  ginsn_set_where (ginsn);
>> +	}
> 
> Since earlier on you've been mentioning that you primarily target insn
> forms actually in use in existing code, I wonder whether you've ever
> seen any use of this. The same is better (shorter) expressed by MOV,
> and hence I'd expect people to prefer that form. IOW the question here
> is: Is there much value in having this code, rather than simply
> penalizing people bogusly using such by having this case end at the
> common x86_ginsn_unhandled path as well.
> 

IMO this is not a useful pattern either for the case when it is better 
expressed by a MOV.  I have not seen such usages so far, and I dont 
expect this to be seen either.

I am OK with removing this handling and default to x86_ginsn_unhandled 
code path with code comments here. Makes sense.

>> +      /* Skip handling lea symbol, %rN here.  Deal with it in the
>> +	 x86_ginsn_unhandled code path.  TBD_GINSN_GEN_NOT_SCFI.  */
>>       }
>> -  else if (i.base_reg && !i.index_reg)
>> +  else if ((i.base_reg && !i.index_reg)
>> +	   || (!i.base_reg && i.index_reg))
> 
> I frequently see conditionals like this written this way. Maybe it's
> indeed clearer to a majority; personally I'd prefer the shorter
> 
>    else if (!i.base_reg != !i.index_reg)
> 
> However, considering the earlier if() this is an else-if to, even
> 
>    else if (!i.base_reg || !i.index_reg)
> 
> would also suffice (but as per above that if() may want to go away).
> 

OK.  I will surely spend a few extra seconds reading this
   (!i.base_reg != !i.index_reg)
as compared to the original construct, but I am fine with using it if 
that's your preference.  I have switched to that expression.

>>       {
>> -      /* lea    -0x2(%base),%dst.  */
>> -      base_reg = ginsn_dw2_regnum (i.base_reg);
>> -      dst_reg = ginsn_dw2_regnum (i.op[1].regs);
>> +      /* lea disp(%base) %dst    or    lea disp(,%index,imm) %dst.  */
> 
> Would be nice if the missing commas were added here.
> 

OK.

>> -      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);
>> +      src1 = (i.base_reg) ? i.base_reg : i.index_reg;
>> +      src1_reg = ginsn_dw2_regnum (src1);
> 
> Since I can't spot any other use of src1, why not simply
> 
>        src1_reg = ginsn_dw2_regnum (i.base_reg ? i.base_reg : i.index_reg);
> 
> ? Otherwise at the very least please omit the pointless parentheses in
> the conditional expression.
> 

Will remove the parentheses.

>>         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
>> -    {
>> -      /* lea disp(%base,%index,imm) %dst.  */
>> -      /* TBD_GINSN_INFO_LOSS - Skip adding information about the disp and imm
>> -	 for index reg.  */
>> -      base_reg = ginsn_dw2_regnum (i.base_reg);
>> -      index_reg = ginsn_dw2_regnum (i.index_reg);
>> -      dst_reg = ginsn_dw2_regnum (i.op[1].regs);
>> -      /* Generate an GINSN_TYPE_OTHER ginsn.  */
>> -      ginsn = ginsn_new_other (insn_end_sym, true,
>> -			       GINSN_SRC_REG, base_reg,
>> -			       GINSN_SRC_REG, index_reg,
>> -			       GINSN_DST_REG, dst_reg);
>> -    }
>> +      /* It makes sense to represent a scale factor of 1 precisely here
>> +	 (i.e., not using GINSN_TYPE_OTHER, but rather similar to the
>> +	 base-without-index case).  Ignore the case when disp has a symbol
>> +	 instead.  */
>> +      if (!index_scale
>> +	  && (!i.disp_operands
>> +	      || (i.disp_operands && i.op[0].disps->X_op == O_constant)))
> 
> This is functionally identical to the shorter
> 
>        if (!index_scale
> 	  && (!i.disp_operands || i.op[0].disps->X_op == O_constant))
> 
> But: What about any of
> 
> 	lea	(%rax,%riz),%rbp
> 	lea	(%rax,4),%rbp
> 	lea	(%rax,%riz,4),%rbp
> 
> ?

Current behaviour is:

lea  (%rax,%riz),%rbp
ginsn: OTH 0, 0, %r6

lea  (%rax,4),%rbp
****  Warning: scale factor of 4 without an index register
ginsn: MOV %r0, %r6

lea  (%rax,%riz,4),%rbp
ginsn: OTH 0, 0, %r6

lea  sym(,%riz), %rbp
ginsn: OTH 0, 0, %r6

lea  (,%riz), %rbp
ginsn: MOV %r4, %r6
(We use DWARF register number 4 {%rsi} in lieu of %riz).

With respect to SCFI correctness, I dont see an issue...

Re: [PATCH,V2] gas: x86: ginsn: adjust ginsns for certain lea ops

[Jan Beulich]

On 25.01.2024 00:56, Indu Bhagat wrote:
> On 1/23/24 23:51, Jan Beulich wrote:
>> On 24.01.2024 07:40, Indu Bhagat wrote:
>>> +      /* lea disp(%base) %dst    or    lea disp(,%index,imm) %dst.  */
>>
>> Would be nice if the missing commas were added here.
>>
> 
> OK.
> 
>>> -      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);
>>> +      src1 = (i.base_reg) ? i.base_reg : i.index_reg;
>>> +      src1_reg = ginsn_dw2_regnum (src1);
>>
>> Since I can't spot any other use of src1, why not simply
>>
>>        src1_reg = ginsn_dw2_regnum (i.base_reg ? i.base_reg : i.index_reg);
>>
>> ? Otherwise at the very least please omit the pointless parentheses in
>> the conditional expression.
>>
> 
> Will remove the parentheses.
> 
>>>         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
>>> -    {
>>> -      /* lea disp(%base,%index,imm) %dst.  */
>>> -      /* TBD_GINSN_INFO_LOSS - Skip adding information about the disp and imm
>>> -	 for index reg.  */
>>> -      base_reg = ginsn_dw2_regnum (i.base_reg);
>>> -      index_reg = ginsn_dw2_regnum (i.index_reg);
>>> -      dst_reg = ginsn_dw2_regnum (i.op[1].regs);
>>> -      /* Generate an GINSN_TYPE_OTHER ginsn.  */
>>> -      ginsn = ginsn_new_other (insn_end_sym, true,
>>> -			       GINSN_SRC_REG, base_reg,
>>> -			       GINSN_SRC_REG, index_reg,
>>> -			       GINSN_DST_REG, dst_reg);
>>> -    }
>>> +      /* It makes sense to represent a scale factor of 1 precisely here
>>> +	 (i.e., not using GINSN_TYPE_OTHER, but rather similar to the
>>> +	 base-without-index case).  Ignore the case when disp has a symbol
>>> +	 instead.  */
>>> +      if (!index_scale
>>> +	  && (!i.disp_operands
>>> +	      || (i.disp_operands && i.op[0].disps->X_op == O_constant)))
>>
>> This is functionally identical to the shorter
>>
>>        if (!index_scale
>> 	  && (!i.disp_operands || i.op[0].disps->X_op == O_constant))
>>
>> But: What about any of
>>
>> 	lea	(%rax,%riz),%rbp
>> 	lea	(%rax,4),%rbp
>> 	lea	(%rax,%riz,4),%rbp
>>
>> ?
> 
> Current behaviour is:
> 
> lea  (%rax,%riz),%rbp
> ginsn: OTH 0, 0, %r6

Ought to be MOV?

> lea  (%rax,4),%rbp
> ****  Warning: scale factor of 4 without an index register
> ginsn: MOV %r0, %r6

Oh, right - the scale factor is zapped in that case along with issuing
the warning. That's not quite right though, I suppose.

> lea  (%rax,%riz,4),%rbp
> ginsn: OTH 0, 0, %r6

Ought to be MOV again?

> lea  sym(,%riz), %rbp
> ginsn: OTH 0, 0, %r6
> 
> lea  (,%riz), %rbp
> ginsn: MOV %r4, %r6
> (We use DWARF register number 4 {%rsi} in lieu of %riz).

Where's the 4 coming from? That doesn't look right. Imo when you
create insns other than OTHER, they should be correct. Irrespective
of SCFI's needs. (The case here is no different from "lea const, ..."
so ought to be handled like that.)

Jan

Re: [PATCH,V2] gas: x86: ginsn: adjust ginsns for certain lea ops

[Indu Bhagat]

On 1/24/24 23:33, Jan Beulich wrote:
>>>>          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
>>>> -    {
>>>> -      /* lea disp(%base,%index,imm) %dst.  */
>>>> -      /* TBD_GINSN_INFO_LOSS - Skip adding information about the disp and imm
>>>> -	 for index reg.  */
>>>> -      base_reg = ginsn_dw2_regnum (i.base_reg);
>>>> -      index_reg = ginsn_dw2_regnum (i.index_reg);
>>>> -      dst_reg = ginsn_dw2_regnum (i.op[1].regs);
>>>> -      /* Generate an GINSN_TYPE_OTHER ginsn.  */
>>>> -      ginsn = ginsn_new_other (insn_end_sym, true,
>>>> -			       GINSN_SRC_REG, base_reg,
>>>> -			       GINSN_SRC_REG, index_reg,
>>>> -			       GINSN_DST_REG, dst_reg);
>>>> -    }
>>>> +      /* It makes sense to represent a scale factor of 1 precisely here
>>>> +	 (i.e., not using GINSN_TYPE_OTHER, but rather similar to the
>>>> +	 base-without-index case).  Ignore the case when disp has a symbol
>>>> +	 instead.  */
>>>> +      if (!index_scale
>>>> +	  && (!i.disp_operands
>>>> +	      || (i.disp_operands && i.op[0].disps->X_op == O_constant)))
>>> This is functionally identical to the shorter
>>>
>>>         if (!index_scale
>>> 	  && (!i.disp_operands || i.op[0].disps->X_op == O_constant))
>>>
>>> But: What about any of
>>>
>>> 	lea	(%rax,%riz),%rbp
>>> 	lea	(%rax,4),%rbp
>>> 	lea	(%rax,%riz,4),%rbp
>>>
>>> ?
>> Current behaviour is:
>>
>> lea  (%rax,%riz),%rbp
>> ginsn: OTH 0, 0, %r6
> Ought to be MOV?
> 

MOV is more precise, thats true here...

>> lea  (%rax,4),%rbp
>> ****  Warning: scale factor of 4 without an index register
>> ginsn: MOV %r0, %r6
> Oh, right - the scale factor is zapped in that case along with issuing
> the warning. That's not quite right though, I suppose.
> 
>> lea  (%rax,%riz,4),%rbp
>> ginsn: OTH 0, 0, %r6
> Ought to be MOV again?
> 

..and here again. But I am inclined to just leave them as OTH for the 
same reason, we left the sub-case handling of lea symbol, %dst : this is 
better expressed as a MOV itself.

>> lea  sym(,%riz), %rbp
>> ginsn: OTH 0, 0, %r6
>>
>> lea  (,%riz), %rbp
>> ginsn: MOV %r4, %r6
>> (We use DWARF register number 4 {%rsi} in lieu of %riz).
> Where's the 4 coming from? That doesn't look right. Imo when you
> create insns other than OTHER, they should be correct. Irrespective
> of SCFI's needs. (The case here is no different from "lea const, ..."
> so ought to be handled like that.)

Since %riz is a pseudo register, and we need a DWARF register number in 
the ginsn representation, I designated %rsi to be used instead when %riz 
or %rip appears. This adds a inexistent data flow dependence, but I 
judged that this _should_ be ok (as not callee-saved register).

That said, for this specific case of 'lea (,%riz), %rbp', generating 
ginsn GINSN_TYPE_OTHER does seem better. I agree that when we create 
ginsn other than GINSN_TYPE_OTHER, they should be as precise as possible.

I can add a check of (!i.index_reg || i.index_reg->reg_num != RegIZ) and 
  handle lea (,%riz), %rbp as GINSN_TYPE_OTHER.