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

[Indu Bhagat <indu.bhagat@oracle.com>]

On 7/18/24 3:42 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);
> 
> I think we should check ginsn_dw2_regnum_invalid_p on all three of these,
> since:
> 
> 	add	xzr, xzr, xzr
> 
> is a thing.
> 

I saw these appearing as opcode->iclass of addsub_shift.  So I did not 
add any check to the addsub_reg. But perhaps they could be modeled as 
addsub_reg later. I have added a

   if (ginsn_dw2_regnum_invalid_p (dst_reg)
       || ginsn_dw2_regnum_invalid_p (src1_reg) 

       || ginsn_dw2_regnum_invalid_p (src2_reg)) 

     return ginsn;

to aarch64_ginsn_addsub_reg.  Thanks.

>> [...]
>> +/* Generate ginsn for the load pair and store pair instructions.  */
>> +
>> +static ginsnS *
>> +aarch64_ginsn_ldstp (const symbolS *insn_end_sym)
>> +{
>> +  ginsnS *ginsn = NULL;
>> +  ginsnS *ginsn_ind = NULL;
>> +  ginsnS *ginsn_mem1 = NULL;
>> +  ginsnS *ginsn_mem2 = NULL;
>> +  unsigned int opnd_reg, addr_reg;
>> +  offsetT offset, mem_offset;
>> +  unsigned int width = 8;
>> +  bool load_p = false;
>> +  bool store_p = false;
>> +  bool other_p = false;
>> +
>> +  aarch64_opnd_info *opnd1, *opnd2, *addr;
>> +  aarch64_inst *base = &inst.base;
>> +  const aarch64_opcode *opcode = base->opcode;
>> +
>> +  /* This function is for handling ldp / stp ops only.  */
>> +  gas_assert (opcode->iclass == ldstpair_indexed
>> +	      || opcode->iclass == ldstnapair_offs
>> +	      || opcode->iclass == ldstpair_off);
>> +  gas_assert (aarch64_num_of_operands (opcode) == 3);
>> +
>> +  opnd1 = &base->operands[0];
>> +  opnd2 = &base->operands[1];
>> +  addr = &base->operands[2];
>> +
>> +  load_p = aarch64_opcode_subclass_p (opcode, F_LDST_LOAD);
>> +  store_p = aarch64_opcode_subclass_p (opcode, F_LDST_STORE);
>> +  other_p = aarch64_opcode_subclass_p (opcode, F_SUBCLASS_OTHER);
>> +  gas_assert (load_p || store_p || other_p);
>> +
>> +  addr_reg = ginsn_dw2_regnum (addr);
>> +  gas_assert (!addr->addr.offset.is_reg);
>> +  mem_offset = addr->addr.offset.imm;
>> +
>> +  offset = mem_offset;
>> +  /* Handle address calculation.  */
>> +  if ((addr->addr.preind || addr->addr.postind) && addr->addr.writeback)
>> +    {
>> +      /* Pre-indexed store, e.g., stp x29, x30, [sp, -128]!
>> +	 Pre-indexed addressing is like offset addressing, except that
>> +	 the base pointer is updated as a result of the instruction.
>> +
>> +	 Post-indexed store, e.g., stp     x29, x30, [sp],128
>> +	 Post-index addressing is useful for popping off the stack.  The
>> +	 instruction loads the value from the location pointed at by the stack
>> +	 pointer, and then moves the stack pointer on to the next full location
>> +	 in the stack.  */
>> +      ginsn_ind = ginsn_new_add (insn_end_sym, false,
>> +				 GINSN_SRC_REG, addr_reg, 0,
>> +				 GINSN_SRC_IMM, 0, mem_offset,
>> +				 GINSN_DST_REG, addr_reg, 0);
>> +      ginsn_set_where (ginsn_ind);
>> +
>> +      /* With post-index addressing, the value is loaded from the address in
>> +	 the base pointer, and then the pointer is updated.  With pre-index
>> +	 addressing, the addr computation has already been explicitly done.  */
>> +      offset = 0;
>> +    }
>> +
>> +  /* Insns like ldpsw (marked with subclass F_SUBCLASS_OTHER) do not need to
>> +     generate any load or store for SCFI purposes.  Next, enforce that for CFI
>> +     purposes, the width of save / restore operation has to be 8 bytes or more.
>> +     However, the address processing component may have updated the stack
>> +     pointer.  At least, emit that ginsn and return.  Also note,
>> +     TBD_GINSN_GEN_NOT_SCFI.  */
>> +  if (other_p || aarch64_get_qualifier_esize (opnd1->qualifier) < 8)
>> +    return ginsn_ind;
>> +
>> +  /* Save / restore of WZR is not of interest for SCFI.  */
>> +  opnd_reg = ginsn_dw2_regnum (opnd1);
>> +  if (ginsn_dw2_regnum_invalid_p (opnd_reg))
>> +    return ginsn_ind;
> 
> Sorry for not noticing earlier, but I think this should protect
> the individual ginsn_new_*s below.  E.g.:
> 
> 	stp	xzr, x19, [sp, #16]
> 	stp	x19, xzr, [sp, #16]
> 
> are at least vaguely plausible SCFI sequences.  E.g. the second one
> might combine a register save with a variable initialisation.
> 
> The combination is less plausible for loads, but still technically
> possible.
> 

Ah thanks for pointing these out. I didnt check to see that these were 
possible.

I will update aarch64_ginsn_ldstp () to handle these ops.

>> [...]
>> +/* Generate ginsn for mov instructions with reg opnd.  */
>> +
>> +static ginsnS *
>> +aarch64_ginsn_mov_reg (const symbolS *insn_end_sym)
>> +{
>> +  ginsnS *ginsn = NULL;
>> +  unsigned int src_reg = 0, dst_reg;
>> +  aarch64_opnd_info *src, *dst;
>> +  offsetT src_imm = 0;
>> +  enum ginsn_src_type src_type;
>> +
>> +  aarch64_inst *base = &inst.base;
>> +  const aarch64_opcode *opcode = base->opcode;
>> +
>> +  gas_assert (aarch64_num_of_operands (opcode) == 2);
>> +
>> +  dst = &base->operands[0];
>> +  src = &base->operands[1];
>> +
>> +  dst_reg = ginsn_dw2_regnum (dst);
>> +  src_reg = ginsn_dw2_regnum (src);
>> +  src_type = GINSN_SRC_REG;
>> +
>> +  /* FIXME Explicitly bar these GINSN_TYPE_MOV at this time.  This can be
>> +     removed later when SCFI machinery is more robust to deal with
>> +     GINSN_DW2_REGNUM_INVALID.  */
>> +  if (ginsn_dw2_regnum_invalid_p (dst_reg))
>> +    return ginsn;
> 
> We should probably check src_reg too, for:
> 
> 	mov	x1, xzr
> 

Right. Added the check for src_reg.

> (This could be modelled as a mov-immediate though, as a later follow-on.)
> 
> (I think it's ok to check ginsn_dw2_regnum_invalid_p after every call to
> ginsn_dw2_regnum if you prefer, even if the operand doesn't allow WZR/XZR.)
> 

Yes. I am finally seeing that this is better to do. I _think_ I have 
taken care of this in each function now..

>> +
>> +  ginsn = ginsn_new_mov (insn_end_sym, false,
>> +			 src_type, src_reg, src_imm,
>> +			 GINSN_DST_REG, dst_reg, 0);
>> +  ginsn_set_where (ginsn);
>> +
>> +  return ginsn;
>> +}
>> +
>> +/* Generate ginsn for mov instructions with imm opnd.  */
>> +
>> +static ginsnS *
>> +aarch64_ginsn_mov_imm (const symbolS *insn_end_sym)
>> +{
>> +  ginsnS *ginsn = NULL;
>> +  unsigned int src_reg = 0, dst_reg;
>> +  aarch64_opnd_info *src, *dst;
>> +  offsetT src_imm = 0;
>> +  enum ginsn_src_type src_type;
>> +
>> +  aarch64_inst *base = &inst.base;
>> +  const aarch64_opcode *opcode = base->opcode;
>> +
>> +  gas_assert (aarch64_num_of_operands (opcode) == 2);
>> +
>> +  dst = &base->operands[0];
>> +  src = &base->operands[1];
>> +
>> +  dst_reg = ginsn_dw2_regnum (dst);
>> +
>> +  /* For some mov ops, e.g., movn, movk, or movz, there may optionally be more
>> +     work than just a simple mov.  Skip handling these mov altogether and let
>> +     the aarch64_ginsn_unhandled () alert if these insns affect SCFI
>> +     correctness.  TBD_GINSN_GEN_NOT_SCFI.  */
>> +  if (src->type == AARCH64_OPND_HALF)
>> +    return ginsn;
>> +
>> +  /* FIXME Explicitly bar these GINSN_TYPE_MOV at this time.  This can be
>> +     removed later when SCFI machinery is more robust to deal with
>> +     GINSN_DW2_REGNUM_INVALID.  */
>> +  if (ginsn_dw2_regnum_invalid_p (dst_reg))
>> +    return ginsn;
> 
> Very minor, but IMO it'd be more easy to follow if this was partnered
> with the ginsn_dw2_regnum call.
> 

Makes sense. Moved ginsn_dw2_regnum and ginsn_dw2_regnum_invalid_p close 
to each other.

> LGTM otherwise.
> 
> Thanks,
> Richard