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

[Tamar Christina]

Hi Jakub,

Thanks for the feedback, but I think those are changes for another patch.
The patch here does not introduce the problems you highlighted and I don't feel these changes are appropriate for stage4 or that they should block this patch.

Kind regards,
Tamar


________________________________________
From: Jakub Jelinek <jakub@redhat.com>
Sent: Wednesday, January 23, 2019 4:27:40 PM
To: Kyrill Tkachov
Cc: Tamar Christina; gcc-patches@gcc.gnu.org; nd; James Greenhalgh; Richard Earnshaw; Marcus Shawcroft
Subject: Re: [PATCH][GCC][AArch64] Have empty HWCAPs string ignored during native feature detection

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