[PATCH][GCC][AArch64] Have empty HWCAPs string ignored during native feature detection

[Jakub Jelinek]

On Thu, Jan 10, 2019 at 04:57:47PM +0000, Kyrill Tkachov wrote:
> --- a/gcc/config/aarch64/driver-aarch64.c
> +++ b/gcc/config/aarch64/driver-aarch64.c
> @@ -253,6 +253,12 @@ host_detect_local_cpu (int argc, const char **argv)
>  	      char *p = NULL;
>  	      char *feat_string
>  		= concat (aarch64_extensions[i].feat_string, NULL);
> +
> +	      /* If the feature contains no HWCAPS string then ignore it for the
> +		 auto detection.  */
> +	      if (strlen (feat_string) == 0)
> +		continue;
> 
> I think this can avoid a strlen call by checking (*feat_string == '\0') though I believe most compilers will optimise it that way anyway.
> It might be more immediately readable your way.
> I wouldn't let it hold off this patch.

Well, that isn't the only problem of this code.
Another one is that concat (str, NULL) is much better written as xstrdup (str);
Another one is that the if (*feat_string == '\0') check
should be probably if (aarch64_extensions[i].feat_string[0] == '\0')
before the xstrdup, because there is no point to allocate the memory
in that case.
Another one is that it leaks the feat_string (right now always, otherwise
if it isn't empty).

Another one, I wonder if the xstrdup + strtok isn't a too heavy hammer for
something so simple, especially when you have complete control over those
feature strings.  They use exactly one space to separate.
So just do
	      const char *p = aarch64_extensions[i].feat_string;
	      bool enabled = true;

	      /* If the feature contains no HWCAPS string then ignore it for the
		 auto detection.  */
	      if (*p == '\0')
		continue;

	      size_t len = strlen (buf);
	      do
		{
		  const char *end = strchr (p, ' ');
		  if (end == NULL)
		    end = strchr (p, '\0');
		  if (memmem (buf, len, p, end - p) == NULL)
		    {
		      /* Failed to match this token.  Turn off the
			 features we'd otherwise enable.  */
		      enabled = false;
		      break;
		    }
		  if (*end == '\0')
		    break;
		  p = end + 1;
		}
	      while (1);

	      if (enabled)
		extension_flags |= aarch64_extensions[i].flag;
	      else
		extension_flags &= ~(aarch64_extensions[i].flag);
?

Last thing, for not_found, there is:
return "";, why not return NULL; ?  That is what other detect cpu routines
do, returning "" means a confusion between when a heap allocated string is
returned that needs freeing and when a .rodata string is returned which
doesn't.

	Jakub