gas: x86: ginsn: handle previously missed indirect call and jmp ops

Some flavors of indirect call and jmp instructions were not being handled earlier, leading to a GAS error (#1): (#1) "Error: SCFI: unhandled op 0xff may cause incorrect CFI" Not handling jmp/call (direct or indirect) ops is an error (as shown above) because SCFI needs an accurate CFG to synthesize CFI correctly. Recall that the presence of indirect jmp/call, however, does make the CFG ineligible for SCFI. In other words, generating the ginsns for them now, will eventually cause SCFI to bail out later with an error (#2) anyway: (#2) "Error: untraceable control flow for func 'XXX'" The first error (#1) gives the impression of missing functionality in GAS. So, it seems cleaner to synthesize a GINSN_TYPE_JUMP / GINSN_TYPE_CALL accurately in the backend, and let SCFI machinery complain with the error as expected. The handling for these indirect jmp/call instructions is similar, so reuse the code by carving out a function for the same. Adjust the testcase to include the now handled jmp/call instructions as well. gas/ * config/tc-i386.c (x86_ginsn_indirect_jump_call): New function. (x86_ginsn_new): Refactor out functionality to above. gas/testsuite/ * gas/scfi/x86_64/ginsn-cofi-1.l: Adjust the output. * gas/scfi/x86_64/ginsn-cofi-1.s: Add further varieties of jmp/call opcodes.
bminor · Mar 28, 2024 · 977da52 · 977da52
1 parent 0d12d72
commit 977da52
Show file tree

Hide file tree

Showing 3 changed files with 81 additions and 59 deletions.
diff --git a/gas/config/tc-i386.c b/gas/config/tc-i386.c
@@ -5815,6 +5815,54 @@ x86_ginsn_jump (const symbolS *insn_end_sym, bool cond_p)
   return ginsn;
 }
 
+static ginsnS *
+x86_ginsn_indirect_jump_call (const symbolS *insn_end_sym)
+{
+  ginsnS *ginsn = NULL;
+  const reg_entry *mem_reg;
+  unsigned int dw2_regnum;
+
+  ginsnS * (*ginsn_func) (const symbolS *sym, bool real_p,
+			  enum ginsn_src_type src_type, unsigned int src_reg,
+			  const symbolS *src_ginsn_sym);
+
+  if (i.tm.extension_opcode == 4)
+    /* 0xFF /4 (jmp r/m).  */
+    ginsn_func = ginsn_new_jump;
+  else if (i.tm.extension_opcode == 2)
+    /* 0xFF /2 (call).  */
+    ginsn_func = ginsn_new_call;
+  else
+    /* Other cases are not expected.  Caller must screen.  */
+    return ginsn;
+
+  if (i.reg_operands)
+    {
+      dw2_regnum = ginsn_dw2_regnum (i.op[0].regs);
+      ginsn = ginsn_func (insn_end_sym, true,
+			  GINSN_SRC_REG, dw2_regnum, NULL);
+      ginsn_set_where (ginsn);
+    }
+  else if (i.mem_operands)
+    {
+      mem_reg = i.base_reg ? i.base_reg : i.index_reg;
+      /* Use dummy register if no base or index.  Unlike other opcodes,
+	 where we simply return NULL, we must try to generate a ginsn here.
+	 Otherwise, the user gets the impression of missing functionality:
+	   - a call insn is an IMPLICIT_STACK_OP.
+	   - jmp insns are necessary for accurate control flow.  */
+      dw2_regnum = (mem_reg
+		    ? ginsn_dw2_regnum (mem_reg)
+		    : GINSN_DW2_REGNUM_RSI_DUMMY);
+      /* jmp/call *sym(,%rN,imm)  or  jmp/call *sym(%rN).  */
+      ginsn = ginsn_func (insn_end_sym, true,
+			  GINSN_SRC_REG, dw2_regnum, NULL);
+      ginsn_set_where (ginsn);
+    }
+
+  return ginsn;
+}
+
 static ginsnS *
 x86_ginsn_enter (const symbolS *insn_end_sym)
 {
@@ -6324,50 +6372,9 @@ x86_ginsn_new (const symbolS *insn_end_sym, enum ginsn_gen_mode gmode)
 	  ginsn_set_where (ginsn_next);
 	  gas_assert (!ginsn_link_next (ginsn, ginsn_next));
 	}
