diff mbox series

[V3,5/6] aarch64: Add front-end argument type checking for target builtins

Message ID 20231102163852.1860658-6-victor.donascimento@arm.com
State New
Headers show
Series aarch64: Add support for __arm_rsr and __arm_wsr ACLE function family | expand

Commit Message

Victor Do Nascimento Nov. 2, 2023, 4:38 p.m. UTC 
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

Comments

Richard Sandiford Nov. 28, 2023, 11:19 p.m. UTC | #1 
Victor Do Nascimento <victor.donascimento@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" } */
> +}
diff mbox series

Patch

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)
+{
+  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
+	  || TREE_CODE (TREE_TYPE (args[0])) != POINTER_TYPE
+	  || (TREE_CODE (TREE_OPERAND (TREE_OPERAND (args[0], 0) , 0))
+	      != STRING_CST))
+	{
+	  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" } */
+}