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

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 select lea ops 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.
tromey · Feb 1, 2024 · 09812f0 · 09812f0
1 parent b3c670a
commit 09812f0
Show file tree

Hide file tree

Showing 4 changed files with 134 additions and 65 deletions.
diff --git a/gas/config/tc-i386.c b/gas/config/tc-i386.c
@@ -5661,85 +5661,77 @@ 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 *
 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;
+  bool index_regiz_p;
 
-  if (!i.index_reg && !i.base_reg)
+  if (!i.base_reg != (!i.index_reg || i.index_reg->reg_num == RegIZ))
     {
-      /* 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);
-    }
-  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);
+      /* lea disp(%base), %dst    or    lea disp(,%index,imm), %dst.
+	 Either index_reg or base_reg exists, but not both.  Further, as per
+	 above, the case when just %index exists but is equal to RegIZ is
+	 excluded.  If not excluded, a GINSN_TYPE_MOV of %rsi
+	 (GINSN_DW2_REGNUM_RSI_DUMMY) to %dst will be generated by this block.
+	 Such a mov ginsn is imprecise; so, exclude now and generate
+	 GINSN_TYPE_OTHER instead later via the x86_ginsn_unhandled ().
+	 Excluding other cases is required due to
+	 TBD_GINSN_REPRESENTATION_LIMIT.  */
 
-      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);
+      index_regiz_p = i.index_reg && i.index_reg->reg_num == RegIZ;
+      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).  A non-zero scale factor is still OK if
+	 the index reg is zero reg.
+	 However, skip from here the case when disp has a symbol instead.
+	 TBD_GINSN_REPRESENTATION_LIMIT.  */
+      if ((!index_scale || index_regiz_p)
+	  && (!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,
+     - when (i.index_reg && i.base_reg) is true,
+       e.g., lea disp(%base,%index,imm), %dst
+       We do not have a ginsn representation for multiply.
+     - or, when (!i.index_reg && !i.base_reg) is true,
+       e.g., lea symbol, %dst
+       Not a frequent pattern.  If %dst is a register of interest, the user is
+       likely to use a MOV op anyway.
+     Deal with these via the x86_ginsn_unhandled () code path to generate
+     GINSN_TYPE_OTHER when necessary.  TBD_GINSN_GEN_NOT_SCFI.  */
 
   return ginsn;
 }
@@ -6168,7 +6160,7 @@ x86_ginsn_new (const symbolS *insn_end_sym, enum ginsn_gen_mode gmode)
     case 0x8d:
       if (i.tm.opcode_space != SPACE_BASE)
 	break;
-      /* lea disp(%base,%index,imm) %dst.  */
+      /* lea disp(%base,%index,imm), %dst.  */
       ginsn = x86_ginsn_lea (insn_end_sym);
       break;
 

diff --git a/gas/testsuite/gas/scfi/x86_64/ginsn-lea-1.l b/gas/testsuite/gas/scfi/x86_64/ginsn-lea-1.l
@@ -0,0 +1,54 @@
+.*: Assembler messages:
+.*: Warning: scale factor of 4 without an index register
+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 488D2C25 		lea  0x9090, %rbp
+   7      90900000 
+   7              	ginsn: OTH 0, 0, %r6
+   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              	
+  15              		.allow_index_reg
+  16 0039 488D2C20 		lea  \(%rax,%riz\),%rbp
+  16              	ginsn: MOV %r0, %r6
+  17 003d 488D28   		lea  \(%rax,4\),%rbp
+\*\*\*\*  Warning: scale factor of 4 without an index register
+  17              	ginsn: MOV %r0, %r6
+  18 0040 488D2CA0 		lea  \(%rax,%riz,4\),%rbp
+  18              	ginsn: MOV %r0, %r6
+  19 0044 488D2C25 		lea  sym\(,%riz\), %rbp
+  19      00000000 
+  19              	ginsn: OTH 0, 0, %r6
+  20 004c 488D2C25 		lea  \(,%riz\), %rbp
+  20      00000000 
+  20              	ginsn: OTH 0, 0, %r6
+  21              	.LFE0:
+  21              	ginsn: SYM .LFE0
+  22              		.size   foo, .-foo
+  22              	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
@@ -0,0 +1,22 @@
+## Testcase with a variety of lea.
+	.text
+	.globl  foo
+	.type   foo, @function
+foo:
+	lea  symbol, %rbp
+	lea  0x9090, %rbp
+	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
+
+	.allow_index_reg
+	lea  (%rax,%riz),%rbp
+	lea  (%rax,4),%rbp
+	lea  (%rax,%riz,4),%rbp
+	lea  sym(,%riz), %rbp
+	lea  (,%riz), %rbp
+.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
@@ -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"
     run_list_test "ginsn-cofi-1" "--scfi=experimental -ali -W"