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:
>
> * gcc/config/aarch64/aarch64-builtins.cc (check_general_builtin_call):
> New.
> * gcc/config/aarch64/aarch64-c.cc (aarch64_check_builtin_call):
> Add check_general_builtin_call call.
> * gcc/config/aarch64/aarch64-protos.h (check_general_builtin_call):
> New.
>
> gcc/testsuite/ChangeLog:
>
> * gcc/testsuite/gcc.target/aarch64/acle/rwsr-2.c: New.
> ---
> gcc/config/aarch64/aarch64-builtins.cc | 33 +++++++++++++++++++
> gcc/config/aarch64/aarch64-c.cc | 4 +--
> gcc/config/aarch64/aarch64-protos.h | 3 ++
> .../gcc.target/aarch64/acle/rwsr-2.c | 15 +++++++++
> 4 files changed, 53 insertions(+), 2 deletions(-)
> create mode 100644 gcc/testsuite/gcc.target/aarch64/acle/rwsr-2.c
>
> diff --git a/gcc/config/aarch64/aarch64-builtins.cc
> b/gcc/config/aarch64/aarch64-builtins.cc
> index d8bb2a989a5..6734361f4f4 100644
> --- a/gcc/config/aarch64/aarch64-builtins.cc
> +++ b/gcc/config/aarch64/aarch64-builtins.cc
> @@ -2126,6 +2126,39 @@ aarch64_general_builtin_decl (unsigned code, bool)
> return aarch64_builtin_decls[code];
> }
>
> +bool
> +check_general_builtin_call (location_t location, vec<location_t>,
> + unsigned int code, tree fndecl,
> + unsigned int nargs ATTRIBUTE_UNUSED, tree *args)
> +{
How about aarch64_general_check_builtin_call? It's better to use
aarch64_* prefixes where possible.
> + 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]) == VAR_DECL
> + || TREE_CODE (TREE_TYPE (args[0])) != POINTER_TYPE
> + || TREE_CODE (TREE_OPERAND (TREE_OPERAND (args[0], 0) , 0))
> + != STRING_CST)
Similarly to the expand code in 5/7, I think this should check
positively for specific tree codes rather than negatively for a
VAR_DECL. That is, we should ensure TREE_CODE (x) is something
(rather than isn't something) before accessing TREE_OPERAND (x, 0).
> + {
> + const char *fn_name, *err_msg;
> + fn_name = IDENTIFIER_POINTER (DECL_NAME (fndecl));
> + err_msg = "first argument to %<%s%> must be a string literal";
> + error_at (location, err_msg, fn_name);
The error message needs to remain part of the error_at call,
since being in error_at ensures that it gets picked up for translation.
It's simpler to use %qD rather than %<%s%>, and pass fndecl directly.
> + 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..c2a9a59df73 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 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 a134e2fcf8e..9ef96ff511f 100644
> --- a/gcc/config/aarch64/aarch64-protos.h
> +++ b/gcc/config/aarch64/aarch64-protos.h
> @@ -990,6 +990,9 @@ tree aarch64_general_builtin_rsqrt (unsigned int);
> void handle_arm_acle_h (void);
> void handle_arm_neon_h (void);
>
> +bool 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-2.c
> b/gcc/testsuite/gcc.target/aarch64/acle/rwsr-2.c
> new file mode 100644
> index 00000000000..72e5fb75b21
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/aarch64/acle/rwsr-2.c
> @@ -0,0 +1,15 @@
> +/* Test the __arm_[r,w]sr ACLE intrinsics family. */
> +/* Ensure that illegal behavior is rejected by the compiler. */
> +
> +/* { dg-do compile } */
> +/* { dg-options "-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" } */
> +}
It would be good to have a check with a nullptr argument too. That'll
need -std=c2x.
Thanks,
Richard
