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

[Richard Sandiford <richard.sandiford@arm.com>]

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).

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,
Richard