[AArch64] Validate and set default parameters for stack-clash. [Patch (3/3)]

Message ID 20180711112506.GA16112@arm.com
State New
Headers show
Series
  • [AArch64] Validate and set default parameters for stack-clash. [Patch (3/3)]
Related show

Commit Message

Tamar Christina July 11, 2018, 11:25 a.m.
Hi All,

This patch defines the default parameters and validation for the aarch64
stack clash probing interval and guard sizes.  It cleans up the previous
implementation and insures that at no point the invalidate arguments are
present in the pipeline for AArch64.  Currently they are only corrected once
cc1 initalizes the back-end.

The default for AArch64 is 64 KB for both of these and we only support 4 KB and 64 KB
probes.  We also enforce that any value you set here for the parameters must be
in sync.

If an invalid value is specified an error will be generated and compilation aborted.

Bootstrapped Regtested on aarch64-none-linux-gnu and no issues.
Target was tested with stack clash on and off by default.

Ok for trunk?

Thanks,
Tamar

gcc/
2018-07-11  Tamar Christina  <tamar.christina@arm.com>

	* common/config/aarch64/aarch64-common.c (TARGET_OPTION_DEFAULT_PARAM,
	aarch64_option_default_param):	New.
	(params.h): Include.
	(TARGET_OPTION_VALIDATE_PARAM, aarch64_option_validate_param): New.
	* config/aarch64/aarch64.c (aarch64_override_options_internal): Simplify
	stack-clash protection validation code.

--

Comments

Tamar Christina July 13, 2018, 4:35 p.m. | #1
Hi All,

I am sending an updated patch which takes into account a
case where the set parameter value would not be safe to call.

No change in the cover letter.

Regards,
Tamar

> -----Original Message-----
> From: Tamar Christina <tamar.christina@arm.com>
> Sent: Wednesday, July 11, 2018 12:25
> To: gcc-patches@gcc.gnu.org
> Cc: nd <nd@arm.com>; James Greenhalgh <James.Greenhalgh@arm.com>;
> Richard Earnshaw <Richard.Earnshaw@arm.com>; Marcus Shawcroft
> <Marcus.Shawcroft@arm.com>
> Subject: [PATCH][GCC][AArch64] Validate and set default parameters for
> stack-clash. [Patch (3/3)]
> 
> Hi All,
> 
> This patch defines the default parameters and validation for the aarch64
> stack clash probing interval and guard sizes.  It cleans up the previous
> implementation and insures that at no point the invalidate arguments are
> present in the pipeline for AArch64.  Currently they are only corrected once
> cc1 initalizes the back-end.
> 
> The default for AArch64 is 64 KB for both of these and we only support 4 KB
> and 64 KB probes.  We also enforce that any value you set here for the
> parameters must be in sync.
> 
> If an invalid value is specified an error will be generated and compilation
> aborted.
> 
> Bootstrapped Regtested on aarch64-none-linux-gnu and no issues.
> Target was tested with stack clash on and off by default.
> 
> Ok for trunk?
> 
> Thanks,
> Tamar
> 
> gcc/
> 2018-07-11  Tamar Christina  <tamar.christina@arm.com>
> 
> 	* common/config/aarch64/aarch64-common.c
> (TARGET_OPTION_DEFAULT_PARAM,
> 	aarch64_option_default_param):	New.
> 	(params.h): Include.
> 	(TARGET_OPTION_VALIDATE_PARAM,
> aarch64_option_validate_param): New.
> 	* config/aarch64/aarch64.c (aarch64_override_options_internal):
> Simplify
> 	stack-clash protection validation code.
> 
> --
diff --git a/gcc/common/config/aarch64/aarch64-common.c b/gcc/common/config/aarch64/aarch64-common.c
index 292fb818705d4650113da59a6d88cf2aa7c9e57d..30bbec1380d6db60475f0d770944af98f566773d 100644
--- a/gcc/common/config/aarch64/aarch64-common.c
+++ b/gcc/common/config/aarch64/aarch64-common.c
@@ -30,6 +30,7 @@
 #include "opts.h"
 #include "flags.h"
 #include "diagnostic.h"
+#include "params.h"
 
 #ifdef  TARGET_BIG_ENDIAN_DEFAULT
 #undef  TARGET_DEFAULT_TARGET_FLAGS
@@ -41,6 +42,10 @@
 
 #undef	TARGET_OPTION_OPTIMIZATION_TABLE
 #define TARGET_OPTION_OPTIMIZATION_TABLE aarch_option_optimization_table
