Re: [PATCH,V4 10/14] gas: synthesize CFI for hand-written asm

[Jan Beulich <jbeulich@suse.com>]

On 03.01.2024 08:15, Indu Bhagat wrote:
> --- a/gas/config/obj-elf.c
> +++ b/gas/config/obj-elf.c
> @@ -24,6 +24,7 @@
>  #include "subsegs.h"
>  #include "obstack.h"
>  #include "dwarf2dbg.h"
> +#include "ginsn.h"
>  
>  #ifndef ECOFF_DEBUGGING
>  #define ECOFF_DEBUGGING 0
> @@ -2311,6 +2312,13 @@ obj_elf_size (int ignore ATTRIBUTE_UNUSED)
>        symbol_get_obj (sym)->size = XNEW (expressionS);
>        *symbol_get_obj (sym)->size = exp;
>      }
> +
> +  /* If the symbol in the directive matches the current function being
> +     processed, indicate end of the current stream of ginsns.  */
> +  if (flag_synth_cfi
> +      && S_IS_FUNCTION (sym) && sym == ginsn_data_func_symbol ())
> +    ginsn_data_end (symbol_temp_new_now ());
> +
>    demand_empty_rest_of_line ();
>  }
>  
> @@ -2499,6 +2507,14 @@ obj_elf_type (int ignore ATTRIBUTE_UNUSED)
>  	elfsym->symbol.flags &= ~mask;
>      }
>  
> +  if (S_IS_FUNCTION (sym) && flag_synth_cfi)
> +    {
> +      /* Wrap up processing the previous block of ginsns first.  */
> +      if (frchain_now->frch_ginsn_data)
> +	ginsn_data_end (symbol_temp_new_now ());
> +      ginsn_data_begin (sym);
> +    }
> +
>    demand_empty_rest_of_line ();
>  }

Documentation about .type and .size use could be more precise. Generally
it is entirely benign where exactly these directives live relative to
the code they annotate.

> --- a/gas/config/tc-i386.c
> +++ b/gas/config/tc-i386.c
> @@ -30,6 +30,7 @@
>  #include "subsegs.h"
>  #include "dwarf2dbg.h"
>  #include "dw2gencfi.h"
> +#include "scfi.h"
>  #include "gen-sframe.h"
>  #include "sframe.h"
>  #include "elf/x86-64.h"
> @@ -5287,6 +5288,978 @@ static INLINE bool may_need_pass2 (const insn_template *t)
>  	       && t->base_opcode == 0x63);
>  }
>  
> +bool
> +x86_scfi_callee_saved_p (unsigned int dw2reg_num)

Iirc SCFI is ELF-only. We're not in ELF-only code here, though. Non-ELF
gas, as indicated before, would better not carry any unreachable code.

> +{
> +  if (dw2reg_num == 3 /* rbx.  */
> +      || dw2reg_num == REG_FP /* rbp.  */
> +      || dw2reg_num == REG_SP /* rsp.  */
> +      || (dw2reg_num >= 12 && dw2reg_num <= 15) /* r12 - r15.  */)
> +    return true;
> +
> +  return false;
> +}

This entire function is SysV-ABI-specific without this being made clear
by a comment.

> +/* Check whether a '66H' prefix accompanies the instruction.

With APX 16-bit operand size isn't necessarily represented by a 66h
prefix, but perhaps with an "embedded prefix" inside the EVEX one.
Therefore both the comment and even more so ...

> +   The current users of this API are in the handlers for PUSH, POP
> +   instructions.  These instructions affect the stack pointer implicitly:  the
> +   operand size (16, 32, or 64 bits) determines the amount by which the stack
> +   pointer is decremented (2, 4 or 8).  When '66H' prefix is present, the
> +   instruction has a 16-bit operand.  */
> +
> +static bool
> +ginsn_prefix_66H_p (i386_insn insn)

... the function name would better not allude to just the legacy
encoding. Maybe ginsn_opsize_prefix_p()?

> +{
> +  return (insn.prefix[DATA_PREFIX] == 0x66);
> +}

I take it that all ginsn / scfi interaction is late enough in the
process for this array element already having been reliably set?

> +/* Get the DWARF register number for the given register entry.
> +   For specific byte/word register accesses like al, cl, ah, ch, r8dyte etc.,

What's "r8dyte"? I expect it's a typo, but I can't derive what was
intended to be written.

> +   there is no valid DWARF register number.  This function is a hack - it
> +   relies on relative ordering of reg entries in the i386_regtab.  FIXME - it
> +   will be good to allow a more direct way to get this information.  */

Saying it's a hack is a helpful sign, but it still would be useful to
also briefly describe what the intentions here are. It's hard to spot
whether the code is correct without knowing what's intended (i.e. how
8- and 16-bit registers, or non-GPRs, are meant to be treated).

> +static unsigned int
> +ginsn_dw2_regnum (const reg_entry *ireg)
> +{
> +  /* PS: Note the data type here as int32_t, because of Dw2Inval (-1).  */
> +  int32_t dwarf_reg = Dw2Inval;
> +  const reg_entry *temp = ireg;
> +
> +  /* ginsn creation is available for AMD64 abi only ATM.  Other flag_code
> +     are not expected.  */
> +  gas_assert (flag_code == CODE_64BIT);

With this assertion it is kind of odd to see a further use of flag_code
below.

> +  if (ireg <= &i386_regtab[3])
> +    /* For al, cl, dl, bl, bump over to axl, cxl, dxl, bxl respectively by
> +       adding 8.  */
> +    temp = ireg + 8;
> +  else if (ireg <= &i386_regtab[7])
> +    /* For ah, ch, dh, bh, bump over to axl, cxl, dxl, bxl respectively by
> +       adding 4.  */
> +    temp = ireg + 4;

Assuming this is a frequently executed path, why do these not live ...

> +  dwarf_reg = temp->dw2_regnum[flag_code >> 1];
> +  if (dwarf_reg == Dw2Inval)
> +    {

... here, thus at least not affecting the code path taken for 64-bit GPRs.

> +      /* The code relies on the relative ordering of the reg entries in
> +	 i386_regtab.  The assertion here ensures the code does not recurse
> +	 indefinitely.  */
> +      gas_assert (temp + 16 < &i386_regtab[i386_regtab_size - 1]);

Afaict this is (still) undefined behavior. You may not add to a pointer
without knowing whether the result still points into or exactly past
the underlying array.

> +      temp = temp + 16;

Also - where's the 16 coming from? Was this not updated when rebasing over
APX?

> +      dwarf_reg = ginsn_dw2_regnum (temp);
> +    }
> +
> +  gas_assert (dwarf_reg != Dw2Inval); /* Needs to be addressed.  */

Without actually addressing this (and possible similar cases elsewhere), I
don't think this can go in as other than experimental code (which the
NEWS entry then should state, and where there then should be a plan for an
easy approach of probing gas for no-longer-experimental SCFI support).

