[v1,2/4] aarch64 SFrame: use preferred CFI directive for AArch64 PAC
Checks
| Context |
Check |
Description |
| linaro-tcwg-bot/tcwg_binutils_build--master-arm |
success
|
Build passed
|
| linaro-tcwg-bot/tcwg_binutils_build--master-aarch64 |
success
|
Build passed
|
| linaro-tcwg-bot/tcwg_binutils_check--master-aarch64 |
success
|
Test passed
|
| linaro-tcwg-bot/tcwg_binutils_check--master-arm |
success
|
Test passed
|
Commit Message
ARMv8.3 addded support for a new security feature named Pointer
Authentication. Support for this feature in SFrame already exists,
but is relying on the deprecated "AArch64" (actually Sparc) CFI
directive .cfi_gnu_window_save. .cfi_negate_ra_state CFI directive
should be used instead to convey this information.
In GCC 14 and older, the Sparc DWARF extension .cfi_gnu_window_save
is emitted instead of .cfi_negate_ra_state.
GCC 15 fixed this issue, but this behavior is preserved for backward
compatibility.
The existing sframe test for AArch64 PAC was using .cfi_gnu_window_save.
This patch replaces this CFI in the existing test by the preferred one,
and adds a new test to check for backward compatibility when using
.cfi_gnu_window_save.
---
.../gas/cfi-sframe/cfi-sframe-aarch64-3.d | 20 ++++++++++++++++++
.../gas/cfi-sframe/cfi-sframe-aarch64-3.s | 21 +++++++++++++++++++
.../cfi-sframe-aarch64-pac-ab-key-1.s | 8 +++----
gas/testsuite/gas/cfi-sframe/cfi-sframe.exp | 1 +
4 files changed, 46 insertions(+), 4 deletions(-)
create mode 100644 gas/testsuite/gas/cfi-sframe/cfi-sframe-aarch64-3.d
create mode 100644 gas/testsuite/gas/cfi-sframe/cfi-sframe-aarch64-3.s
Comments
On 11/25/24 8:28 AM, Matthieu Longo wrote:
> ARMv8.3 addded support for a new security feature named Pointer
> Authentication. Support for this feature in SFrame already exists,
> but is relying on the deprecated "AArch64" (actually Sparc) CFI
> directive .cfi_gnu_window_save. .cfi_negate_ra_state CFI directive
> should be used instead to convey this information.
>
> In GCC 14 and older, the Sparc DWARF extension .cfi_gnu_window_save
> is emitted instead of .cfi_negate_ra_state.
> GCC 15 fixed this issue, but this behavior is preserved for backward
> compatibility.
>
> The existing sframe test for AArch64 PAC was using .cfi_gnu_window_save.
> This patch replaces this CFI in the existing test by the preferred one,
> and adds a new test to check for backward compatibility when using
> .cfi_gnu_window_save.
Hi Matthieu,
See one comment inlined below.
Thanks
Indu
> ---
> .../gas/cfi-sframe/cfi-sframe-aarch64-3.d | 20 ++++++++++++++++++
> .../gas/cfi-sframe/cfi-sframe-aarch64-3.s | 21 +++++++++++++++++++
> .../cfi-sframe-aarch64-pac-ab-key-1.s | 8 +++----
> gas/testsuite/gas/cfi-sframe/cfi-sframe.exp | 1 +
> 4 files changed, 46 insertions(+), 4 deletions(-)
> create mode 100644 gas/testsuite/gas/cfi-sframe/cfi-sframe-aarch64-3.d
> create mode 100644 gas/testsuite/gas/cfi-sframe/cfi-sframe-aarch64-3.s
>
> diff --git a/gas/testsuite/gas/cfi-sframe/cfi-sframe-aarch64-3.d b/gas/testsuite/gas/cfi-sframe/cfi-sframe-aarch64-3.d
> new file mode 100644
> index 00000000000..f72b70a970a
> --- /dev/null
> +++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-aarch64-3.d
> @@ -0,0 +1,20 @@
> +#as: --gsframe
> +#objdump: --sframe=.sframe
> +#name: SFrame cfi_negate_ra_state test (using cfi_window_save)
> +#...
> +Contents of the SFrame section .sframe:
> +
> + Header :
> +
> + Version: SFRAME_VERSION_2
> + Flags: NONE
> + Num FDEs: 1
> + Num FREs: 2
> +
> + Function Index :
> + func idx \[0\]: pc = 0x0, size = 8 bytes
> + STARTPC + CFA + FP + RA +
> +#...
> + 0+0004 +sp\+16 +u +u\[s\] +
> +
> +#pass
> diff --git a/gas/testsuite/gas/cfi-sframe/cfi-sframe-aarch64-3.s b/gas/testsuite/gas/cfi-sframe/cfi-sframe-aarch64-3.s
> new file mode 100644
> index 00000000000..df6ff6e8c8f
> --- /dev/null
> +++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-aarch64-3.s
> @@ -0,0 +1,21 @@
> +## ARMv8.3 addded support a new security feature named Pointer Authentication. The
> +## main idea behind this is to use the unused bits in the pointer values.
> +## Each pointer is patched with a PAC before writing to memory, and is verified
> +## before using it.
> +## When the pointers are mangled, the stack trace generator needs to know so it
> +## can mask off the PAC from the pointer value to recover the return address,
> +## and conversely, skip doing so if the pointers are not mangled.
> +##
> +## .cfi_negate_ra_state CFI directive is usually used to convey this information.
> +## In GCC 14 and older, the Sparc DWARF extension .cfi_window_save is emitted
> +## instead of .cfi_negate_ra_state. GCC 15 fixed this issue, but this behavior
> +## is preserved in binutils for backward compatibility.
> +##
> +## SFrame has support for this. This testcase ensures that the directive
> +## is interpreted successfully.
> + .cfi_startproc
> + .long 0
> + .cfi_def_cfa_offset 16
> + .cfi_window_save
> + .long 0
> + .cfi_endproc
Since the purpose is to ensure that the old behavior is maintained, I am
wondering if it is not better to keep the old test as is (which also
tests the .cfi_window_save *together with* the .cfi_b_key_frame
directives). WDYT ?
> diff --git a/gas/testsuite/gas/cfi-sframe/cfi-sframe-aarch64-pac-ab-key-1.s b/gas/testsuite/gas/cfi-sframe/cfi-sframe-aarch64-pac-ab-key-1.s
> index d9a408c668c..84230a99e3c 100644
> --- a/gas/testsuite/gas/cfi-sframe/cfi-sframe-aarch64-pac-ab-key-1.s
> +++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-aarch64-pac-ab-key-1.s
> @@ -8,12 +8,12 @@ _Z5foo_av:
> .LFB0:
> .cfi_startproc
> hint 25 // paciasp
> - .cfi_window_save
> + .cfi_negate_ra_state
> stp x29, x30, [sp, -16]!
> .cfi_def_cfa_offset 16
> .cfi_offset 29, -16
> .cfi_offset 30, -8
> - ret
> + ret
> .cfi_endproc
> .LFE0:
> .size _Z5foo_av, .-_Z5foo_av
> @@ -25,12 +25,12 @@ _Z5foo_bv:
> .cfi_startproc
> .cfi_b_key_frame
> hint 27 // pacibsp
> - .cfi_window_save
> + .cfi_negate_ra_state
> stp x29, x30, [sp, -16]!
> .cfi_def_cfa_offset 16
> .cfi_offset 29, -16
> .cfi_offset 30, -8
> nop
> nop
> - ret
> + ret
> .cfi_endproc
> diff --git a/gas/testsuite/gas/cfi-sframe/cfi-sframe.exp b/gas/testsuite/gas/cfi-sframe/cfi-sframe.exp
> index cfae28d39aa..c646b109895 100644
> --- a/gas/testsuite/gas/cfi-sframe/cfi-sframe.exp
> +++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe.exp
> @@ -97,5 +97,6 @@ if { [istarget "x86_64-*-*"] && [gas_sframe_check] } then {
> if { [istarget "aarch64*-*-*"] && [gas_sframe_check] } then {
> run_dump_test "cfi-sframe-aarch64-1"
> run_dump_test "cfi-sframe-aarch64-2"
> + run_dump_test "cfi-sframe-aarch64-3"
> run_dump_test "cfi-sframe-aarch64-pac-ab-key-1"
> }
On 2024-12-01 03:23, Indu Bhagat wrote:
> On 11/25/24 8:28 AM, Matthieu Longo wrote:
>> ARMv8.3 addded support for a new security feature named Pointer
>> Authentication. Support for this feature in SFrame already exists,
>> but is relying on the deprecated "AArch64" (actually Sparc) CFI
>> directive .cfi_gnu_window_save. .cfi_negate_ra_state CFI directive
>> should be used instead to convey this information.
>>
>> In GCC 14 and older, the Sparc DWARF extension .cfi_gnu_window_save
>> is emitted instead of .cfi_negate_ra_state.
>> GCC 15 fixed this issue, but this behavior is preserved for backward
>> compatibility.
>>
>> The existing sframe test for AArch64 PAC was using .cfi_gnu_window_save.
>> This patch replaces this CFI in the existing test by the preferred one,
>> and adds a new test to check for backward compatibility when using
>> .cfi_gnu_window_save.
>
> Hi Matthieu,
>
> See one comment inlined below.
>
> Thanks
> Indu
>
>> ---
>> .../gas/cfi-sframe/cfi-sframe-aarch64-3.d | 20 ++++++++++++++++++
>> .../gas/cfi-sframe/cfi-sframe-aarch64-3.s | 21 +++++++++++++++++++
>> .../cfi-sframe-aarch64-pac-ab-key-1.s | 8 +++----
>> gas/testsuite/gas/cfi-sframe/cfi-sframe.exp | 1 +
>> 4 files changed, 46 insertions(+), 4 deletions(-)
>> create mode 100644 gas/testsuite/gas/cfi-sframe/cfi-sframe-aarch64-3.d
>> create mode 100644 gas/testsuite/gas/cfi-sframe/cfi-sframe-aarch64-3.s
>>
>> diff --git a/gas/testsuite/gas/cfi-sframe/cfi-sframe-aarch64-3.d b/
>> gas/testsuite/gas/cfi-sframe/cfi-sframe-aarch64-3.d
>> new file mode 100644
>> index 00000000000..f72b70a970a
>> --- /dev/null
>> +++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-aarch64-3.d
>> @@ -0,0 +1,20 @@
>> +#as: --gsframe
>> +#objdump: --sframe=.sframe
>> +#name: SFrame cfi_negate_ra_state test (using cfi_window_save)
>> +#...
>> +Contents of the SFrame section .sframe:
>> +
>> + Header :
>> +
>> + Version: SFRAME_VERSION_2
>> + Flags: NONE
>> + Num FDEs: 1
>> + Num FREs: 2
>> +
>> + Function Index :
>> + func idx \[0\]: pc = 0x0, size = 8 bytes
>> + STARTPC + CFA + FP + RA +
>> +#...
>> + 0+0004 +sp\+16 +u +u\[s\] +
>> +
>> +#pass
>> diff --git a/gas/testsuite/gas/cfi-sframe/cfi-sframe-aarch64-3.s b/
>> gas/testsuite/gas/cfi-sframe/cfi-sframe-aarch64-3.s
>> new file mode 100644
>> index 00000000000..df6ff6e8c8f
>> --- /dev/null
>> +++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-aarch64-3.s
>> @@ -0,0 +1,21 @@
>> +## ARMv8.3 addded support a new security feature named Pointer
>> Authentication. The
>> +## main idea behind this is to use the unused bits in the pointer
>> values.
>> +## Each pointer is patched with a PAC before writing to memory, and
>> is verified
>> +## before using it.
>> +## When the pointers are mangled, the stack trace generator needs to
>> know so it
>> +## can mask off the PAC from the pointer value to recover the return
>> address,
>> +## and conversely, skip doing so if the pointers are not mangled.
>> +##
>> +## .cfi_negate_ra_state CFI directive is usually used to convey this
>> information.
>> +## In GCC 14 and older, the Sparc DWARF extension .cfi_window_save is
>> emitted
>> +## instead of .cfi_negate_ra_state. GCC 15 fixed this issue, but
>> this behavior
>> +## is preserved in binutils for backward compatibility.
>> +##
>> +## SFrame has support for this. This testcase ensures that the directive
>> +## is interpreted successfully.
>> + .cfi_startproc
>> + .long 0
>> + .cfi_def_cfa_offset 16
>> + .cfi_window_save
>> + .long 0
>> + .cfi_endproc
>
> Since the purpose is to ensure that the old behavior is maintained, I am
> wondering if it is not better to keep the old test as is (which also
> tests the .cfi_window_save *together with* the .cfi_b_key_frame
> directives). WDYT ?
>
The old behavior is not going to be maintained with GCC 15 and newer.
From my perspective, the purpose of this test is to test the behavior
of A and B keys with the *correct* AArch64 CFI directives. That's the
reason why I added a separate test cfi-sframe-aarch64-3.s to ensure that
the old directive is still correctly parsed and processed.
Matthieu.
>> diff --git a/gas/testsuite/gas/cfi-sframe/cfi-sframe-aarch64-pac-ab-
>> key-1.s b/gas/testsuite/gas/cfi-sframe/cfi-sframe-aarch64-pac-ab-key-1.s
>> index d9a408c668c..84230a99e3c 100644
>> --- a/gas/testsuite/gas/cfi-sframe/cfi-sframe-aarch64-pac-ab-key-1.s
>> +++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-aarch64-pac-ab-key-1.s
>> @@ -8,12 +8,12 @@ _Z5foo_av:
>> .LFB0:
>> .cfi_startproc
>> hint 25 // paciasp
>> - .cfi_window_save
>> + .cfi_negate_ra_state
>> stp x29, x30, [sp, -16]!
>> .cfi_def_cfa_offset 16
>> .cfi_offset 29, -16
>> .cfi_offset 30, -8
>> - ret
>> + ret
>> .cfi_endproc
>> .LFE0:
>> .size _Z5foo_av, .-_Z5foo_av
>> @@ -25,12 +25,12 @@ _Z5foo_bv:
>> .cfi_startproc
>> .cfi_b_key_frame
>> hint 27 // pacibsp
>> - .cfi_window_save
>> + .cfi_negate_ra_state
>> stp x29, x30, [sp, -16]!
>> .cfi_def_cfa_offset 16
>> .cfi_offset 29, -16
>> .cfi_offset 30, -8
>> nop
>> nop
>> - ret
>> + ret
>> .cfi_endproc
>> diff --git a/gas/testsuite/gas/cfi-sframe/cfi-sframe.exp b/gas/
>> testsuite/gas/cfi-sframe/cfi-sframe.exp
>> index cfae28d39aa..c646b109895 100644
>> --- a/gas/testsuite/gas/cfi-sframe/cfi-sframe.exp
>> +++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe.exp
>> @@ -97,5 +97,6 @@ if { [istarget "x86_64-*-*"] && [gas_sframe_check] }
>> then {
>> if { [istarget "aarch64*-*-*"] && [gas_sframe_check] } then {
>> run_dump_test "cfi-sframe-aarch64-1"
>> run_dump_test "cfi-sframe-aarch64-2"
>> + run_dump_test "cfi-sframe-aarch64-3"
>> run_dump_test "cfi-sframe-aarch64-pac-ab-key-1"
>> }
>
On 10/12/2024 11:13, Matthieu Longo wrote:
> On 2024-12-01 03:23, Indu Bhagat wrote:
>> On 11/25/24 8:28 AM, Matthieu Longo wrote:
>>> ARMv8.3 addded support for a new security feature named Pointer
>>> Authentication. Support for this feature in SFrame already exists,
>>> but is relying on the deprecated "AArch64" (actually Sparc) CFI
>>> directive .cfi_gnu_window_save. .cfi_negate_ra_state CFI directive
>>> should be used instead to convey this information.
>>>
>>> In GCC 14 and older, the Sparc DWARF extension .cfi_gnu_window_save
>>> is emitted instead of .cfi_negate_ra_state.
>>> GCC 15 fixed this issue, but this behavior is preserved for backward
>>> compatibility.
>>>
>>> The existing sframe test for AArch64 PAC was using .cfi_gnu_window_save.
>>> This patch replaces this CFI in the existing test by the preferred one,
>>> and adds a new test to check for backward compatibility when using
>>> .cfi_gnu_window_save.
>>
>> Hi Matthieu,
>>
>> See one comment inlined below.
>>
>> Thanks
>> Indu
>>
>>> ---
>>> .../gas/cfi-sframe/cfi-sframe-aarch64-3.d | 20 ++++++++++++++++++
>>> .../gas/cfi-sframe/cfi-sframe-aarch64-3.s | 21 +++++++++++++++++++
>>> .../cfi-sframe-aarch64-pac-ab-key-1.s | 8 +++----
>>> gas/testsuite/gas/cfi-sframe/cfi-sframe.exp | 1 +
>>> 4 files changed, 46 insertions(+), 4 deletions(-)
>>> create mode 100644 gas/testsuite/gas/cfi-sframe/cfi-sframe-aarch64-3.d
>>> create mode 100644 gas/testsuite/gas/cfi-sframe/cfi-sframe-aarch64-3.s
>>>
>>> diff --git a/gas/testsuite/gas/cfi-sframe/cfi-sframe-aarch64-3.d b/
>>> gas/testsuite/gas/cfi-sframe/cfi-sframe-aarch64-3.d
>>> new file mode 100644
>>> index 00000000000..f72b70a970a
>>> --- /dev/null
>>> +++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-aarch64-3.d
>>> @@ -0,0 +1,20 @@
>>> +#as: --gsframe
>>> +#objdump: --sframe=.sframe
>>> +#name: SFrame cfi_negate_ra_state test (using cfi_window_save)
>>> +#...
>>> +Contents of the SFrame section .sframe:
>>> +
>>> + Header :
>>> +
>>> + Version: SFRAME_VERSION_2
>>> + Flags: NONE
>>> + Num FDEs: 1
>>> + Num FREs: 2
>>> +
>>> + Function Index :
>>> + func idx \[0\]: pc = 0x0, size = 8 bytes
>>> + STARTPC + CFA + FP + RA +
>>> +#...
>>> + 0+0004 +sp\+16 +u +u\[s\] +
>>> +
>>> +#pass
>>> diff --git a/gas/testsuite/gas/cfi-sframe/cfi-sframe-aarch64-3.s b/
>>> gas/testsuite/gas/cfi-sframe/cfi-sframe-aarch64-3.s
>>> new file mode 100644
>>> index 00000000000..df6ff6e8c8f
>>> --- /dev/null
>>> +++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-aarch64-3.s
>>> @@ -0,0 +1,21 @@
>>> +## ARMv8.3 addded support a new security feature named Pointer
>>> Authentication. The
>>> +## main idea behind this is to use the unused bits in the pointer
>>> values.
>>> +## Each pointer is patched with a PAC before writing to memory, and
>>> is verified
>>> +## before using it.
>>> +## When the pointers are mangled, the stack trace generator needs to
>>> know so it
>>> +## can mask off the PAC from the pointer value to recover the return
>>> address,
>>> +## and conversely, skip doing so if the pointers are not mangled.
>>> +##
>>> +## .cfi_negate_ra_state CFI directive is usually used to convey this
>>> information.
>>> +## In GCC 14 and older, the Sparc DWARF extension .cfi_window_save
>>> is emitted
>>> +## instead of .cfi_negate_ra_state. GCC 15 fixed this issue, but
>>> this behavior
>>> +## is preserved in binutils for backward compatibility.
>>> +##
>>> +## SFrame has support for this. This testcase ensures that the
>>> directive
>>> +## is interpreted successfully.
>>> + .cfi_startproc
>>> + .long 0
>>> + .cfi_def_cfa_offset 16
>>> + .cfi_window_save
>>> + .long 0
>>> + .cfi_endproc
>>
>> Since the purpose is to ensure that the old behavior is maintained, I
>> am wondering if it is not better to keep the old test as is (which
>> also tests the .cfi_window_save *together with* the .cfi_b_key_frame
>> directives). WDYT ?
>>
>
> The old behavior is not going to be maintained with GCC 15 and newer.
> From my perspective, the purpose of this test is to test the behavior
> of A and B keys with the *correct* AArch64 CFI directives. That's the
> reason why I added a separate test cfi-sframe-aarch64-3.s to ensure that
> the old directive is still correctly parsed and processed.
We do need to ensure that a new version of gas works with old versions
of GCC, so if GCC-14 or earlier emit the wrong directive we will need to
continue to accept it until such versions are truly obsolete (a long time).
R.
>
> Matthieu.
>
>>> diff --git a/gas/testsuite/gas/cfi-sframe/cfi-sframe-aarch64-pac-ab-
>>> key-1.s b/gas/testsuite/gas/cfi-sframe/cfi-sframe-aarch64-pac-ab-key-1.s
>>> index d9a408c668c..84230a99e3c 100644
>>> --- a/gas/testsuite/gas/cfi-sframe/cfi-sframe-aarch64-pac-ab-key-1.s
>>> +++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-aarch64-pac-ab-key-1.s
>>> @@ -8,12 +8,12 @@ _Z5foo_av:
>>> .LFB0:
>>> .cfi_startproc
>>> hint 25 // paciasp
>>> - .cfi_window_save
>>> + .cfi_negate_ra_state
>>> stp x29, x30, [sp, -16]!
>>> .cfi_def_cfa_offset 16
>>> .cfi_offset 29, -16
>>> .cfi_offset 30, -8
>>> - ret
>>> + ret
>>> .cfi_endproc
>>> .LFE0:
>>> .size _Z5foo_av, .-_Z5foo_av
>>> @@ -25,12 +25,12 @@ _Z5foo_bv:
>>> .cfi_startproc
>>> .cfi_b_key_frame
>>> hint 27 // pacibsp
>>> - .cfi_window_save
>>> + .cfi_negate_ra_state
>>> stp x29, x30, [sp, -16]!
>>> .cfi_def_cfa_offset 16
>>> .cfi_offset 29, -16
>>> .cfi_offset 30, -8
>>> nop
>>> nop
>>> - ret
>>> + ret
>>> .cfi_endproc
>>> diff --git a/gas/testsuite/gas/cfi-sframe/cfi-sframe.exp b/gas/
>>> testsuite/gas/cfi-sframe/cfi-sframe.exp
>>> index cfae28d39aa..c646b109895 100644
>>> --- a/gas/testsuite/gas/cfi-sframe/cfi-sframe.exp
>>> +++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe.exp
>>> @@ -97,5 +97,6 @@ if { [istarget "x86_64-*-*"] &&
>>> [gas_sframe_check] } then {
>>> if { [istarget "aarch64*-*-*"] && [gas_sframe_check] } then {
>>> run_dump_test "cfi-sframe-aarch64-1"
>>> run_dump_test "cfi-sframe-aarch64-2"
>>> + run_dump_test "cfi-sframe-aarch64-3"
>>> run_dump_test "cfi-sframe-aarch64-pac-ab-key-1"
>>> }
>>
>
On 2024-12-10 18:00, Richard Earnshaw (lists) wrote:
> On 10/12/2024 11:13, Matthieu Longo wrote:
>> On 2024-12-01 03:23, Indu Bhagat wrote:
>>> On 11/25/24 8:28 AM, Matthieu Longo wrote:
>>>> ARMv8.3 addded support for a new security feature named Pointer
>>>> Authentication. Support for this feature in SFrame already exists,
>>>> but is relying on the deprecated "AArch64" (actually Sparc) CFI
>>>> directive .cfi_gnu_window_save. .cfi_negate_ra_state CFI directive
>>>> should be used instead to convey this information.
>>>>
>>>> In GCC 14 and older, the Sparc DWARF extension .cfi_gnu_window_save
>>>> is emitted instead of .cfi_negate_ra_state.
>>>> GCC 15 fixed this issue, but this behavior is preserved for backward
>>>> compatibility.
>>>>
>>>> The existing sframe test for AArch64 PAC was
>>>> using .cfi_gnu_window_save.
>>>> This patch replaces this CFI in the existing test by the preferred one,
>>>> and adds a new test to check for backward compatibility when using
>>>> .cfi_gnu_window_save.
>>>
>>> Hi Matthieu,
>>>
>>> See one comment inlined below.
>>>
>>> Thanks
>>> Indu
>>>
>>>> ---
>>>> .../gas/cfi-sframe/cfi-sframe-aarch64-3.d | 20 ++++++++++++++++++
>>>> .../gas/cfi-sframe/cfi-sframe-aarch64-3.s | 21 +++++++++++++++
>>>> ++++
>>>> .../cfi-sframe-aarch64-pac-ab-key-1.s | 8 +++----
>>>> gas/testsuite/gas/cfi-sframe/cfi-sframe.exp | 1 +
>>>> 4 files changed, 46 insertions(+), 4 deletions(-)
>>>> create mode 100644 gas/testsuite/gas/cfi-sframe/cfi-sframe-
>>>> aarch64-3.d
>>>> create mode 100644 gas/testsuite/gas/cfi-sframe/cfi-sframe-
>>>> aarch64-3.s
>>>>
>>>> diff --git a/gas/testsuite/gas/cfi-sframe/cfi-sframe-aarch64-3.d b/
>>>> gas/testsuite/gas/cfi-sframe/cfi-sframe-aarch64-3.d
>>>> new file mode 100644
>>>> index 00000000000..f72b70a970a
>>>> --- /dev/null
>>>> +++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-aarch64-3.d
>>>> @@ -0,0 +1,20 @@
>>>> +#as: --gsframe
>>>> +#objdump: --sframe=.sframe
>>>> +#name: SFrame cfi_negate_ra_state test (using cfi_window_save)
>>>> +#...
>>>> +Contents of the SFrame section .sframe:
>>>> +
>>>> + Header :
>>>> +
>>>> + Version: SFRAME_VERSION_2
>>>> + Flags: NONE
>>>> + Num FDEs: 1
>>>> + Num FREs: 2
>>>> +
>>>> + Function Index :
>>>> + func idx \[0\]: pc = 0x0, size = 8 bytes
>>>> + STARTPC + CFA + FP + RA +
>>>> +#...
>>>> + 0+0004 +sp\+16 +u +u\[s\] +
>>>> +
>>>> +#pass
>>>> diff --git a/gas/testsuite/gas/cfi-sframe/cfi-sframe-aarch64-3.s b/
>>>> gas/testsuite/gas/cfi-sframe/cfi-sframe-aarch64-3.s
>>>> new file mode 100644
>>>> index 00000000000..df6ff6e8c8f
>>>> --- /dev/null
>>>> +++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-aarch64-3.s
>>>> @@ -0,0 +1,21 @@
>>>> +## ARMv8.3 addded support a new security feature named Pointer
>>>> Authentication. The
>>>> +## main idea behind this is to use the unused bits in the pointer
>>>> values.
>>>> +## Each pointer is patched with a PAC before writing to memory, and
>>>> is verified
>>>> +## before using it.
>>>> +## When the pointers are mangled, the stack trace generator needs
>>>> to know so it
>>>> +## can mask off the PAC from the pointer value to recover the
>>>> return address,
>>>> +## and conversely, skip doing so if the pointers are not mangled.
>>>> +##
>>>> +## .cfi_negate_ra_state CFI directive is usually used to convey
>>>> this information.
>>>> +## In GCC 14 and older, the Sparc DWARF extension .cfi_window_save
>>>> is emitted
>>>> +## instead of .cfi_negate_ra_state. GCC 15 fixed this issue, but
>>>> this behavior
>>>> +## is preserved in binutils for backward compatibility.
>>>> +##
>>>> +## SFrame has support for this. This testcase ensures that the
>>>> directive
>>>> +## is interpreted successfully.
>>>> + .cfi_startproc
>>>> + .long 0
>>>> + .cfi_def_cfa_offset 16
>>>> + .cfi_window_save
>>>> + .long 0
>>>> + .cfi_endproc
>>>
>>> Since the purpose is to ensure that the old behavior is maintained, I
>>> am wondering if it is not better to keep the old test as is (which
>>> also tests the .cfi_window_save *together with* the .cfi_b_key_frame
>>> directives). WDYT ?
>>>
>>
>> The old behavior is not going to be maintained with GCC 15 and newer.
>> From my perspective, the purpose of this test is to test the behavior
>> of A and B keys with the *correct* AArch64 CFI directives. That's the
>> reason why I added a separate test cfi-sframe-aarch64-3.s to ensure
>> that the old directive is still correctly parsed and processed.
>
> We do need to ensure that a new version of gas works with old versions
> of GCC, so if GCC-14 or earlier emit the wrong directive we will need to
> continue to accept it until such versions are truly obsolete (a long time).
>
> R.
The new test cfi-sframe-aarch64-3.s is introduced to ensure that GCC-14
or earlier continue to accept .cfi_window_save as it used to be. This is
the backward compatibility test.
.cfi_window_save can be tested separately from the A and B keys. The
.cfi_b_key_frame directive was not changed, so it is working as it
always has worked.
The test cfi-sframe-aarch64-pac-ab-key-1.s was updated to use the new
directive. I chose to replace it because we are supposed to use
.cfi_negate_ra_state instead. If someone looks at the tests, he will see
.cfi_negate_ra_state which is used everywhere except in a backward
compatibility test, so it is clearly *the recommended way*.
In the same way, in the previous commit, pac_ab_key.s was updated to use
.cfi_negate_ra_state and a new test pac_compat_cfi_window_save.s was
added to ensure the backward compatibility.
I hope the situation is clearer now. I probably should have explained it
better in the commit message.
Matthieu
>>
>> Matthieu.
>>
>>>> diff --git a/gas/testsuite/gas/cfi-sframe/cfi-sframe-aarch64-pac-ab-
>>>> key-1.s b/gas/testsuite/gas/cfi-sframe/cfi-sframe-aarch64-pac-ab-
>>>> key-1.s
>>>> index d9a408c668c..84230a99e3c 100644
>>>> --- a/gas/testsuite/gas/cfi-sframe/cfi-sframe-aarch64-pac-ab-key-1.s
>>>> +++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-aarch64-pac-ab-key-1.s
>>>> @@ -8,12 +8,12 @@ _Z5foo_av:
>>>> .LFB0:
>>>> .cfi_startproc
>>>> hint 25 // paciasp
>>>> - .cfi_window_save
>>>> + .cfi_negate_ra_state
>>>> stp x29, x30, [sp, -16]!
>>>> .cfi_def_cfa_offset 16
>>>> .cfi_offset 29, -16
>>>> .cfi_offset 30, -8
>>>> - ret
>>>> + ret
>>>> .cfi_endproc
>>>> .LFE0:
>>>> .size _Z5foo_av, .-_Z5foo_av
>>>> @@ -25,12 +25,12 @@ _Z5foo_bv:
>>>> .cfi_startproc
>>>> .cfi_b_key_frame
>>>> hint 27 // pacibsp
>>>> - .cfi_window_save
>>>> + .cfi_negate_ra_state
>>>> stp x29, x30, [sp, -16]!
>>>> .cfi_def_cfa_offset 16
>>>> .cfi_offset 29, -16
>>>> .cfi_offset 30, -8
>>>> nop
>>>> nop
>>>> - ret
>>>> + ret
>>>> .cfi_endproc
>>>> diff --git a/gas/testsuite/gas/cfi-sframe/cfi-sframe.exp b/gas/
>>>> testsuite/gas/cfi-sframe/cfi-sframe.exp
>>>> index cfae28d39aa..c646b109895 100644
>>>> --- a/gas/testsuite/gas/cfi-sframe/cfi-sframe.exp
>>>> +++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe.exp
>>>> @@ -97,5 +97,6 @@ if { [istarget "x86_64-*-*"] &&
>>>> [gas_sframe_check] } then {
>>>> if { [istarget "aarch64*-*-*"] && [gas_sframe_check] } then {
>>>> run_dump_test "cfi-sframe-aarch64-1"
>>>> run_dump_test "cfi-sframe-aarch64-2"
>>>> + run_dump_test "cfi-sframe-aarch64-3"
>>>> run_dump_test "cfi-sframe-aarch64-pac-ab-key-1"
>>>> }
>>>
>>
>
On 25/11/2024 16:28, Matthieu Longo wrote:
> ARMv8.3 addded support for a new security feature named Pointer
> Authentication. Support for this feature in SFrame already exists,
> but is relying on the deprecated "AArch64" (actually Sparc) CFI
> directive .cfi_gnu_window_save. .cfi_negate_ra_state CFI directive
> should be used instead to convey this information.
>
> In GCC 14 and older, the Sparc DWARF extension .cfi_gnu_window_save
> is emitted instead of .cfi_negate_ra_state.
> GCC 15 fixed this issue, but this behavior is preserved for backward
> compatibility.
>
> The existing sframe test for AArch64 PAC was using .cfi_gnu_window_save.
> This patch replaces this CFI in the existing test by the preferred one,
> and adds a new test to check for backward compatibility when using
> .cfi_gnu_window_save.
> ---
> .../gas/cfi-sframe/cfi-sframe-aarch64-3.d | 20 ++++++++++++++++++
> .../gas/cfi-sframe/cfi-sframe-aarch64-3.s | 21 +++++++++++++++++++
> .../cfi-sframe-aarch64-pac-ab-key-1.s | 8 +++----
> gas/testsuite/gas/cfi-sframe/cfi-sframe.exp | 1 +
> 4 files changed, 46 insertions(+), 4 deletions(-)
> create mode 100644 gas/testsuite/gas/cfi-sframe/cfi-sframe-aarch64-3.d
> create mode 100644 gas/testsuite/gas/cfi-sframe/cfi-sframe-aarch64-3.s
>
> diff --git a/gas/testsuite/gas/cfi-sframe/cfi-sframe-aarch64-3.d b/gas/testsuite/gas/cfi-sframe/cfi-sframe-aarch64-3.d
> new file mode 100644
> index 00000000000..f72b70a970a
> --- /dev/null
> +++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-aarch64-3.d
> @@ -0,0 +1,20 @@
> +#as: --gsframe
> +#objdump: --sframe=.sframe
> +#name: SFrame cfi_negate_ra_state test (using cfi_window_save)
> +#...
> +Contents of the SFrame section .sframe:
> +
> + Header :
> +
> + Version: SFRAME_VERSION_2
> + Flags: NONE
> + Num FDEs: 1
> + Num FREs: 2
> +
> + Function Index :
> + func idx \[0\]: pc = 0x0, size = 8 bytes
> + STARTPC + CFA + FP + RA +
> +#...
> + 0+0004 +sp\+16 +u +u\[s\] +
> +
> +#pass
> diff --git a/gas/testsuite/gas/cfi-sframe/cfi-sframe-aarch64-3.s b/gas/testsuite/gas/cfi-sframe/cfi-sframe-aarch64-3.s
> new file mode 100644
> index 00000000000..df6ff6e8c8f
> --- /dev/null
> +++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-aarch64-3.s
> @@ -0,0 +1,21 @@
> +## ARMv8.3 addded support a new security feature named Pointer Authentication. The
You refer in the rest of the sentence to the feature in the present tense, so I think it's more natural to use the present tense at the start as well, so...
Armv8.3 adds support for a new ...
> +## main idea behind this is to use the unused bits in the pointer values.
> +## Each pointer is patched with a PAC before writing to memory, and is verified
> +## before using it.
> +## When the pointers are mangled, the stack trace generator needs to know so it
> +## can mask off the PAC from the pointer value to recover the return address,
> +## and conversely, skip doing so if the pointers are not mangled.
> +##
> +## .cfi_negate_ra_state CFI directive is usually used to convey this information.
> +## In GCC 14 and older, the Sparc DWARF extension .cfi_window_save is emitted
> +## instead of .cfi_negate_ra_state. GCC 15 fixed this issue, but this behavior
> +## is preserved in binutils for backward compatibility.
So the key reason for this confusion is that .cfi_negate_ra_state and .cfi_window_save are in the processor-specific numbering space, but use the same code value in the dwarf tables. It was because of this that older versions of GCC were mapping this to a single name rather than emitting the correct name on aarch64. The consequence of this is that there is no change to the object file created when the source is assembled, but it is a portability improvement as we cannot require other assemblers to accept the SPARC directive when assembling for AArch64. Nevertheless, in order to ensure that gas remains compatible with existing GCC releases we need to handle the SPARC name as well in gas, hence this test.
> +##
> +## SFrame has support for this. This testcase ensures that the directive
> +## is interpreted successfully.
> + .cfi_startproc
> + .long 0
> + .cfi_def_cfa_offset 16
> + .cfi_window_save
Add a comment on this line for clarity?
+ .cfi_window_save // really .cfi_negate_ra_state
> + .long 0
> + .cfi_endproc
> diff --git a/gas/testsuite/gas/cfi-sframe/cfi-sframe-aarch64-pac-ab-key-1.s b/gas/testsuite/gas/cfi-sframe/cfi-sframe-aarch64-pac-ab-key-1.s
> index d9a408c668c..84230a99e3c 100644
> --- a/gas/testsuite/gas/cfi-sframe/cfi-sframe-aarch64-pac-ab-key-1.s
> +++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-aarch64-pac-ab-key-1.s
> @@ -8,12 +8,12 @@ _Z5foo_av:
> .LFB0:
> .cfi_startproc
> hint 25 // paciasp
> - .cfi_window_save
> + .cfi_negate_ra_state
> stp x29, x30, [sp, -16]!
> .cfi_def_cfa_offset 16
> .cfi_offset 29, -16
> .cfi_offset 30, -8
> - ret
> + ret
> .cfi_endproc
> .LFE0:
> .size _Z5foo_av, .-_Z5foo_av
> @@ -25,12 +25,12 @@ _Z5foo_bv:
> .cfi_startproc
> .cfi_b_key_frame
> hint 27 // pacibsp
> - .cfi_window_save
> + .cfi_negate_ra_state
> stp x29, x30, [sp, -16]!
> .cfi_def_cfa_offset 16
> .cfi_offset 29, -16
> .cfi_offset 30, -8
> nop
> nop
> - ret
> + ret
> .cfi_endproc
> diff --git a/gas/testsuite/gas/cfi-sframe/cfi-sframe.exp b/gas/testsuite/gas/cfi-sframe/cfi-sframe.exp
> index cfae28d39aa..c646b109895 100644
> --- a/gas/testsuite/gas/cfi-sframe/cfi-sframe.exp
> +++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe.exp
> @@ -97,5 +97,6 @@ if { [istarget "x86_64-*-*"] && [gas_sframe_check] } then {
> if { [istarget "aarch64*-*-*"] && [gas_sframe_check] } then {
> run_dump_test "cfi-sframe-aarch64-1"
> run_dump_test "cfi-sframe-aarch64-2"
> + run_dump_test "cfi-sframe-aarch64-3"
> run_dump_test "cfi-sframe-aarch64-pac-ab-key-1"
> }
new file mode 100644
@@ -0,0 +1,20 @@
+#as: --gsframe
+#objdump: --sframe=.sframe
+#name: SFrame cfi_negate_ra_state test (using cfi_window_save)
+#...
+Contents of the SFrame section .sframe:
+
+ Header :
+
+ Version: SFRAME_VERSION_2
+ Flags: NONE
+ Num FDEs: 1
+ Num FREs: 2
+
+ Function Index :
+ func idx \[0\]: pc = 0x0, size = 8 bytes
+ STARTPC + CFA + FP + RA +
+#...
+ 0+0004 +sp\+16 +u +u\[s\] +
+
+#pass
new file mode 100644
@@ -0,0 +1,21 @@
+## ARMv8.3 addded support a new security feature named Pointer Authentication. The
+## main idea behind this is to use the unused bits in the pointer values.
+## Each pointer is patched with a PAC before writing to memory, and is verified
+## before using it.
+## When the pointers are mangled, the stack trace generator needs to know so it
+## can mask off the PAC from the pointer value to recover the return address,
+## and conversely, skip doing so if the pointers are not mangled.
+##
+## .cfi_negate_ra_state CFI directive is usually used to convey this information.
+## In GCC 14 and older, the Sparc DWARF extension .cfi_window_save is emitted
+## instead of .cfi_negate_ra_state. GCC 15 fixed this issue, but this behavior
+## is preserved in binutils for backward compatibility.
+##
+## SFrame has support for this. This testcase ensures that the directive
+## is interpreted successfully.
+ .cfi_startproc
+ .long 0
+ .cfi_def_cfa_offset 16
+ .cfi_window_save
+ .long 0
+ .cfi_endproc
@@ -8,12 +8,12 @@ _Z5foo_av:
.LFB0:
.cfi_startproc
hint 25 // paciasp
- .cfi_window_save
+ .cfi_negate_ra_state
stp x29, x30, [sp, -16]!
.cfi_def_cfa_offset 16
.cfi_offset 29, -16
.cfi_offset 30, -8
- ret
+ ret
.cfi_endproc
.LFE0:
.size _Z5foo_av, .-_Z5foo_av
@@ -25,12 +25,12 @@ _Z5foo_bv:
.cfi_startproc
.cfi_b_key_frame
hint 27 // pacibsp
- .cfi_window_save
+ .cfi_negate_ra_state
stp x29, x30, [sp, -16]!
.cfi_def_cfa_offset 16
.cfi_offset 29, -16
.cfi_offset 30, -8
nop
nop
- ret
+ ret
.cfi_endproc
@@ -97,5 +97,6 @@ if { [istarget "x86_64-*-*"] && [gas_sframe_check] } then {
if { [istarget "aarch64*-*-*"] && [gas_sframe_check] } then {
run_dump_test "cfi-sframe-aarch64-1"
run_dump_test "cfi-sframe-aarch64-2"
+ run_dump_test "cfi-sframe-aarch64-3"
run_dump_test "cfi-sframe-aarch64-pac-ab-key-1"
}