Victor Do Nascimento <victor.donascime...@arm.com> writes:
> In implementing the ACLE read/write system register builtins it was
> observed that leaving argument type checking to be done at expand-time
> meant that poorly-formed function calls were being "fixed" by certain
> optimization passes, meaning bad code wasn't being properly picked up
> in checking.
>
> Example:
>
> const char *regname = "amcgcr_el0";
> long long a = __builtin_aarch64_rsr64 (regname);
>
> is reduced by the ccp1 pass to
>
> long long a = __builtin_aarch64_rsr64 ("amcgcr_el0");
>
> As these functions require an argument of STRING_CST type, there needs
> to be a check carried out by the front-end capable of picking this up.
>
> The introduced `check_general_builtin_call' function will be called by
> the TARGET_CHECK_BUILTIN_CALL hook whenever a call to a builtin
> belonging to the AARCH64_BUILTIN_GENERAL category is encountered,
> carrying out any appropriate checks associated with a particular
> builtin function code.
>
> gcc/ChangeLog:
>
> * config/aarch64/aarch64-builtins.cc (check_general_builtin_call):
> New.
> * config/aarch64/aarch64-c.cc (aarch64_check_builtin_call):
> Add check_general_builtin_call call.
> * config/aarch64/aarch64-protos.h (check_general_builtin_call):
> New.
>
> gcc/testsuite/ChangeLog:
>
> * gcc.target/aarch64/acle/rwsr-3.c: New.
> ---
> gcc/config/aarch64/aarch64-builtins.cc | 31 +++++++++++++++++++
> gcc/config/aarch64/aarch64-c.cc | 4 +--
> gcc/config/aarch64/aarch64-protos.h | 4 +++
> .../gcc.target/aarch64/acle/rwsr-3.c | 18 +++++++++++
> 4 files changed, 55 insertions(+), 2 deletions(-)
> create mode 100644 gcc/testsuite/gcc.target/aarch64/acle/rwsr-3.c
>
> diff --git a/gcc/config/aarch64/aarch64-builtins.cc
> b/gcc/config/aarch64/aarch64-builtins.cc
> index dd76cca611b..c5f20f68bca 100644
> --- a/gcc/config/aarch64/aarch64-builtins.cc
> +++ b/gcc/config/aarch64/aarch64-builtins.cc
> @@ -2127,6 +2127,37 @@ aarch64_general_builtin_decl (unsigned code, bool)
> return aarch64_builtin_decls[code];
> }
>
> +bool
> +aarch64_check_general_builtin_call (location_t location, vec<location_t>,
> + unsigned int code, tree fndecl,
> + unsigned int nargs ATTRIBUTE_UNUSED, tree *args)
I'd prefer aarch64_general_check_builtin_call, to avoid breaking up
the name of the target hook. "aarch64_general" is kind of a prefix here,
to distinguish it from aarch64_sve::
> +{
> + switch (code)
> + {
> + case AARCH64_RSR:
> + case AARCH64_RSRP:
> + case AARCH64_RSR64:
> + case AARCH64_RSRF:
> + case AARCH64_RSRF64:
> + case AARCH64_WSR:
> + case AARCH64_WSRP:
> + case AARCH64_WSR64:
> + case AARCH64_WSRF:
> + case AARCH64_WSRF64:
> + if (TREE_CODE (args[0]) != NOP_EXPR
It's probably best not to require a NOP_EXPR. Let's just accept one
if it's there. The easiest way of doing that is:
{
rtx addr = STRIP_NOPS (args[0]);
and then checking "addr".
> + || TREE_CODE (TREE_TYPE (args[0])) != POINTER_TYPE
> + || (TREE_CODE (TREE_OPERAND (TREE_OPERAND (args[0], 0) , 0))
> + != STRING_CST))
We need to check what TREE_OPERAND (args[0], 0) is before using
TREE_OPERAND (TREE_OPERAND (args[0], 0), 0). I assume it's checking
for an ADDR_EXPR. (Also, minor formatting nit, but there should be
no space before ", 0".)
Looks good otherwise, thanks.
Richard
> + {
> + error_at (location, "first argument to %qD must be a string literal",
> + fndecl);
> + return false;
> + }
> + }
> + /* Default behavior. */
> + return true;
> +}
> +
> typedef enum
> {
> SIMD_ARG_COPY_TO_REG,
> diff --git a/gcc/config/aarch64/aarch64-c.cc b/gcc/config/aarch64/aarch64-c.cc
> index ab8844f6049..be8b7236cf9 100644
> --- a/gcc/config/aarch64/aarch64-c.cc
> +++ b/gcc/config/aarch64/aarch64-c.cc
> @@ -339,8 +339,8 @@ aarch64_check_builtin_call (location_t loc,
> vec<location_t> arg_loc,
> switch (code & AARCH64_BUILTIN_CLASS)
> {
> case AARCH64_BUILTIN_GENERAL:
> - return true;
> -
> + return aarch64_check_general_builtin_call (loc, arg_loc, subcode,
> + orig_fndecl, nargs, args);
> case AARCH64_BUILTIN_SVE:
> return aarch64_sve::check_builtin_call (loc, arg_loc, subcode,
> orig_fndecl, nargs, args);
> diff --git a/gcc/config/aarch64/aarch64-protos.h
> b/gcc/config/aarch64/aarch64-protos.h
> index 5d6a1e75700..dbd486cfea4 100644
> --- a/gcc/config/aarch64/aarch64-protos.h
> +++ b/gcc/config/aarch64/aarch64-protos.h
> @@ -990,6 +990,10 @@ tree aarch64_general_builtin_rsqrt (unsigned int);
> void handle_arm_acle_h (void);
> void handle_arm_neon_h (void);
>
> +bool aarch64_check_general_builtin_call (location_t, vec<location_t>,
> + unsigned int, tree, unsigned int,
> + tree *);
> +
> namespace aarch64_sve {
> void init_builtins ();
> void handle_arm_sve_h ();
> diff --git a/gcc/testsuite/gcc.target/aarch64/acle/rwsr-3.c
> b/gcc/testsuite/gcc.target/aarch64/acle/rwsr-3.c
> new file mode 100644
> index 00000000000..17038fefbf6
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/aarch64/acle/rwsr-3.c
> @@ -0,0 +1,18 @@
> +/* Test the __arm_[r,w]sr ACLE intrinsics family. */
> +/* Ensure that illegal behavior is rejected by the compiler. */
> +
> +/* { dg-do compile } */
> +/* { dg-options "-std=c2x -O3 -march=armv8.4-a" } */
> +
> +#include <arm_acle.h>
> +
> +void
> +test_non_const_sysreg_name ()
> +{
> + const char *regname = "trcseqstr";
> + long long a = __arm_rsr64 (regname); /* { dg-error "first argument to
> '__builtin_aarch64_rsr64' must be a string literal" } */
> + __arm_wsr64 (regname, a); /* { dg-error "first argument to
> '__builtin_aarch64_wsr64' must be a string literal" } */
> +
> + long long b = __arm_rsr64(nullptr); /* { dg-error "first argument to
> '__builtin_aarch64_rsr64' must be a string literal" } */
> + __arm_wsr64(nullptr, b); /* { dg-error "first argument to
> '__builtin_aarch64_wsr64' must be a string literal" } */
> +}