> +  return (unsigned int) dwarf_reg;
> +}
> +
> +static ginsnS *
> +x86_ginsn_addsub_reg_mem (const symbolS *insn_end_sym)
> +{
> +  unsigned int dw2_regnum;
> +  unsigned int src2_dw2_regnum;
> +  ginsnS *ginsn = NULL;
> +  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);
> +  uint16_t opcode = i.tm.base_opcode;
> +
> +  gas_assert (opcode == 0x1 || opcode == 0x29);
> +  ginsn_func = (opcode == 0x1) ? ginsn_new_add : ginsn_new_sub;

As mentioned before - checking opcode without also checking opcode
space is pretty meaningless.

> +  /* op %reg, symbol.  */
> +  if (i.mem_operands == 1 && !i.base_reg && !i.index_reg)
> +    return ginsn;

Why does this need special treatment, and why is returning NULL here
okay?

> +  /* op reg, reg/mem.  */
> +  dw2_regnum = ginsn_dw2_regnum (i.op[0].regs);
> +  if (i.reg_operands == 2)
> +    {
> +      src2_dw2_regnum = ginsn_dw2_regnum (i.op[1].regs);
> +      ginsn = ginsn_func (insn_end_sym, true,
> +			  GINSN_SRC_REG, dw2_regnum, 0,
> +			  GINSN_SRC_REG, src2_dw2_regnum, 0,
> +			  GINSN_DST_REG, src2_dw2_regnum, 0);
> +      ginsn_set_where (ginsn);
> +    }
> +  /* Other cases where destination involves indirect access are unnecessary
> +     for SCFI correctness.  TBD_GINSN_GEN_NOT_SCFI.  */
> +
> +  return ginsn;
> +}
> +
> +static ginsnS *
> +x86_ginsn_addsub_mem_reg (const symbolS *insn_end_sym)
> +{
> +  unsigned int dw2_regnum;
> +  unsigned int src2_dw2_regnum;
> +  const reg_entry *mem_reg;
> +  int32_t gdisp = 0;
> +  ginsnS *ginsn = NULL;
> +  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);
> +  uint16_t opcode = i.tm.base_opcode;
> +
> +  gas_assert (opcode == 0x3 || opcode == 0x2b);
> +  ginsn_func = (opcode == 0x3) ? ginsn_new_add : ginsn_new_sub;
> +
> +  /* op symbol, %reg.  */
> +  if (i.mem_operands && !i.base_reg && !i.index_reg)
> +    return ginsn;
> +  /* op mem, %reg.  */

/* op reg/mem, reg.  */ you mean? Which then raises the question ...

> +  dw2_regnum = ginsn_dw2_regnum (i.op[1].regs);
> +
> +  if (i.mem_operands)
> +    {
> +      mem_reg = (i.base_reg) ? i.base_reg : i.index_reg;
> +      src2_dw2_regnum = ginsn_dw2_regnum (mem_reg);
> +      if (i.disp_operands == 1)
> +	gdisp = i.op[0].disps->X_add_number;
> +      ginsn = ginsn_func (insn_end_sym, true,
> +			  GINSN_SRC_INDIRECT, src2_dw2_regnum, gdisp,
> +			  GINSN_SRC_REG, dw2_regnum, 0,
> +			  GINSN_DST_REG, dw2_regnum, 0);
> +      ginsn_set_where (ginsn);
> +    }
> +
> +  return ginsn;
> +}

... why a register source isn't dealt with here.

> +static ginsnS *
> +x86_ginsn_alu_imm (const symbolS *insn_end_sym)
> +{
> +  offsetT src_imm;
> +  unsigned int dw2_regnum;
> +  ginsnS *ginsn = NULL;
> +  enum ginsn_src_type src_type = GINSN_SRC_REG;
> +  enum ginsn_dst_type dst_type = GINSN_DST_REG;
> +
> +  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);
> +
> +  /* FIXME - create ginsn where dest is REG_SP / REG_FP only ? */
> +  /* Map for insn.tm.extension_opcode
> +     000 ADD    100 AND
> +     001 OR     101 SUB
> +     010 ADC    110 XOR
> +     011 SBB    111 CMP  */
> +
> +  /* add/sub/and imm, %reg only at this time for SCFI.
> +     Although all three (and, or , xor) make the destination reg untraceable,

Why would this also be done for CMP? And neither ADC nor SBB are
mentioned at all in ...

> +     and op is handled but not or/xor because we will look into supporting
> +     the DRAP pattern at some point.  */

... this entire comment, justifying the choice made.

> +  if (i.tm.extension_opcode == 5)
> +    ginsn_func = ginsn_new_sub;
> +  else if (i.tm.extension_opcode == 4)
> +    ginsn_func = ginsn_new_and;
> +  else if (i.tm.extension_opcode == 0)
> +    ginsn_func = ginsn_new_add;
> +  else
> +    return ginsn;
> +
> +  /* TBD_GINSN_REPRESENTATION_LIMIT: There is no representation for when a
> +     symbol is used as an operand, like so:
> +	  addq    $simd_cmp_op+8, %rdx
> +     Skip generating any ginsn for this.  */
> +  if (i.imm_operands == 1
> +      && i.op[0].imms->X_op != O_constant)
> +    return ginsn;
> +
> +  /* addq    $1, symbol
> +     addq    $1, -16(%rbp)
> +     Such instructions are not of interest for SCFI.  */
> +  if (i.mem_operands == 1)
> +    return ginsn;

Perhaps not just here: TBD_GINSN_GEN_NOT_SCFI?

