Re: [PATCH,V4 7/8] gas: aarch64: add experimental support for SCFI

[Richard Sandiford <richard.sandiford@arm.com>]

Indu Bhagat <indu.bhagat@oracle.com> writes:
> On 7/11/24 06:15, Richard Sandiford wrote:
>> Indu Bhagat <indu.bhagat@oracle.com> writes:
>>> On 7/1/24 12:49, Richard Sandiford wrote:
>>>> Indu Bhagat <indu.bhagat@oracle.com> writes:
>>>>> +{
>>>>> +  enum aarch64_operand_class opnd_class;
>>>>> +  unsigned int dw2reg_num = 0;
>>>>> +
>>>>> +  opnd_class = aarch64_get_operand_class (opnd->type);
>>>>> +
>>>>> +  switch (opnd_class)
>>>>> +    {
>>>>> +    case AARCH64_OPND_CLASS_FP_REG:
>>>>> +      dw2reg_num = opnd->reg.regno + 64;
>>>>> +      break;
>>>>> +    case AARCH64_OPND_CLASS_SVE_REGLIST:
>>>>> +      dw2reg_num = opnd->reglist.first_regno + 64;
>>>>> +      break;
>>>>> +    case AARCH64_OPND_CLASS_MODIFIED_REG:
>>>>> +    case AARCH64_OPND_CLASS_INT_REG:
>>>>> +    case AARCH64_OPND_CLASS_ADDRESS:
>>>>> +      /* Use a dummy register value in case of WZR, else this will be an
>>>>> +	 incorrect dependency on REG_SP.  */
>>>>> +      if (!sp_allowed_p && opnd->reg.regno == REG_SP)
>>>>> +	dw2reg_num = GINSN_DW2_REGNUM_R1_DUMMY;
>>>>> +      else
>>>>> +	/* For GPRs of our interest (callee-saved regs, SP, FP, LR),
>>>>> +	   DWARF register number is the same as AArch64 register number.  */
>>>>> +	dw2reg_num = opnd->reg.regno;
>>>>> +      break;
>>>>
>>>> I think the AARCH64_OPND_CLASS_ADDRESS case should look at opnd->addr
>>>> instead.  AARCH64_OPND_CLASS_MODIFIED_REG should look at opnd->shifter.
>>>>
>>>
>>> Made the correction for AARCH64_OPND_CLASS_ADDRESS.
>>>
>>> But for AARCH64_OPND_CLASS_MODIFIED_REG, the register information is
>>> still correct in opnd->reg.regno, IIUC.  It seems to me that the only
>>> information available in the opnd->shifter is how the register is
>>> modified by additional work (shift, multiply etc.); This information is
>>> not used by SCFI:
>>>     - an add/sub with two source register and destination REG_SP/ REG_FP
>>> makes REG_SP/ REG_FP untraceable. So ignoring the shift amount etc does
>>> not hurt SCFI correctness.
>>>     - Cant think of other operations where the shift amount will affect
>>> SCFI correctness..
>>>
>>> So I am not sure of the "AARCH64_OPND_CLASS_MODIFIED_REG should look at
>>> opnd->shifter." of the review comment.
>> 
>> It's more about type correctness.  "shifter" is the data associated with
>> AARCH64_OPND_CLASS_MODIFIED_REG and "reg" is the data associated with
>> AARCH64_OPND_CLASS_INT_REG etc.  I think it's mostly a coincidence
>> that the "reg" and "shifter" alternatives of the union put the register
>> at the same byte offset from the start of the structure.
>> 
>
> The shifter struct is out of the union in struct aarch64_opnd_info.

Oops, yes, I shouldn't have relied on memory.  Sorry for the noise.

Richard