[PATCH 3/6] gas: aarch64: ginsn: add safeguards for reg-based stack pointer update
Alice Carlotti
alice.carlotti@arm.com
Mon Jul 7 11:01:44 GMT 2025
On Thu, Jun 12, 2025 at 04:46:24PM -0700, Indu Bhagat wrote:
> 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...
I think we ultimately need a more robust mechanism than the current complicated
set of checks (at least for cases where ignoring an unrecognised instruction
could give the wrong answer). One idea I've contemplated is adding flags to
indicate which registers are destination registers (when the default assumption
would be wrong). I don't have complete answers at the moment, but I'll keep
the issue in the back of my mind and see if I have any more ideas.
Alice
More information about the Binutils
mailing list