[PATCH 5/6] gas: scfi: allow additional workflow for split sp update code pattern
Indu Bhagat
indu.bhagat@oracle.com
Mon Jun 9 22:07:25 GMT 2025
On 6/5/25 3:51 AM, Alice Carlotti wrote:
> On Thu, Aug 08, 2024 at 08:38:52AM -0700, Indu Bhagat wrote:
>> In aarch64, the following code pattern may be used for managing stack
>> statically:
>>
>> mov x16, IMM
>> add sp, sp, x16
>>
>> To allow for such a pattern, the SCFI machinery needs to book-keep some
>> more data flow and associated constants. This is done by using the
>> scratch reg state in the SCFI state object. Without this special
>> treatment to memorize the fact that x16 holds a constant, the update to
>> stack would otherwise be deemed untraceable.
>>
>> Add new APIs to manipulate state in split_sp_state and use them to allow
>> these patterns in SCFI.
>>
>> When a GINSN_TYPE_OTHER is seen, the scratch reg state corresponding to
>> the destination register is reset. When any change of flow instruction is
>> seen, the scratch reg state of all registers in state REG_SCRATCH_VALUE
>> is reset: this is to say that we do not propagate this information across
>> control flow.
>>
>> The previously present testcase of scfi-unsupported-1.s is now
>> repurposed as a testcase for a static stack usage with split sp update
>> code pattern.
>>
>> gas/
>> * scfi.c (scfi_state_scratch_reg_value_p): New definition.
>> (scfi_state_scratch_reg_get_value): Likewise.
>> (scfi_state_scratch_reg_reset): Likewise.
>> (scfi_state_scratch_reg_reset_value): Likewise.
>> (verify_heuristic_traceable_stack_manipulation): Update
>> heuristics to allow selected addsub ops with two reg operands.
>> (gen_scfi_ops): Add handling for the involved ops.
>>
>> gas/testsuite/
>> * gas/scfi/aarch64/scfi-aarch64.exp: Adjust tests.
>> * gas/scfi/aarch64/scfi-static-stack-1.s: ...here.
>> * gas/scfi/aarch64/scfi-static-stack-1.d: New test.
>> * gas/scfi/aarch64/scfi-static-stack-1.l: New test.
>> * gas/scfi/aarch64/scfi-unsupported-1.l: Remove.
>> * gas/scfi/aarch64/scfi-unsupported-1.s: Move to...
>> ---
>> gas/scfi.c | 128 +++++++++++++++---
>> .../gas/scfi/aarch64/scfi-aarch64.exp | 2 +-
>> .../gas/scfi/aarch64/scfi-static-stack-1.d | 33 +++++
>> .../gas/scfi/aarch64/scfi-static-stack-1.l | 2 +
>> ...-unsupported-1.s => scfi-static-stack-1.s} | 23 ++--
>> .../gas/scfi/aarch64/scfi-unsupported-1.l | 4 -
>> 6 files changed, 154 insertions(+), 38 deletions(-)
>> create mode 100644 gas/testsuite/gas/scfi/aarch64/scfi-static-stack-1.d
>> create mode 100644 gas/testsuite/gas/scfi/aarch64/scfi-static-stack-1.l
>> rename gas/testsuite/gas/scfi/aarch64/{scfi-unsupported-1.s => scfi-static-stack-1.s} (54%)
>> delete mode 100644 gas/testsuite/gas/scfi/aarch64/scfi-unsupported-1.l
>>
>> diff --git a/gas/scfi.c b/gas/scfi.c
>> index d9a5d59587d..f14118e16af 100644
>> --- a/gas/scfi.c
>> +++ b/gas/scfi.c
>
> There's a comment on scfi_state that also needs updating to say that rsp = rsp
> op reg instructions don't always make te stack size untraceable.
>
Thanks. Will fix it.
Am considering something like this:
/* Whether the stack size is known.
Stack size may become untraceable depending on the specific stack
manipulation machine insn. In most cases, REG_SP = REG_SP op REG insn
makes the stack size untraceable.
However, to improve the overall stack size traceability, the SCFI
mechanism
also tracks 'MOV' instructions (as a special case) that update a
register
with a constant (SPLIT_SP_UPDATE); See how the scratch register state
object may also track values. The SPLIT_SP_UPDATE pattern is
particularly
of interest on AArch64. */
bool traceable_p;
>> @@ -219,6 +219,63 @@ scfi_state_scratch_reg_get_loc (scfi_stateS *state, unsigned int reg)
>> return value;
>> }
>>
>> +/* Whether REG has previously been saved in the scratch reg state due to
>> + a mov imm, reg insn. */
>> +
>> +static bool
>> +scfi_state_scratch_reg_value_p (scfi_stateS *state, unsigned int reg)
>> +{
>> + bool value_p;
>> +
>> + if (reg >= MAX_NUM_SCFI_REGS)
>> + return false;
>> +
>> + value_p = state->scratch[reg].state == REG_SCRATCH_VALUE;
>> +
>> + return value_p;
>> +}
>> +
>> +/* Return the constant value of REG stashed away in the scratch reg state.
>> + IMP: Caller _must_ ensure this is done when scfi_state_scratch_reg_value_p ()
>> + is true for this to be the intended value. */
>> +
>> +static offsetT
>> +scfi_state_scratch_reg_get_value (scfi_stateS *state, unsigned int reg)
>> +{
>> + offsetT value = 0;
>> +
>> + if (scfi_state_scratch_reg_value_p (state, reg))
>> + value = state->scratch[reg].r.value;
>> +
>> + return value;
>> +}
>> +
>> +/* Reset the scratch reg state of the given REG if in state
>> + REG_SCRATCH_VALUE. */
>> +
>> +static void
>> +scfi_state_scratch_reg_value_reset (scfi_stateS *state, unsigned int reg)
>> +{
>> + if (reg >= MAX_NUM_SCFI_REGS)
>> + return;
>> +
>> + if (scfi_state_scratch_reg_value_p (state, reg))
>> + {
>> + state->scratch[reg].state = REG_SCRATCH_UNKNOWN;
>> + state->scratch[reg].r.value = 0;
>> + }
>> +}
>> +
>> +/* Reset the scratch reg state of all registers with a state of
>> + REG_SCRATCH_VALUE. */
>> +
>> +static void
>> +scfi_state_scratch_reg_value_reset_all (scfi_stateS *state)
>> +{
>> + for (int i = 0; i < MAX_NUM_SCFI_REGS; i++)
>> + scfi_state_scratch_reg_value_reset (state, i);
>> +}
>> +
>> /* Initialize a new SCFI op. */
>>
>> static scfi_opS *
>> @@ -663,6 +720,7 @@ verify_heuristic_traceable_stack_manipulation (ginsnS *ginsn,
>> struct ginsn_src *src1;
>> struct ginsn_src *src2;
>> unsigned int src1_reg;
>> + unsigned int src2_reg;
>> unsigned int dst_reg;
>> enum ginsn_src_type src1_type;
>> enum ginsn_src_type src2_type;
>> @@ -675,6 +733,7 @@ verify_heuristic_traceable_stack_manipulation (ginsnS *ginsn,
>> dst = ginsn_get_dst (ginsn);
>>
>> src1_reg = ginsn_get_src_reg (src1);
>> + src2_reg = ginsn_get_src_reg (src2);
>> dst_reg = ginsn_get_dst_reg (dst);
>>
>> src1_type = ginsn_get_src_type (src1);
>> @@ -704,13 +763,17 @@ verify_heuristic_traceable_stack_manipulation (ginsnS *ginsn,
>> if (!scfi_state_scratch_reg_loc_p (state, src1_reg))
>> possibly_untraceable = true;
>> }
>> - /* Check add/sub/and insn usage when CFA base register is REG_SP.
>> - Any stack size manipulation, including stack realignment is not allowed
>> - if CFA base register is REG_SP. */
>> + /* A non constant operand with add/sub insn when CFA base register is REG_SP
>> + is not OK. */
>> else if (dst_type == GINSN_DST_REG && dst_reg == REG_SP
>> - && (((gtype == GINSN_TYPE_ADD || gtype == GINSN_TYPE_SUB)
>> + && ((ginsn->type == GINSN_TYPE_ADD || ginsn->type == GINSN_TYPE_SUB)
>> && src2_type != GINSN_SRC_IMM)
>> - || gtype == GINSN_TYPE_AND || gtype == GINSN_TYPE_OTHER))
>> + && !scfi_state_scratch_reg_value_p (state, src2_reg))
>> + possibly_untraceable = true;
>> + /* Any stack size manipulation, including stack realignment is not allowed
>> + if CFA base register is REG_SP. */
>> + else if (dst_type == GINSN_DST_REG && dst_reg == REG_SP
>> + && (gtype == GINSN_TYPE_AND || gtype == GINSN_TYPE_OTHER))
>> possibly_untraceable = true;
>> /* If a register save operation is seen when REG_SP is untraceable,
>> CFI cannot be synthesized for register saves, hence bail out. */
>> @@ -830,6 +893,7 @@ gen_scfi_ops (ginsnS *ginsn, scfi_stateS *state)
>> struct ginsn_src *src2;
>> struct ginsn_dst *dst;
>> unsigned int src1_reg;
>> + unsigned int src2_reg;
>> unsigned int dst_reg;
>> enum ginsn_src_type src1_type;
>> enum ginsn_src_type src2_type;
>> @@ -853,6 +917,7 @@ gen_scfi_ops (ginsnS *ginsn, scfi_stateS *state)
>> dst = ginsn_get_dst (ginsn);
>>
>> src1_reg = ginsn_get_src_reg (src1);
>> + src2_reg = ginsn_get_src_reg (src2);
>> dst_reg = ginsn_get_dst_reg (dst);
>>
>> src1_type = ginsn_get_src_type (src1);
>> @@ -933,36 +998,50 @@ gen_scfi_ops (ginsnS *ginsn, scfi_stateS *state)
>> #endif
>>
>> }
>> + else if (src1_type == GINSN_SRC_IMM && dst_type == GINSN_DST_REG)
>> + {
>> + scfi_state_update_scratch_reg (state, dst_reg,
>> + ginsn_get_src_imm (src1), false);
>> + }
>> break;
>> case GINSN_TYPE_SUB:
>> if (src1_type == GINSN_SRC_REG && src1_reg == REG_SP
>> - && dst_type == GINSN_DST_REG && dst_reg == REG_SP
>> - && src2_type == GINSN_SRC_IMM)
>> + && dst_type == GINSN_DST_REG && dst_reg == REG_SP)
>> {
>> /* Stack inc/dec offset, when generated due to stack push and pop is
>> target-specific. Use the value encoded in the ginsn. */
>> - state->stack_size += ginsn_get_src_imm (src2);
>> + if (src2_type == GINSN_SRC_IMM)
>> + offset = ginsn_get_src_imm (src2);
>> + else if (src2_type == GINSN_SRC_REG
>> + && scfi_state_scratch_reg_value_p (state, src2_reg))
>> + offset = scfi_state_scratch_reg_get_value (state, src2_reg);
>> + else
>> + break;
>> +
>> + state->stack_size += offset;
>> if (state->regs[REG_CFA].base == REG_SP)
>> - {
>> - /* push reg. */
>> - scfi_op_add_cfa_offset_dec (state, ginsn, ginsn_get_src_imm (src2));
>> - }
>> + scfi_op_add_cfa_offset_dec (state, ginsn, offset);
>> }
>> break;
>> case GINSN_TYPE_ADD:
>> if (src1_type == GINSN_SRC_REG && src1_reg == REG_SP
>> - && dst_type == GINSN_DST_REG && dst_reg == REG_SP
>> - && src2_type == GINSN_SRC_IMM)
>> + && dst_type == GINSN_DST_REG && dst_reg == REG_SP)
>> {
>> /* Stack inc/dec offset is target-specific. Use the value
>> encoded in the ginsn. */
>> - state->stack_size -= ginsn_get_src_imm (src2);
>> + if (src2_type == GINSN_SRC_IMM)
>> + offset = ginsn_get_src_imm (src2);
>> + else if (src2_type == GINSN_SRC_REG
>> + && scfi_state_scratch_reg_value_p (state, src2_reg))
>> + offset = scfi_state_scratch_reg_get_value (state, src2_reg);
>> + else
>> + break;
>> +
>> + state->stack_size -= offset;
>> /* pop %reg affects CFA offset only if CFA is currently
>> stack-pointer based. */
>> if (state->regs[REG_CFA].base == REG_SP)
>> - {
>> - scfi_op_add_cfa_offset_inc (state, ginsn, ginsn_get_src_imm (src2));
>> - }
>> + scfi_op_add_cfa_offset_inc (state, ginsn, offset);
>> }
>> else if (src1_type == GINSN_SRC_REG && src1_reg == REG_FP
>> && dst_type == GINSN_DST_REG && dst_reg == REG_SP
>> @@ -1000,6 +1079,9 @@ gen_scfi_ops (ginsnS *ginsn, scfi_stateS *state)
>> }
>> }
>> break;
>> + case GINSN_TYPE_OTHER:
>> + scfi_state_scratch_reg_value_reset (state, dst_reg);
>> + break;
>> default:
>> break;
>> }
>> @@ -1039,9 +1121,15 @@ gen_scfi_ops (ginsnS *ginsn, scfi_stateS *state)
>> }
>> break;
>>
>> + case GINSN_DST_UNKNOWN:
>> + /* Demarcate the end of the SPLIT_SP_UPDATE pattern if a control flow
>> + instruction is seen. */
>> + if (ginsn_cofi_p (ginsn))
>> + scfi_state_scratch_reg_value_reset_all (state);
>> + break;
>> +
>> default:
>> - /* Skip GINSN_DST_UNKNOWN and GINSN_DST_MEM as they are uninteresting
>> - currently for SCFI. */
>> + /* Skip GINSN_DST_MEM as they are uninteresting currently for SCFI. */
>> break;
>> }
>>
>> diff --git a/gas/testsuite/gas/scfi/aarch64/scfi-aarch64.exp b/gas/testsuite/gas/scfi/aarch64/scfi-aarch64.exp
>> index 9a0b2856cb4..b4cd7e22f2b 100644
>> --- a/gas/testsuite/gas/scfi/aarch64/scfi-aarch64.exp
>> +++ b/gas/testsuite/gas/scfi/aarch64/scfi-aarch64.exp
>> @@ -33,13 +33,13 @@ if { ([istarget "aarch64-*-*"]) } then {
>> run_list_test "scfi-diag-2" "--scfi=experimental"
>> run_list_test "scfi-diag-3" "--scfi=experimental"
>>
>> - run_list_test "scfi-unsupported-1" "--scfi=experimental"
>> run_list_test "scfi-unsupported-2" "--scfi=experimental"
>>
>> run_dump_test "scfi-callee-saved-fp-1"
>> run_list_test "scfi-callee-saved-fp-1" "--scfi=experimental --warn"
>> run_dump_test "scfi-callee-saved-fp-2"
>> run_list_test "scfi-callee-saved-fp-2" "--scfi=experimental --warn"
>> + run_list_test "scfi-static-stack-1" "--scfi=experimental"
>>
>> run_dump_test "scfi-ldrp-1"
>> run_list_test "scfi-ldrp-1" "--scfi=experimental --warn"
>> diff --git a/gas/testsuite/gas/scfi/aarch64/scfi-static-stack-1.d b/gas/testsuite/gas/scfi/aarch64/scfi-static-stack-1.d
>> new file mode 100644
>> index 00000000000..c353158114e
>> --- /dev/null
>> +++ b/gas/testsuite/gas/scfi/aarch64/scfi-static-stack-1.d
>> @@ -0,0 +1,33 @@
>> +#as: --scfi=experimental -W
>> +#objdump: -Wf
>> +#name: Synthesize CFI for ldp ldr instructions
>> +#...
>> +Contents of the .eh_frame section:
>> +
>> +00000000 0+0010 00000000 CIE
>> + Version: 1
>> + Augmentation: "zR"
>> + Code alignment factor: 4
>> + Data alignment factor: -8
>> + Return address column: 30
>> + Augmentation data: 1b
>> + DW_CFA_def_cfa: r31 (sp) ofs 0
>> +
>> +00000014 0+0028 00000018 FDE cie=00000000 pc=0+0000..0+002c
>> + DW_CFA_advance_loc: 8 to 0000000000000008
>> + DW_CFA_def_cfa_offset: 4384
>> + DW_CFA_advance_loc: 4 to 000000000000000c
>> + DW_CFA_offset: r29 \(x29\) at cfa-4384
>> + DW_CFA_offset: r30 \(x30\) at cfa-4376
>> + DW_CFA_advance_loc: 4 to 0000000000000010
>> + DW_CFA_offset: r19 \(x19\) at cfa-4368
>> + DW_CFA_advance_loc: 16 to 0000000000000020
>> + DW_CFA_restore: r29 \(x29\)
>> + DW_CFA_restore: r30 \(x30\)
>> + DW_CFA_advance_loc: 4 to 0000000000000024
>> + DW_CFA_restore: r19 \(x19\)
>> + DW_CFA_advance_loc: 4 to 0000000000000028
>> + DW_CFA_def_cfa_offset: 0
>> + DW_CFA_nop
>> +
>> +#pass
>> diff --git a/gas/testsuite/gas/scfi/aarch64/scfi-static-stack-1.l b/gas/testsuite/gas/scfi/aarch64/scfi-static-stack-1.l
>> new file mode 100644
>> index 00000000000..6ec24387b8c
>> --- /dev/null
>> +++ b/gas/testsuite/gas/scfi/aarch64/scfi-static-stack-1.l
>> @@ -0,0 +1,2 @@
>> +.*Assembler messages:
>> +.*7: Warning: SCFI ignores most user-specified CFI directives
>> diff --git a/gas/testsuite/gas/scfi/aarch64/scfi-unsupported-1.s b/gas/testsuite/gas/scfi/aarch64/scfi-static-stack-1.s
>> similarity index 54%
>> rename from gas/testsuite/gas/scfi/aarch64/scfi-unsupported-1.s
>> rename to gas/testsuite/gas/scfi/aarch64/scfi-static-stack-1.s
>> index c143185d03e..a19854158a3 100644
>> --- a/gas/testsuite/gas/scfi/aarch64/scfi-unsupported-1.s
>> +++ b/gas/testsuite/gas/scfi/aarch64/scfi-static-stack-1.s
>> @@ -1,6 +1,6 @@
>> -# Testcase where immediate used for stack allocation is a wide
>> -# one. Since SCFI does not currently have any data-flow
>> -# capabilities, this is currently not supported.
>> +# Testcase where stack pointer update is done using a reg-based insn
>> +# SCFI must not choke; it is not necessary for the mov and add/sub insn to be
>> +# consequent insns
>> .global foo
>> .type foo, %function
>> foo:
>> @@ -11,20 +11,17 @@ foo:
>> stp x29, x30, [sp]
>> .cfi_offset 29, -4384
>> .cfi_offset 30, -4376
>> - mov x29, sp
>> - str x0, [sp, 24]
>> - str x1, [sp, 16]
>> - add x0, sp, 4096
>> - add x0, x0, 112
>> + str x19, [sp, 16]
>> + .cfi_offset 19, -4368
>> + mov x19, x0
>> bl bar
>> -.L1:
>> - str xzr, [sp, 4376]
>> -.L2:
>> - ldp x29, x30, [sp]
>> mov x16, 4384
>> - add sp, sp, x16
>> + ldp x29, x30, [sp]
>> .cfi_restore 29
>> .cfi_restore 30
>> + ldr x19, [sp, 16]
>> + .cfi_restore 19
>> + add sp, sp, x16
>> .cfi_def_cfa_offset 0
>> ret
>> .cfi_endproc
>> diff --git a/gas/testsuite/gas/scfi/aarch64/scfi-unsupported-1.l b/gas/testsuite/gas/scfi/aarch64/scfi-unsupported-1.l
>> deleted file mode 100644
>> index de3ed86250b..00000000000
>> --- a/gas/testsuite/gas/scfi/aarch64/scfi-unsupported-1.l
>> +++ /dev/null
>> @@ -1,4 +0,0 @@
>> -.*Assembler messages:
>> -.*7: Warning: SCFI ignores most user-specified CFI directives
>> -.*9: Error: SCFI: unsupported stack manipulation pattern
>> -.*31: Error: SCFI: forward pass failed for func 'foo'
>> --
>> 2.43.0
>>
More information about the Binutils
mailing list