[PATCH 3/6] gas: aarch64: ginsn: add safeguards for reg-based stack pointer update
Indu Bhagat
indu.bhagat@oracle.com
Mon Jun 9 22:10:40 GMT 2025
On 6/5/25 3:51 AM, Alice Carlotti wrote:
> On Thu, Aug 08, 2024 at 08:38:50AM -0700, Indu Bhagat wrote:
>> On aarch64, a user may update the stack pointer using a value previously
>> updated in a register, e.g.,
>>
>> ...[C1]...
>> mov x16, 4128
>> ...[C2]...
>> add/sub sp, sp, x16
>> ...[C3]...
>>
>> (Such patterns are informally referred to as SPLIT_SP_UPDATE code
>> patterns in the implementation.)
>>
>> The SCFI machinery currently does not handle these code patterns. It
>> does not track values or the data flow in these code patterns; So, it
>> bails out and issues an error:
>> "Error: SCFI: unsupported stack manipulation pattern"
>>
>> Moving forward, we will later add such capability to the SCFI machinery (in a
>> subsequent patch). Supporting SPLIT_SP_UPDATE will make SCFI more useful
>> as such code patterns seem prevalent in aarch64 targets. That said,
>> SCFI will support only those SPLIT_SP_UPDATE patterns where no control
>> flow operation is present in C2 (This will be enforced by the SCFI
>> machinery later).
>>
>> Note that, the ginsns for such mov and add/sub aarch64 instructions are
>> already being created. But currently, the ginsn creation machinery does
>> not include enough checks for safeguarding against incorrect SCFI in
>> presence of such code patterns. Specifically at the ginsn creation
>> time, we need to ensure:
>> - No writes to the involved register between the mov and add/sub go
>> unnoticed. In the simple code example above, no writes to x16 must
>> go unnoticed in code block C2, as it impacts the traceability of
>> REG_SP.
>>
>> This will allow SCFI machinery to continue to guarantee that:
>> - a non-constant increment/decrement to REG_SP make CFA untraceable
>> and hence, are disallowed for SCFI purposes.
>> - constant increment/decrement to REG_SP do not make CFA untraceable.
>>
>> The ginsn creation machinery in the aarch64 backend now uses a new
>> static variable of type struct split_sp_state. This allows to keep
>> track of the following:
>> - (#1) Registers the sight of every 'mov reg, imm' insn.
>> - (#2) If any insn writes to 'reg' _and_ no ginsn is created for it,
>> the machinery now emits a GINSN_TYPE_OTHER with dest=reg. To do
>> this, the whitelisting logic needs to be made aware of the special
>> case.
>> - (#3) If a subsequent 'add sp, sp, reg' is now seen, the ginsn
>> creation goes on as normal.
>>
>> Note that, the ginsn creation process cannot possibly know if an
>> applicable mov op will go on to serve an eventual stack update insn.
>> This will remain the job of the SCFI machinery. So, the ginsn creation
>> process _must_ treat all occurrences of a mov op equally.
>>
>> In other words, the ginsn creation process should simply make sure that
>> all write-after-write dependencies for all the interesting registers are
>> reflected in ginsns. Finally, regarding when is the right time to reset
>> the split_sp_state - currently, this is done:
>> - at each code block begin boundary via hooking on to the
>> TC_GINSN_DATA_INIT target-specific implementation.
>> Arguably, the split_sp_state could also be reset when a change of flow
>> instruction is seen (as later SCFI will implement the same restriction),
>> but the current implementation in this patch choses to not do so.
>>
>> gas/
>> * config/tc-aarch64-ginsn.c (AARCH64_MAX_INT_DWARF_REG): New
>> definition.
>> (struct split_sp_update): Likewise.
>> (split_sp_update_reg_match_p): Likewise.
>> (split_sp_update_add): Likewise.
>> (split_sp_update_reset_reg): Likewise.
>> (split_sp_update_reset_all): Likewise.
>> (aarch64_ginsn_data_init): Likewise.
>> (aarch64_ginsn_mov_imm): Add reg to split_sp_upate state.
>> (aarch64_ginsn_safe_to_skip_p): Adjust whitelisting logic.
>> (aarch64_ginsn_unhandled): A skipped ginsn with a WAW dependence
>> on a reg involved in split sp update must lead to generation of
>> GINSN_TYPE_OTHER.
>> * config/tc-aarch64.h (aarch64_ginsn_data_init): New
>> declaration.
>> (TC_GINSN_DATA_INIT): New definition.
>> ---
>> gas/config/tc-aarch64-ginsn.c | 113 +++++++++++++++++++++++++++++++++-
>> gas/config/tc-aarch64.h | 3 +
>> 2 files changed, 115 insertions(+), 1 deletion(-)
>>
>> diff --git a/gas/config/tc-aarch64-ginsn.c b/gas/config/tc-aarch64-ginsn.c
>> index 0dce2a71bd9..4eab74afc5d 100644
>> --- a/gas/config/tc-aarch64-ginsn.c
>> +++ b/gas/config/tc-aarch64-ginsn.c
>> @@ -26,6 +26,37 @@
>>
>> /* Invalid DWARF register number. Used when WZR / XZR is seen. */
>> #define GINSN_DW2_REGNUM_INVALID (~0U)
>> +/* The maximum DWARF register number for any integer register. */
>> +#define AARCH64_MAX_INT_DWARF_REG REG_SP
>> +
>> +/* Structure for additional book-keeping for code patterns where reg-based
>> + REG_SP update is done, e.g.,
>> + (A) mov r16, 4128
>> + (B) add sp, sp, r16
>> + This is informally referred to as SPLIT_SP_UPDATE code pattern in the
>> + implementation. Such book-keeping is necessary to ensure all
>> + write-after-write dependencies on x16, atleast between (A) and (B) are
>> + reflected in generated ginsns. In other words, this means:
>> + - correctly whitelist: an instruction otherwise deemed uninteresting for
>> + SCFI purposes, now should be considered necessary (because it writes to
>> + x16)
>> + - correctly create: add appropriate ginsns to convey the WAW dependence.
>> + The above two items are somewhat correlated but verbosity here helps
>> + underline that such book-keeping is necessary to ensure SCFI
>> + correctness. */
>> +
>> +struct split_sp_update
>> +{
>> + /* Whether a mov imm op to a reg has been seen. */
>> + bool mov_p;
>
> This boolean seems unnecessary - it looks like it just indicates whether
> movreg_mask is nonzero, but we can just test movreg_mask directly.
>
>> + /* Bitfield to book-keep the DWARF register numbers involved in the
>> + SPLIT_SP_UPDATE code pattern. Only integer registers are expected for
>> + such patterns. Writers to movreg_mask must mind AARCH64_MAX_INT_DWARF_REG
>> + limit. */
>> + unsigned int movreg_mask;
>> +};
>> +
>> +static struct split_sp_update split_sp;
>>
>> /* Return whether the given register number is a callee-saved register for
>> SCFI purposes.
>> @@ -97,6 +128,65 @@ ginsn_dw2_regnum_invalid_p (unsigned int opnd_reg)
>> return (opnd_reg == GINSN_DW2_REGNUM_INVALID);
>> }
>>
>> +/* Return whether the given REG matches with the movreg_mask in
>> + the SPLIT_SP_UPDATE pattern. */
>> +
>> +static bool
>> +split_sp_update_reg_match_p (unsigned int reg)
>> +{
>> + bool match_p = false;
>> +
>> + if (!split_sp.mov_p)
>> + return false;
>> +
>> + if (reg <= AARCH64_MAX_INT_DWARF_REG && (split_sp.movreg_mask & (1 << reg)))
>> + match_p = true;
>> +
>> + return match_p;
>> +}
>> +
>> +/* Add REG to the set of tracked registers involved in a potential
>> + SPLIT_SP_UPDATE update pattern. */
>> +
>> +static void
>> +split_sp_update_add (unsigned int reg)
>> +{
>> + /* The dataype of movreg_mask is unsigned int. */
>> + if (reg <= AARCH64_MAX_INT_DWARF_REG)
>> + {
>> + split_sp.mov_p = true;
>> + split_sp.movreg_mask |= (1 << reg);
>> + }
>> +}
>> +
>> +/* Reset the given REG from the set of tracked registers potentially involved
>> + in SPLIT_SP_UPDATE pattern. */
>> +
>> +static void
>> +split_sp_update_reset_reg (unsigned int reg)
>> +{
>> + /* The dataype of movreg_mask is unsigned int. */
>> + if (reg <= AARCH64_MAX_INT_DWARF_REG)
>> + split_sp.movreg_mask &= ~(1 << reg);
>> + if (!split_sp.movreg_mask)
>> + split_sp.mov_p = false;
>> +}
>> +
>> +/* Reset all state related to SPLIT_SP_UPDATE pattern. */
>> +
>> +static void
>> +split_sp_update_reset_all (void)
>> +{
>> + split_sp.mov_p = false;
>> + split_sp.movreg_mask = 0;
>> +}
>> +
>> +void
>> +aarch64_ginsn_data_init (void)
>> +{
>> + split_sp_update_reset_all ();
>> +}
>> +
>> /* Generate ginsn for addsub instructions with immediate opnd. */
>>
>> static ginsnS *
>> @@ -634,6 +724,10 @@ aarch64_ginsn_mov_imm (const symbolS *insn_end_sym)
>> {
>> src_imm = inst.reloc.exp.X_add_number;
>> src_type = GINSN_SRC_IMM;
>> + /* Only integer registers are expected here. */
>> + gas_assert (dst_reg <= AARCH64_MAX_INT_DWARF_REG);
>> + /* Update the state now. */
>> + split_sp_update_add (dst_reg);
>> }
>> else
>> /* Skip now and handle via aarch64_ginsn_unhandled () code path. */
>> @@ -703,6 +797,17 @@ aarch64_ginsn_safe_to_skip_p (void)
>> break;
>> }
>>
>> + /* Irrespective of iclass, if SPLIT_SP_UPDATE is currently ongoing
>> + and destination reg matches a previously seen mov reg, skipping
>> + this will affect SCFI correctness. Don't. */
>> + if (!skip_p && opnd && (opnd->type == AARCH64_OPND_Rd
>> + || opnd->type == AARCH64_OPND_Rd_SP))
>> + {
>
> This condition looks wrong - the '!skip_p' check makes the following assignment
> a noop.
>
Sorry I don't clearly see how this is a noop...
skip_p is initialized to false in the function, indicating any op is
_not_ safe to skip unless deemed so by the whitelisting logic in this
function. (Perhaps a better name for that variable would have been
safe_skip_p...)
Now, at this point in the function, if the instruction is not already
whitelisted (i.e., skipping it is not safe, ==> !skip_p), we check if
this instruction's destination reg matches any of those in the
split_sp_update struct.
Following is a small test to show that this block is not a noop.
$ cat test3.s
# Note: the 'mov x16, 4392' insn is removed. Hence insn #A is
# whitelisted, and no GINSN_TYPE_OTHER is generated..
.type foo, %function
foo:
add x16, x5, x2, sxtw #A
add sp, sp, x16
.size foo, .-foo
$ as --scfi=experimental -ali test3.s
test3.s: Assembler messages:
test3.s:4: Error: SCFI: unsupported stack manipulation pattern
test3.s:5: Error: SCFI: forward pass failed for func 'foo'
AARCH64 GAS test3.s page 1
1 .type foo, %function
1 ginsn: SYM FUNC_BEGIN
2 foo:
2 ginsn: SYM foo
3 ???? B0C0228B add x16, x5, x2, sxtw
4 ???? FF63308B add sp, sp, x16
4 ginsn: ADD %r31, %r16, %r31
5 .size foo, .-foo
5 ginsn: SYM FUNC_END
See how there is no GINSN_TYPE_OTHER for 'add x16, x5, x2, sxtw' as
x16 is not a register of interest. Adding ' mov x16, 4392' before
this instruction, causes x16 to become a register of interest due to
split_sp_update pattern.
$ cat test2.s
.type foo, %function
foo:
mov x16, 4392
add x16, x5, x2, sxtw
add sp, sp, x16
.size foo, .-foo
$ as --scfi=experimental -ali test2.s
test2.s: Assembler messages:
test2.s:5: Error: SCFI: unsupported stack manipulation pattern
test2.s:6: Error: SCFI: forward pass failed for func 'foo'
AARCH64 GAS test2.s page 1
1 .type foo, %function
1 ginsn: SYM FUNC_BEGIN
2 foo:
2 ginsn: SYM foo
3 ???? 102582D2 mov x16, 4392
3 ginsn: MOV 4392, %r16
4 ???? B0C0228B add x16, x5, x2, sxtw
4 ginsn: OTH 0, 0, %r16
5 ???? FF63308B add sp, sp, x16
5 ginsn: ADD %r31, %r16, %r31
6 .size foo, .-foo
6 ginsn: SYM FUNC_END
> Additionally, there are some cases where a later operand is a destination - for
> example, ldp has two destination operands, and ldadd has its register
> destination as the second operand. Are these cases already covered elsewhere?
>
ldp/stp are handled in aarch64_ginsn_ldstp () in tc-aarch64-ginsn.c.
For an ldp instruction, where we see two destination operands, we
generate multiple ginsns. Some examples can be seen in
gas/testsuite/gas/scfi/aarch64/ginsn-ldst-1.l.
E.g., for ldp d8, d9, [sp], 64
ginsns look like:
LOAD [%r31+0], %r72
LOAD [%r31+8], %r73
ADD %r31, 64, %r31
For stp d8, d9, \[sp, -64\]!
ginsns look like:
ADD %r31, -64, %r31
STORE %r72, \[%r31\+0\]
STORE %r73, \[%r31\+8\]
For, ldp q31, q30, [x0]
ginsns look like:
LOAD [%r0+0], %r95
LOAD [%r0+16], %r94
For, ldp s5, s6, [sp], 96
ginsns look like
ADD %r31, 96, %r31
etc.
For ldadd, currently we skip generating any ginsns. IIUC, ldadd cannot
be meaningfully used as register save/restore op, neither can it be used
in pre-index/post-index forms (hence, will not affect REG_SP if REG_SP
is used in addr operand). So skipping ldadd is deemed safe for SCFI
purposes. At this time, the generation of ginsn is catered to the use
case of SCFI only (ginsn_gen_mode value of GINSN_GEN_SCFI)
>> + opnd_reg = ginsn_dw2_regnum (opnd);
>> + if (split_sp_update_reg_match_p (opnd_reg))
>> + skip_p = false;
>> + }
>> +
>> return skip_p;
>> }
>>
>> @@ -800,7 +905,8 @@ aarch64_ginsn_unhandled (void)
>> }
>>
>> /* Finally, irrespective of the iclass, check if the missed instructions are
>> - affecting REG_SP or REG_FP. */
>> + affecting REG_SP or REG_FP. Also, check if the SPLIT_SP_UPDATE pattern is
>> + in effect and destination reg matches. */
>> else if (dest && (dest->type == AARCH64_OPND_Rd
>> || dest->type == AARCH64_OPND_Rd_SP))
>> {
>> @@ -808,6 +914,11 @@ aarch64_ginsn_unhandled (void)
>>
>> if (dw2_regnum == REG_SP || dw2_regnum == REG_FP)
>> err = AARCH64_GINSN_UNHANDLED_DEST_REG;
>> + else if (split_sp_update_reg_match_p (dw2_regnum))
>> + {
>> + err = AARCH64_GINSN_UNHANDLED_DEST_REG;
>> + split_sp_update_reset_reg (dw2_regnum);
>> + }
>> }
>>
>> return err;
>> diff --git a/gas/config/tc-aarch64.h b/gas/config/tc-aarch64.h
>> index 15e22436bf7..67561cd0d5f 100644
>> --- a/gas/config/tc-aarch64.h
>> +++ b/gas/config/tc-aarch64.h
>> @@ -284,6 +284,9 @@ extern void aarch64_after_parse_args (void);
>> #define SCFI_CALLEE_SAVED_REG_P(dw2reg) aarch64_scfi_callee_saved_p (dw2reg)
>> extern bool aarch64_scfi_callee_saved_p (uint32_t dw2reg_num);
>>
>> +extern void aarch64_ginsn_data_init (void);
>> +#define TC_GINSN_DATA_INIT aarch64_ginsn_data_init
>> +
>> /* Whether SFrame stack trace info is supported. */
>> extern bool aarch64_support_sframe_p (void);
>> #define support_sframe_p aarch64_support_sframe_p
>> --
>> 2.43.0
>>
More information about the Binutils
mailing list