> +  gas_assert (i.imm_operands == 1);
> +  src_imm = i.op[0].imms->X_add_number;
> +  /* The second operand may be a register or indirect access.  For SCFI, only
> +     the case when the second opnd is a register is interesting.  Revisit this
> +     if generating ginsns for a different gen mode TBD_GINSN_GEN_NOT_SCFI. */
> +  if (i.reg_operands == 1)
> +    {
> +      dw2_regnum = ginsn_dw2_regnum (i.op[1].regs);
> +      /* For ginsn, keep the imm as second src operand.  */
> +      ginsn = ginsn_func (insn_end_sym, true,
> +			  src_type, dw2_regnum, 0,
> +			  GINSN_SRC_IMM, 0, src_imm,
> +			  dst_type, dw2_regnum, 0);
> +
> +      ginsn_set_where (ginsn);
> +    }
> +
> +  return ginsn;
> +}
> +
> +static ginsnS *
> +x86_ginsn_move (const symbolS *insn_end_sym)
> +{
> +  ginsnS *ginsn;
> +  unsigned int dst_reg;
> +  unsigned int src_reg;
> +  offsetT src_disp = 0;
> +  offsetT dst_disp = 0;
> +  const reg_entry *dst = NULL;
> +  const reg_entry *src = NULL;
> +  uint16_t opcode = i.tm.base_opcode;
> +  enum ginsn_src_type src_type = GINSN_SRC_REG;
> +  enum ginsn_dst_type dst_type = GINSN_DST_REG;
> +
> +  if (opcode == 0x8b || opcode == 0x8a)

Above when handling ALU insns you didn't care about byte ops - why do
you do so here (by checking for 0x8a, and 0x88 below)?

> +    {
> +      /* mov  disp(%reg), %reg.  */
> +      if (i.mem_operands && i.base_reg)
> +	{
> +	  src = i.base_reg;
> +	  if (i.disp_operands == 1)
> +	    src_disp = i.op[0].disps->X_add_number;
> +	  src_type = GINSN_SRC_INDIRECT;
> +	}
> +      else
> +	src = i.op[0].regs;

Even when there's no base, the source isn't necessarily a register.
And in such a case using i.op[0].regs isn't valid.

> +      dst = i.op[1].regs;
> +    }
> +  else if (opcode == 0x89 || opcode == 0x88)
> +    {
> +      /* mov %reg, disp(%reg).  */
> +      src = i.op[0].regs;
> +      if (i.mem_operands && i.base_reg)
> +	{
> +	  dst = i.base_reg;
> +	  if (i.disp_operands == 1)
> +	    dst_disp = i.op[1].disps->X_add_number;
> +	  dst_type = GINSN_DST_INDIRECT;
> +	}
> +      else
> +	dst = i.op[1].regs;

Similarly here then.

> +    }
> +
> +  src_reg = ginsn_dw2_regnum (src);
> +  dst_reg = ginsn_dw2_regnum (dst);
> +
> +  ginsn = ginsn_new_mov (insn_end_sym, true,
> +			 src_type, src_reg, src_disp,
> +			 dst_type, dst_reg, dst_disp);
> +  ginsn_set_where (ginsn);
> +
> +  return ginsn;
> +}
> +
> +/* Generate appropriate ginsn for lea.
> +   Sub-cases marked with TBD_GINSN_INFO_LOSS indicate some loss of information
> +   in the ginsn.  But these are fine for now for GINSN_GEN_SCFI generation
> +   mode.  */
> +
> +static ginsnS *
> +x86_ginsn_lea (const symbolS *insn_end_sym)
> +{
> +  offsetT src_disp = 0;
> +  ginsnS *ginsn = NULL;
> +  unsigned int base_reg;
> +  unsigned int index_reg;
> +  offsetT index_scale;
> +  unsigned int dst_reg;
> +
> +  if (!i.index_reg && !i.base_reg)
> +    {
> +      /* lea symbol, %rN.  */
> +      dst_reg = ginsn_dw2_regnum (i.op[1].regs);
> +      /* TBD_GINSN_INFO_LOSS - Skip encoding information about the symbol.  */
> +      ginsn = ginsn_new_mov (insn_end_sym, false,
> +			     GINSN_SRC_IMM, 0xf /* arbitrary const.  */, 0,
> +			     GINSN_DST_REG, dst_reg, 0);
> +    }
> +  else if (i.base_reg && !i.index_reg)
> +    {
> +      /* lea    -0x2(%base),%dst.  */
> +      base_reg = ginsn_dw2_regnum (i.base_reg);

What if base is %eip? Aiui ginsn_dw2_regnum() will hit an assertion
then.

And what about e.g. "lea symbol(%rbx), %rbp"? The ...

> +      dst_reg = ginsn_dw2_regnum (i.op[1].regs);
> +
> +      if (i.disp_operands)
> +	src_disp = i.op[0].disps->X_add_number;

... constant retrieved here won't properly represent the displacement
then.

> +      if (src_disp)
> +	/* Generate an ADD ginsn.  */
> +	ginsn = ginsn_new_add (insn_end_sym, true,
> +			       GINSN_SRC_REG, base_reg, 0,
> +			       GINSN_SRC_IMM, 0, src_disp,
> +			       GINSN_DST_REG, dst_reg, 0);
> +      else
> +	/* Generate a MOV ginsn.  */
> +	ginsn = ginsn_new_mov (insn_end_sym, true,
> +			       GINSN_SRC_REG, base_reg, 0,
> +			       GINSN_DST_REG, dst_reg, 0);
> +    }
> +  else if (!i.base_reg && i.index_reg)
> +    {
> +      /* lea (,%index,imm), %dst.  */
> +      /* TBD_GINSN_INFO_LOSS - There is no explicit ginsn multiply operation,
> +	 instead use GINSN_TYPE_OTHER.  */

You're also losing the displacement here.

> +      index_scale = i.log2_scale_factor;
> +      index_reg = ginsn_dw2_regnum (i.index_reg);
> +      dst_reg = ginsn_dw2_regnum (i.op[1].regs);
> +      ginsn = ginsn_new_other (insn_end_sym, true,
> +			       GINSN_SRC_REG, index_reg,
> +			       GINSN_SRC_IMM, index_scale,
> +			       GINSN_DST_REG, dst_reg);

Wouldn't it make sense to represent a scale factor of 1 correctly
here (i.e. not as "other", but rather similar to the base-without-
index case above)?

> +    }
> +  else
> +    {
> +      /* lea disp(%base,%index,imm) %dst.  */
> +      /* TBD_GINSN_INFO_LOSS - Skip adding information about the disp and imm
> +	 for index reg. */
> +      base_reg = ginsn_dw2_regnum (i.base_reg);
> +      index_reg = ginsn_dw2_regnum (i.index_reg);
> +      dst_reg = ginsn_dw2_regnum (i.op[1].regs);
> +      /* Generate an ADD ginsn.  */
> +      ginsn = ginsn_new_add (insn_end_sym, true,
> +			     GINSN_SRC_REG, base_reg, 0,
> +			     GINSN_SRC_REG, index_reg, 0,
> +			     GINSN_DST_REG, dst_reg, 0);

Seeing the use of "other" above, why is this (wrongly) represented
as "add"?

> +    }
> +
> +  ginsn_set_where (ginsn);
> +
> +  return ginsn;
> +}

Throughout this function (and perhaps others as well), how come you
don't consider operand size at all? It matters whether results are
64-bit, 32-bit, or 16-bit, after all.

