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 <ja...@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