+#undef TARGET_OPTION_DEFAULT_PARAMS
+#define TARGET_OPTION_DEFAULT_PARAMS aarch64_option_default_params
+#undef TARGET_OPTION_VALIDATE_PARAM
+#define TARGET_OPTION_VALIDATE_PARAM aarch64_option_validate_param
 
 /* Set default optimization options.  */
 static const struct default_options aarch_option_optimization_table[] =
@@ -60,6 +65,48 @@ static const struct default_options aarch_option_optimization_table[] =
     { OPT_LEVELS_NONE, 0, NULL, 0 }
   };
 
+/* Implement target validation TARGET_OPTION_DEFAULT_PARAM.  */
+
+static bool
+aarch64_option_validate_param (const int value, const int param)
+{
+  /* Check that both parameters are the same.  */
+  if (param == (int) PARAM_STACK_CLASH_PROTECTION_GUARD_SIZE)
+    {
+      if (value != 12 && value != 16)
+	{
+	error ("only values 12 (4 KB) and 16 (64 KB) are supported for guard "
+		"size.  Given value %d (%llu KB) is out of range.\n",
+		value, (1ULL << value) / 1024ULL);
+	return false;
+	}
+    }
+
+  return true;
+}
+
+/* Implement TARGET_OPTION_DEFAULT_PARAMS.  */
+
+static void
+aarch64_option_default_params (void)
+{
+  /* We assume the guard page is 64k.  */
+  int index = (int) PARAM_STACK_CLASH_PROTECTION_GUARD_SIZE;
+  if (!compiler_params[index].configure_value)
+     set_default_param_value (PARAM_STACK_CLASH_PROTECTION_GUARD_SIZE, 16);
+
+  int guard_size
+    = default_param_value (PARAM_STACK_CLASH_PROTECTION_GUARD_SIZE);
+
+  /* Set the interval parameter to be the same as the guard size.  This way the
+     mid-end code does the right thing for us.  */
+  set_default_param_value (PARAM_STACK_CLASH_PROTECTION_PROBE_INTERVAL,
+			   guard_size);
+
+  /* Validate the options.  */
+  aarch64_option_validate_param (guard_size, index);
+}
+
 /* Implement TARGET_HANDLE_OPTION.
    This function handles the target specific options for CPU/target selection.
 
diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index f2a4e0b3db62d9da87458bff98c16e8fb876f431..3fe0e47c561dfb7d1abf06a3322b4a6df63c7a21 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -10905,19 +10905,7 @@ aarch64_override_options_internal (struct gcc_options *opts)
 			 opts->x_param_values,
 			 global_options_set.x_param_values);
 
-  /* If the user hasn't change it via configure then set the default to 64 KB
-     for the backend.  */
-  if (DEFAULT_STK_CLASH_GUARD_SIZE == 0)
-      maybe_set_param_value (PARAM_STACK_CLASH_PROTECTION_GUARD_SIZE, 16,
-			    opts->x_param_values,
-			    global_options_set.x_param_values);
-
-  /* Validate the guard size.  */
   int guard_size = PARAM_VALUE (PARAM_STACK_CLASH_PROTECTION_GUARD_SIZE);
-  if (guard_size != 12 && guard_size != 16)
-      error ("only values 12 (4 KB) and 16 (64 KB) are supported for guard "
-	     "size.  Given value %d (%llu KB) is out of range.\n",
-	     guard_size, (1ULL << guard_size) / 1024ULL);
 
   /* Enforce that interval is the same size as size so the mid-end does the
      right thing.  */
Tamar Christina Aug. 7, 2018, 10:09 a.m. | #2
Hi All,

This is an updated patch which applies the same style changes as requested in patch 5/6.

Ok for trunk?

Thanks,
Tamar

gcc/
2018-08-07  Tamar Christina  <tamar.christina@arm.com>

	* common/config/aarch64/aarch64-common.c (TARGET_OPTION_DEFAULT_PARAM,
	aarch64_option_default_param):	New.
	(params.h): Include.
	(TARGET_OPTION_VALIDATE_PARAM, aarch64_option_validate_param): New.
	* config/aarch64/aarch64.c (aarch64_override_options_internal): Simplify
	stack-clash protection validation code.