> +static ginsnS *
> +x86_ginsn_jump (const symbolS *insn_end_sym)
> +{
> +  ginsnS *ginsn = NULL;
> +  symbolS *src_symbol;

Here and elsewhere - please use const whenever possible. (I think I said
so already on an earlier version.)

> +  gas_assert (i.disp_operands == 1);
> +
> +  /* A non-zero addend in jump target makes control-flow tracking difficult.
> +     Skip SCFI for now.  */
> +  if (i.op[0].disps->X_op == O_symbol && i.op[0].disps->X_add_number)
> +    {
> +      as_bad ("SCFI: jmp insn with non-zero addend to sym not supported");
> +      return ginsn;
> +    }
> +
> +  if (i.op[0].disps->X_op == O_symbol)

Why the redundant evaluation of ->X_op? In fact, if you moved the
earlier if() ...

> +    {

... here, this ...

> +      gas_assert (!i.op[0].disps->X_add_number);

... assertion would become entirely redundant.

> +      src_symbol = i.op[0].disps->X_add_symbol;
> +      ginsn = ginsn_new_jump (insn_end_sym, true,
> +			      GINSN_SRC_SYMBOL, 0, src_symbol);
> +
> +      ginsn_set_where (ginsn);
> +    }
> +
> +  return ginsn;
> +}
> +
> +static ginsnS *
> +x86_ginsn_jump_cond (const symbolS *insn_end_sym)
> +{
> +  ginsnS *ginsn = NULL;
> +  symbolS *src_symbol;
> +
> +  gas_assert (i.disp_operands == 1);
> +
> +  /* A non-zero addend in JCC target makes control-flow tracking difficult.
> +     Skip SCFI for now.  */
> +  if (i.op[0].disps->X_op == O_symbol && i.op[0].disps->X_add_number)
> +    {
> +      as_bad ("SCFI: jcc insn with non-zero addend to sym not supported");
> +      return ginsn;
> +    }
> +
> +  if (i.op[0].disps->X_op == O_symbol)
> +    {
> +      gas_assert (i.op[0].disps->X_add_number == 0);
> +      src_symbol = i.op[0].disps->X_add_symbol;
> +      ginsn = ginsn_new_jump_cond (insn_end_sym, true,
> +				   GINSN_SRC_SYMBOL, 0, src_symbol);
> +      ginsn_set_where (ginsn);
> +    }
> +
> +  return ginsn;
> +}

This looks almost identical to x86_ginsn_jump() - can't the two be
folded?

