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

[Indu Bhagat]

On 7/31/24 12:22 AM, Jan Beulich wrote:
> 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.
> 

Done.

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

Since the code uses gas_assert (), I added a gas_assert () for the 
expected values in V2.

Thanks