[PATCH 09/11] aarch64,arm: Fix branch-protection= parsing

[Szabolcs Nagy <szabolcs.nagy@arm.com>]

Refactor the parsing to have a single API and fix a few parsing issues:

- Different handling of "bti+none" and "none+bti": these should be
  rejected because "none" can only appear alone.

- Accepted empty strings such as "bti++pac-ret" or "bti+", this bug
  was caused by using strtok_r.

- Memory got leaked (str_root was never freed). And two buffers got
  allocated when one is enough.

The callbacks now have no failure mode, only parsing can fail and
all failures are handled locally.  The "-mbranch-protection=" vs
"target("branch-protection=")" difference in the error message is
handled by a separate argument to aarch_validate_mbranch_protection.

gcc/ChangeLog:

	* config/aarch64/aarch64.cc (aarch64_override_options): Update.
	(aarch64_handle_attr_branch_protection): Update.
	* config/arm/aarch-common-protos.h (aarch_parse_branch_protection):
	Remove.
	(aarch_validate_mbranch_protection): Add new argument.
	* config/arm/aarch-common.cc (aarch_handle_no_branch_protection):
	Update.
	(aarch_handle_standard_branch_protection): Update.
	(aarch_handle_pac_ret_protection): Update.
	(aarch_handle_pac_ret_leaf): Update.
	(aarch_handle_pac_ret_b_key): Update.
	(aarch_handle_bti_protection): Update.
	(aarch_parse_branch_protection): Remove.
	(next_tok): New.
	(aarch_validate_mbranch_protection): Rewrite.
	* config/arm/aarch-common.h (struct aarch_branch_protect_type):
	Add field "alone".
	* config/arm/arm.cc (arm_configure_build_target): Update.
---
 gcc/config/aarch64/aarch64.cc        |  37 +----
 gcc/config/arm/aarch-common-protos.h |   5 +-
 gcc/config/arm/aarch-common.cc       | 214 ++++++++++++---------------
 gcc/config/arm/aarch-common.h        |  14 +-
 gcc/config/arm/arm.cc                |   3 +-
 5 files changed, 109 insertions(+), 164 deletions(-)

diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc
index 7f0a22fae9c..661ac12cacc 100644
--- a/gcc/config/aarch64/aarch64.cc
+++ b/gcc/config/aarch64/aarch64.cc
@@ -18539,7 +18539,8 @@ aarch64_override_options (void)
     aarch64_validate_sls_mitigation (aarch64_harden_sls_string);
 
   if (aarch64_branch_protection_string)
-    aarch_validate_mbranch_protection (aarch64_branch_protection_string);
+    aarch_validate_mbranch_protection (aarch64_branch_protection_string,
+				       "-mbranch-protection=");
 
   /* -mcpu=CPU is shorthand for -march=ARCH_FOR_CPU, -mtune=CPU.
      If either of -march or -mtune is given, they override their
@@ -18913,34 +18914,12 @@ aarch64_handle_attr_cpu (const char *str)
 
 /* Handle the argument STR to the branch-protection= attribute.  */
 
- static bool
- aarch64_handle_attr_branch_protection (const char* str)
- {
-  char *err_str = (char *) xmalloc (strlen (str) + 1);
-  enum aarch_parse_opt_result res = aarch_parse_branch_protection (str,
-								   &err_str);
-  bool success = false;
-  switch (res)
-    {
-     case AARCH_PARSE_MISSING_ARG:
-       error ("missing argument to %<target(\"branch-protection=\")%> pragma or"
-	      " attribute");
-       break;
-     case AARCH_PARSE_INVALID_ARG:
-       error ("invalid protection type %qs in %<target(\"branch-protection"
-	      "=\")%> pragma or attribute", err_str);
-       break;
-     case AARCH_PARSE_OK:
-       success = true;
-      /* Fall through.  */
-     case AARCH_PARSE_INVALID_FEATURE:
-       break;
-     default:
-       gcc_unreachable ();
-    }
-  free (err_str);
-  return success;
- }
+static bool
+aarch64_handle_attr_branch_protection (const char* str)
+{
+  return aarch_validate_mbranch_protection (str,
+					    "target(\"branch-protection=\")");
+}
 
 /* Handle the argument STR to the tune= target attribute.  */
 
