[v1,3/4] aarch64 testsuite: explain expectections for pr94515* tests
Checks
| Context |
Check |
Description |
| linaro-tcwg-bot/tcwg_gcc_build--master-arm |
fail
|
Build failed
|
| linaro-tcwg-bot/tcwg_gcc_build--master-aarch64 |
success
|
Build passed
|
| linaro-tcwg-bot/tcwg_gcc_check--master-aarch64 |
success
|
Test passed
|
Commit Message
gcc/testsuite/ChangeLog:
* g++.target/aarch64/pr94515-1.C: Improve test documentation.
* g++.target/aarch64/pr94515-2.C: Same.
---
gcc/testsuite/g++.target/aarch64/pr94515-1.C | 8 ++++++
gcc/testsuite/g++.target/aarch64/pr94515-2.C | 28 +++++++++++++++-----
2 files changed, 30 insertions(+), 6 deletions(-)
Comments
Matthieu Longo <matthieu.longo@arm.com> writes:
> gcc/testsuite/ChangeLog:
>
> * g++.target/aarch64/pr94515-1.C: Improve test documentation.
> * g++.target/aarch64/pr94515-2.C: Same.
The patch is OK as-is, since it's clearly a strict improvement over
the status quo. But a suggestion below:
> ---
> gcc/testsuite/g++.target/aarch64/pr94515-1.C | 8 ++++++
> gcc/testsuite/g++.target/aarch64/pr94515-2.C | 28 +++++++++++++++-----
> 2 files changed, 30 insertions(+), 6 deletions(-)
>
> diff --git a/gcc/testsuite/g++.target/aarch64/pr94515-2.C b/gcc/testsuite/g++.target/aarch64/pr94515-2.C
> index f4a3333beed..af6b128b8fd 100644
> --- a/gcc/testsuite/g++.target/aarch64/pr94515-2.C
> +++ b/gcc/testsuite/g++.target/aarch64/pr94515-2.C
> @@ -6,6 +6,7 @@
> volatile int zero = 0;
> int global = 0;
>
> +/* This is a leaf function, so no .cfi_negate_ra_state directive is expected. */
> __attribute__((noinline))
> int bar(void)
> {
> @@ -13,29 +14,44 @@ int bar(void)
> return 0;
> }
>
> +/* This function does not return normally, so the address is signed but no
> + * authentication code is emitted. It means that only one CFI directive is
> + * supposed to be emitted at signing time. */
> __attribute__((noinline, noreturn))
> void unwind (void)
> {
> throw 42;
> }
>
> +/* This function has several return instructions, and alternates different RA
> + * states. 4 .cfi_negate_ra_state and a .cfi_remember_state/.cfi_restore_state
> + * should be emitted.
> + */
> __attribute__((noinline, noipa))
> int test(int x)
> {
> - if (x==1) return 2; /* This return path may not use the stack. */
> + // This return path may not use the stack. This means that the return address
> + // won't be signed.
> + if (x==1) return 2;
> +
> + // All the return paths of the code below must have RA mangle state set, and
> + // the return address must be signed.
> int y = bar();
> if (y > global) global=y;
> - if (y==3) unwind(); /* This return path must have RA mangle state set. */
> - return 0;
> + if (y==3) unwind(); // authentication of the return address is not required.
> + return 0; // authentication of the return address is required.
> }
I wasn't sure from reading this why it would give 4 .cfi_negate_ra_states,
and had to run the test to find out. I think a key part is that the
current block layout is:
A: path to return 0 without assignment to global
B: global=y + branch back into A
C: return 2
D: unwind
so we have:
A: sign -> authenticate [2 negate_ra_states + remember_state for B]
B: signed [restore_state]
C: unsigned [negate_ra_state]
D: signed [negate_ra_state]
If the order had been ABDC then we would only need 3 negate_ra_states.
If the x==1 branch had been:
cmp w0, 1
bne .L10
mov w0, 2
ret
.L10:
followed by the rest of A, B and D, then we would only have needed
2 negate_ra_states.
So it might be helpful to give the block layout above too.
Like I say, it's just a suggestion though.
Thanks,
Richard
> +/* This function requires only 2 .cfi_negate_ra_state. */
> int main ()
> {
> + // paciasp -> cfi_negate_ra_state: RA_no_signing -> RA_signing_SP
> try {
> test (zero);
> - __builtin_abort ();
> + __builtin_abort (); // authentication of the return address is not required.
> } catch (...) {
> + // autiasp -> cfi_negate_ra_state: RA_signing_SP -> RA_no_signing
> return 0;
> }
> - __builtin_abort ();
> -}
> + __builtin_abort (); // authentication of the return address is not required.
> +}
> \ No newline at end of file
On 2024-08-13 16:53, Richard Sandiford wrote:
> Matthieu Longo <matthieu.longo@arm.com> writes:
>> gcc/testsuite/ChangeLog:
>>
>> * g++.target/aarch64/pr94515-1.C: Improve test documentation.
>> * g++.target/aarch64/pr94515-2.C: Same.
>
> The patch is OK as-is, since it's clearly a strict improvement over
> the status quo. But a suggestion below:
>
>> ---
>> gcc/testsuite/g++.target/aarch64/pr94515-1.C | 8 ++++++
>> gcc/testsuite/g++.target/aarch64/pr94515-2.C | 28 +++++++++++++++-----
>> 2 files changed, 30 insertions(+), 6 deletions(-)
>>
>> diff --git a/gcc/testsuite/g++.target/aarch64/pr94515-2.C b/gcc/testsuite/g++.target/aarch64/pr94515-2.C
>> index f4a3333beed..af6b128b8fd 100644
>> --- a/gcc/testsuite/g++.target/aarch64/pr94515-2.C
>> +++ b/gcc/testsuite/g++.target/aarch64/pr94515-2.C
>> @@ -6,6 +6,7 @@
>> volatile int zero = 0;
>> int global = 0;
>>
>> +/* This is a leaf function, so no .cfi_negate_ra_state directive is expected. */
>> __attribute__((noinline))
>> int bar(void)
>> {
>> @@ -13,29 +14,44 @@ int bar(void)
>> return 0;
>> }
>>
>> +/* This function does not return normally, so the address is signed but no
>> + * authentication code is emitted. It means that only one CFI directive is
>> + * supposed to be emitted at signing time. */
>> __attribute__((noinline, noreturn))
>> void unwind (void)
>> {
>> throw 42;
>> }
>>
>> +/* This function has several return instructions, and alternates different RA
>> + * states. 4 .cfi_negate_ra_state and a .cfi_remember_state/.cfi_restore_state
>> + * should be emitted.
>> + */
>> __attribute__((noinline, noipa))
>> int test(int x)
>> {
>> - if (x==1) return 2; /* This return path may not use the stack. */
>> + // This return path may not use the stack. This means that the return address
>> + // won't be signed.
>> + if (x==1) return 2;
>> +
>> + // All the return paths of the code below must have RA mangle state set, and
>> + // the return address must be signed.
>> int y = bar();
>> if (y > global) global=y;
>> - if (y==3) unwind(); /* This return path must have RA mangle state set. */
>> - return 0;
>> + if (y==3) unwind(); // authentication of the return address is not required.
>> + return 0; // authentication of the return address is required.
>> }
>
> I wasn't sure from reading this why it would give 4 .cfi_negate_ra_states,
> and had to run the test to find out. I think a key part is that the
> current block layout is:
>
> A: path to return 0 without assignment to global
> B: global=y + branch back into A
> C: return 2
> D: unwind
>
> so we have:
>
> A: sign -> authenticate [2 negate_ra_states + remember_state for B]
> B: signed [restore_state]
> C: unsigned [negate_ra_state]
> D: signed [negate_ra_state]
>
> If the order had been ABDC then we would only need 3 negate_ra_states.
> If the x==1 branch had been:
>
> cmp w0, 1
> bne .L10
> mov w0, 2
> ret
> .L10:
>
> followed by the rest of A, B and D, then we would only have needed
> 2 negate_ra_states.
>
> So it might be helpful to give the block layout above too.
> Like I say, it's just a suggestion though.
>
> Thanks,
> Richard
Done in the next revision.
Thanks,
Matthieu
>
>> +/* This function requires only 2 .cfi_negate_ra_state. */
>> int main ()
>> {
>> + // paciasp -> cfi_negate_ra_state: RA_no_signing -> RA_signing_SP
>> try {
>> test (zero);
>> - __builtin_abort ();
>> + __builtin_abort (); // authentication of the return address is not required.
>> } catch (...) {
>> + // autiasp -> cfi_negate_ra_state: RA_signing_SP -> RA_no_signing
>> return 0;
>> }
>> - __builtin_abort ();
>> -}
>> + __builtin_abort (); // authentication of the return address is not required.
>> +}
>> \ No newline at end of file
@@ -15,12 +15,20 @@ void unwind (void)
__attribute__((noinline, noipa, target("branch-protection=pac-ret")))
int test (int z)
{
+ // paciasp -> cfi_negate_ra_state: RA_no_signing -> RA_signing_SP
if (z) {
asm volatile ("":::"x20","x21");
unwind ();
+ // autiasp -> cfi_negate_ra_state: RA_signing_SP -> RA_no_signing
return 1;
} else {
+ // 2nd cfi_negate_ra_state because the CFI directives are processed linearily.
+ // At this point, the unwinder would believe that the address is not signed
+ // due to the previous return. That's why the compiler has to emit second
+ // cfi_negate_ra_state to mean that the return address is still signed.
+ // cfi_negate_ra_state: RA_no_signing -> RA_signing_SP
unwind ();
+ // autiasp -> cfi_negate_ra_state: RA_signing_SP -> RA_no_signing
return 2;
}
}
@@ -6,6 +6,7 @@
volatile int zero = 0;
int global = 0;
+/* This is a leaf function, so no .cfi_negate_ra_state directive is expected. */
__attribute__((noinline))
int bar(void)
{
@@ -13,29 +14,44 @@ int bar(void)
return 0;
}
+/* This function does not return normally, so the address is signed but no
+ * authentication code is emitted. It means that only one CFI directive is
+ * supposed to be emitted at signing time. */
__attribute__((noinline, noreturn))
void unwind (void)
{
throw 42;
}
+/* This function has several return instructions, and alternates different RA
+ * states. 4 .cfi_negate_ra_state and a .cfi_remember_state/.cfi_restore_state
+ * should be emitted.
+ */
__attribute__((noinline, noipa))
int test(int x)
{
- if (x==1) return 2; /* This return path may not use the stack. */
+ // This return path may not use the stack. This means that the return address
+ // won't be signed.
+ if (x==1) return 2;
+
+ // All the return paths of the code below must have RA mangle state set, and
+ // the return address must be signed.
int y = bar();
if (y > global) global=y;
- if (y==3) unwind(); /* This return path must have RA mangle state set. */
- return 0;
+ if (y==3) unwind(); // authentication of the return address is not required.
+ return 0; // authentication of the return address is required.
}
+/* This function requires only 2 .cfi_negate_ra_state. */
int main ()
{
+ // paciasp -> cfi_negate_ra_state: RA_no_signing -> RA_signing_SP
try {
test (zero);
- __builtin_abort ();
+ __builtin_abort (); // authentication of the return address is not required.
} catch (...) {
+ // autiasp -> cfi_negate_ra_state: RA_signing_SP -> RA_no_signing
return 0;
}
- __builtin_abort ();
-}
+ __builtin_abort (); // authentication of the return address is not required.
+}
\ No newline at end of file