Re: [PATCH 0/2] Add SCFI support for aarch64

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

On 6/26/24 04:01, Richard Sandiford wrote:
> Hi,
> 
> I was having a look at the v3 series, but had a question about the
> design & known limitations that I thought was better to ask here:
> 
> Indu Bhagat <indu.bhagat@oracle.com> writes:
>> Known limitations
>> -----------------
>> These are planned to be worked on in the near future:
>>
>>   - The current SCFI machinery does not currently synthesize the PAC-related
>>     aarch64-specific CFI directives: .cfi_b_key_frame.  Other opcodes used when
>>     pointer authentication is enabled also need to be handled (braa, brab,
>>     retaa, etc.).
>>
>>   - Supporting the following pattern:
>>     mov x16,4266
>>     add sp, x16, sp
>>     ...
>>
>>   - Not a limitation per se, but a note that ATM, that predicated insns are
>>     skipped from ginsn translation.  IIUC, these instructions are not such that
>>     can be used alongside stack management ops. To be double-checked.
> 
> AFAICT, the current code only handles GPRs.  It doesn't handle D8-D15,
> which are also call-preserved under the base AAPCS64.  Is that right?
> I think we should try to handle those as well.

Ah yes, the current code only handles GPRS. I will need to add the 
D8-D15 registers. The code was also explicitly skipping ldstp with FP 
registers. Thanks.

> 
> D8-D15 are "interesting" because they are the low 64 bits of Q8-Q15,
> and of Z8-Z15 if SVE is used.  However, a CFI save slot always represents
> the low 64 bits, regardless of whether a save occurs on D, Q or Z registers.
> This matters for big-endian code, because there are two additional
> PCS variants:
> 
> * the "vector PCS", which preserves Q8-Q23
> * the "SVE PCS", which preserves Z8-Z23 and P3-P15
> 

Is there a way to annotate that a (hand-written asm) function adheres to 
vectors PCS or SVE PCS ?  I see that there is a .variant_pcs but that 
does not help differentiate between the above two?

I _think_ gas will need to know which of SVE vs vector PCS is in effect 
for a specific function so that the P3-P15 can be added to the set of 
callee-saved registers being tracked for SCFI for SVE PCS but not for 
vector PCS.

> So vector PCS functions might need to save and restore Q8 when returning
> normally, but the CFI only describes the save of the D8 portion (since
> that's the only portion that is preserved by exceptions).  This means
> that, on big-endian:
> 
> 	str	q8, [sp, #16]
> 
> should record D8 as being saved at sp+24 rather than sp+16.
> 
> A further complication is that STR Qn and STR Zn do not store in
> the same byte order for big-endian: STR Qn stores as a 128-bit
> integer (MSB first), whereas STR Zn stores as a stream of bytes
> (LSB first).  This means that GCC-generated big-endian SVE PCS
> functions use things like:
> 
> 	st1d	z8.d, p2, [sp, #1, mul vl]
> 
> with the D8 save slot then being at sp + 2*VL - 64.
> 
> I think it's OK to punt on the big-endian SVE PCS case for now (provided
> that there's a warning that the code isn't understood, which it looks
> like there is).  But I think it's worth handling the Q register saves.

It looks to me that using reg name / size is an unambiguous proxy to 
deciding  whether SVE PCS is in effect. Is this correct ?

> 
> Other comments:
> 
> - I like the new approach of using a combination of the iclass and a
>    "subclass" field of the flags.  How about making aarch64-gen.c enforce
>    that:
> 
>    - if aarch64-ginsn.c looks at the subclass of a particular iclass,
>      every instruction of that iclass has a nonzero subclass field
> 

(Let me refer to the above as #1). I can see that there can be ways to 
achieve this...

>    - every other instruction has a zero subclass field
> 

..but I am not sure I follow this statement. (Let me refer to the above 
as #2).

>    This would help to ensure that the data stays up to date.
>    The subclass enum could include a nonzero "other" value where
>    necessary.
> 

Currently, we are using the opcode->flags bits to encode:

In include/opcode/aarch64.h:

/* 4-bit flag field to indicate subclass of operations.
    Note that there is an (intended) overlap between the three flag sets
    (F_LDST*, F_ARITH* and F_BRANCH*).  This allows space savings.  */
#define F_LDST_LOAD (1ULL << 36)
#define F_LDST_STORE (2ULL << 36)
/* A load followed by a store (using the same address). */
#define F_LDST_SWAP (F_LDST_LOAD | F_LDST_STORE)
/* Subclasses to denote add, sub and mov insns.  */
#define F_ARITH_ADD (1ULL << 36)
#define F_ARITH_SUB (2ULL << 36)
#define F_ARITH_MOV (4ULL << 36)
/* Subclasses to denote call and ret insns.  */
#define F_BRANCH_CALL (1ULL << 36)
#define F_BRANCH_RET (2ULL << 36)

We can dedicate F_SUBCLASS_NONE (8ULL << 36) and enforce this subclass 
on all insns which use none of the above subclasses in a specific 
iclass.  This can help address (#1), but not sure about (#2).

> - I think we should only add things like F_LDST_LOAD and F_LDST_STORE
>    to instructions that are semantically simple loads and stores
>    (unless the iclass gives us the information needed to handle
>    more complicated cases).  E.g. it looks like patch 2/7 adds
>    F_LDST_LOAD to things like ld4, which are AoS->SoA loads.
>    It would not be correct to interpret an LD4 on byte elements
>    (say) as a register restore for CFI purposes.
> 
>    I realise the information could be useful for other things
>    besides ginsns.  But while ginsns are the only things using the
>    information, I think we should be careful to make sure that the
>    information can't be misunderstood.
> 

If some safeguards like #1 are placed for the specific iclasses, and 
further we only allow subclass information retrieval for selected 
iclasses in aarch64-gen.c, I think we can afford to go this route as you 
suggest: Only add subclasses for those iclasses relevant for SCFI 
purposes ATM.

Thanks for review and feedback
Indu