diff --git a/gcc/config/arm/aarch-common-protos.h b/gcc/config/arm/aarch-common-protos.h
index f8cb6562096..75ffdfbb050 100644
--- a/gcc/config/arm/aarch-common-protos.h
+++ b/gcc/config/arm/aarch-common-protos.h
@@ -159,10 +159,7 @@ rtx_insn *arm_md_asm_adjust (vec<rtx> &outputs, vec<rtx> & /*inputs*/,
 			     vec<rtx> &clobbers, HARD_REG_SET &clobbered_regs,
 			     location_t loc);
 
-/* Parsing routine for branch-protection common to AArch64 and Arm.  */
-enum aarch_parse_opt_result aarch_parse_branch_protection (const char*, char**);
-
 /* Validation routine for branch-protection common to AArch64 and Arm.  */
-bool aarch_validate_mbranch_protection (const char *);
+bool aarch_validate_mbranch_protection (const char *, const char *);
 
 #endif /* GCC_AARCH_COMMON_PROTOS_H */
diff --git a/gcc/config/arm/aarch-common.cc b/gcc/config/arm/aarch-common.cc
index cbc7f68a8bf..159c61b786c 100644
--- a/gcc/config/arm/aarch-common.cc
+++ b/gcc/config/arm/aarch-common.cc
@@ -659,169 +659,143 @@ arm_md_asm_adjust (vec<rtx> &outputs, vec<rtx> & /*inputs*/,
   return saw_asm_flag ? seq : NULL;
 }
 
-static enum aarch_parse_opt_result
-aarch_handle_no_branch_protection (char* str, char* rest)
+static void
+aarch_handle_no_branch_protection (void)
 {
   aarch_ra_sign_scope = AARCH_FUNCTION_NONE;
   aarch_enable_bti = 0;
-  if (rest)
-    {
-      error ("unexpected %<%s%> after %<%s%>", rest, str);
-      return AARCH_PARSE_INVALID_FEATURE;
-    }
-  return AARCH_PARSE_OK;
 }
 
-static enum aarch_parse_opt_result
-aarch_handle_standard_branch_protection (char* str, char* rest)
+static void
+aarch_handle_standard_branch_protection (void)
 {
   aarch_ra_sign_scope = AARCH_FUNCTION_NON_LEAF;
   aarch_ra_sign_key = AARCH_KEY_A;
   aarch_enable_bti = 1;
-  if (rest)
-    {
-      error ("unexpected %<%s%> after %<%s%>", rest, str);
-      return AARCH_PARSE_INVALID_FEATURE;
-    }
-  return AARCH_PARSE_OK;
 }
 
-static enum aarch_parse_opt_result
-aarch_handle_pac_ret_protection (char* str ATTRIBUTE_UNUSED,
-				 char* rest ATTRIBUTE_UNUSED)
+static void
+aarch_handle_pac_ret_protection (void)
 {
   aarch_ra_sign_scope = AARCH_FUNCTION_NON_LEAF;
   aarch_ra_sign_key = AARCH_KEY_A;
-  return AARCH_PARSE_OK;
 }
 
-static enum aarch_parse_opt_result
-aarch_handle_pac_ret_leaf (char* str ATTRIBUTE_UNUSED,
-			   char* rest ATTRIBUTE_UNUSED)
+static void
+aarch_handle_pac_ret_leaf (void)
 {
   aarch_ra_sign_scope = AARCH_FUNCTION_ALL;
-  return AARCH_PARSE_OK;
 }
 
-static enum aarch_parse_opt_result
-aarch_handle_pac_ret_b_key (char* str ATTRIBUTE_UNUSED,
-			    char* rest ATTRIBUTE_UNUSED)
+static void
+aarch_handle_pac_ret_b_key (void)
 {
   aarch_ra_sign_key = AARCH_KEY_B;
-  return AARCH_PARSE_OK;
 }
 
