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

[Jan Beulich]

On 31.07.2024 09:13, Indu Bhagat wrote:
> On 7/30/24 12:28 AM, Jan Beulich wrote:
>> On 29.07.2024 21:03, Indu Bhagat wrote:
>>> --- a/gas/config/tc-i386.c
>>> +++ b/gas/config/tc-i386.c
>>> +{
>>> +  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).  */

Btw, noticing only now: You want to be consistent with mentioning (or
not) "r/m" in the two comments.

>>> +    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?
>>
> 
> The caller does screen (in x86_ginsn_new):
> 
>        else if (i.tm.extension_opcode == 4 || i.tm.extension_opcode == 2)
>          ginsn = x86_ginsn_indirect_branch (insn_end_sym);

Oh, I see what you mean, but that's not what I read out of "screen" when
the comment is place with the "return". In that case my expectation would
be that the caller is expected to look at the result. I wonder whether,
consistent with what we do elsewhere (yet not really nice imo) abort() or
gas_assert() (or know()) would better be used on this path then.

Jan