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

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

On 1/9/24 01:30, Jan Beulich wrote:
> On 08.01.2024 20:33, Indu Bhagat wrote:
>> On 1/5/24 05:58, Jan Beulich wrote:
>>> On 03.01.2024 08:15, Indu Bhagat wrote:
>>>> @@ -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.
>>
>> Added a comment for V5.
>>
>> As stated in as.texi, usage of .type and .size will be bread and butter
>> for SCFI: "The input asm must begin with the @code{.type} directive, and
>> should ideally be closed off using a @code{.size} directive."
> 
> Except that to me "must begin" talks about a source file, not individual
> functions. Hence that wording (which I saw) is at bets ambiguous, which
> led me to ask for it to be "more precise".
> 

I have updated both code comments and gas/doc/as.texi.

Code comments:

"When using SCFI, .type directive indicates start of a new FDE for 
SCFI processing.  So, we must first demarcate the previous block of 
ginsns, if any, to mark the end of a previous FDE."

gas/doc/as.texi:
" Each input function in assembly must begin with the @code{.type} 
directive, and should ideally be closed off using a @code{.size} 
directive.  When using SCFI, each @code{.type} directive prompts GAS to 
start a new FDE (a.k.a., Function Descriptor Entry).  This implies that 
with each @code{.type} directive, a previous block of instructions, if 
any, is finalised as a distinct FDE."