-static enum aarch_parse_opt_result
-aarch_handle_bti_protection (char* str ATTRIBUTE_UNUSED,
-			     char* rest ATTRIBUTE_UNUSED)
+static void
+aarch_handle_bti_protection (void)
 {
   aarch_enable_bti = 1;
-  return AARCH_PARSE_OK;
 }
 
 static const struct aarch_branch_protect_type aarch_pac_ret_subtypes[] = {
-  { "leaf", aarch_handle_pac_ret_leaf, NULL, 0 },
-  { "b-key", aarch_handle_pac_ret_b_key, NULL, 0 },
-  { NULL, NULL, NULL, 0 }
+  { "leaf", false, aarch_handle_pac_ret_leaf, NULL, 0 },
+  { "b-key", false, aarch_handle_pac_ret_b_key, NULL, 0 },
+  { NULL, false, NULL, NULL, 0 }
 };
 
 static const struct aarch_branch_protect_type aarch_branch_protect_types[] = {
-  { "none", aarch_handle_no_branch_protection, NULL, 0 },
-  { "standard", aarch_handle_standard_branch_protection, NULL, 0 },
-  { "pac-ret", aarch_handle_pac_ret_protection, aarch_pac_ret_subtypes,
+  { "none", true, aarch_handle_no_branch_protection, NULL, 0 },
+  { "standard", true, aarch_handle_standard_branch_protection, NULL, 0 },
+  { "pac-ret", false, aarch_handle_pac_ret_protection, aarch_pac_ret_subtypes,
     ARRAY_SIZE (aarch_pac_ret_subtypes) },
-  { "bti", aarch_handle_bti_protection, NULL, 0 },
-  { NULL, NULL, NULL, 0 }
+  { "bti", false, aarch_handle_bti_protection, NULL, 0 },
+  { NULL, false, NULL, NULL, 0 }
 };
 
-/* Parses CONST_STR for branch protection features specified in
-   aarch64_branch_protect_types, and set any global variables required.  Returns
-   the parsing result and assigns LAST_STR to the last processed token from
-   CONST_STR so that it can be used for error reporting.  */
+/* In-place split *str at delim, return *str and set *str to the tail
+   of the string or NULL if the end is reached.  */
 
-enum aarch_parse_opt_result
-aarch_parse_branch_protection (const char *const_str, char** last_str)
+static char *
+next_tok (char **str, int delim)
 {
-  char *str_root = xstrdup (const_str);
-  char* token_save = NULL;
-  char *str = strtok_r (str_root, "+", &token_save);
-  enum aarch_parse_opt_result res = AARCH_PARSE_OK;
-  if (!str)
-    res = AARCH_PARSE_MISSING_ARG;
-  else
+  char *tok = *str;
+  for (char *p = tok; p && *p != '\0'; p++)
     {
-      char *next_str = strtok_r (NULL, "+", &token_save);
-      /* Reset the branch protection features to their defaults.  */
-      aarch_handle_no_branch_protection (NULL, NULL);
-
-      while (str && res == AARCH_PARSE_OK)
+      if (*p == delim)
 	{
-	  const aarch_branch_protect_type* type = aarch_branch_protect_types;
-	  bool found = false;
-	  /* Search for this type.  */
-	  while (type && type->name && !found && res == AARCH_PARSE_OK)
-	    {
-	      if (strcmp (str, type->name) == 0)
-		{
-		  found = true;
-		  res = type->handler (str, next_str);
-		  str = next_str;
-		  next_str = strtok_r (NULL, "+", &token_save);
-		}
-	      else
-		type++;
-	    }
-	  if (found && res == AARCH_PARSE_OK)
-	    {
-	      bool found_subtype = true;
-	      /* Loop through each token until we find one that isn't a
-		 subtype.  */
-	      while (found_subtype)
-		{
-		  found_subtype = false;
-		  const aarch_branch_protect_type *subtype = type->subtypes;
-		  /* Search for the subtype.  */
-		  while (str && subtype && subtype->name && !found_subtype
-			  && res == AARCH_PARSE_OK)
-		    {
-		      if (strcmp (str, subtype->name) == 0)
-			{
-			  found_subtype = true;
-			  res = subtype->handler (str, next_str);
-			  str = next_str;
-			  next_str = strtok_r (NULL, "+", &token_save);
-			}
-		      else
-			subtype++;
-		    }
-		}
-	    }
-	  else if (!found)
-	    res = AARCH_PARSE_INVALID_ARG;
+	  *p = '\0';
+	  *str = p + 1;
+	  return tok;
 	}
     }
-  /* Copy the last processed token into the argument to pass it back.
-    Used by option and attribute validation to print the offending token.  */
-  if (last_str)
-    {
-      if (str)
-	strcpy (*last_str, str);
-      else
-	*last_str = NULL;
-    }
-  return res;
+  *str = NULL;
+  return tok;
 }
 
+/* Parses CONST_STR for branch protection features specified in
+   aarch64_branch_protect_types, and set any global variables required.
+   Returns true on success.  */
+
 bool
-aarch_validate_mbranch_protection (const char *const_str)
+aarch_validate_mbranch_protection (const char *const_str, const char *opt)
 {
-  char *str = (char *) xmalloc (strlen (const_str));
-  enum aarch_parse_opt_result res =
-    aarch_parse_branch_protection (const_str, &str);
-  if (res == AARCH_PARSE_INVALID_ARG)
-    error ("invalid argument %<%s%> for %<-mbranch-protection=%>", str);
-  else if (res == AARCH_PARSE_MISSING_ARG)
-    error ("missing argument for %<-mbranch-protection=%>");
-  free (str);
-  return res == AARCH_PARSE_OK;
+  char *str_root = xstrdup (const_str);
+  char *next_str = str_root;
+  char *str = next_tok (&next_str, '+');
+  char *alone_str = NULL;
+  bool reject_alone = false;
+  bool res = true;
+
+  /* First entry is "none" and it is used to reset the state.  */
+  aarch_branch_protect_types[0].handler ();
+
+  while (str)
+    {
+      const aarch_branch_protect_type *type = aarch_branch_protect_types;
+      for (; type->name; type++)
+	if (strcmp (str, type->name) == 0)
+	  break;
+      if (type->name == NULL)
+	{
+	  res = false;
+	  error ("invalid argument %<%s%> for %<%s%>", str, opt);
+	  break;
+	}
+
+      if (type->alone && alone_str == NULL)
+	alone_str = str;
+      else
+	reject_alone = true;
+      if (reject_alone && alone_str != NULL)
+	{
+	  res = false;
+	  error ("argument %<%s%> can only appear alone in %<%s%>",
+		 alone_str, opt);
+	  break;
+	}
+
+      type->handler ();
+      str = next_tok (&next_str, '+');
+      if (type->subtypes == NULL)
+	continue;
+
+      /* Loop through tokens until we find one that isn't a subtype.  */
+      while (str)
+	{
+	  const aarch_branch_protect_type *subtype = type->subtypes;
+	  for (; subtype->name; subtype++)
+	    if (strcmp (str, subtype->name) == 0)
+	      break;
+	  if (subtype->name == NULL)
+	    break;
+
+	  subtype->handler ();
+	  str = next_tok (&next_str, '+');
+	}
+    }
+
+  free (str_root);
+  return res;
 }
diff --git a/gcc/config/arm/aarch-common.h b/gcc/config/arm/aarch-common.h
index c6a67f0d05c..f72e21127fc 100644
--- a/gcc/config/arm/aarch-common.h
+++ b/gcc/config/arm/aarch-common.h
@@ -55,16 +55,10 @@ struct aarch_branch_protect_type
   /* The type's name that the user passes to the branch-protection option
      string.  */
   const char* name;
-  /* Function to handle the protection type and set global variables.
-     First argument is the string token corresponding with this type and the
-     second argument is the next token in the option string.
-     Return values:
-     * AARCH_PARSE_OK: Handling was sucessful.
-     * AARCH_INVALID_ARG: The type is invalid in this context and the caller
-     should print an error.
-     * AARCH_INVALID_FEATURE: The type is invalid and the handler prints its
-     own error.  */
-  enum aarch_parse_opt_result (*handler)(char*, char*);
+  /* The type can only appear alone, other types should be rejected.  */
+  int alone;
+  /* Function to handle the protection type and set global variables.  */
+  void (*handler)(void);
   /* A list of types that can follow this type in the option string.  */
   const struct aarch_branch_protect_type* subtypes;
   unsigned int num_subtypes;
diff --git a/gcc/config/arm/arm.cc b/gcc/config/arm/arm.cc
index f49312cace0..5681073a948 100644
--- a/gcc/config/arm/arm.cc
+++ b/gcc/config/arm/arm.cc
@@ -3297,7 +3297,8 @@ arm_configure_build_target (struct arm_build_target *target,
 
   if (opts->x_arm_branch_protection_string)
     {
-      aarch_validate_mbranch_protection (opts->x_arm_branch_protection_string);
+      aarch_validate_mbranch_protection (opts->x_arm_branch_protection_string,
+					 "-mbranch-protection=");
 
       if (aarch_ra_sign_key != AARCH_KEY_A)
 	{
-- 
2.25.1