> +static ginsnS *
> +x86_ginsn_enter (const symbolS *insn_end_sym)
> +{
> +  ginsnS *ginsn = NULL;
> +  ginsnS *ginsn_next = NULL;
> +  ginsnS *ginsn_last = NULL;
> +
> +  gas_assert (i.imm_operands == 2);
> +
> +  /* For non-zero size operands, bail out as untraceable for SCFI.  */
> +  if ((i.op[0].imms->X_op != O_constant || i.op[0].imms->X_add_symbol != 0)
> +      || (i.op[1].imms->X_op != O_constant || i.op[1].imms->X_add_symbol != 0))

While the comment makes sufficiently clear what's meant, the use of (inner)
parentheses here is still confusing as to whether indeed the || are meant.

> +    {
> +      as_bad ("SCFI: enter insn with non-zero operand not supported");
> +      return ginsn;
> +    }
> +
> +  /* If the nesting level is 0, the processor pushes the frame pointer from
> +     the BP/EBP/RBP register onto the stack, copies the current stack
> +     pointer from the SP/ESP/RSP register into the BP/EBP/RBP register, and
> +     loads the SP/ESP/RSP register with the current stack-pointer value
> +     minus the value in the size operand.  */
> +  ginsn = ginsn_new_sub (insn_end_sym, false,
> +			 GINSN_SRC_REG, REG_SP, 0,
> +			 GINSN_SRC_IMM, 0, 8,

I guess 8 is the operand size and you simply hope no-one's going to use
a 16-bit ENTER?

> +			 GINSN_DST_REG, REG_SP, 0);
> +  ginsn_set_where (ginsn);
> +  ginsn_next = ginsn_new_store (insn_end_sym, false,
> +				GINSN_SRC_REG, REG_FP,
> +				GINSN_DST_INDIRECT, REG_SP, 0);
> +  ginsn_set_where (ginsn_next);
> +  gas_assert (!ginsn_link_next (ginsn, ginsn_next));
> +  ginsn_last = ginsn_new_mov (insn_end_sym, false,
> +			      GINSN_SRC_REG, REG_SP, 0,
> +			      GINSN_DST_REG, REG_FP, 0);
> +  ginsn_set_where (ginsn_last);
> +  gas_assert (!ginsn_link_next (ginsn_next, ginsn_last));
> +
> +  return ginsn;
> +}
> +
> +static bool
> +x86_ginsn_safe_to_skip (void)
> +{
> +  bool skip_p = false;
> +  uint16_t opcode = i.tm.base_opcode;
> +
> +  switch (opcode)
> +    {
> +    case 0x39:

This isn't the only CMP encoding, and ...

> +      if (i.tm.opcode_space != SPACE_BASE)
> +	break;
> +      /* cmp reg, reg.  */
> +      skip_p = true;
> +      break;
> +    case 0x85:

... this isn't the only TEST one.

> +      /* test reg, reg/mem.  */
> +      if (i.tm.opcode_space != SPACE_BASE)
> +	break;
> +      skip_p = true;
> +      break;
> +    default:
> +      break;
> +    }
> +
> +  return skip_p;
> +}
> +
> +#define X86_GINSN_UNHANDLED_NONE      0
> +#define X86_GINSN_UNHANDLED_DEST_REG  1
> +#define X86_GINSN_UNHANDLED_CFG       2
> +#define X86_GINSN_UNHANDLED_STACKOP   3
> +
> +/* Check the input insn for its impact on the correctness of the synthesized
> +   CFI.  Returns an error code to the caller.  */
> +
> +static int
> +x86_ginsn_unhandled (void)
> +{
> +  int err = X86_GINSN_UNHANDLED_NONE;
> +  const reg_entry *reg_op;
> +  unsigned int dw2_regnum;
> +
> +  /* Keep an eye out for instructions affecting control flow.  */
> +  if (i.tm.opcode_modifier.jump)
> +    err = X86_GINSN_UNHANDLED_CFG;
> +  /* Also, for any instructions involving an implicit update to the stack
> +     pointer.  */
> +  else if (i.tm.opcode_modifier.implicitstackop)
> +    err = X86_GINSN_UNHANDLED_STACKOP;
> +  /* Finally, also check if the missed instructions are affecting REG_SP or
> +     REG_FP.  The destination operand is the last at all stages of assembly
> +     (due to following AT&T syntax layout in the internal representation).  In
> +     case of Intel syntax input, this still remains true as swap_operands ()
> +     is done by now.
> +     PS: These checks do not involve index / base reg, as indirect memory
> +     accesses via REG_SP or REG_FP do not affect SCFI correctness.
> +     (Also note these instructions are candidates for other ginsn generation
> +     modes in future.  TBD_GINSN_GEN_NOT_SCFI.)  */
> +  else if (i.operands && i.reg_operands
> +	   && !(i.flags[i.operands - 1] & Operand_Mem))
> +    {
> +      reg_op = i.op[i.operands - 1].regs;
> +      if (reg_op)
> +	{
> +	  dw2_regnum = ginsn_dw2_regnum (reg_op);
> +	  if (dw2_regnum == REG_SP || dw2_regnum == REG_FP)
> +	    err = X86_GINSN_UNHANDLED_DEST_REG;
> +	}

else
  err = X86_GINSN_UNHANDLED_CONFUSED;

? You can't let this case go silently. An alternative would be to
assert instead of using if().

> +    }
> +
> +  return err;
> +}
> +
> +/* Generate one or more generic GAS instructions, a.k.a, ginsns for the current
> +   machine instruction.
> +
> +   Returns the head of linked list of ginsn(s) added, if success; Returns NULL
> +   if failure.
> +
> +   The input ginsn_gen_mode GMODE determines the set of minimal necessary
> +   ginsns necessary for correctness of any passes applicable for that mode.
> +   For supporting the GINSN_GEN_SCFI generation mode, following is the list of
> +   machine instructions that must be translated into the corresponding ginsns
> +   to ensure correctness of SCFI:
> +     - All instructions affecting the two registers that could potentially
> +       be used as the base register for CFA tracking.  For SCFI, the base
> +       register for CFA tracking is limited to REG_SP and REG_FP only for
> +       now.
> +     - All change of flow instructions: conditional and unconditional branches,
> +       call and return from functions.
> +     - All instructions that can potentially be a register save / restore
> +       operation.

This could do with being more fine grained, as "potentially" is pretty vague,
and (as per earlier version review comments) my take on this is a much wider
set than yours.

> +     - All instructions that perform stack manipulation implicitly: the CALL,
> +       RET, PUSH, POP, ENTER, and LEAVE instructions.
> +
> +   The function currently supports GINSN_GEN_SCFI ginsn generation mode only.
> +   To support other generation modes will require work on this target-specific
> +   process of creation of ginsns:
> +     - Some of such places are tagged with TBD_GINSN_GEN_NOT_SCFI to serve as
> +       possible starting points.

Oh, I see you're not meaning to have this annotation consistently. That's a
little sad ...

> +     - Also note that ginsn representation may need enhancements.  Specifically,
> +       note some TBD_GINSN_INFO_LOSS and TBD_GINSN_REPRESENTATION_LIMIT markers.
> +   */
> +
> +static ginsnS *
> +x86_ginsn_new (const symbolS *insn_end_sym, enum ginsn_gen_mode gmode)
> +{
> +  int err = 0;
> +  uint16_t opcode;
> +  unsigned int dw2_regnum;
> +  ginsnS *ginsn = NULL;
> +  ginsnS *ginsn_next = NULL;
> +  ginsnS *ginsn_last = NULL;
> +  /* In 64-bit mode, the default stack update size is 8 bytes.  */
> +  int stack_opnd_size = 8;
> +
> +  /* Currently supports generation of selected ginsns, sufficient for
> +     the use-case of SCFI only.  */
> +  if (gmode != GINSN_GEN_SCFI)
> +    return ginsn;
> +
> +  opcode = i.tm.base_opcode;
> +
> +  switch (opcode)
> +    {
> +    case 0x1:
> +      /* add reg, reg/mem.  */
> +    case 0x29:
> +      if (i.tm.opcode_space != SPACE_BASE)
> +	break;

What about the new APX NDD encodings in EVEX map 4?

> +      /* sub reg, reg/mem.  */

Please be careful with placing such comments when there are multiple
case labels (or fall-through). I think these would better go on the
same lines as the case labels themselves.

> +      ginsn = x86_ginsn_addsub_reg_mem (insn_end_sym);
> +      break;
> +
> +    case 0x3:
> +      /* add reg/mem, reg.  */
> +    case 0x2b:
> +      if (i.tm.opcode_space != SPACE_BASE)
> +	break;
> +      /* sub reg/mem, reg.  */
> +      ginsn = x86_ginsn_addsub_mem_reg (insn_end_sym);
> +      break;
> +
> +    case 0xa0:
> +    case 0xa8:
> +      /* push fs / push gs have opcode_space == SPACE_0F.  */
> +      if (i.tm.opcode_space != SPACE_0F)
> +	break;
> +      dw2_regnum = ginsn_dw2_regnum (i.op[0].regs);
> +      /* In 64-bit mode, presence of 66H prefix indicates 16-bit op.  */
> +      if (flag_code == CODE_64BIT && ginsn_prefix_66H_p (i))
> +	stack_opnd_size = 2;

But only if there's not also REX.W / REX2.W.

> +      /* push fs / push gs.  */
> +      ginsn = ginsn_new_sub (insn_end_sym, false,
> +			     GINSN_SRC_REG, REG_SP, 0,
> +			     GINSN_SRC_IMM, 0, stack_opnd_size,
> +			     GINSN_DST_REG, REG_SP, 0);
> +      ginsn_set_where (ginsn);
> +      ginsn_next = ginsn_new_store (insn_end_sym, false,
> +				    GINSN_SRC_REG, dw2_regnum,
> +				    GINSN_DST_INDIRECT, REG_SP, 0);
> +      ginsn_set_where (ginsn_next);
> +      gas_assert (!ginsn_link_next (ginsn, ginsn_next));
> +      break;
> +
> +    case 0xa1:
> +    case 0xa9:
> +      /* pop fs / pop gs have opcode_space == SPACE_0F.  */
> +      if (i.tm.opcode_space != SPACE_0F)
> +	break;
> +      dw2_regnum = ginsn_dw2_regnum (i.op[0].regs);
> +      /* In 64-bit mode, presence of 66H prefix indicates 16-bit op.  */
> +      if (flag_code == CODE_64BIT && ginsn_prefix_66H_p (i))
> +	stack_opnd_size = 2;
> +      /* pop fs / pop gs.  */
> +      ginsn = ginsn_new_load (insn_end_sym, false,
> +			      GINSN_SRC_INDIRECT, REG_SP, 0,
> +			      GINSN_DST_REG, dw2_regnum);
> +      ginsn_set_where (ginsn);
> +      ginsn_next = ginsn_new_add (insn_end_sym, false,
> +				  GINSN_SRC_REG, REG_SP, 0,
> +				  GINSN_SRC_IMM, 0, stack_opnd_size,
> +				  GINSN_DST_REG, REG_SP, 0);
> +      ginsn_set_where (ginsn_next);
> +      gas_assert (!ginsn_link_next (ginsn, ginsn_next));
> +      break;
> +
> +    case 0x50 ... 0x57:
> +      if (i.tm.opcode_space != SPACE_BASE)
> +	break;
> +      /* push reg.  */
> +      dw2_regnum = ginsn_dw2_regnum (i.op[0].regs);
> +      /* In 64-bit mode, presence of 66H prefix indicates 16-bit op.  */
> +      if (flag_code == CODE_64BIT && ginsn_prefix_66H_p (i))
> +	stack_opnd_size = 2;
> +      ginsn = ginsn_new_sub (insn_end_sym, false,
> +			     GINSN_SRC_REG, REG_SP, 0,
> +			     GINSN_SRC_IMM, 0, stack_opnd_size,
> +			     GINSN_DST_REG, REG_SP, 0);
> +      ginsn_set_where (ginsn);
> +      ginsn_next = ginsn_new_store (insn_end_sym, false,
> +				    GINSN_SRC_REG, dw2_regnum,
> +				    GINSN_DST_INDIRECT, REG_SP, 0);
> +      ginsn_set_where (ginsn_next);
> +      gas_assert (!ginsn_link_next (ginsn, ginsn_next));
> +      break;
> +
> +    case 0x58 ... 0x5f:
> +      if (i.tm.opcode_space != SPACE_BASE)
> +	break;
> +      /* pop reg.  */
> +      dw2_regnum = ginsn_dw2_regnum (i.op[0].regs);
> +      ginsn = ginsn_new_load (insn_end_sym, false,
> +			      GINSN_SRC_INDIRECT, REG_SP, 0,
> +			      GINSN_DST_REG, dw2_regnum);
> +      ginsn_set_where (ginsn);
> +      /* In 64-bit mode, presence of 66H prefix indicates 16-bit op.  */
> +      if (flag_code == CODE_64BIT && ginsn_prefix_66H_p (i))
> +	stack_opnd_size = 2;
> +      ginsn_next = ginsn_new_add (insn_end_sym, false,
> +				  GINSN_SRC_REG, REG_SP, 0,
> +				  GINSN_SRC_IMM, 0, stack_opnd_size,
> +				  GINSN_DST_REG, REG_SP, 0);
> +      ginsn_set_where (ginsn_next);
> +      gas_assert (!ginsn_link_next (ginsn, ginsn_next));
> +      break;
> +
> +    case 0x6a:
> +    case 0x68:
> +      if (i.tm.opcode_space != SPACE_BASE)
> +	break;
> +      /* push imm8/imm16/imm32.  */
> +      if (opcode == 0x6a)
> +	stack_opnd_size = 1;
> +      /* In 64-bit mode, presence of 66H prefix indicates 16-bit op.
> +	 However, this prefix may only be present when opcode is 0x68.  */

Why would this be limited to opcode 0x68?

> +      else if (flag_code == CODE_64BIT && ginsn_prefix_66H_p (i))
> +	stack_opnd_size = 2;
> +      else
> +	stack_opnd_size = 4;

In 64-bit mode stack operations are never 32-bit.

> +      /* Skip getting the value of imm from machine instruction
> +	 because this is not important for SCFI.  */
> +      ginsn = ginsn_new_sub (insn_end_sym, false,
> +			     GINSN_SRC_REG, REG_SP, 0,
> +			     GINSN_SRC_IMM, 0, stack_opnd_size,
> +			     GINSN_DST_REG, REG_SP, 0);
> +      ginsn_set_where (ginsn);
> +      ginsn_next = ginsn_new_store (insn_end_sym, false,
> +				    GINSN_SRC_IMM, 0,
> +				    GINSN_DST_INDIRECT, REG_SP, 0);
> +      ginsn_set_where (ginsn_next);
> +      gas_assert (!ginsn_link_next (ginsn, ginsn_next));
> +      break;
> +
> +    case 0x70 ... 0x7f:
> +      if (i.tm.opcode_space != SPACE_BASE)
> +	break;
> +      ginsn = x86_ginsn_jump_cond (insn_end_sym);
> +      break;

I think this wants a comment briefly explaining why SPACE_0F opcodes
0x8[0-f] don't need handling explicitly. Same for JMP (0xeb) below.

> +    case 0x80:
> +    case 0x81:
> +    case 0x83:
> +      if (i.tm.opcode_space != SPACE_BASE)
> +	break;
> +      ginsn = x86_ginsn_alu_imm (insn_end_sym);
> +      break;
> +
> +    case 0x8a:
> +    case 0x8b:
> +      /* Move reg/mem, reg (8/16/32/64).  */
> +    case 0x88:
> +    case 0x89:
> +      if (i.tm.opcode_space != SPACE_BASE)
> +	break;
> +      /* mov reg, reg/mem. (8/16/32/64).  */
> +      ginsn = x86_ginsn_move (insn_end_sym);
> +      break;
> +
> +    case 0x8d:
> +      if (i.tm.opcode_space != SPACE_BASE)
> +	break;
> +      /* lea disp(%src), %dst */

disp(%src) doesn't really represent the full set of possibilities.
Why not use "mem" as you do elsewhere?

> +      ginsn = x86_ginsn_lea (insn_end_sym);
> +      break;
> +
> +    case 0x8f:
> +      if (i.tm.opcode_space != SPACE_BASE)
> +	break;
> +      /* pop to mem.  */

Or to register. While this won't happen today, allowing a means to
have the programmer request that alternative encoding would surely
miss to update the code here. Hence this code would better be ready
to deal with the case right away.

> +      gas_assert (i.base_reg);

POP isn't different from other explicit memory accesses: All forms
are allowed, and hence a base register may not be in use.

Both remarks also apply to PUSH further down.

> +      dw2_regnum = ginsn_dw2_regnum (i.base_reg);
> +      ginsn = ginsn_new_load (insn_end_sym, false,
> +			      GINSN_SRC_INDIRECT, REG_SP, 0,
> +			      GINSN_DST_INDIRECT, dw2_regnum);
> +      ginsn_set_where (ginsn);
> +      /* In 64-bit mode, presence of 66H prefix indicates 16-bit op.  */
> +      if (flag_code == CODE_64BIT && ginsn_prefix_66H_p (i))
> +	stack_opnd_size = 2;
> +      ginsn_next = ginsn_new_add (insn_end_sym, false,
> +				  GINSN_SRC_REG, REG_SP, 0,
> +				  GINSN_SRC_IMM, 0, stack_opnd_size,
> +				  GINSN_DST_REG, REG_SP, 0);
> +      ginsn_set_where (ginsn_next);
> +      gas_assert (!ginsn_link_next (ginsn, ginsn_next));
> +      break;
> +
> +    case 0x9c:
> +      if (i.tm.opcode_space != SPACE_BASE)
> +	break;
> +      /* pushf / pushfq.  */
> +      /* In 64-bit mode, presence of 66H prefix indicates 16-bit op.  */
> +      if (flag_code == CODE_64BIT && ginsn_prefix_66H_p (i))
> +	stack_opnd_size = 2;
> +      ginsn = ginsn_new_sub (insn_end_sym, false,
> +			     GINSN_SRC_REG, REG_SP, 0,
> +			     GINSN_SRC_IMM, 0, stack_opnd_size,
> +			     GINSN_DST_REG, REG_SP, 0);
> +      ginsn_set_where (ginsn);
> +      /* Tracking EFLAGS register by number is not necessary.  */

How does this fit with ...

> +      ginsn_next = ginsn_new_store (insn_end_sym, false,
> +				    GINSN_SRC_IMM, 0,
> +				    GINSN_DST_INDIRECT, REG_SP, 0);
> +      ginsn_set_where (ginsn_next);
> +      gas_assert (!ginsn_link_next (ginsn, ginsn_next));
> +      break;
> +
> +    case 0x9d:
> +      if (i.tm.opcode_space != SPACE_BASE)
> +	break;
> +      /* popf / popfq.  */
> +      /* In 64-bit mode, presence of 66H prefix indicates 16-bit op.  */
> +      if (flag_code == CODE_64BIT && ginsn_prefix_66H_p (i))
> +	stack_opnd_size = 2;
> +      /* FIXME - hardcode the actual DWARF reg number value.  As for SCFI
> +	 correctness, although this behaves simply a placeholder value; its
> +	 just clearer if the value is correct.  */
> +      dw2_regnum = 49;

... this?

> +      ginsn = ginsn_new_load (insn_end_sym, false,
> +			      GINSN_SRC_INDIRECT, REG_SP, 0,
> +			      GINSN_DST_REG, dw2_regnum);
> +      ginsn_set_where (ginsn);
> +      ginsn_next = ginsn_new_add (insn_end_sym, false,
> +				  GINSN_SRC_REG, REG_SP, 0,
> +				  GINSN_SRC_IMM, 0, stack_opnd_size,
> +				  GINSN_DST_REG, REG_SP, 0);
> +      ginsn_set_where (ginsn_next);
> +      gas_assert (!ginsn_link_next (ginsn, ginsn_next));
> +      break;
> +
> +    case 0xff:
> +      if (i.tm.opcode_space != SPACE_BASE)
> +	break;
> +      /* push from mem.  */
> +      if (i.tm.extension_opcode == 6)
> +	{
> +	  /* In 64-bit mode, presence of 66H prefix indicates 16-bit op.  */
> +	  if (flag_code == CODE_64BIT && ginsn_prefix_66H_p (i))
> +	    stack_opnd_size = 2;
> +	  ginsn = ginsn_new_sub (insn_end_sym, false,
> +				 GINSN_SRC_REG, REG_SP, 0,
> +				 GINSN_SRC_IMM, 0, stack_opnd_size,
> +				 GINSN_DST_REG, REG_SP, 0);
> +	  ginsn_set_where (ginsn);
> +	  /* These instructions have no imm, only indirect access.  */
> +	  gas_assert (i.base_reg);
> +	  dw2_regnum = ginsn_dw2_regnum (i.base_reg);
> +	  ginsn_next = ginsn_new_store (insn_end_sym, false,
> +					GINSN_SRC_INDIRECT, dw2_regnum,
> +					GINSN_DST_INDIRECT, REG_SP, 0);
> +	  ginsn_set_where (ginsn_next);
> +	  gas_assert (!ginsn_link_next (ginsn, ginsn_next));
> +	}
> +      else if (i.tm.extension_opcode == 4)
> +	{
> +	  /* jmp r/m.  E.g., notrack jmp *%rax.  */
> +	  if (i.reg_operands)
> +	    {
> +	      dw2_regnum = ginsn_dw2_regnum (i.op[0].regs);
> +	      ginsn = ginsn_new_jump (insn_end_sym, true,
> +				      GINSN_SRC_REG, dw2_regnum, NULL);
> +	      ginsn_set_where (ginsn);
> +	    }
> +	  else if (i.mem_operands && i.index_reg)
> +	    {
> +	      /* jmp    *0x0(,%rax,8).  */
> +	      dw2_regnum = ginsn_dw2_regnum (i.index_reg);
> +	      ginsn = ginsn_new_jump (insn_end_sym, true,
> +				      GINSN_SRC_REG, dw2_regnum, NULL);
> +	      ginsn_set_where (ginsn);

What if both base and index are in use? Like for PUSH/POP, all addressing
forms are permitted here and ...

> +	    }
> +	  else if (i.mem_operands && i.base_reg)
> +	    {
> +	      dw2_regnum = ginsn_dw2_regnum (i.base_reg);
> +	      ginsn = ginsn_new_jump (insn_end_sym, true,
> +				      GINSN_SRC_REG, dw2_regnum, NULL);
> +	      ginsn_set_where (ginsn);
> +	    }
> +	}
> +      else if (i.tm.extension_opcode == 2)
> +	{
> +	  /* 0xFF /2 (call).  */
> +	  if (i.reg_operands)
> +	    {
> +	      dw2_regnum = ginsn_dw2_regnum (i.op[0].regs);
> +	      ginsn = ginsn_new_call (insn_end_sym, true,
> +				      GINSN_SRC_REG, dw2_regnum, NULL);
> +	      ginsn_set_where (ginsn);
> +	    }
> +	  else if (i.mem_operands && i.base_reg)
> +	    {
> +	      dw2_regnum = ginsn_dw2_regnum (i.base_reg);
> +	      ginsn = ginsn_new_call (insn_end_sym, true,
> +				      GINSN_SRC_REG, dw2_regnum, NULL);
> +	      ginsn_set_where (ginsn);
> +	    }

... here.

> +	}
> +      break;
> +
> +    case 0xc2:
> +    case 0xc3:
> +      if (i.tm.opcode_space != SPACE_BASE)
> +	break;
> +      /* Near ret.  */
> +      ginsn = ginsn_new_return (insn_end_sym, true);
> +      ginsn_set_where (ginsn);
> +      break;

No tracking of the stack pointer adjustment?

> +    case 0xc8:
> +      if (i.tm.opcode_space != SPACE_BASE)
> +	break;
> +      /* enter.  */
> +      ginsn = x86_ginsn_enter (insn_end_sym);
> +      break;
> +
> +    case 0xc9:
> +      if (i.tm.opcode_space != SPACE_BASE)
> +	break;
> +      /* The 'leave' instruction copies the contents of the RBP register
> +	 into the RSP register to release all stack space allocated to the
> +	 procedure.  */
> +      ginsn = ginsn_new_mov (insn_end_sym, false,
> +			     GINSN_SRC_REG, REG_FP, 0,
> +			     GINSN_DST_REG, REG_SP, 0);
> +      ginsn_set_where (ginsn);
> +      /* Then it restores the old value of the RBP register from the stack.  */
> +      ginsn_next = ginsn_new_load (insn_end_sym, false,
> +				   GINSN_SRC_INDIRECT, REG_SP, 0,
> +				   GINSN_DST_REG, REG_FP);
> +      ginsn_set_where (ginsn_next);
> +      gas_assert (!ginsn_link_next (ginsn, ginsn_next));
> +      ginsn_last = ginsn_new_add (insn_end_sym, false,
> +				  GINSN_SRC_REG, REG_SP, 0,
> +				  GINSN_SRC_IMM, 0, 8,

Same comment as for ENTER wrt operand size.

> +				  GINSN_DST_REG, REG_SP, 0);
> +      ginsn_set_where (ginsn_next);
> +      gas_assert (!ginsn_link_next (ginsn_next, ginsn_last));
> +      break;
> +
> +    case 0xe0 ... 0xe2:
> +      /* loop / loope / loopne.  */
> +    case 0xe3:
> +      /* jecxz / jrcxz.  */
> +      if (i.tm.opcode_space != SPACE_BASE)
> +	break;
> +      ginsn = x86_ginsn_jump_cond (insn_end_sym);
> +      ginsn_set_where (ginsn);
> +      break;
> +
> +    case 0xe8:
> +      if (i.tm.opcode_space != SPACE_BASE)
> +	break;
> +      /* PS: SCFI machinery does not care about which func is being
> +	 called.  OK to skip that info.  */
> +      ginsn = ginsn_new_call (insn_end_sym, true,
> +			      GINSN_SRC_SYMBOL, 0, NULL);
> +      ginsn_set_where (ginsn);
> +      break;

Again - what about the stack pointer adjustment? Or wait - you only
care about the local function's view. Just that this will get you
in trouble for something like

	call	1f
1:
	pop	%rax

CALL with zero displacement really acts as if "push %rip".

> +    case 0xeb:
> +      /* If opcode_space != SPACE_BASE, this is not a jmp insn.  Skip it
> +	 for GINSN_GEN_SCFI.  */
> +      if (i.tm.opcode_space != SPACE_BASE)
> +	break;
> +      /* Unconditional jmp.  */
> +      ginsn = x86_ginsn_jump (insn_end_sym);
> +      ginsn_set_where (ginsn);
> +      break;
> +
> +    default:
> +      /* TBD_GINSN_GEN_NOT_SCFI: Skip all other opcodes uninteresting for
> +	 GINSN_GEN_SCFI mode.  */
> +      break;
> +    }
> +
> +  if (!ginsn && !x86_ginsn_safe_to_skip ())
> +    {
> +      /* For all unhandled insns that are not whitelisted, check that they do
> +	 not impact SCFI correctness.  */
> +      err = x86_ginsn_unhandled ();
> +      switch (err)
> +	{
> +	case X86_GINSN_UNHANDLED_NONE:
> +	  break;
> +	case X86_GINSN_UNHANDLED_DEST_REG:
> +	  /* Not all writes to REG_FP are harmful in context of SCFI.  Simply
> +	     generate a GINSN_TYPE_OTHER with destination set to the
> +	     appropriate register.  The SCFI machinery will bail out if this
> +	     ginsn affects SCFI correctness.  */
> +	  dw2_regnum = ginsn_dw2_regnum (i.op[i.operands - 1].regs);
> +	  ginsn = ginsn_new_other (insn_end_sym, true,
> +				   GINSN_SRC_IMM, 0,
> +				   GINSN_SRC_IMM, 0,
> +				   GINSN_DST_REG, dw2_regnum);
> +	  ginsn_set_where (ginsn);
> +	  break;
> +	case X86_GINSN_UNHANDLED_CFG:
> +	  /* Fall through.  */
> +	case X86_GINSN_UNHANDLED_STACKOP:

No fall-through comment please between immediately successive case labels.

> +	  as_bad (_("SCFI: unhandled op 0x%x may cause incorrect CFI"),
> +		  i.tm.base_opcode);

As a remark: %#x is a one byte shorter representation with largely the
same effect (plus, nicely imo, omitting the 0x when the value is zero).

> +	  break;
> +	default:
> +	  abort ();
> +	  break;
> +	}
> +    }
> +
> +  return ginsn;
> +}
> +
>  /* This is the guts of the machine-dependent assembler.  LINE points to a
>     machine dependent instruction.  This function is supposed to emit
>     the frags/bytes it assembles to.  */
> @@ -5299,6 +6272,7 @@ md_assemble (char *line)
>    const char *end, *pass1_mnem = NULL;
>    enum i386_error pass1_err = 0;
>    const insn_template *t;
> +  ginsnS *ginsn;
>    struct last_insn *last_insn
>      = &seg_info(now_seg)->tc_segment_info_data.last_insn;
>  
> @@ -5830,6 +6804,14 @@ md_assemble (char *line)
>    /* We are ready to output the insn.  */
>    output_insn (last_insn);
>  
> +  /* PS: SCFI is enabled only for AMD64 ABI.  The ABI check has been
> +     performed in i386_target_format.  */

