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

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

On 12/19/23 23:51, Jan Beulich wrote:
> On 19.12.2023 22:02, Indu Bhagat wrote:
>> On 12/18/23 00:47, Jan Beulich wrote:
>>> 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 ().
> 
> That would cover only the REX2 subset of additions. The promoted-to-EVEX
> insns would need covering as well.
> 

Since I worked out 2b, I have omitted these checks from V4.

>> (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.
> 
> We may certainly hope that any new control flow insns would also have
> this attribute set.
> 
> This reminds me of another guard you may need, though: Use of .byte to
> hand-craft insns.
> 

I have added a thunk in V4 to not allow .byte usage with --scfi.

>> 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
>>        }
> 
> The condition here isn't sufficient (the destination could also be a
> memory operand), but something along these lines would certainly
> address one of my fundamental concerns.
> 
>> 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).
> 
> While I'm generally hesitant to see new attributes added, if one's
> needed for a clear purpose (like looks to be the case here), that's
> certainly fine. Much will depend on people (at least one of submitter
> or reviewer) remembering to (ask to) add such an attribute whenever
> needed.
> 
>> 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.
> 
> If 2b was properly in place, I could probably be convinced to agree with
> your work going in ahead of the APX stuff. It's in particular unclear to
> me in how far APX is really going to make 2.42, considering how much of
> a change it is, and how hard is has been so far to review the changes. It
> would nevertheless feel better to me if your work was given some more
> time, to go in only after 2.42 was branched off. I certainly can't
> promise I will be able get around to reviewing either SCFI or APX again
> before the end of the year (more precisely: before the holidays).
> 

I have addressed 2b in the SCFI V4 series sent earlier to the mailing 
list.  I hope that the SCFI series can make it to 2.42.

Thanks for reviewing,
Indu