> -----Original Message-----
> From: Tamar Christina
> Sent: Friday, July 13, 2018 17:36
> To: Tamar Christina <Tamar.Christina@arm.com>; gcc-patches@gcc.gnu.org
> Cc: nd <nd@arm.com>; James Greenhalgh <James.Greenhalgh@arm.com>;
> Richard Earnshaw <Richard.Earnshaw@arm.com>; Marcus Shawcroft
> <Marcus.Shawcroft@arm.com>
> Subject: RE: [PATCH][GCC][AArch64] Validate and set default parameters for
> stack-clash. [Patch (3/3)]
> 
> Hi All,
> 
> I am sending an updated patch which takes into account a case where the set
> parameter value would not be safe to call.
> 
> No change in the cover letter.
> 
> Regards,
> Tamar
> 
> > -----Original Message-----
> > From: Tamar Christina <tamar.christina@arm.com>
> > Sent: Wednesday, July 11, 2018 12:25
> > To: gcc-patches@gcc.gnu.org
> > Cc: nd <nd@arm.com>; James Greenhalgh <James.Greenhalgh@arm.com>;
> > Richard Earnshaw <Richard.Earnshaw@arm.com>; Marcus Shawcroft
> > <Marcus.Shawcroft@arm.com>
> > Subject: [PATCH][GCC][AArch64] Validate and set default parameters for
> > stack-clash. [Patch (3/3)]
> >
> > Hi All,
> >
> > This patch defines the default parameters and validation for the
> > aarch64 stack clash probing interval and guard sizes.  It cleans up
> > the previous implementation and insures that at no point the
> > invalidate arguments are present in the pipeline for AArch64.
> > Currently they are only corrected once
> > cc1 initalizes the back-end.
> >
> > The default for AArch64 is 64 KB for both of these and we only support
> > 4 KB and 64 KB probes.  We also enforce that any value you set here
> > for the parameters must be in sync.
> >
> > If an invalid value is specified an error will be generated and
> > compilation aborted.
> >
> > Bootstrapped Regtested on aarch64-none-linux-gnu and no issues.
> > Target was tested with stack clash on and off by default.
> >
> > Ok for trunk?
> >
> > Thanks,
> > Tamar
> >
> > gcc/
> > 2018-07-11  Tamar Christina  <tamar.christina@arm.com>
> >
> > 	* common/config/aarch64/aarch64-common.c
> > (TARGET_OPTION_DEFAULT_PARAM,
> > 	aarch64_option_default_param):	New.
> > 	(params.h): Include.
> > 	(TARGET_OPTION_VALIDATE_PARAM,
> > aarch64_option_validate_param): New.
> > 	* config/aarch64/aarch64.c (aarch64_override_options_internal):
> > Simplify
> > 	stack-clash protection validation code.
> >
> > --
diff --git a/gcc/common/config/aarch64/aarch64-common.c b/gcc/common/config/aarch64/aarch64-common.c
index 292fb818705d4650113da59a6d88cf2aa7c9e57d..97d7770190b8388ba277db96c07609f7b6f46817 100644
--- a/gcc/common/config/aarch64/aarch64-common.c
+++ b/gcc/common/config/aarch64/aarch64-common.c
@@ -30,6 +30,7 @@
 #include "opts.h"
 #include "flags.h"
 #include "diagnostic.h"
+#include "params.h"
 
 #ifdef  TARGET_BIG_ENDIAN_DEFAULT
 #undef  TARGET_DEFAULT_TARGET_FLAGS
@@ -41,6 +42,10 @@
 
 #undef	TARGET_OPTION_OPTIMIZATION_TABLE
 #define TARGET_OPTION_OPTIMIZATION_TABLE aarch_option_optimization_table
+#undef TARGET_OPTION_DEFAULT_PARAMS
+#define TARGET_OPTION_DEFAULT_PARAMS aarch64_option_default_params
+#undef TARGET_OPTION_VALIDATE_PARAM
+#define TARGET_OPTION_VALIDATE_PARAM aarch64_option_validate_param
 
 /* Set default optimization options.  */
 static const struct default_options aarch_option_optimization_table[] =
@@ -60,6 +65,49 @@ static const struct default_options aarch_option_optimization_table[] =
     { OPT_LEVELS_NONE, 0, NULL, 0 }
   };
 