>>>> +/* 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()?
>>>
>>
>> Isnt 66H_p more readable and easier to follow because that's what the
>> function is currently checking ?  If more scenarios were being handled,
>> ginsn_opsize_prefix_p () would fit better.
> 
> Well, as said - with APX you can't get away with just 0x66 prefix checking.
> That prefix is simply illegal to use with EVEX-encoded insns.
> 

I am using the following in ginsn_opsize_prefix_p ():

!(i.prefix[REX_PREFIX] & REX_W) && i.prefix[DATA_PREFIX] == 0x66

>>>> +/* 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.
>>
>> Typo it is.  I meant to write r8d. I have updated this to " like al, cl,
>> ah, ch, r8d, r20w etc."
> 
> Then perhaps further s;byte/word;byte/word/dword; ?
> 

Yes. Done.

>>>> +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.
>>
>> I think you are referring to the checks like:
>>
>>     /* 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;
>>
>> Although the check on flag_code is redundant now, I chose to have them
>> here to keep it aligned to how the prefix is meant to be used.
> 
> Yet the same is true in 32-bit mode (minus, of course, the REX.W aspect
> mentioned elsewhere, but that could be ignored here as 32-bit code has
> no way of setting that flag). IOW - either you drop the use of flag_code,
> or you make it actually correct (as in: not misleading).
> 

Dropped the use of flag_code  in these instances for V5.

>>>> +  /* 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?
>>
>> An instruction like "addq    %rax, symbol" etc are uninteresting for
>> SCFI.  One of feedback in a previous iteration was to "consider not
>> generating ginsns for cases that are known to be uninteresting".
> 
> But then why not simply
> 
>    if (i.mem_operands)
>      return ginsn;
> 
> ? What memory is added to doesn't matter for SFCI, does it? Aiui you
> only really want to notice adds to registers.
> 

Correct. Updated the code with additional comments.

>>>> +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 ...
>>>
>>
>> Yes (updated the comment for V5).
>>
>>>> +  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.
>>
>> I saw that the opcode used when the source is reg is either 0x1 or 0x29,
>> hence concluded that the handling in x86_ginsn_addsub_reg_mem should
>> suffice.  Perhaps this is a GAS implementation artifact, and I should
>> have handling for register source here in x86_ginsn_addsub_mem_reg (for
>> opcodes 0x3 and 0x2b) ?
> 
> I think you should, yes. Try using the {load} / {store} pseudo-prefixes,
> and I think you'll see these opcodes used.
> 

I see them now with the suggested pseudo prefixes. Added handling in the 
appropriate function and an additional instruction in ginsn-add-1 testcase.

>>>> +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)?
>>
>> Because there may be weird code that saves and restores 8-byte registers
>> on stack.  For ALU insns, if the destination is REG_SP/REG_FP, we will
>> detect them in the unhandled track.
> 
> You talk about 8-byte registers when I asked about byte reg moves. If
> there's a MOV targeting %spl or %bpl, is that really any different from,
> say, an ADD doing so?
> 

ATM, Yes. SCFI has heuristics implemented, _some_ of which (can be 
evolved) are opcode specific. E.g.,
   - MOV %rsp, %rbp will make SCFI machinery check if this is making the 
CFA switch to %rbp.  If the target was %bpl, since we track 8-byte 
registers, it will still trigger the same code path. A bug as I ack below.
   - ADD rsp + 0 = rbp will not trigger CFA switch to RBP. Should it ? 
Perhaps yes? Its on my TODO list (with low priority) to evolve this bit.

>>>> +    }
>>>> +
>>>> +  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.
>>
>> Operation size matters: No, not for all instructions in context of SCFI.
>> For instructions using stack (save / restore), the size is important.
>> But for other instructions, operation size will not affect SCFI correctness.
> 
> But aren't you wrongly treating an update of %rbp and an update of,
> say, %bp the same then? The latter should end up as untracable, aiui.
> 

For ALU ops:
   - ADD/SUB reg1, reg2
     If reg2 is the same as REG_CFA, then even in 64-bit mode, this 
causes untraceability. So there is untraceability irrespective of 
operation size. On the other hand, if uninteresting, it will remain 
unintersting irrespective of operation size.
   - ADD/SUB imm, reg will never trigger untraceability, irrespective of 
size. There is the element of ignoring the carry bit, but I think the 
current diagnostics around "asymmetrical save/restore" and the planned 
"unbalanced stack at return" should provide user with some safety net.
   - Other ALU ops should all trigger untracebility alike, irrespective 
of operation size.
Hence, my statement that ignoring operation size is fine here for SCFI.

For MOV ops:
   - 8-bit/16-bit MOV should trigger untracebility. I ack this as bug to 
be fixed. Thanks to your careful review. ATM, I plan to deal with this 
after the series goes in, unless you have strong opinion about this.

>>>> +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().
>>
>> No, the other cases are not important for SCFI correctness.  The case of
>> potential register save/restore operation cannot be detected
>> automatically.  Keeping an eye on ISA additions will be the only way for
>> that category.
> 
> That wasn't my point. reg_op turning out to be NULL is bogus considering
> the earlier checks. Hence why an assertion may make sense, and hence why
> otherwise I suggested an error indicator towards "internal error".
> 

Sorry, I misunderstood.  I added a new X86_GINSN_UNHANDLED_UNEXPECTED 
now for V5.

>>>> +/* 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.
>>
>> I would like to understand more on this comment, especially the "my take
>> on this is a much wider set than yours".  I see its being hinted at in
>> different flavors in the current review.
>>
>> I see some issues pointed out in this review (addressing modes of mov
>> etc, safe to skip opcodes for TEST, CMP) etc., but it seems that your
>> concerns are wider than this.
> 
> I earlier version review I mentioned that even vector or mask registers
> could in principle be use to hold preserved GPR values. I seem to recall
> that you said you wouldn't want to deal with such. Hence my use of
> "wider set": Just to give an example, "kmovq %rbp, %k0" plus later
> "kmovq %k0, %rbp" is a pair of "instructions that can potentially be a
> register save / restore operation".
> 

Hmm. I will need to understand them on a case to case basis.  For the 
case of "kmovq %rbp, %k0" / "kmovq %k0, %rbp" how can this be used as 
save/restore to/from stack ?

>>>> +    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.
>>
>> This needs to be done for both push and pop then.  I am not sure about
>> the REX2.W check though.
> 
> It's not just PUSH/POP, but all stack operations.
> 

Sorry I was imprecise.  Now checking REX.W in ginsn_opsize_prefix_p; for 
all stack related instructions.

>> Something like
>> !is_apx_rex2_encoding () && !(i.prefix[REX_PREFIX] & REX_W)
>> covers it ?
> 
> I don't see why you'd have is_apx_rex2_encoding() here. I don't recall
> where/when exactly you invoke your code. It may therefore be either, as
> you have it, i.prefix[REX_PREFIX], or (before establish_rex()) it could
> (additionally) be i.rex.
> 

This is done after output_insn () (hence after establish_rex ()).  I am 
now using the following in ginsn_opsize_prefix_p ():

!(i.prefix[REX_PREFIX] & REX_W) && i.prefix[DATA_PREFIX] == 0x66

>>>> +      /* 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?
>>
>> My understanding from the manual is that 0x6a will always be push imm8.
>> Hence, 66H is expected only with 0x68. Isnt this incorrect ?
> 
> The opcode byte determines merely the size of the immediate field.
> Operand size determines to which size the signed 8-bit field would be
> extended and then pushed onto the stack. There's never a single byte
> that would be pushed.
> 

This is now fixed for V5.

>>>> +      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.
>>
>> For the indirect jump and call instructions, the target destination is
>> not necessary to be known.  Indirect jumps will cause SCFI to error out
>> as control flow cannot be constructed.
>>
>> Call instructions are assumed to transfer control out of function.
>>
>> In other words, more information in these cases will not be of use to SCFI.
> 
> But then aren't you already doing too much work here? With what you say,
> you don't care about the kind of operand at all, merely the fact it's a
> CALL might be interesting. Albeit even there I'd then wonder why - if
> the function called isn't of interest, how is CALL different from just
> NOP? The only CALL you'd need to be concerned about would be the direct
> form with a displacement of 0, as indicated elsewhere. But of course
> tricky code might also use variants of that; see e.g. the retpoline code
> that was introduced into kernels a couple of years back. Recognizing
> and bailing on such tricky code may be desirable, if not necessary.
> 

Tracking operands for CALL instructions does not provide value ATM.  We 
do not even split a BB if there is a CALL instruction (the assumption is 
that CALL insn transfers control out of function).

You're right that we need to treat some CALLs differently (because of 
its affect of control flow and SCFI correctness).

RE: doing more work. Sure, but the vision for ginsn was to allow a 
representation where other analyses may be added. The representation of 
ginsn will need revisions to get there, but keeping an explicit 
GINSN_TYPE_CALL seems good.

>>>> +	}
>>>> +      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?
>>
>> No stack unwind information for a function is relevant after the
>> function has returned.  So, tracking of stack pointer adjustment by
>> return is not necessary.
> 
> What information does the "return" insn then carry, beyond it being
> an unconditional branch (which you have a different insn for)?
> 

"return" does not carry any more information than just the 
GINSN_TYPE_RETURN as ginsn->type.

So then why support both "return" and an unconditional branch: The 
intention is to carry the semantic difference between ret and 
unconditional jump.  Unconditional jumps may be to a label within 
function, and in those cases, we use it for some validation and BB 
linking when creating CFG. Return, OTOH, always indicates exit from 
function.

For SCFI purposes, above is the one use.  Future analyses may find other 
use-cases for an explicit return ginsn.  But IMO, keeping 
GINSN_TYPE_RETURN as an explicit insn makes the overall offering cleaner.

>>>> @@ -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).
>>
>> Not clear, can you please elaborate ?
> 
> Hmm, it's not clear to me what's not clear in my earlier comment. The
> set of preserved registers, for example, differs between the SysV and
> the Windows ABIs (see x86_scfi_callee_saved_p()). Then again, thinking
> of it, talking of anything ABI-ish in assembly code is questionable.
> An assembly function calling another assembly function may use an
> entirely custom "ABI". You just cannot guess upon preserved registers.
> 

I think the confusion is stemming from my usage of AMD64 colloquially to 
refer to System V ABI for x86_64. I think "System V AMD64 ABI" is the 
correct term. I will use that.

And yes, GAS can only work out SCFI if there is ABI adherence.  If input 
asm does not adhere to the supported ABIs, and the user invokes SCFI, 
then the user is on their own.  GAS will not (rather can not) warn about 
ABI incompliance.

>>>> @@ -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.
>>
>> Sorry, not clear to me. I am not sure how moving simply the "unhandled"
>> part of x86_ginsn_new will work better.  If we dont handle the
>> instructions, more instructions will potentially trigger the "unhandled"
>> case.
> 
> Correct. You simply shouldn't try to interpret what the user encoded
> via .insn. Much like gcc doesn't try to interpret what the string
> literal of an asm() contains.
> 

Removed the call to x86_ginsn_new () in s_insn () and replaced by an 
error if SCFI is in effect.

>>>> @@ -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).
>>
>> I took the suggestion to not compile unnecessarily for non-ELF targets.
>>
>> Use IS_ELF in if (): But flag_synth_cfi does not imply IS_ELF, it
>> implies OBJ_ELF || OBJ_MAYBE_ELF. Looks to me then, that I should
>> include IS_ELF check in each of the 3 blocks.
> 
> Why? If you use IS_ELF here, you won't make it there when IS_ELF
> returned "false", simply because you use as_fatal() here. It is
> once you pass the check here that flag_synth_cfi implies IS_ELF
> (as much as it then also implies x86_elf_abi == X86_64_ABI).
> 

Got it now.