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

[Richard Sandiford]

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.

> [...]
> +/* 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.

> [...]
> +/* 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

(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.)

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

LGTM otherwise.

Thanks,
Richard