[PATCH,V5 8/9] gas: aarch64: add experimental support for SCFI

[Richard Sandiford]

Indu Bhagat <indu.bhagat@oracle.com> writes:
> On 7/17/24 5:17 AM, Richard Sandiford wrote:
>> Indu Bhagat <indu.bhagat@oracle.com> writes:
>>>>> +/* Generate ginsn for addsub instructions with reg opnd.  */
>>>>> +
>>>>> +static ginsnS *
>>>>> +aarch64_ginsn_addsub_reg (const symbolS *insn_end_sym)
>>>>> +{
>>>>> +  ginsnS *ginsn = NULL;
>>>>> +  bool add_p, sub_p;
>>>>> +  unsigned int dst_reg, src1_reg, src2_reg;
>>>>> +  aarch64_opnd_info *dst, *src1, *src2;
>>>>> +  ginsnS *(*ginsn_func) (const symbolS *, bool,
>>>>> +			 enum ginsn_src_type, unsigned int, offsetT,
>>>>> +			 enum ginsn_src_type, unsigned int, offsetT,
>>>>> +			 enum ginsn_dst_type, unsigned int, offsetT);
>>>>> +
>>>>> +  aarch64_inst *base = &inst.base;
>>>>> +  const aarch64_opcode *opcode = base->opcode;
>>>>> +
>>>>> +  add_p = aarch64_opcode_subclass_p (opcode, F_ARITH_ADD);
>>>>> +  sub_p = aarch64_opcode_subclass_p (opcode, F_ARITH_SUB);
>>>>> +  gas_assert (add_p || sub_p);
>>>>> +  ginsn_func = add_p ? ginsn_new_add : ginsn_new_sub;
>>>>> +
>>>>> +  gas_assert (aarch64_num_of_operands (opcode) == 3);
>>>>> +  dst = &base->operands[0];
>>>>> +  src1 = &base->operands[1];
>>>>> +  src2 = &base->operands[2];
>>>>> +
>>>>> +  dst_reg = ginsn_dw2_regnum (dst);
>>>>> +  src1_reg = ginsn_dw2_regnum (src1);
>>>>> +  src2_reg = ginsn_dw2_regnum (src2);
>>>>> +
>>>>> +  /* Ignoring shift amount, if any,  does not affect SCFI correctness.
>>>>> +     Note TBD_GINSN_INFO_LOSS.  */
>>>>
>>>> Could we return null if:
>>>>
>>>>     src2->shifter.kind != AARCH64_MOD_NONE
>>>>
>>>> instead of ignoring?  It seems better not to create an "incorrect"
>>>> description.  (Can keep the TBD comment at the same time.)
>> 
>> Just wanted to highlight the above in case you didn't see it first
>> time round.  (Email review with big patch quotes isn't the most
>> easy-to-track system.)
>> 
>
> Ah yes. Thanks for reminding me.  Had addressed this but totally missed 
> replying to it. I should have replied especially because I see something 
> that may or may not be an anomaly.
>
> I finally used the check as follows:
>    /* ATM, shift amount, if any, cannot be represented in the 
> GINSN_TYPE_ADD or
>       GINSN_TYPE_SUB.  As the extra information does not impact SCFI
>       correctness, skip generating ginsn for these cases.  Note
>       TBD_GINSN_INFO_LOSS.  */
>    if (src2->shifter.kind != AARCH64_MOD_NONE
>        && (src2->shifter.operator_present || src2->shifter.amount_present))
>      return ginsn;
>
> Because I noticed that for
>
> - (#1) sub sp,x29,x2
>    src2->shifter = {kind = AARCH64_MOD_LSL, operator_present = 0, 
> amount_present = 0, amount = 0}
>
> - (#2) add sp,x5,x2,sxtw
>    src2->shifter = {kind = AARCH64_MOD_SXTW, operator_present = 1, 
> amount_present = 0, amount = 0}
>
> I have not been able to study the code initializing opnd->shifter yet 
> and I am not sure if #1 is expected (It seems harmless the way it is 
> currently, but I was sort of expecting to see a AARCH64_MOD_NONE for #1).

Hmm, yeah, it seems to be deliberate:

	  if (!info->shifter.operator_present)
	    {
	      /* Default to LSL if not present.  Libopcodes prefers shifter
		 kind to be explicit.  */
	      gas_assert (info->shifter.kind == AARCH64_MOD_NONE);
	      info->shifter.kind = AARCH64_MOD_LSL;

but "libopcodes prefers" is a bit vague.

It might be better to check for MOD_LSL of 0, i.e. something like.

   if (src2->shifter.kind != AARCH64_MOD_NONE
       && (src2->shifter.kind != AARCH64_MOD_LSL || src2->shifter.amount != 0))
     return ginsn;

since something like:

   sub sp,x29,x2,lsl #0

is fine (though the lsl is redundant).

Thanks,
Richard