-      else if (i.tm.extension_opcode == 4)
-	{
-	  /* jmp r/m.  E.g., notrack jmp *%rax.  */
-	  if (i.reg_operands)
-	    {
-	      dw2_regnum = ginsn_dw2_regnum (i.op[0].regs);
-	      ginsn = ginsn_new_jump (insn_end_sym, true,
-				      GINSN_SRC_REG, dw2_regnum, NULL);
-	      ginsn_set_where (ginsn);
-	    }
-	  else if (i.mem_operands && i.index_reg)
-	    {
-	      /* jmp    *0x0(,%rax,8).  */
-	      dw2_regnum = ginsn_dw2_regnum (i.index_reg);
-	      ginsn = ginsn_new_jump (insn_end_sym, true,
-				      GINSN_SRC_REG, dw2_regnum, NULL);
-	      ginsn_set_where (ginsn);
-	    }
-	  else if (i.mem_operands && i.base_reg)
-	    {
-	      dw2_regnum = ginsn_dw2_regnum (i.base_reg);
-	      ginsn = ginsn_new_jump (insn_end_sym, true,
-				      GINSN_SRC_REG, dw2_regnum, NULL);
-	      ginsn_set_where (ginsn);
-	    }
-	}
-      else if (i.tm.extension_opcode == 2)
-	{
-	  /* 0xFF /2 (call).  */
-	  if (i.reg_operands)
-	    {
-	      dw2_regnum = ginsn_dw2_regnum (i.op[0].regs);
-	      ginsn = ginsn_new_call (insn_end_sym, true,
-				      GINSN_SRC_REG, dw2_regnum, NULL);
-	      ginsn_set_where (ginsn);
-	    }
-	  else if (i.mem_operands && i.base_reg)
-	    {
-	      dw2_regnum = ginsn_dw2_regnum (i.base_reg);
-	      ginsn = ginsn_new_call (insn_end_sym, true,
-				      GINSN_SRC_REG, dw2_regnum, NULL);
-	      ginsn_set_where (ginsn);
-	    }
-	}
+      else if (i.tm.extension_opcode == 4 || i.tm.extension_opcode == 2)
+	ginsn = x86_ginsn_indirect_jump_call (insn_end_sym);
+
       break;
 
     case 0xc2: /* ret imm16.  */

diff --git a/gas/testsuite/gas/scfi/x86_64/ginsn-cofi-1.l b/gas/testsuite/gas/scfi/x86_64/ginsn-cofi-1.l
@@ -1,8 +1,7 @@
 .*: Assembler messages:
-.*:20: Error: untraceable control flow for func 'foo'
+.*:24: Error: untraceable control flow for func 'foo'
 GAS LISTING .*
 
-
    1              	# Testcase with a variety of "change of flow instructions"
    2              	#
    3              	# This test does not have much going on wrt synthesis of CFI;
@@ -22,17 +21,29 @@ GAS LISTING .*
   12              	ginsn: JMP %r0, 
   13 \?\?\?\? 41FFD0   		call    \*%r8
   13              	ginsn: CALL
-  14 \?\?\?\? 67E305   		jecxz   .L179
-  14              	ginsn: JCC 
-  15 \?\?\?\? FF6730   		jmp     \*48\(%rdi\)
-  15              	ginsn: JMP %r5, 
-  16 \?\?\?\? 7000     		jo      .L179
+  14 \?\?\?\? FF14C500 		call    \*cost_arr\(,%rax,8\)
+  14      000000
+  14              	ginsn: CALL
+  15 \?\?\?\? FF142500 		call    \*symbol\+1
+  15      000000
+  15              	ginsn: CALL
+  16 \?\?\?\? 67E313   		jecxz   .L179
   16              	ginsn: JCC 
-  17              	.L179:
-  17              	ginsn: SYM .L179
-  18 \?\?\?\? C3       		ret
-  18              	ginsn: RET
-  19              	.LFE0:
-  19              	ginsn: SYM .LFE0
-  20              		.size   foo, .-foo
-  20              	ginsn: SYM FUNC_END
+  17 \?\?\?\? FF6730   		jmp     \*48\(%rdi\)
+  17              	ginsn: JMP %r5, 
+  18 \?\?\?\? FF24C500 		jmp     \*cost_arr\(,%rax,8\)
+  18      000000
+  18              	ginsn: JMP %r0, 
+  19 \?\?\?\? FF242500 		jmp     \*symbol\+1
+  19      000000
+  19              	ginsn: JMP %r4, 
+  20 \?\?\?\? 7000     		jo      .L179
+  20              	ginsn: JCC 
+  21              	.L179:
+  21              	ginsn: SYM .L179
+  22 \?\?\?\? C3       		ret
+  22              	ginsn: RET
+  23              	.LFE0:
+  23              	ginsn: SYM .LFE0
+  24              		.size   foo, .-foo
+  24              	ginsn: SYM FUNC_END
diff --git a/gas/testsuite/gas/scfi/x86_64/ginsn-cofi-1.s b/gas/testsuite/gas/scfi/x86_64/ginsn-cofi-1.s
@@ -11,8 +11,12 @@ foo:
 	loop    foo
 	notrack jmp     *%rax
 	call    *%r8
+	call    *cost_arr(,%rax,8)
+	call    *symbol+1
 	jecxz   .L179
 	jmp     *48(%rdi)
+	jmp     *cost_arr(,%rax,8)
+	jmp     *symbol+1
 	jo      .L179
 .L179:
 	ret