See my earlier comment - it's yet more restrictive (as in not covering
e.g. the Windows ABI, which importantly is also used in EFI).

> +  if (flag_synth_cfi)
> +    {
> +      ginsn = x86_ginsn_new (symbol_temp_new_now (), frch_ginsn_gen_mode ());
> +      frch_ginsn_data_append (ginsn);
> +    }
> +
>    insert_lfence_after ();
>  
>    if (i.tm.opcode_modifier.isprefix)
> @@ -11333,6 +12315,7 @@ s_insn (int dummy ATTRIBUTE_UNUSED)
>    const char *end;
>    unsigned int j;
>    valueT val;
> +  ginsnS *ginsn;
>    bool vex = false, xop = false, evex = false;
>    struct last_insn *last_insn;
>  
> @@ -12104,6 +13087,14 @@ s_insn (int dummy ATTRIBUTE_UNUSED)
>    last_insn->name = ".insn directive";
>    last_insn->file = as_where (&last_insn->line);
>  
> +  /* PS: SCFI is enabled only for AMD64 ABI.  The ABI check has been
> +     performed in i386_target_format.  */
> +  if (flag_synth_cfi)
> +    {
> +      ginsn = x86_ginsn_new (symbol_temp_new_now (), frch_ginsn_gen_mode ());

If you really want to use this function here, more cases will need
handling (perhaps even beyond what I've commented on above). However,
I'd strongly suggest splitting off the "unhandled" part of that
function, and using only that here. After all you hardly know what
exactly the programmer's intentions are. Because of that, you may
also want to consider simply forbidding use of .insn when SCFI is to
be generated.

> +      frch_ginsn_data_append (ginsn);
> +    }
> +
>   done:
>    *saved_ilp = saved_char;
>    input_line_pointer = line;
> @@ -15748,6 +16739,11 @@ i386_target_format (void)
>    else
>      as_fatal (_("unknown architecture"));
>  
> +#if defined (OBJ_ELF) || defined (OBJ_MAYBE_ELF)
> +  if (flag_synth_cfi && x86_elf_abi != X86_64_ABI)
> +    as_fatal (_("Synthesizing CFI is not supported for this ABI"));
> +#endif

Elsewhere I've raised the question of whether we should really check
OBJ_MAYBE_ELF anywhere in this file. However, as long as we do, you'll
need to accompany that with an IS_ELF check in the if(). If, to
address my unreachable code comment near the top, you elect to add
further OBJ_ELF / OBJ_MAYBE_ELF checks, there you then wouldn't need
that further check (as flag_synth_cfi set then implies ELF).

Jan