+/* Implement target validation TARGET_OPTION_DEFAULT_PARAM.  */
+
+static bool
+aarch64_option_validate_param (const int value, const int param)
+{
+  /* Check that both parameters are the same.  */
+  if (param == (int) PARAM_STACK_CLASH_PROTECTION_GUARD_SIZE)
+    {
+      if (value != 12 && value != 16)
+	{
+	  error ("only values 12 (4 KB) and 16 (64 KB) are supported for guard "
+		 "size.  Given value %d (%llu KB) is out of range.",
+		 value, (1ULL << value) / 1024ULL);
+	  return false;
+	}
+    }
+
+  return true;
+}
+
+/* Implement TARGET_OPTION_DEFAULT_PARAMS.  */
+
+static void
+aarch64_option_default_params (void)
+{
+  /* We assume the guard page is 64k.  */
+  int index = (int) PARAM_STACK_CLASH_PROTECTION_GUARD_SIZE;
+  set_default_param_value (PARAM_STACK_CLASH_PROTECTION_GUARD_SIZE,
+			   DEFAULT_STK_CLASH_GUARD_SIZE == 0
+			     ? 16 : DEFAULT_STK_CLASH_GUARD_SIZE);
+
+  int guard_size
+    = default_param_value (PARAM_STACK_CLASH_PROTECTION_GUARD_SIZE);
+
+  /* Set the interval parameter to be the same as the guard size.  This way the
+     mid-end code does the right thing for us.  */
+  set_default_param_value (PARAM_STACK_CLASH_PROTECTION_PROBE_INTERVAL,
+			   guard_size);
+
+  /* Validate the options.  */
+  aarch64_option_validate_param (guard_size, index);
+}
+
 /* Implement TARGET_HANDLE_OPTION.
    This function handles the target specific options for CPU/target selection.
 
diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index 31948dc67c8ae1e879a3d3f1ca8cb96e975afc30..ed85093a1e5a9126f2498b2aecc369f054839e0c 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -10938,10 +10938,6 @@ aarch64_override_options_internal (struct gcc_options *opts)
 
   /* Validate the guard size.  */
   int guard_size = PARAM_VALUE (PARAM_STACK_CLASH_PROTECTION_GUARD_SIZE);
-  if (guard_size != 12 && guard_size != 16)
-    error ("only values 12 (4 KB) and 16 (64 KB) are supported for guard "
-	   "size.  Given value %d (%llu KB) is out of range.",
-	   guard_size, (1ULL << guard_size) / 1024ULL);
 
   /* Enforce that interval is the same size as size so the mid-end does the
      right thing.  */
James Greenhalgh Aug. 7, 2018, 4:20 p.m. | #3
On Tue, Aug 07, 2018 at 05:09:30AM -0500, Tamar Christina wrote:
> Hi All,
> 
> This is an updated patch which applies the same style changes as requested in patch 5/6.


No full stop on an error message IIRC.

Otherwise, OK.

Thanks,
James

> gcc/
> 2018-08-07  Tamar Christina  <tamar.christina@arm.com>
> 
> 	* common/config/aarch64/aarch64-common.c (TARGET_OPTION_DEFAULT_PARAM,
> 	aarch64_option_default_param):	New.
> 	(params.h): Include.
> 	(TARGET_OPTION_VALIDATE_PARAM, aarch64_option_validate_param): New.
> 	* config/aarch64/aarch64.c (aarch64_override_options_internal): Simplify
> 	stack-clash protection validation code.

Patch

diff --git a/gcc/common/config/aarch64/aarch64-common.c b/gcc/common/config/aarch64/aarch64-common.c
index 292fb818705d4650113da59a6d88cf2aa7c9e57d..73f2f95e8cb989f93e7a17bbf274f4364e660c0d 100644
--- a/gcc/common/config/aarch64/aarch64-common.c
+++ b/gcc/common/config/aarch64/aarch64-common.c
@@ -30,6 +30,7 @@ 
 #include "opts.h"
 #include "flags.h"
 #include "diagnostic.h"
+#include "params.h"
 
 #ifdef  TARGET_BIG_ENDIAN_DEFAULT
 #undef  TARGET_DEFAULT_TARGET_FLAGS
@@ -41,6 +42,10 @@ 
 
 #undef	TARGET_OPTION_OPTIMIZATION_TABLE
 #define TARGET_OPTION_OPTIMIZATION_TABLE aarch_option_optimization_table
+#undef TARGET_OPTION_DEFAULT_PARAMS
+#define TARGET_OPTION_DEFAULT_PARAMS aarch64_option_default_params
+#undef TARGET_OPTION_VALIDATE_PARAM
+#define TARGET_OPTION_VALIDATE_PARAM aarch64_option_validate_param
 
 /* Set default optimization options.  */
 static const struct default_options aarch_option_optimization_table[] =
@@ -60,6 +65,52 @@  static const struct default_options aarch_option_optimization_table[] =
     { OPT_LEVELS_NONE, 0, NULL, 0 }
   };
 
