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.

diff --git a/gas/config/tc-i386.c b/gas/config/tc-i386.c
index 3c64057fd67b58d82fd0542b8490e9252b668b74..2e578e2b0d7b95201a04d16940567b7449d59057 100644 (file)
--- a/gas/config/tc-i386.c
+++ b/gas/config/tc-i386.c
@@ -5661,8 +5661,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 *
@@ -5670,76 +5672,66 @@ 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
new file mode 100644 (file)
index 0000000..f787660
--- /dev/null
+++ 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
new file mode 100644 (file)
index 0000000..aa2e8fd
--- /dev/null
+++ 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
index 9005c47d789de1f30b7e99fa5b381b4f55ff7f5b..9c76974fefe313a5d641a926fb175c236bd41c7b 100644 (file)
--- 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"