[PATCH 0/2] Add SCFI support for aarch64

[Indu Bhagat]

On 6/27/24 02:40, Richard Sandiford wrote:
> Indu Bhagat <indu.bhagat@oracle.com> writes:
>> On 6/26/24 04:01, Richard Sandiford wrote:
>>> 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.
> 
> Only the normal base AAPCS64 register set is preserved across abnormal
> control flow (setjmp/longjmp, exceptions, etc.)  The extra call-preserved
> guarantees for vector and SVE PCS functions only apply to normal returns.
> 
> [This means, for example, that:
> 
>    void foo();
>    svbool_t f() {
>      try {
>        foo();
>      } catch (...) {};
>      return svptrue_b8();
>    }
> 
> must manually restore the additional register state when catching
> and returning normally.]
> 
> The CFI requirements therefore don't change: only D8-D15 matter,
> like for normal functions.  But that's also where the big-endian
> complications that I mentioned come from.
> 
> So I don't think the code needs to know which kind of function is
> being assembled.  The code just needs to be able to recognise Q-based
> and Z-based loads and stores of D8-D15 and work out the correct offset
> of the low 64 bits.  (Although, like I say, I think we can punt on
> big-endian SVE PCS functions.)
> 
>>> 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 ?
> 
> Not necessarily.  There's nothing stopping code from using Q-based
> loads and stores for normal functions (although it would be an
> odd choice).

Of course, I dont know what I was thinking when I wrote that.

As for Z registers, I realized that I need more time to take a look at 
the SVE insns and see what patterns need to be handled etc for memory 
offset calculation.

For V4 (to be posted soon), I have added handling for D and Q registers 
(little-endian and big-endian), but skipped Z altogether for now (SCFI 
errors out when correctness is affected).  Also added this to the set of 
known limitations to be addressed in a future patch.

>  
> There's also the possiblity of ad-hoc PCSes, but the assumption there
> too would be that only the base AAPCS64 set needs to be preserved
> through unwinding.
> 
>>> 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 the 4 bits are really an enum rather than true independent flags.
> So it might be better to use 15ULL, so that the other 14 nonzero values are
> consecutive.
> 
> But yeah, I think it addresses both #1 and #2.  #2 makes sure that a
> subclass is only present when we expect one.  If we define:
> 
> #define F_SUBCLASS (15ULL << 36)
> 
> then #2 makes sure that (flags & F_SUBCLASS) == 0 for classes that
> are not interpreted by ginsns.
> 

Thanks.  This should be addressed in the V4 series which I will be 
posting soon.

Thanks
Indu