Matthieu Longo <matthieu.lo...@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
