[PATCH][AArch64] Properly reject invalid attribute strings

[PATCH][AArch64] Properly reject invalid attribute strings

[Kyrill Tkachov]

[-- Attachment #1: Type: text/plain, Size: 637 bytes --]

Hi all,

A bug in the target attribute parsing logic led to us silently accepting attribute strings
that did not appear in the attributes table i.e invalid attributes.

This patch fixes that oversight so we now error out on obviously bogus strings.

Bootstrapped and tested on aarch64.

Ok for trunk?

Thanks,
Kyrill

2016-01-15  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     * config/aarch64/aarch64.c (aarch64_process_one_target_attr): Return
     false when argument string is not found in the attributes table
     at all.

2016-01-15  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     * gcc.target/aarch64/target_attr_17.c: New test.

[-- Attachment #2: aarch64-attrs-bogus.patch --]
[-- Type: text/x-patch, Size: 1798 bytes --]

diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index e54ce7985f52c6a61b2ef1e3d7f847f22b1a959f..f2e4b45ac0ad1223e8149d1a35782c13f493a740 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -8938,6 +8938,7 @@ aarch64_process_one_target_attr (char *arg_str, const char* pragma_or_attr)
       arg++;
     }
   const struct aarch64_attribute_info *p_attr;
+  bool found = false;
   for (p_attr = aarch64_attributes; p_attr->name; p_attr++)
     {
       /* If the names don't match up, or the user has given an argument
@@ -8946,6 +8947,7 @@ aarch64_process_one_target_attr (char *arg_str, const char* pragma_or_attr)
       if (strcmp (str_to_check, p_attr->name) != 0)
 	continue;
 
+      found = true;
       bool attr_need_arg_p = p_attr->attr_type == aarch64_attr_custom
 			      || p_attr->attr_type == aarch64_attr_enum;
 
@@ -9025,7 +9027,10 @@ aarch64_process_one_target_attr (char *arg_str, const char* pragma_or_attr)
 	}
     }
 
-  return true;
+  /* If we reached here we either have found an attribute and validated
+     it or didn't match any.  If we matched an attribute but its arguments
+     were malformed we will have returned false already.  */
+  return found;
 }
 
 /* Count how many times the character C appears in
diff --git a/gcc/testsuite/gcc.target/aarch64/target_attr_17.c b/gcc/testsuite/gcc.target/aarch64/target_attr_17.c
new file mode 100644
index 0000000000000000000000000000000000000000..483cc6d4a1d0b78ac404ae903c7e98a6ad288d17
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/target_attr_17.c
@@ -0,0 +1,8 @@
+__attribute__((target("invalid-attr-string")))
+int
+foo (int a)
+{
+  return a + 5;
+}
+
+/* { dg-error "target attribute.*is invalid" "" { target *-*-* } 0 } */
\ No newline at end of file

Re: [PATCH][AArch64] Properly reject invalid attribute strings

[James Greenhalgh]

On Fri, Jan 15, 2016 at 01:39:54PM +0000, Kyrill Tkachov wrote:
> Hi all,
> 
> A bug in the target attribute parsing logic led to us silently accepting
> attribute strings that did not appear in the attributes table i.e invalid
> attributes.
> 
> This patch fixes that oversight so we now error out on obviously bogus
> strings.

This is ok.

> 2016-01-15  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
> 
>     * config/aarch64/aarch64.c (aarch64_process_one_target_attr): Return
>     false when argument string is not found in the attributes table
>     at all.
> 
> 2016-01-15  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
> 
>     * gcc.target/aarch64/target_attr_17.c: New test.

> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
> index e54ce7985f52c6a61b2ef1e3d7f847f22b1a959f..f2e4b45ac0ad1223e8149d1a35782c13f493a740 100644
> --- a/gcc/config/aarch64/aarch64.c
> +++ b/gcc/config/aarch64/aarch64.c
> @@ -8938,6 +8938,7 @@ aarch64_process_one_target_attr (char *arg_str, const char* pragma_or_attr)
>        arg++;
>      }
>    const struct aarch64_attribute_info *p_attr;
> +  bool found = false;
>    for (p_attr = aarch64_attributes; p_attr->name; p_attr++)
>      {
>        /* If the names don't match up, or the user has given an argument
> @@ -8946,6 +8947,7 @@ aarch64_process_one_target_attr (char *arg_str, const char* pragma_or_attr)
>        if (strcmp (str_to_check, p_attr->name) != 0)
>  	continue;
>  
> +      found = true;
>        bool attr_need_arg_p = p_attr->attr_type == aarch64_attr_custom
>  			      || p_attr->attr_type == aarch64_attr_enum;
>  
> @@ -9025,7 +9027,10 @@ aarch64_process_one_target_attr (char *arg_str, const char* pragma_or_attr)
>  	}
>      }
>  
> -  return true;
> +  /* If we reached here we either have found an attribute and validated
> +     it or didn't match any.  If we matched an attribute but its arguments
> +     were malformed we will have returned false already.  */
> +  return found;

I don't like this "found" variable, it normally smells of a refactoring
opportunity. I wonder whether you could clean the function up a bit by
restructuring the logic.

Regardless, this is OK for trunk.

Thanks,
James