[PATCH 1/2] gas: aarch64: add experimental support for SCFI

[Indu Bhagat]

On 5/21/24 05:34, Richard Earnshaw (lists) wrote:
> On 11/04/2024 08:44, Indu Bhagat wrote:
>> For synthesizing CFI (SCFI) for hand-written asm, the SCFI machinery in
>> GAS works on the generic GAS insns (ginsns).  This patch adds support in
>> the aarch64 backend to create ginsns for a subset of the supported
>> machine instructions.  The subset includes the minimal necessary
>> instructions to ensure SCFI correctness:
>>
>>    - Any potential register saves and unsaves.  Hence, process
>>      instructions belonging to a variety of iclasses involving str, ldr,
>>      stp, ldp.
>>    - Any change of flow instructions.  This includes all conditional and
>>      unconditional branches, call (bl, blr, etc.) and return.
>>    - Most importantly, any instruction that could affect the two registers of
>>      interest: REG_SP, REG_FP.  This set includes all pre-indexed and
>>      post-indexed memory operations, with writeback, on the stack.  This
>>      set must also include other instructions (e.g., arithmetic insns) where
>>      the destination register is one of the afore-mentioned registers.
>>
>> Apart from creating ginsn, another key responsibility of the backend is
>> to make sure there are safeguards in place to detect and alert if an
>> instruction of interest may have been skipped.  This is done via
>> aarch64_ginsn_unhandled () (similar to the x86 backend).  This function
>> , hence, is also intended to alert when future ISA changes may otherwise
>> render SCFI results incorrect, because of missing ginsns for the newly
>> added machine instructions.
>>
>> The current SCFI machinery does not currently synthesize the
>> PAC-related, aarch64-specific CFI directives: .cfi_b_key_frame.  The
>> support for this is planned for near future.
>>
>> SCFI is enabled for ELF targets only.
>>
>> gas/
>> 	* config/tc-aarch64.c (aarch64_scfi_callee_saved_p): New
>> 	definition.
>> 	(ginsn_dw2_regnum): Likewise.
>> 	(aarch64_ginsn_addsub_imm): Likewise.
>> 	(aarch64_ginsn_addsub_reg): Likewise.
>> 	(aarch64_ginsn_ldstp): Likewise.
>> 	(aarch64_ginsn_ldstr): Likewise.
>> 	(aarch64_ginsn_jump): Likewise.
>> 	(aarch64_ginsn_jump_cond): Likewise.
>> 	(aarch64_ginsn_mov): Likewise.
>> 	(aarch64_ginsn_safe_to_skip_p): Likewise.
>> 	(AARCH64_GINSN_UNHANDLED_NONE): Likewise.
>> 	(AARCH64_GINSN_UNHANDLED_DEST_REG): Likewise.
>> 	(AARCH64_GINSN_UNHANDLED_CFG): Likewise.
>> 	(AARCH64_GINSN_UNHANDLED_STACKOP): Likewise.
>> 	(AARCH64_GINSN_UNHANDLED_UNEXPECTED): Likewise.
>> 	(aarch64_ginsn_unhandled): Likewise.
>> 	(aarch64_ginsn_new): Likewise.
>> 	(md_assemble): Invoke aarch64_ginsn_new.
>> 	* gas/config/tc-aarch64.h (TARGET_USE_GINSN): Define for SCFI
>> 	enablement.
>> 	(TARGET_USE_SCFI): Likewise.
>> 	(SCFI_MAX_REG_ID): New definition.
>> 	(REG_FP): Likewise.
>> 	(REG_LR): Likewise.
>> 	(REG_SP): Likewise.
>> 	(SCFI_INIT_CFA_OFFSET): Likewise.
>> 	(SCFI_CALLEE_SAVED_REG_P): Likewise.
>> 	(aarch64_scfi_callee_saved_p): New declaration.
>> ---
>>   gas/config/tc-aarch64.c | 744 ++++++++++++++++++++++++++++++++++++++++
>>   gas/config/tc-aarch64.h |  20 ++
>>   2 files changed, 764 insertions(+)
> Hi Indu,
> 
> Firstly, apologies for the delay replying.  I'm trying to work through a large backlog of arm/aarch64 patches as quickly as I can.
> 
> I haven't done an in-depth review of all the code here, but on a quick scan my major concern is the excessive use of bit-pattern matches on the opcode field.  This makes the code very unweildy and thus likely very difficult to maintain going forwards.  I think we can do a lot better than that, but it probably requires adding a bit more information to each of the relevant insn definitions.
> 
> The first thing to note is that we already have the iclass field which tells us the general shape and purpose of the insn and testing that is far preferable to trying to decode the opcodes.  But it's clear that isn't sufficient in that it doesn't tell us whether an instruction is a load or a store.  That could be fixed though by adding some additional information in the flags field of the insn.  I'd suggest something along the lines of the following:
> 
> Firstly, define a 4-bit sub-field of the flags component in include/opcode/aarch64.h (the next free bit is 36).
> 
> #define F_SUBCLASS (0xfULL << 36)
> 
> we can then define some subclass values, eg for loads and stores we could have
> F_LDST_LOAD (1ULL << 36)
> F_LDST_STORE (2ULL << 36)
> F_LDST_SWAP (3ULL << 36) // a load followed by a store using the same address
> 
> The above would be valid for any iclass that represents a load or a store operation
> 
> We can also add some subclass values for arithmetic operations as well (add/sub)
> F_ARITH_ADD (1ULL << 36)
> F_ARITH_SUB (2ULL << 36)
> etc.  Note that these overlap the definitions for the load/store subclass, so we have to be careful which iclass the insn is in when interpreting these values, but we can save a lot of flag space this way.  We can do something similar as well for the various jump/call insns.
> 

I have addressed this in V2 (will post soon). This does make the code 
more readable/maintainable.  I disliked the checks on opcode since 
beginning, but didn't have ideas on any better ways to do the task at 
hand. Thanks.

> I think once we have this it should be possible to replace the big switch on opcode->opcode in aarch64_ginsn_new() with a switch on the iclass, plus, where needed, a further test of the subclass we've just added.
> 
> My other suggestion at this time is that rather than adding this to tc-aarch64.c we create a new file for the scfi support - it doesn't look to me as though this code has a high dependency on existing private functions within tc-aarch64.c, so keeping things more modular will help with long-term maintenance.  See 'bfin' in gas/configure.ac for an example of how to do this.
> 

I created a tc-aarch64-ginsn.c and moved all ginsn creation code to there.

(BTW, currently, we create ginsns sufficient for SCFI purposes only, but 
this may change in future.  So I thought the name of tc-aarch64-ginsn.c 
is more apt as this file is not just for SCFI support.  Also, the SCFI 
algorithm is arch-neutral and sits in gas/scfi.[ch] files.)

Reg adding the new file in $extra_objects, I have not fully explored it 
yet. I wanted to get your opinion on whats posted in V2. In V2 that I 
will post, tc-aarch64.c #includes the other C file (tc-aarch64-ginsn.c). 
It appeared to me that this approach may also work out well?

ATM, there is some code sharing needed between tc-aarch64.c and the 
ginsn creation functions:
   - struct aarch64_instruction and inst object will need to be exposed,
   - bool aarch64_gas_internal_fixup_p ()
   - the list may grow slightly in future, but overall not too high 
dependency.

Perhaps the V2 patch will help make it clearer.  Let me know.

Thanks for your review.