Re: [PATCH,V3 00/13] Synthesize CFI for hand-written asm

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

On 12/18/23 00:47, Jan Beulich wrote:
> On 15.12.2023 10:13, Nick Clifton wrote:
>>> This patch series adds support in GAS to synthesize CFI for hand-written
>>> asm, acronym'd as SCFI.
>>>
>>> Previous postings:
>>>     - RFC patch series (https://sourceware.org/pipermail/binutils/2023-September/129560.html).
>>>     - V1 (https://sourceware.org/pipermail/binutils/2023-October/130163.html)
>>>     - V2 (https://sourceware.org/pipermail/binutils/2023-October/130210.html)
>>>
>> The patch series looks great to me.  Series approved - please apply (with a
>> small update to the 09 patch as already mentioned).
> 
> Please can you hold off committing the x86 part(s) of this, until I got a
> chance to look over them again? Furthermore, as expressed before, I'm wary
> of these additions going stale the minute the APX patches are committed on
> top, if your changes went in first. Despite APX work still being in flight,
> I would much prefer if that went in first, and then you re-based your work
> on top, such that the new MOV and ALU insns are covered right away.
> 

There will still remain handling the newly added APX Push/Pop 
instructions as well.

> While this is an unusual situation - new very general purpose insns aren't
> introduced frequently -, I'd also like to see more formally addressed the
> idea of ongoing support: From the original review I recall that you need
> to minimally track insns altering GPRs, in order to avoid silently
> generating bad CFI. Remember that I haven't looked at v3 yet, but as long
> as that tracking is based on specific insns rather than a generalized
> pattern, any ISA addition allowing GPRs to be altered would be at risk of
> rendering the CFI generator code stale. Yet people, once they've started
> to detect availability of this functionality, may validly expect that
> their use of the functionality won't silently break behind their backs. In
> this respect, did you consider constraining under what conditions the
> generator code may actually come into play (at least for the time being)?
> 

(Step 1) I propose that, for now, we add a check such that if any APX 
insn is seen for --scfi invocation, we bail out.  IIUC, we could check 
using the is_any_apx_rex2_encoding ().

(Step 2a) We can remove this check once there is support for all APX 
instructions for SCFI. I can add support for ginsns for APX instructions 
once the APX work is pushed.

(Step 2b) Orthogonal to supporting the APX instruction set: For SCFI, it 
is ideal to add a way to raise alert if new instructions are added in 
the three categories (For APX, I see we have additions in #2, and #3):

1. Control flow instructions
    We can detect them by checking for insn.tm.opcode_modifier.jump.

2. Operations altering GPRs
    We can detect them by checking for:
    if (insn.operands && insn.reg_operands)
      {
        reg_op = i.op[insn.operands - 1].regs;
        if (reg_op)
          check if destination reg is REG_SP/REG_FP
      }

3. Operations with implicit update to stack pointer.
    Currently we have no marker, but how about adding something like 
unsigned int implicitstackop:1 in i386_opcode_modifier ? We will also 
need to ensure this property is correctly conveyed for all existing 
instructions in i386-opc.tbl.  When new instructions are added, the SCFI 
machinery will be able to warn the user of missing functionality (and 
not generate wrong CFI).

After this support for #3 is added to the backend, we will be in good 
stead for future ISA additions wrt SCFI.

I can _try_ to accommodate 2b before the 2.42 release is cut. However, I 
think its best to plan for both 2a and 2b for 2.43 release (given the 
2.42 is around the corner), if there is agreement.

What do you think about this plan ?

Thanks