[PATCH 3/6] gas: aarch64: ginsn: add safeguards for reg-based stack pointer update
Indu Bhagat
indu.bhagat@oracle.com
Thu Jun 12 23:46:24 GMT 2025
On 6/10/25 3:56 AM, Alice Carlotti wrote:
> On Mon, Jun 09, 2025 at 03:10:40PM -0700, Indu Bhagat wrote:
>> 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.
>
> I think my point wasn't clear - my original observation was that we only enter
> this code block when (skip_p == false), so the assignment skip_p = false is
> reassigning the same value that skip_p already has:
>
> + if (!skip_p && opnd && (opnd->type == AARCH64_OPND_Rd
> + || opnd->type == AARCH64_OPND_Rd_SP))
> + {
> + opnd_reg = ginsn_dw2_regnum (opnd);
> + if (split_sp_update_reg_match_p (opnd_reg))
> + skip_p = false;
> + }
>
>
Ah true. Given what I wanted to accomplish with this stub, this should
have been just coded without the !skip_p check:
if (opnd && (opnd->type == AARCH64_OPND_Rd
|| opnd->type == AARCH64_OPND_Rd_SP))
>>
>> $ 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.
>
> This looks reasonable to me.
>
>>
>> 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)
>
> The sort of sequence I'm concerned about is:
>
> mov x16, 4392
> ldadd x17, x16, [x18]
> add sp, sp, x16
>
> In this example, the ldadd instruction clobbers the value of x16, but I think
> this patch only checks the first operand for otherwise unhandled instructions.
>
My bad, I didnt quite register the X[t] update when I looked at the
instruction semantics for ldadd.
So, yes, not handling ldadd is a problem in the current patchset. For
ensuring correctness when handling the split_sp_update pattern, we
should detect all loads to a register if the register is currently
involved in the split_sp_update pattern. For 8-byte loads, this problem
can be resolved by checking something like the following in
aarch64_ginsn_unhandled () and emit a GINSN_TYPE_OTHER for such ops:
if (dest && (dest->type == AARCH64_OPND_Rd
|| dest->type == AARCH64_OPND_Rd_SP
|| (dest->type == AARCH64_OPND_Rt
&& (opcode->iclass == ldst_imm9
|| opcode->iclass == ldst_pos
|| opcode->iclass == ldst_regoff
|| opcode->iclass == ldst_unscaled)
&& aarch64_opcode_subclass_p (opcode, F_LDST_LOAD))))
(We dont want to detect stores of the register, hence the check for
F_LDST_LOAD subclass.)
But...
Hmm. The problem I now see is with sub-8-byte loads in context of
split_sp_update pattern: A sub-8-byte load may affect the register of
interest and it needs to be detected. At the moment, we cannot
differentiate between loads vs stores when the size is < 8 bytes (See
how ldrb and strb are tagged with F_SUBCLASS_OTHER; This is because,
back when we added SCFI for AArch64, the simplification of classifying
sub-8-byte loads/stores with a F_SUBCLASS_OTHER was made based on the
observation that these would not be used for save/restore operations.
https://sourceware.org/pipermail/binutils/2024-June/135108.html
I think this now needs closer look to:
- reclassify the sub-8-byte loads/stores in the ldst_imm9, ldst_pos,
ldst_regoff and ldst_unscaled opclass with F_SUBCLASS_LOAD and
F_SUBCLASS_STORE instead of the current F_SUBCLASS_OTHER (or none in
case of ldst_regoff, ldst_unscaled) as a preparatory patch for this
series.
- I think classification of loadlit and lse_atomic opclass in
opcodes/aarch64-tbl.h can remain as is. It should be possible to
detect the cases of interest by, say, something like:
- if (opcode->iclass == loadlit), check for &base->operands[0]
- if if (opcode->iclass == lse_atomic && num_operands >= 2),
check for &base->operands[1] being of type AARCH64_OPND_Rt.
- Other operations with Rt that need detection ? I am unsure about
ldst_unpriv, ldstexcl yet...
More information about the Binutils
mailing list