+/* Implement target validation TARGET_OPTION_DEFAULT_PARAM.  */
+
+static bool
+aarch64_option_validate_param (const int value, const int param)
+{
+  /* Check that both parameters are the same.  */
+  if (param == (int) PARAM_STACK_CLASH_PROTECTION_GUARD_SIZE)
+    {
+      if (value != 12 && value != 16)
+	{
+	error ("only values 12 (4 KB) and 16 (64 KB) are supported for guard "
+		"size.  Given value %d (%llu KB) is out of range.\n",
+		value, (1ULL << value) / 1024ULL);
+	return false;
+	}
+
+      /* Enforce that they are the same.  */
+      set_default_param_value (PARAM_STACK_CLASH_PROTECTION_PROBE_INTERVAL,
+			       value);
+    }
+
+  return true;
+}
+
+/* Implement TARGET_OPTION_DEFAULT_PARAMS.  */
+
+static void
+aarch64_option_default_params (void)
+{
+  /* We assume the guard page is 64k.  */
+  int index = (int) PARAM_STACK_CLASH_PROTECTION_GUARD_SIZE;
+  if (!compiler_params[index].configure_value)
+     set_default_param_value (PARAM_STACK_CLASH_PROTECTION_GUARD_SIZE, 16);
+
+  int guard_size
+    = default_param_value (PARAM_STACK_CLASH_PROTECTION_GUARD_SIZE);
+
+  /* Set the interval parameter to be the same as the guard size.  This way the
+     mid-end code does the right thing for us.  */
+  set_default_param_value (PARAM_STACK_CLASH_PROTECTION_PROBE_INTERVAL,
+			   guard_size);
+
+  /* Validate the options.  */
+  aarch64_option_validate_param (guard_size, index);
+}
+
 /* Implement TARGET_HANDLE_OPTION.
    This function handles the target specific options for CPU/target selection.
 
diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index e62d8a92ff53128e5e10ffd3b52eb8898869b756..dfc0da6a27d6007c669db32abe5c7b0248ea28a5 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -10900,41 +10900,21 @@  aarch64_override_options_internal (struct gcc_options *opts)
 			   opts->x_param_values,
 			   global_options_set.x_param_values);
 
-  /* Use the alternative scheduling-pressure algorithm by default.  */
-  maybe_set_param_value (PARAM_SCHED_PRESSURE_ALGORITHM, SCHED_PRESSURE_MODEL,
-			 opts->x_param_values,
-			 global_options_set.x_param_values);
-
-  /* If the user hasn't change it via configure then set the default to 64 KB
-     for the backend.  */
-  if (DEFAULT_STK_CLASH_GUARD_SIZE == 0)
-      maybe_set_param_value (PARAM_STACK_CLASH_PROTECTION_GUARD_SIZE, 16,
-			    opts->x_param_values,
-			    global_options_set.x_param_values);
-
-  /* Validate the guard size.  */
-  int guard_size = PARAM_VALUE (PARAM_STACK_CLASH_PROTECTION_GUARD_SIZE);
-  if (guard_size != 12 && guard_size != 16)
-      error ("only values 12 (4 KB) and 16 (64 KB) are supported for guard "
-	     "size.  Given value %d (%llu KB) is out of range.\n",
-	     guard_size, (1ULL << guard_size) / 1024ULL);
-
-  /* Enforce that interval is the same size as size so the mid-end does the
-     right thing.  */
-  maybe_set_param_value (PARAM_STACK_CLASH_PROTECTION_PROBE_INTERVAL,
-			 guard_size,
-			 opts->x_param_values,
-			 global_options_set.x_param_values);
-
   /* The maybe_set calls won't update the value if the user has explicitly set
      one.  Which means we need to validate that probing interval and guard size
      are equal.  */
   int probe_interval
     = PARAM_VALUE (PARAM_STACK_CLASH_PROTECTION_PROBE_INTERVAL);
+  int guard_size = PARAM_VALUE (PARAM_STACK_CLASH_PROTECTION_GUARD_SIZE);
   if (guard_size != probe_interval)
     error ("stack clash guard size '%d' must be equal to probing interval "
 	   "'%d'\n", guard_size, probe_interval);
 
+  /* Use the alternative scheduling-pressure algorithm by default.  */
+  maybe_set_param_value (PARAM_SCHED_PRESSURE_ALGORITHM, SCHED_PRESSURE_MODEL,
+			 opts->x_param_values,
+			 global_options_set.x_param_values);
+
   /* Enable sw prefetching at specified optimization level for
      CPUS that have prefetch.  Lower optimization level threshold by 1
      when profiling is enabled.  */