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

[Jan Beulich]

On 29.07.2024 21:03, Indu Bhagat wrote:
> --- a/gas/config/tc-i386.c
> +++ b/gas/config/tc-i386.c
> @@ -6677,6 +6677,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)

May I suggest s/jump_call/branch/ ?

> +{
> +  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;

The comment is odd: The (presently) sole caller clearly doesn't care at all.
What is the comment about then?

> +  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);

This is technically wrong, and hence the comment needs to reflect that
something needs fixing here before ginsn can be used for further purposes.

> +      /* jmp/call *sym(,%rN,imm)  or  jmp/call *sym(%rN).  */

There isn't necessarily "sym" here, and if I'm not mistaken ...

> +      ginsn = ginsn_func (insn_end_sym, true,
> +			  GINSN_SRC_REG, dw2_regnum, NULL);

... you also don't pass anything about a possible symbol here. That then
similarly needs flagging as an open issue. Use of GINSN_SRC_REG likely
also is merely a placeholder rather than the correct thing to pass in
this case.

There really should already have been FIXME comments in the original
code (according to comments of mine back when all of this was introduced).

> --- 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

While both are handled in the same function now, I think it would still be
a good idea to also cover CALL with no symbol involved and JMP with a
register operand. Additionally %rip-relative operand cases may want adding.

Also a question: Are multiple consecutive unconditional jumps actually
meaningful in such a test? Shouldn't call flow analysis determine code past
an unconditional jump (and with no intermediate label, including no
"implicit" one, as could result from e.g. "JMP .+17") as unreachable?

Jan