On Fri, Jan 06, 2017 at 11:47:07AM +0000, Jiong Wang wrote:
> On 11/11/16 18:22, Jiong Wang wrote:
> gcc/
> 2017-01-06 Jiong Wang <jiong.w...@arm.com>
>
> * config/aarch64/aarch64-opts.h (aarch64_function_type): New enum.
> * config/aarch64/aarch64-protos.h
> (aarch64_return_address_signing_enabled): New declaration.
> * config/aarch64/aarch64.c (aarch64_return_address_signing_enabled):
> New function.
> (aarch64_expand_prologue): Sign return address before it's pushed onto
> stack.
> (aarch64_expand_epilogue): Authenticate return address fetched from
> stack.
> (aarch64_override_options): Sanity check for ILP32 and ISA level.
> (aarch64_attributes): New function attributes for
> "sign-return-address".
> * config/aarch64/aarch64.md (UNSPEC_AUTI1716, UNSPEC_AUTISP,
> UNSPEC_PACI1716, UNSPEC_PACISP, UNSPEC_XPACLRI): New unspecs.
> ("*do_return"): Generate combined instructions according to key index.
> ("<pauth_mnem_prefix>sp", "<pauth_mnem_prefix1716", "xpaclri"): New.
> * config/aarch64/iterators.md (PAUTH_LR_SP, PAUTH_17_16): New integer
> iterators.
> (pauth_mnem_prefix, pauth_hint_num_a): New integer attributes.
> * config/aarch64/aarch64.opt (msign-return-address=): New.
> * doc/extend.texi (AArch64 Function Attributes): Documents
> "sign-return-address=".
> * doc/invoke.texi (AArch64 Options): Documents
> "-msign-return-address=".
>
> gcc/testsuite/
> 2017-01-06 Jiong Wang <jiong.w...@arm.com>
>
> * gcc.target/aarch64/return_address_sign_1.c: New testcase.
> * gcc.target/aarch64/return_address_sign_scope_1.c: New testcase.
I have a few comments on this patch, mostly around the wording of comments.
> --- a/gcc/config/aarch64/aarch64.c
> +++ b/gcc/config/aarch64/aarch64.c
> @@ -3670,6 +3690,16 @@ aarch64_expand_epilogue (bool for_sibcall)
> RTX_FRAME_RELATED_P (insn) = 1;
> }
>
> + /* sibcall won't generate normally return, therefore we need to
> authenticate
> + at here. !TARGET_ARMV8_3 disallows GCC to use combined authentication
> + instruction which we prefer, so we need to generate a seperate
> + authentication. eh_return path can't do combined authentication, as it
> will
> + do extra stack adjustment which updates CFA to EH handler's CFA while we
> + want to use the CFA of the function which calls eh_return. */
This comment does not read correctly to me, and I'm not sure what you
mean by it. I'd rewrite it as so:
We prefer to emit the combined return/authenticate instruction RETAA,
however there are three cases in which we must instead emit an explicit
authentication instruction.
1) Sibcalls don't return in a normal way, so if we're about to
call one we must authenticate.
2) The RETAA instruction is not available before ARMv8.3-A, so if we
are generating code for !TARGET_ARMV8_3 we can't use it and must
explicitly authenticate.
3) On an eh_return path we make extra stack adjustments to update the
canonical frame address to be the exception handler's CFA. We want
to authenticate using the CFA of the function which calls eh_return.
> + if (aarch64_return_address_signing_enabled ()
> + && (for_sibcall || !TARGET_ARMV8_3 || crtl->calls_eh_return))
> + emit_insn (gen_autisp ());
> +
> /* Stack adjustment for exception handler. */
> if (crtl->calls_eh_return)
> {
> @@ -8894,6 +8924,9 @@ aarch64_override_options (void)
> error ("Assembler does not support -mabi=ilp32");
> #endif
>
> + if (aarch64_ra_sign_scope != AARCH64_FUNCTION_NONE && TARGET_ILP32)
> + error ("Return address signing is only supported on LP64");
Should this be a sorry? I think this is something which has not been
implemented rather than a fundamental restriction.
Maybe:
sorry ("Return address signing is only supported for -mabi=lp64");
Would be clear for our users.
> +
> /* Make sure we properly set up the explicit options. */
> if ((aarch64_cpu_string && valid_cpu)
> || (aarch64_tune_string && valid_tune))
> @@ -9277,6 +9310,8 @@ static const struct aarch64_attribute_info
> aarch64_attributes[] =
> { "cpu", aarch64_attr_custom, false, aarch64_handle_attr_cpu, OPT_mcpu_ },
> { "tune", aarch64_attr_custom, false, aarch64_handle_attr_tune,
> OPT_mtune_ },
> + { "sign-return-address", aarch64_attr_enum, false, NULL,
> + OPT_msign_return_address_ },
> { NULL, aarch64_attr_custom, false, NULL, OPT____ }
> };
>
> diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
> index
> bde42317f1bfd91a9d38a4cfa94d4cedd5246003..9b6f99dee3cecc0f15f24ec2375d3641fe03892d
> 100644
> --- a/gcc/config/aarch64/aarch64.md
> +++ b/gcc/config/aarch64/aarch64.md
> @@ -67,6 +67,8 @@
> )
>
> (define_c_enum "unspec" [
> + UNSPEC_AUTI1716
> + UNSPEC_AUTISP
> UNSPEC_CASESI
> UNSPEC_CRC32B
> UNSPEC_CRC32CB
> @@ -106,6 +108,8 @@
> UNSPEC_LD4_LANE
> UNSPEC_MB
> UNSPEC_NOP
> + UNSPEC_PACI1716
> + UNSPEC_PACISP
> UNSPEC_PRLG_STK
> UNSPEC_RBIT
> UNSPEC_SCVTF
> @@ -135,6 +139,7 @@
> UNSPEC_RSQRTE
> UNSPEC_RSQRTS
> UNSPEC_NZCV
> + UNSPEC_XPACLRI
> ])
>
> (define_c_enum "unspecv" [
> @@ -575,7 +580,14 @@
> (define_insn "*do_return"
> [(return)]
> ""
> - "ret"
> + {
> + if (aarch64_return_address_signing_enabled ()
> + && TARGET_ARMV8_3
> + && !crtl->calls_eh_return)
> + return "retaa";
> +
> + return "ret";
> + }
> [(set_attr "type" "branch")]
> )
>
> @@ -5341,6 +5353,31 @@
> [(set_attr "length" "0")]
> )
>
> +;; Pointer authentication patterns are available for all architectures. On
> +;; pre-ARMv8.3-A architectures they are NOP. This let the user write
> portable
> +;; software which can get pointer authentication on new hardware while still
> +;; runs correctly on old hardware though no pointer authentication
> protection.
This comment should be reworded slightly:
;; Pointer authentication patterns are always provided. In architecture
;; revisions prior to ARMv8.3-A these HINT instructions operate as NOPs.
;; This lets the user write portable software which authenticates pointers
;; when run on something which implements ARMv8.3-A, and which runs
;; correctly, but does not authenticate pointers, where ARMv8.3-A is not
;; implemented.
Each of these new instructions could do with a comment explaining
> diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
> index
> 30bdcf07ad8b1bd086bbb87acb36fb0333944087..47250ebb149bbde021ef64eba8e96e35c2623284
> 100644
> --- a/gcc/doc/extend.texi
> +++ b/gcc/doc/extend.texi
> @@ -3513,6 +3513,12 @@ Specifies the core for which to tune the performance
> of this function and also
> whose architectural features to use. The behavior and valid arguments are
> the
> same as for the @option{-mcpu=} command-line option.
>
> +@item sign-return-address
> +@cindex @code{sign-return-address} function attribute, AArch64
> +Select the function scope we want to do return address signing on. The
> behavior
> +and permissible arguments are the same as for the command-line option
> +@option{-msign-return-address=}. The default value is @code{none}
> +
This could do with rewording slightly:
Select the function scope on which return address signing will be applied.
The behaviour and permissible arguments are the same as for the
command-line option @option{-msign-return-address=}. The default value is
@code{none}.
> @end table
>
> The above target attributes can be specified as follows:
> diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
> index
> aa0ac8b41128483be53645b91bdcead096582e23..2792dbd0a2d0ed9e3f46237685f8ae65a5d8bcf5
> 100644
> --- a/gcc/doc/invoke.texi
> +++ b/gcc/doc/invoke.texi
> @@ -14044,6 +14044,13 @@ accessed using a single instruction and emitted
> after each function. This
> limits the maximum size of functions to 1MB. This is enabled by default for
> @option{-mcmodel=tiny}.
>
> +@item -msign-return-address=@var{scope}
> +@opindex msign-return-address
> +Select the function scope we want to do return address signing on.
> Permissible
> +values are @samp{none}, @samp{none-leaf} and @samp{all}. @samp{none} means
Typo: s/none-leaf/non-leaf/
> +return address signing is disabled. @samp{non-leaf} enables it for non-leaf
> +functions. @samp{all} for all functions and is the default value.
This documentation does not follow the patch. In the patch, the default is
"none".
I'd reword this as so:
Select the function scope on which return address signing will be applied.
Permissible values are @samp{none}, which disables return address signing,
@samp{non-leaf}, which enables pointer signing for functions which
are not leaf functions, and @samp{all}, which enables pointer signing for
all functions. The default value is @samp{none}.
> @end table
>
> @subsubsection @option{-march} and @option{-mcpu} Feature Modifiers
> diff --git a/gcc/testsuite/gcc.target/aarch64/return_address_sign_1.c
> b/gcc/testsuite/gcc.target/aarch64/return_address_sign_1.c
> new file mode 100644
> index
> 0000000000000000000000000000000000000000..9a0332abd41ca6ae994e6ed72771fca8ee532cd0
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/aarch64/return_address_sign_1.c
> @@ -0,0 +1,56 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -msign-return-address=all" } */
> +
> +int foo (int);
> +int bar (int, int);
> +
> +/* sibcall only. */
> +int __attribute__ ((target ("arch=armv8.3-a")))
> +func1 (int a, int b)
> +{
> + /* paciasp */
> + return foo (a + b);
> + /* autiasp */
> +}
> +
> +/* non-leaf function with sibcall. */
> +int __attribute__ ((target ("arch=armv8.3-a")))
> +func2 (int a, int b)
> +{
> + /* paciasp */
> + if (a < b)
> + return b;
> +
> + a = foo (b);
> +
> + return foo (a);
> + /* autiasp */
> +}
> +
> +/* non-leaf function, legacy arch. */
> +int __attribute__ ((target ("arch=armv8.2-a")))
> +func3 (int a, int b, int c)
> +{
> + /* paciasp */
> + return a + foo (b) + c;
> + /* autiasp */
> +}
> +
> +/* non-leaf function. */
> +int __attribute__ ((target ("arch=armv8.3-a")))
> +func4 (int a, int b, int c)
> +{
> + /* paciasp */
> + return a + foo (b) + c;
> + /* retab */
> +}
> +
> +int __attribute__ ((target ("arch=armv8.3-a, sign-return-address=none")))
> +func4_disable (int a, int b, int c, int d)
> +{
> + return c + bar (a, b) + d;
> +}
> +
> +/* { dg-final { scan-assembler-times "autiasp" 3 } } */
> +/* { dg-final { scan-assembler-times "paciasp" 4 } } */
> +/* { dg-final { scan-assembler-times "retaa" 1 } } */
These testcases should both be split.
As it stands, multiple buggy implementations could cancel each other out
when it comes to a scan-assembler-times and the test would still pass.
It would be much clearer to test each expected behaviour in a single file.
Thanks,
James
