From: Adhemerval Zanella Netto <adhemerval.zanella@linaro.org>
To: Siddhesh Poyarekar <siddhesh@sourceware.org>, libc-alpha@sourceware.org
Subject: Re: [PATCH v3 11/19] elf: Do not duplicate the GLIBC_TUNABLES string
Date: Tue, 21 Nov 2023 15:12:46 -0300 [thread overview]
Message-ID: <ca6b4ce7-46ab-45d7-829d-12845982f8b4@linaro.org> (raw)
In-Reply-To: <0be5babe-a123-47af-98e1-abab4d1c7fd2@sourceware.org>
On 20/11/23 19:44, Siddhesh Poyarekar wrote:
> On 2023-11-06 15:25, Adhemerval Zanella wrote:
>> The tunable parsing duplicates the tunable environment variable so it
>> null-terminates each one since it simplifies the later parsing. It has
>> the drawback of adding another point of failure (__minimal_malloc
>> failing), and the memory copy requires tuning the compiler to avoid mem
>> operations calls.
>>
>> The parsing now tracks the tunable start and its size. The
>> dl-tunable-parse.h adds helper functions to help parsing, like a strcmp
>> that also checks for size and an iterator for suboptions that are
>> comma-separated (used on hwcap parsing by x86, powerpc, and s390x).
>>
>> Since the environment variable is allocated on the stack by the kernel,
>> it is safe to keep the references to the suboptions for later parsing
>> of string tunables (as done by set_hwcaps by multiple architectures).
>>
>> Checked on x86_64-linux-gnu, powerpc64le-linux-gnu, and
>> aarch64-linux-gnu.
>> ---
>> elf/dl-tunables.c | 66 +++----
>> elf/dl-tunables.h | 6 +-
>> elf/tst-tunables.c | 66 ++++++-
>> sysdeps/generic/dl-tunables-parse.h | 129 ++++++++++++++
>> sysdeps/s390/cpu-features.c | 167 +++++++-----------
>> .../unix/sysv/linux/aarch64/cpu-features.c | 38 ++--
>> .../unix/sysv/linux/powerpc/cpu-features.c | 45 ++---
>> .../sysv/linux/powerpc/tst-hwcap-tunables.c | 6 +-
>> sysdeps/x86/Makefile | 4 +-
>> sysdeps/x86/cpu-tunables.c | 118 +++++--------
>> sysdeps/x86/tst-hwcap-tunables.c | 148 ++++++++++++++++
>> 11 files changed, 517 insertions(+), 276 deletions(-)
>> create mode 100644 sysdeps/generic/dl-tunables-parse.h
>> create mode 100644 sysdeps/x86/tst-hwcap-tunables.c
>>
>> diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
>> index 6e3e6a2cf8..f406521735 100644
>> --- a/elf/dl-tunables.c
>> +++ b/elf/dl-tunables.c
>> @@ -36,28 +36,6 @@
>> #define TUNABLES_INTERNAL 1
>> #include "dl-tunables.h"
>> -#include <not-errno.h>
>> -
>> -static char *
>> -tunables_strdup (const char *in)
>> -{
>> - size_t i = 0;
>> -
>> - while (in[i++] != '\0');
>> - char *out = __minimal_malloc (i + 1);
>> -
>> - /* For most of the tunables code, we ignore user errors. However,
>> - this is a system error - and running out of memory at program
>> - startup should be reported, so we do. */
>> - if (out == NULL)
>> - _dl_fatal_printf ("failed to allocate memory to process tunables\n");
>> -
>> - while (i-- > 0)
>> - out[i] = in[i];
>> -
>> - return out;
>> -}
>> -
>> static char **
>> get_next_env (char **envp, char **name, size_t *namelen, char **val,
>> char ***prev_envp)
>> @@ -134,14 +112,14 @@ do_tunable_update_val (tunable_t *cur, const tunable_val_t *valp,
>> /* Validate range of the input value and initialize the tunable CUR if it looks
>> good. */
>> static void
>> -tunable_initialize (tunable_t *cur, const char *strval)
>> +tunable_initialize (tunable_t *cur, const char *strval, size_t len)
>> {
>> - tunable_val_t val;
>> + tunable_val_t val = { 0 };
>> if (cur->type.type_code != TUNABLE_TYPE_STRING)
>> val.numval = (tunable_num_t) _dl_strtoul (strval, NULL);
>
> There's an implicit assumption that strval is NULL terminated for numeric values. Is that safe? Maybe all you need to do here is copy strval into a static buffer of size 21 (basically size of the string representing -1ULL) and pass that to _dl_strtoul.
Afaiu the environment variable will always be NULL terminated (although some
might overlap). This is due how the kernel will layout the argv/envp on
process creation at sys_execve:
fs/exec.c
1886 static int do_execveat_common(int fd, struct filename *filename,
1887 struct user_arg_ptr argv,
1888 struct user_arg_ptr envp,
1889 int flags)
1890 {
[...]
1941 retval = copy_strings(bprm->envc, envp, bprm);
1942 if (retval < 0)
1943 goto out_free;
1944
1945 retval = copy_strings(bprm->argc, argv, bprm);
1946 if (retval < 0)
1947 goto out_free;
[...]
518 /*
519 * 'copy_strings()' copies argument/environment strings from the old
520 * processes's memory to the new process's stack. The call to get_user_pages()
521 * ensures the destination page is created and not swapped out.
522 */
523 static int copy_strings(int argc, struct user_arg_ptr argv,
524 struct linux_binprm *bprm)
525 {
[...]
531 while (argc-- > 0) {
[...]
536 ret = -EFAULT;
537 str = get_user_arg_ptr(argv, argc);
538 if (IS_ERR(str))
539 goto out;
540
541 len = strnlen_user(str, MAX_ARG_STRLEN);
542 if (!len)
543 goto out;
So even if caller construct a bogus argv/envp with non null-terminated
strings, if the kernel can not found a NULL execve will eventually fail
with EFAULT:
$ cat spawn.c
#include <assert.h>
#include <errno.h>
#include <unistd.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/mman.h>
extern char **environ;
int main (int argc, char *argv[])
{
pid_t pid = fork ();
assert (pid != -1);
if (pid != 0)
exit (EXIT_FAILURE);
long int pz = sysconf (_SC_PAGESIZE);
assert (pz != -1);
void *buf = mmap (NULL, 2 * pz, PROT_READ | PROT_WRITE,
MAP_ANONYMOUS|MAP_PRIVATE, -1, 0);
assert (buf != MAP_FAILED);
assert (mprotect (buf + pz, pz, PROT_NONE) != -1);
char *env = (buf + pz) - 1;
env[0] = 'a';
char *new_argv[] = { "./spawn", NULL };
char *new_envp[] = { env, NULL };
int r = execve (new_argv[0], new_argv, new_envp);
printf ("r=%d (errno=%s)\n", r, strerrorname_np (errno));
return 0;
}
$ gcc -Wall -O2 spawn.c -D_GNU_SOURCE -o spawn && ./spawn
r=-1 (errno=EFAULT)
>
>> else
>> - val.strval = strval;
>> + val.strval = (struct tunable_str_t) { strval, len };
>> do_tunable_update_val (cur, &val, NULL, NULL);
>> }
>> @@ -158,29 +136,29 @@ struct tunable_toset_t
>> {
>> tunable_t *t;
>> const char *value;
>> + size_t len;
>> };
>> enum { tunables_list_size = array_length (tunable_list) };
>> /* Parse the tunable string VALSTRING and set TUNABLES with the found tunables
>> - and their respectibles values. VALSTRING is a duplicated values, where
>> - delimiters ':' are replaced with '\0', so string tunables are null
>> - terminated.
>> + and their respectibles values. The VALSTRING is parsed in place, with the
>> + tunable start and size recorded in TUNABLES.
>> Return the number of tunables found (including 0 if the string is empty)
>> or -1 if for a ill-formatted definition. */
>> static int
>> -parse_tunables_string (char *valstring, struct tunable_toset_t *tunables)
>> +parse_tunables_string (const char *valstring, struct tunable_toset_t *tunables)
>> {
>> if (valstring == NULL || *valstring == '\0')
>> return 0;
>> - char *p = valstring;
>> + const char *p = valstring;
>> bool done = false;
>> int ntunables = 0;
>> while (!done)
>> {
>> - char *name = p;
>> + const char *name = p;
>> /* First, find where the name ends. */
>> while (*p != '=' && *p != ':' && *p != '\0')
>> @@ -202,7 +180,7 @@ parse_tunables_string (char *valstring, struct tunable_toset_t *tunables)
>> /* Skip the '='. */
>> p++;
>> - char *value = p;
>> + const char *value = p;
>> while (*p != '=' && *p != ':' && *p != '\0')
>> p++;
>> @@ -211,8 +189,6 @@ parse_tunables_string (char *valstring, struct tunable_toset_t *tunables)
>> return -1;
>> else if (*p == '\0')
>> done = true;
>> - else
>> - *p++ = '\0';
>> /* Add the tunable if it exists. */
>> for (size_t i = 0; i < tunables_list_size; i++)
>> @@ -221,7 +197,8 @@ parse_tunables_string (char *valstring, struct tunable_toset_t *tunables)
>> if (tunable_is_name (cur->name, name))
>> {
>> - tunables[ntunables++] = (struct tunable_toset_t) { cur, value };
>> + tunables[ntunables++] =
>> + (struct tunable_toset_t) { cur, value, p - value };
>> break;
>> }
>> }
>> @@ -231,7 +208,7 @@ parse_tunables_string (char *valstring, struct tunable_toset_t *tunables)
>> }
>> static void
>> -parse_tunables (char *valstring)
>> +parse_tunables (const char *valstring)
>> {
>> struct tunable_toset_t tunables[tunables_list_size];
>> int ntunables = parse_tunables_string (valstring, tunables);
>> @@ -243,7 +220,7 @@ parse_tunables (char *valstring)
>> }
>> for (int i = 0; i < ntunables; i++)
>> - tunable_initialize (tunables[i].t, tunables[i].value);
>> + tunable_initialize (tunables[i].t, tunables[i].value, tunables[i].len);
>> }
>> /* Initialize the tunables list from the environment. For now we only use the
>> @@ -264,9 +241,12 @@ __tunables_init (char **envp)
>> while ((envp = get_next_env (envp, &envname, &len, &envval,
>> &prev_envp)) != NULL)
>> {
>> + /* The environment variable is allocated on the stack by the kernel, so
>> + it is safe to keep the references to the suboptions for later parsing
>> + of string tunables. */
>> if (tunable_is_name ("GLIBC_TUNABLES", envname))
>> {
>> - parse_tunables (tunables_strdup (envval));
>> + parse_tunables (envval);
>> continue;
>> }
>> @@ -284,7 +264,7 @@ __tunables_init (char **envp)
>> /* We have a match. Initialize and move on to the next line. */
>> if (tunable_is_name (name, envname))
>> {
>> - tunable_initialize (cur, envval);
>> + tunable_initialize (cur, envval, len);
>> break;
>> }
>> }
>> @@ -298,7 +278,7 @@ __tunables_print (void)
>> {
>> const tunable_t *cur = &tunable_list[i];
>> if (cur->type.type_code == TUNABLE_TYPE_STRING
>> - && cur->val.strval == NULL)
>> + && cur->val.strval.str == NULL)
>> _dl_printf ("%s:\n", cur->name);
>> else
>> {
>> @@ -324,7 +304,9 @@ __tunables_print (void)
>> (size_t) cur->type.max);
>> break;
>> case TUNABLE_TYPE_STRING:
>> - _dl_printf ("%s\n", cur->val.strval);
>> + _dl_printf ("%.*s\n",
>> + (int) cur->val.strval.len,
>> + cur->val.strval.str);
>> break;
>> default:
>> __builtin_unreachable ();
>> @@ -359,7 +341,7 @@ __tunable_get_val (tunable_id_t id, void *valp, tunable_callback_t callback)
>> }
>> case TUNABLE_TYPE_STRING:
>> {
>> - *((const char **)valp) = cur->val.strval;
>> + *((struct tunable_str_t **) valp) = &cur->val.strval;
>> break;
>> }
>> default:
>> diff --git a/elf/dl-tunables.h b/elf/dl-tunables.h
>> index 45c191e021..0e777d7d37 100644
>> --- a/elf/dl-tunables.h
>> +++ b/elf/dl-tunables.h
>> @@ -30,7 +30,11 @@ typedef intmax_t tunable_num_t;
>> typedef union
>> {
>> tunable_num_t numval;
>> - const char *strval;
>> + struct tunable_str_t
>> + {
>> + const char *str;
>> + size_t len;
>> + } strval;
>> } tunable_val_t;
>> typedef void (*tunable_callback_t) (tunable_val_t *);
>> diff --git a/elf/tst-tunables.c b/elf/tst-tunables.c
>> index e1ad44f27c..188345b070 100644
>> --- a/elf/tst-tunables.c
>> +++ b/elf/tst-tunables.c
>> @@ -31,7 +31,8 @@ static int restart;
>> static const struct test_t
>> {
>> - const char *env;
>> + const char *name;
>> + const char *value;
>> int32_t expected_malloc_check;
>> size_t expected_mmap_threshold;
>> int32_t expected_perturb;
>> @@ -39,12 +40,14 @@ static const struct test_t
>> {
>> /* Expected tunable format. */
>> {
>> + "GLIBC_TUNABLES",
>> "glibc.malloc.check=2",
>> 2,
>> 0,
>> 0,
>> },
>> {
>> + "GLIBC_TUNABLES",
>> "glibc.malloc.check=2:glibc.malloc.mmap_threshold=4096",
>> 2,
>> 4096,
>> @@ -52,6 +55,7 @@ static const struct test_t
>> },
>> /* Empty tunable are ignored. */
>> {
>> + "GLIBC_TUNABLES",
>> "glibc.malloc.check=2::glibc.malloc.mmap_threshold=4096",
>> 2,
>> 4096,
>> @@ -59,6 +63,7 @@ static const struct test_t
>> },
>> /* As well empty values. */
>> {
>> + "GLIBC_TUNABLES",
>> "glibc.malloc.check=:glibc.malloc.mmap_threshold=4096",
>> 0,
>> 4096,
>> @@ -66,18 +71,21 @@ static const struct test_t
>> },
>> /* Tunable are processed from left to right, so last one is the one set. */
>> {
>> + "GLIBC_TUNABLES",
>> "glibc.malloc.check=1:glibc.malloc.check=2",
>> 2,
>> 0,
>> 0,
>> },
>> {
>> + "GLIBC_TUNABLES",
>> "glibc.malloc.check=1:glibc.malloc.check=2:glibc.malloc.mmap_threshold=4096",
>> 2,
>> 4096,
>> 0,
>> },
>> {
>> + "GLIBC_TUNABLES",
>> "glibc.malloc.check=2:glibc.malloc.mmap_threshold=4096:glibc.malloc.check=1",
>> 1,
>> 4096,
>> @@ -85,12 +93,14 @@ static const struct test_t
>> },
>> /* 0x800 is larger than tunable maxval (0xff), so the tunable is unchanged. */
>> {
>> + "GLIBC_TUNABLES",
>> "glibc.malloc.perturb=0x800",
>> 0,
>> 0,
>> 0,
>> },
>> {
>> + "GLIBC_TUNABLES",
>> "glibc.malloc.perturb=0x55",
>> 0,
>> 0,
>> @@ -98,6 +108,7 @@ static const struct test_t
>> },
>> /* Out of range values are just ignored. */
>> {
>> + "GLIBC_TUNABLES",
>> "glibc.malloc.perturb=0x800:glibc.malloc.mmap_threshold=4096",
>> 0,
>> 4096,
>> @@ -105,24 +116,28 @@ static const struct test_t
>> },
>> /* Invalid keys are ignored. */
>> {
>> + "GLIBC_TUNABLES",
>> ":glibc.malloc.garbage=2:glibc.malloc.check=1",
>> 1,
>> 0,
>> 0,
>> },
>> {
>> + "GLIBC_TUNABLES",
>> "glibc.malloc.perturb=0x800:not_valid.malloc.check=2:glibc.malloc.mmap_threshold=4096",
>> 0,
>> 4096,
>> 0,
>> },
>> {
>> + "GLIBC_TUNABLES",
>> "glibc.not_valid.check=2:glibc.malloc.mmap_threshold=4096",
>> 0,
>> 4096,
>> 0,
>> },
>> {
>> + "GLIBC_TUNABLES",
>> "not_valid.malloc.check=2:glibc.malloc.mmap_threshold=4096",
>> 0,
>> 4096,
>> @@ -130,24 +145,28 @@ static const struct test_t
>> },
>> /* Invalid subkeys are ignored. */
>> {
>> + "GLIBC_TUNABLES",
>> "glibc.malloc.garbage=2:glibc.maoc.mmap_threshold=4096:glibc.malloc.check=2",
>> 2,
>> 0,
>> 0,
>> },
>> {
>> + "GLIBC_TUNABLES",
>> "glibc.malloc.check=4:glibc.malloc.garbage=2:glibc.maoc.mmap_threshold=4096",
>> 0,
>> 0,
>> 0,
>> },
>> {
>> + "GLIBC_TUNABLES",
>> "not_valid.malloc.check=2",
>> 0,
>> 0,
>> 0,
>> },
>> {
>> + "GLIBC_TUNABLES",
>> "glibc.not_valid.check=2",
>> 0,
>> 0,
>> @@ -156,6 +175,7 @@ static const struct test_t
>> /* An ill-formatted tunable in the for key=key=value will considere the
>> value as 'key=value' (which can not be parsed as an integer). */
>> {
>> + "GLIBC_TUNABLES",
>> "glibc.malloc.mmap_threshold=glibc.malloc.mmap_threshold=4096",
>> 0,
>> 0,
>> @@ -163,41 +183,77 @@ static const struct test_t
>> },
>> /* Ill-formatted tunables string is not parsed. */
>> {
>> + "GLIBC_TUNABLES",
>> "glibc.malloc.mmap_threshold=glibc.malloc.mmap_threshold=4096:glibc.malloc.check=2",
>> 0,
>> 0,
>> 0,
>> },
>> {
>> + "GLIBC_TUNABLES",
>> "glibc.malloc.check=2=2",
>> 0,
>> 0,
>> 0,
>> },
>> {
>> + "GLIBC_TUNABLES",
>> "glibc.malloc.check=2=2:glibc.malloc.mmap_threshold=4096",
>> 0,
>> 0,
>> 0,
>> },
>> {
>> + "GLIBC_TUNABLES",
>> "glibc.malloc.check=2=2:glibc.malloc.check=2",
>> 0,
>> 0,
>> 0,
>> },
>> {
>> + "GLIBC_TUNABLES",
>> "glibc.malloc.check=2:glibc.malloc.mmap_threshold=4096=4096",
>> 0,
>> 0,
>> 0,
>> },
>> {
>> + "GLIBC_TUNABLES",
>> "glibc.malloc.check=2:glibc.malloc.mmap_threshold=4096=4096",
>> 0,
>> 0,
>> 0,
>> },
>> + /* Also check some tunable aliases. */
>> + {
>> + "MALLOC_CHECK_",
>> + "2",
>> + 2,
>> + 0,
>> + 0,
>> + },
>> + {
>> + "MALLOC_MMAP_THRESHOLD_",
>> + "4096",
>> + 0,
>> + 4096,
>> + 0,
>> + },
>> + {
>> + "MALLOC_PERTURB_",
>> + "0x55",
>> + 0,
>> + 0,
>> + 0x55,
>> + },
>> + /* 0x800 is larger than tunable maxval (0xff), so the tunable is unchanged. */
>> + {
>> + "MALLOC_PERTURB_",
>> + "0x800",
>> + 0,
>> + 0,
>> + 0,
>> + },
>> };
>> static int
>> @@ -245,13 +301,17 @@ do_test (int argc, char *argv[])
>> {
>> snprintf (nteststr, sizeof nteststr, "%d", i);
>> - printf ("[%d] Spawned test for %s\n", i, tests[i].env);
>> - setenv ("GLIBC_TUNABLES", tests[i].env, 1);
>> + printf ("[%d] Spawned test for %s=%s\n",
>> + i,
>> + tests[i].name,
>> + tests[i].value);
>> + setenv (tests[i].name, tests[i].value, 1);
>> struct support_capture_subprocess result
>> = support_capture_subprogram (spargv[0], spargv);
>> support_capture_subprocess_check (&result, "tst-tunables", 0,
>> sc_allow_stderr);
>> support_capture_subprocess_free (&result);
>> + unsetenv (tests[i].name);
>> }
>> return 0;
>> diff --git a/sysdeps/generic/dl-tunables-parse.h b/sysdeps/generic/dl-tunables-parse.h
>> new file mode 100644
>> index 0000000000..5b399f852d
>> --- /dev/null
>> +++ b/sysdeps/generic/dl-tunables-parse.h
>> @@ -0,0 +1,129 @@
>> +/* Helper functions to handle tunable strings.
>> + Copyright (C) 2023 Free Software Foundation, Inc.
>> + This file is part of the GNU C Library.
>> +
>> + The GNU C Library is free software; you can redistribute it and/or
>> + modify it under the terms of the GNU Lesser General Public
>> + License as published by the Free Software Foundation; either
>> + version 2.1 of the License, or (at your option) any later version.
>> +
>> + The GNU C Library is distributed in the hope that it will be useful,
>> + but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
>> + Lesser General Public License for more details.
>> +
>> + You should have received a copy of the GNU Lesser General Public
>> + License along with the GNU C Library; if not, see
>> + <https://www.gnu.org/licenses/>. */
>> +
>> +#ifndef _DL_TUNABLES_PARSE_H
>> +#define _DL_TUNABLES_PARSE_H 1
>> +
>> +#include <string.h>
>> +
>> +/* Compare the contents of STRVAL with STR of size LEN. The STR might not
>> + be null-terminated. */
>> +static __always_inline bool
>> +tunable_strcmp (const struct tunable_str_t *strval, const char *str,
>> + size_t len)
>> +{
>> + return strval->len == len && memcmp (strval->str, str, len) == 0;
>> +}
>> +#define tunable_strcmp_cte(__tunable, __str) \
>> + tunable_strcmp (&__tunable->strval, __str, sizeof (__str) - 1)
>> +
>> +/*
>> + Helper functions to iterate over a tunable string composed by multiple
>> + suboptions separated by commaxi; this is a common pattern for CPU. Each
>> + suboptions is return in the form of { address, size } (no null terminated).
>> + For instance:
>> +
>> + struct tunable_str_comma_t ts;
>> + tunable_str_comma_init (&ts, valp);
>> +
>> + struct tunable_str_t t;
>> + while (tunable_str_comma_next (&ts, &t))
>> + {
>> + _dl_printf ("[%s] %.*s (%d)\n",
>> + __func__,
>> + (int) tstr.len,
>> + tstr.str,
>> + (int) tstr.len);
>> +
>> + if (tunable_str_comma_strcmp (&t, opt, opt1_len))
>> + {
>> + [...]
>> + }
>> + else if (tunable_str_comma_strcmp_cte (&t, "opt2"))
>> + {
>> + [...]
>> + }
>> + }
>> +*/
>> +
>> +struct tunable_str_comma_state_t
>> +{
>> + const char *p;
>> + size_t plen;
>> + size_t maxplen;
>> +};
>> +
>> +struct tunable_str_comma_t
>> +{
>> + const char *str;
>> + size_t len;
>> + bool disable;
>> +};
>> +
>> +static inline void
>> +tunable_str_comma_init (struct tunable_str_comma_state_t *state,
>> + tunable_val_t *valp)
>> +{
>
> Maybe add an assertion here that valp->strval.str is not NULL since all functions assume that? Or at least a comment somewhere.
An assert won't work because tunable_val_t is an union. I will add
the comment:
NB: These function are expected to be called from tunable callback
functions along with tunable_val_t with string types.
>
>> + state->p = valp->strval.str;
>> + state->plen = 0;
>> + state->maxplen = valp->strval.len;
>> +}
>> +
>> +static inline bool
>> +tunable_str_comma_next (struct tunable_str_comma_state_t *state,
>> + struct tunable_str_comma_t *str)
>> +{
>> + if (*state->p == '\0' || state->plen >= state->maxplen)
>> + return false;
>> +
>> + const char *c;
>> + for (c = state->p; *c != ','; c++, state->plen++)
>> + if (*c == '\0' || state->plen == state->maxplen)
>> + break;
>> +
>> + str->str = state->p;
>> + str->len = c - state->p;
>> +
>> + if (str->len > 0)
>> + {
>> + str->disable = *str->str == '-';
>> + if (str->disable)
>> + {
>> + str->str = str->str + 1;
>> + str->len = str->len - 1;
>> + }
>> + }
>> +
>> + state->p = c + 1;
>> + state->plen++;
>> +
>> + return true;
>> +}
>> +
>> +/* Compare the contents of T with STR of size LEN. The STR might not be
>> + null-terminated. */
>> +static __always_inline bool
>> +tunable_str_comma_strcmp (const struct tunable_str_comma_t *t, const char *str,
>> + size_t len)
>> +{
>> + return t->len == len && memcmp (t->str, str, len) == 0;
>> +}
>> +#define tunable_str_comma_strcmp_cte(__t, __str) \
>> + tunable_str_comma_strcmp (__t, __str, sizeof (__str) - 1)
>> +
>> +#endif
>> diff --git a/sysdeps/s390/cpu-features.c b/sysdeps/s390/cpu-features.c
>> index 55449ba07f..8b1466fa7b 100644
>> --- a/sysdeps/s390/cpu-features.c
>> +++ b/sysdeps/s390/cpu-features.c
>> @@ -22,6 +22,7 @@
>> #include <ifunc-memcmp.h>
>> #include <string.h>
>> #include <dl-symbol-redir-ifunc.h>
>> +#include <dl-tunables-parse.h>
>> #define S390_COPY_CPU_FEATURES(SRC_PTR, DEST_PTR) \
>> (DEST_PTR)->hwcap = (SRC_PTR)->hwcap; \
>> @@ -51,33 +52,14 @@ TUNABLE_CALLBACK (set_hwcaps) (tunable_val_t *valp)
>> struct cpu_features cpu_features_curr;
>> S390_COPY_CPU_FEATURES (cpu_features, &cpu_features_curr);
>> - const char *token = valp->strval;
>> - do
>> + struct tunable_str_comma_state_t ts;
>> + tunable_str_comma_init (&ts, valp);
>> +
>> + struct tunable_str_comma_t t;
>> + while (tunable_str_comma_next (&ts, &t))
>> {
>> - const char *token_end, *feature;
>> - bool disable;
>> - size_t token_len;
>> - size_t feature_len;
>> -
>> - /* Find token separator or end of string. */
>> - for (token_end = token; *token_end != ','; token_end++)
>> - if (*token_end == '\0')
>> - break;
>> -
>> - /* Determine feature. */
>> - token_len = token_end - token;
>> - if (*token == '-')
>> - {
>> - disable = true;
>> - feature = token + 1;
>> - feature_len = token_len - 1;
>> - }
>> - else
>> - {
>> - disable = false;
>> - feature = token;
>> - feature_len = token_len;
>> - }
>> + if (t.len == 0)
>> + continue;
>> /* Handle only the features here which are really used in the
>> IFUNC-resolvers. All others are ignored as the values are only used
>> @@ -85,85 +67,65 @@ TUNABLE_CALLBACK (set_hwcaps) (tunable_val_t *valp)
>> bool reset_features = false;
>> unsigned long int hwcap_mask = 0UL;
>> unsigned long long stfle_bits0_mask = 0ULL;
>> + bool disable = t.disable;
>> - if ((*feature == 'z' || *feature == 'a'))
>> + if (tunable_str_comma_strcmp_cte (&t, "zEC12")
>> + || tunable_str_comma_strcmp_cte (&t, "arch10"))
>> + {
>> + reset_features = true;
>> + disable = true;
>> + hwcap_mask = HWCAP_S390_VXRS | HWCAP_S390_VXRS_EXT
>> + | HWCAP_S390_VXRS_EXT2;
>> + stfle_bits0_mask = S390_STFLE_MASK_ARCH13_MIE3;
>> + }
>> + else if (tunable_str_comma_strcmp_cte (&t, "z13")
>> + || tunable_str_comma_strcmp_cte (&t, "arch11"))
>> + {
>> + reset_features = true;
>> + disable = true;
>> + hwcap_mask = HWCAP_S390_VXRS_EXT | HWCAP_S390_VXRS_EXT2;
>> + stfle_bits0_mask = S390_STFLE_MASK_ARCH13_MIE3;
>> + }
>> + else if (tunable_str_comma_strcmp_cte (&t, "z14")
>> + || tunable_str_comma_strcmp_cte (&t, "arch12"))
>> + {
>> + reset_features = true;
>> + disable = true;
>> + hwcap_mask = HWCAP_S390_VXRS_EXT2;
>> + stfle_bits0_mask = S390_STFLE_MASK_ARCH13_MIE3;
>> + }
>> + else if (tunable_str_comma_strcmp_cte (&t, "z15")
>> + || tunable_str_comma_strcmp_cte (&t, "z16")
>> + || tunable_str_comma_strcmp_cte (&t, "arch13")
>> + || tunable_str_comma_strcmp_cte (&t, "arch14"))
>> {
>> - if ((feature_len == 5 && *feature == 'z'
>> - && memcmp (feature, "zEC12", 5) == 0)
>> - || (feature_len == 6 && *feature == 'a'
>> - && memcmp (feature, "arch10", 6) == 0))
>> - {
>> - reset_features = true;
>> - disable = true;
>> - hwcap_mask = HWCAP_S390_VXRS | HWCAP_S390_VXRS_EXT
>> - | HWCAP_S390_VXRS_EXT2;
>> - stfle_bits0_mask = S390_STFLE_MASK_ARCH13_MIE3;
>> - }
>> - else if ((feature_len == 3 && *feature == 'z'
>> - && memcmp (feature, "z13", 3) == 0)
>> - || (feature_len == 6 && *feature == 'a'
>> - && memcmp (feature, "arch11", 6) == 0))
>> - {
>> - reset_features = true;
>> - disable = true;
>> - hwcap_mask = HWCAP_S390_VXRS_EXT | HWCAP_S390_VXRS_EXT2;
>> - stfle_bits0_mask = S390_STFLE_MASK_ARCH13_MIE3;
>> - }
>> - else if ((feature_len == 3 && *feature == 'z'
>> - && memcmp (feature, "z14", 3) == 0)
>> - || (feature_len == 6 && *feature == 'a'
>> - && memcmp (feature, "arch12", 6) == 0))
>> - {
>> - reset_features = true;
>> - disable = true;
>> - hwcap_mask = HWCAP_S390_VXRS_EXT2;
>> - stfle_bits0_mask = S390_STFLE_MASK_ARCH13_MIE3;
>> - }
>> - else if ((feature_len == 3 && *feature == 'z'
>> - && (memcmp (feature, "z15", 3) == 0
>> - || memcmp (feature, "z16", 3) == 0))
>> - || (feature_len == 6
>> - && (memcmp (feature, "arch13", 6) == 0
>> - || memcmp (feature, "arch14", 6) == 0)))
>> - {
>> - /* For z15 or newer we don't have to disable something,
>> - but we have to reset to the original values. */
>> - reset_features = true;
>> - }
>> + /* For z15 or newer we don't have to disable something, but we have
>> + to reset to the original values. */
>> + reset_features = true;
>> }
>> - else if (*feature == 'H')
>> + else if (tunable_str_comma_strcmp_cte (&t, "HWCAP_S390_VXRS"))
>> {
>> - if (feature_len == 15
>> - && memcmp (feature, "HWCAP_S390_VXRS", 15) == 0)
>> - {
>> - hwcap_mask = HWCAP_S390_VXRS;
>> - if (disable)
>> - hwcap_mask |= HWCAP_S390_VXRS_EXT | HWCAP_S390_VXRS_EXT2;
>> - }
>> - else if (feature_len == 19
>> - && memcmp (feature, "HWCAP_S390_VXRS_EXT", 19) == 0)
>> - {
>> - hwcap_mask = HWCAP_S390_VXRS_EXT;
>> - if (disable)
>> - hwcap_mask |= HWCAP_S390_VXRS_EXT2;
>> - else
>> - hwcap_mask |= HWCAP_S390_VXRS;
>> - }
>> - else if (feature_len == 20
>> - && memcmp (feature, "HWCAP_S390_VXRS_EXT2", 20) == 0)
>> - {
>> - hwcap_mask = HWCAP_S390_VXRS_EXT2;
>> - if (!disable)
>> - hwcap_mask |= HWCAP_S390_VXRS | HWCAP_S390_VXRS_EXT;
>> - }
>> + hwcap_mask = HWCAP_S390_VXRS;
>> + if (t.disable)
>> + hwcap_mask |= HWCAP_S390_VXRS_EXT | HWCAP_S390_VXRS_EXT2;
>> }
>> - else if (*feature == 'S')
>> + else if (tunable_str_comma_strcmp_cte (&t, "HWCAP_S390_VXRS_EXT"))
>> {
>> - if (feature_len == 10
>> - && memcmp (feature, "STFLE_MIE3", 10) == 0)
>> - {
>> - stfle_bits0_mask = S390_STFLE_MASK_ARCH13_MIE3;
>> - }
>> + hwcap_mask = HWCAP_S390_VXRS_EXT;
>> + if (t.disable)
>> + hwcap_mask |= HWCAP_S390_VXRS_EXT2;
>> + else
>> + hwcap_mask |= HWCAP_S390_VXRS;
>> + }
>> + else if (tunable_str_comma_strcmp_cte (&t, "HWCAP_S390_VXRS_EXT2"))
>> + {
>> + hwcap_mask = HWCAP_S390_VXRS_EXT2;
>> + if (!t.disable)
>> + hwcap_mask |= HWCAP_S390_VXRS | HWCAP_S390_VXRS_EXT;
>> + }
>> + else if (tunable_str_comma_strcmp_cte (&t, "STFLE_MIE3"))
>> + {
>
> Redundant braces.
Ack.
>
>> + stfle_bits0_mask = S390_STFLE_MASK_ARCH13_MIE3;
>> }
>> /* Perform the actions determined above. */
>> @@ -187,14 +149,7 @@ TUNABLE_CALLBACK (set_hwcaps) (tunable_val_t *valp)
>> else
>> cpu_features_curr.stfle_bits[0] |= stfle_bits0_mask;
>> }
>> -
>> - /* Jump over current token ... */
>> - token += token_len;
>> -
>> - /* ... and skip token separator for next round. */
>> - if (*token == ',') token++;
>> }
>> - while (*token != '\0');
>> /* Copy back the features after checking that no unsupported features were
>> enabled by user. */
>> diff --git a/sysdeps/unix/sysv/linux/aarch64/cpu-features.c b/sysdeps/unix/sysv/linux/aarch64/cpu-features.c
>> index 233d5b2407..9b76cb89c7 100644
>> --- a/sysdeps/unix/sysv/linux/aarch64/cpu-features.c
>> +++ b/sysdeps/unix/sysv/linux/aarch64/cpu-features.c
>> @@ -16,10 +16,12 @@
>> License along with the GNU C Library; if not, see
>> <https://www.gnu.org/licenses/>. */
>> +#include <array_length.h>
>> #include <cpu-features.h>
>> #include <sys/auxv.h>
>> #include <elf/dl-hwcaps.h>
>> #include <sys/prctl.h>
>> +#include <dl-tunables-parse.h>
>> #define DCZID_DZP_MASK (1 << 4)
>> #define DCZID_BS_MASK (0xf)
>> @@ -33,28 +35,32 @@
>> struct cpu_list
>> {
>> const char *name;
>> + size_t len;
>> uint64_t midr;
>> };
>> -static struct cpu_list cpu_list[] = {
>> - {"falkor", 0x510FC000},
>> - {"thunderxt88", 0x430F0A10},
>> - {"thunderx2t99", 0x431F0AF0},
>> - {"thunderx2t99p1", 0x420F5160},
>> - {"phecda", 0x680F0000},
>> - {"ares", 0x411FD0C0},
>> - {"emag", 0x503F0001},
>> - {"kunpeng920", 0x481FD010},
>> - {"a64fx", 0x460F0010},
>> - {"generic", 0x0}
>> +static const struct cpu_list cpu_list[] =
>> +{
>> +#define CPU_LIST_ENTRY(__str, __num) { __str, sizeof (__str) - 1, __num }
>> + CPU_LIST_ENTRY ("falkor", 0x510FC000),
>> + CPU_LIST_ENTRY ("thunderxt88", 0x430F0A10),
>> + CPU_LIST_ENTRY ("thunderx2t99", 0x431F0AF0),
>> + CPU_LIST_ENTRY ("thunderx2t99p1", 0x420F5160),
>> + CPU_LIST_ENTRY ("phecda", 0x680F0000),
>> + CPU_LIST_ENTRY ("ares", 0x411FD0C0),
>> + CPU_LIST_ENTRY ("emag", 0x503F0001),
>> + CPU_LIST_ENTRY ("kunpeng920", 0x481FD010),
>> + CPU_LIST_ENTRY ("a64fx", 0x460F0010),
>> + CPU_LIST_ENTRY ("generic", 0x0),
>> };
>> static uint64_t
>> -get_midr_from_mcpu (const char *mcpu)
>> +get_midr_from_mcpu (const struct tunable_str_t *mcpu)
>> {
>> - for (int i = 0; i < sizeof (cpu_list) / sizeof (struct cpu_list); i++)
>> - if (strcmp (mcpu, cpu_list[i].name) == 0)
>> + for (int i = 0; i < array_length (cpu_list); i++) {
>> + if (tunable_strcmp (mcpu, cpu_list[i].name, cpu_list[i].len))
>> return cpu_list[i].midr;
>> + }
>
> Redundant braces.
>
Ack.
>> return UINT64_MAX;
>> }
>> @@ -65,7 +71,9 @@ init_cpu_features (struct cpu_features *cpu_features)
>> register uint64_t midr = UINT64_MAX;
>> /* Get the tunable override. */
>> - const char *mcpu = TUNABLE_GET (glibc, cpu, name, const char *, NULL);
>> + const struct tunable_str_t *mcpu = TUNABLE_GET (glibc, cpu, name,
>> + struct tunable_str_t *,
>> + NULL);
>> if (mcpu != NULL)
>> midr = get_midr_from_mcpu (mcpu);
>> diff --git a/sysdeps/unix/sysv/linux/powerpc/cpu-features.c b/sysdeps/unix/sysv/linux/powerpc/cpu-features.c
>> index 7c6e20e702..390b3fd11a 100644
>> --- a/sysdeps/unix/sysv/linux/powerpc/cpu-features.c
>> +++ b/sysdeps/unix/sysv/linux/powerpc/cpu-features.c
>> @@ -20,6 +20,7 @@
>> #include <stdint.h>
>> #include <cpu-features.h>
>> #include <elf/dl-tunables.h>
>> +#include <dl-tunables-parse.h>
>> #include <unistd.h>
>> #include <string.h>
>> @@ -43,41 +44,26 @@ TUNABLE_CALLBACK (set_hwcaps) (tunable_val_t *valp)
>> struct cpu_features *cpu_features = &GLRO(dl_powerpc_cpu_features);
>> unsigned long int tcbv_hwcap = cpu_features->hwcap;
>> unsigned long int tcbv_hwcap2 = cpu_features->hwcap2;
>> - const char *token = valp->strval;
>> - do
>> +
>> + struct tunable_str_comma_state_t ts;
>> + tunable_str_comma_init (&ts, valp);
>> +
>> + struct tunable_str_comma_t t;
>> + while (tunable_str_comma_next (&ts, &t))
>> {
>> - const char *token_end, *feature;
>> - bool disable;
>> - size_t token_len, i, feature_len, offset = 0;
>> - /* Find token separator or end of string. */
>> - for (token_end = token; *token_end != ','; token_end++)
>> - if (*token_end == '\0')
>> - break;
>> + if (t.len == 0)
>> + continue;
>> - /* Determine feature. */
>> - token_len = token_end - token;
>> - if (*token == '-')
>> - {
>> - disable = true;
>> - feature = token + 1;
>> - feature_len = token_len - 1;
>> - }
>> - else
>> - {
>> - disable = false;
>> - feature = token;
>> - feature_len = token_len;
>> - }
>> - for (i = 0; i < array_length (hwcap_tunables); ++i)
>> + size_t offset = 0;
>> + for (int i = 0; i < array_length (hwcap_tunables); ++i)
>> {
>> const char *hwcap_name = hwcap_names + offset;
>> size_t hwcap_name_len = strlen (hwcap_name);
>> /* Check the tunable name on the supported list. */
>> - if (hwcap_name_len == feature_len
>> - && memcmp (feature, hwcap_name, feature_len) == 0)
>> + if (tunable_str_comma_strcmp (&t, hwcap_name, hwcap_name_len))
>> {
>> /* Update the hwcap and hwcap2 bits. */
>> - if (disable)
>> + if (t.disable)
>> {
>> /* Id is 1 for hwcap2 tunable. */
>> if (hwcap_tunables[i].id)
>> @@ -98,12 +84,7 @@ TUNABLE_CALLBACK (set_hwcaps) (tunable_val_t *valp)
>> }
>> offset += hwcap_name_len + 1;
>> }
>> - token += token_len;
>> - /* ... and skip token separator for next round. */
>> - if (*token == ',')
>> - token++;
>> }
>> - while (*token != '\0');
>> }
>> static inline void
>> diff --git a/sysdeps/unix/sysv/linux/powerpc/tst-hwcap-tunables.c b/sysdeps/unix/sysv/linux/powerpc/tst-hwcap-tunables.c
>> index 2631016a3a..049164f841 100644
>> --- a/sysdeps/unix/sysv/linux/powerpc/tst-hwcap-tunables.c
>> +++ b/sysdeps/unix/sysv/linux/powerpc/tst-hwcap-tunables.c
>> @@ -110,7 +110,11 @@ do_test (int argc, char *argv[])
>> run_test ("-arch_2_06", "__memcpy_power7");
>> if (hwcap & PPC_FEATURE_ARCH_2_05)
>> run_test ("-arch_2_06,-arch_2_05","__memcpy_power6");
>> - run_test ("-arch_2_06,-arch_2_05,-power5+,-power5,-power4", "__memcpy_power4");
>> + run_test ("-arch_2_06,-arch_2_05,-power5+,-power5,-power4",
>> + "__memcpy_power4");
>> + /* Also run with valid, but empty settings. */
>> + run_test (",-,-arch_2_06,-arch_2_05,-power5+,-power5,,-power4,-",
>> + "__memcpy_power4");
>> }
>> else
>> {
>> diff --git a/sysdeps/x86/Makefile b/sysdeps/x86/Makefile
>> index 917c26f116..a64e5f002a 100644
>> --- a/sysdeps/x86/Makefile
>> +++ b/sysdeps/x86/Makefile
>> @@ -12,7 +12,8 @@ CFLAGS-get-cpuid-feature-leaf.o += $(no-stack-protector)
>> tests += tst-get-cpu-features tst-get-cpu-features-static \
>> tst-cpu-features-cpuinfo tst-cpu-features-cpuinfo-static \
>> - tst-cpu-features-supports tst-cpu-features-supports-static
>> + tst-cpu-features-supports tst-cpu-features-supports-static \
>> + tst-hwcap-tunables
>> tests-static += tst-get-cpu-features-static \
>> tst-cpu-features-cpuinfo-static \
>> tst-cpu-features-supports-static
>> @@ -65,6 +66,7 @@ $(objpfx)tst-isa-level-1.out: $(objpfx)tst-isa-level-mod-1-baseline.so \
>> endif
>> tst-ifunc-isa-2-ENV = GLIBC_TUNABLES=glibc.cpu.hwcaps=-SSE4_2,-AVX,-AVX2,-AVX512F
>> tst-ifunc-isa-2-static-ENV = $(tst-ifunc-isa-2-ENV)
>> +tst-hwcap-tunables-ARGS = -- $(host-test-program-cmd)
>> endif
>> ifeq ($(subdir),math)
>> diff --git a/sysdeps/x86/cpu-tunables.c b/sysdeps/x86/cpu-tunables.c
>> index 5697885226..0608209768 100644
>> --- a/sysdeps/x86/cpu-tunables.c
>> +++ b/sysdeps/x86/cpu-tunables.c
>> @@ -24,11 +24,12 @@
>> #include <string.h>
>> #include <cpu-features.h>
>> #include <ldsodefs.h>
>> +#include <dl-tunables-parse.h>
>> #include <dl-symbol-redir-ifunc.h>
>> #define CHECK_GLIBC_IFUNC_CPU_OFF(f, cpu_features, name, len) \
>> _Static_assert (sizeof (#name) - 1 == len, #name " != " #len); \
>> - if (memcmp (f, #name, len) == 0) \
>> + if (tunable_str_comma_strcmp_cte (&f, #name)) \
>> { \
>> CPU_FEATURE_UNSET (cpu_features, name) \
>> break; \
>> @@ -38,7 +39,7 @@
>> which isn't available. */
>> #define CHECK_GLIBC_IFUNC_PREFERRED_OFF(f, cpu_features, name, len) \
>> _Static_assert (sizeof (#name) - 1 == len, #name " != " #len); \
>> - if (memcmp (f, #name, len) == 0) \
>> + if (tunable_str_comma_strcmp_cte (&f, #name) == 0) \
>> { \
>> cpu_features->preferred[index_arch_##name] \
>> &= ~bit_arch_##name; \
>> @@ -46,12 +47,11 @@
>> }
>> /* Enable/disable a preferred feature NAME. */
>> -#define CHECK_GLIBC_IFUNC_PREFERRED_BOTH(f, cpu_features, name, \
>> - disable, len) \
>> +#define CHECK_GLIBC_IFUNC_PREFERRED_BOTH(f, cpu_features, name, len) \
>
> Spurious tab.
Ack.
>
>> _Static_assert (sizeof (#name) - 1 == len, #name " != " #len); \
>> - if (memcmp (f, #name, len) == 0) \
>> + if (tunable_str_comma_strcmp_cte (&f, #name)) \
>> { \
>> - if (disable) \
>> + if (f.disable) \
>> cpu_features->preferred[index_arch_##name] &= ~bit_arch_##name; \
>> else \
>> cpu_features->preferred[index_arch_##name] |= bit_arch_##name; \
>> @@ -61,11 +61,11 @@
>> /* Enable/disable a preferred feature NAME. Enable a preferred feature
>> only if the feature NEED is usable. */
>> #define CHECK_GLIBC_IFUNC_PREFERRED_NEED_BOTH(f, cpu_features, name, \
>> - need, disable, len) \
>> + need, len) \
>> _Static_assert (sizeof (#name) - 1 == len, #name " != " #len); \
>> - if (memcmp (f, #name, len) == 0) \
>> + if (tunable_str_comma_strcmp_cte (&f, #name)) \
>> { \
>> - if (disable) \
>> + if (f.disable) \
>> cpu_features->preferred[index_arch_##name] &= ~bit_arch_##name; \
>> else if (CPU_FEATURE_USABLE_P (cpu_features, need)) \
>> cpu_features->preferred[index_arch_##name] |= bit_arch_##name; \
>> @@ -93,38 +93,20 @@ TUNABLE_CALLBACK (set_hwcaps) (tunable_val_t *valp)
>> NOTE: the IFUNC selection may change over time. Please check all
>> multiarch implementations when experimenting. */
>> - const char *p = valp->strval, *c;
>> struct cpu_features *cpu_features = &GLRO(dl_x86_cpu_features);
>> - size_t len;
>> - do
>> - {
>> - const char *n;
>> - bool disable;
>> - size_t nl;
>> -
>> - for (c = p; *c != ','; c++)
>> - if (*c == '\0')
>> - break;
>> + struct tunable_str_comma_state_t ts;
>> + tunable_str_comma_init (&ts, valp);
>> - len = c - p;
>> - disable = *p == '-';
>> - if (disable)
>> - {
>> - n = p + 1;
>> - nl = len - 1;
>> - }
>> - else
>> - {
>> - n = p;
>> - nl = len;
>> - }
>> - switch (nl)
>> + struct tunable_str_comma_t n;
>> + while (tunable_str_comma_next (&ts, &n))
>> + {
>> + switch (n.len)
>> {
>> default:
>> break;
>> case 3:
>> - if (disable)
>> + if (n.disable)
>> {
>> CHECK_GLIBC_IFUNC_CPU_OFF (n, cpu_features, AVX, 3);
>> CHECK_GLIBC_IFUNC_CPU_OFF (n, cpu_features, CX8, 3);
>> @@ -135,7 +117,7 @@ TUNABLE_CALLBACK (set_hwcaps) (tunable_val_t *valp)
>> }
>> break;
>> case 4:
>> - if (disable)
>> + if (n.disable)
>> {
>> CHECK_GLIBC_IFUNC_CPU_OFF (n, cpu_features, AVX2, 4);
>> CHECK_GLIBC_IFUNC_CPU_OFF (n, cpu_features, BMI1, 4);
>> @@ -149,7 +131,7 @@ TUNABLE_CALLBACK (set_hwcaps) (tunable_val_t *valp)
>> }
>> break;
>> case 5:
>> - if (disable)
>> + if (n.disable)
>> {
>> CHECK_GLIBC_IFUNC_CPU_OFF (n, cpu_features, LZCNT, 5);
>> CHECK_GLIBC_IFUNC_CPU_OFF (n, cpu_features, MOVBE, 5);
>> @@ -159,12 +141,12 @@ TUNABLE_CALLBACK (set_hwcaps) (tunable_val_t *valp)
>> }
>> break;
>> case 6:
>> - if (disable)
>> + if (n.disable)
>> {
>> CHECK_GLIBC_IFUNC_CPU_OFF (n, cpu_features, POPCNT, 6);
>> CHECK_GLIBC_IFUNC_CPU_OFF (n, cpu_features, SSE4_1, 6);
>> CHECK_GLIBC_IFUNC_CPU_OFF (n, cpu_features, SSE4_2, 6);
>> - if (memcmp (n, "XSAVEC", 6) == 0)
>> + if (memcmp (n.str, "XSAVEC", 6) == 0)
>
> Why does this use the bare memcmp and not the tunables_strcmp?
If I recall correctly, I did tried it and it resulted in worse codegen. The
tunable_str_comma_strcmp also checks the size, which on the x86 code is not
required and for some reason compiler can not eliminate.
>
>> {
>> /* Update xsave_state_size to XSAVE state size. */
>> cpu_features->xsave_state_size
>> @@ -174,14 +156,14 @@ TUNABLE_CALLBACK (set_hwcaps) (tunable_val_t *valp)
>> }
>> break;
>> case 7:
>> - if (disable)
>> + if (n.disable)
>> {
>> CHECK_GLIBC_IFUNC_CPU_OFF (n, cpu_features, AVX512F, 7);
>> CHECK_GLIBC_IFUNC_CPU_OFF (n, cpu_features, OSXSAVE, 7);
>> }
>> break;
>> case 8:
>> - if (disable)
>> + if (n.disable)
>> {
>> CHECK_GLIBC_IFUNC_CPU_OFF (n, cpu_features, AVX512CD, 8);
>> CHECK_GLIBC_IFUNC_CPU_OFF (n, cpu_features, AVX512BW, 8);
>> @@ -190,86 +172,72 @@ TUNABLE_CALLBACK (set_hwcaps) (tunable_val_t *valp)
>> CHECK_GLIBC_IFUNC_CPU_OFF (n, cpu_features, AVX512PF, 8);
>> CHECK_GLIBC_IFUNC_CPU_OFF (n, cpu_features, AVX512VL, 8);
>> }
>> - CHECK_GLIBC_IFUNC_PREFERRED_BOTH (n, cpu_features, Slow_BSF,
>> - disable, 8);
>> + CHECK_GLIBC_IFUNC_PREFERRED_BOTH (n, cpu_features, Slow_BSF, 8);
>> break;
>> case 11:
>> {
>> - CHECK_GLIBC_IFUNC_PREFERRED_BOTH (n, cpu_features,
>> - Prefer_ERMS,
>> - disable, 11);
>> - CHECK_GLIBC_IFUNC_PREFERRED_BOTH (n, cpu_features,
>> - Prefer_FSRM,
>> - disable, 11);
>> + CHECK_GLIBC_IFUNC_PREFERRED_BOTH (n, cpu_features, Prefer_ERMS,
>> + 11);
>> + CHECK_GLIBC_IFUNC_PREFERRED_BOTH (n, cpu_features, Prefer_FSRM,
>> + 11);
>> CHECK_GLIBC_IFUNC_PREFERRED_NEED_BOTH (n, cpu_features,
>> Slow_SSE4_2,
>> SSE4_2,
>> - disable, 11);
>> + 11);
>> }
>> break;
>> case 15:
>> {
>> CHECK_GLIBC_IFUNC_PREFERRED_BOTH (n, cpu_features,
>> - Fast_Rep_String,
>> - disable, 15);
>> + Fast_Rep_String, 15);
>> }
>> break;
>> case 16:
>> {
>> CHECK_GLIBC_IFUNC_PREFERRED_NEED_BOTH
>> - (n, cpu_features, Prefer_No_AVX512, AVX512F,
>> - disable, 16);
>> + (n, cpu_features, Prefer_No_AVX512, AVX512F, 16);
>> }
>> break;
>> case 18:
>> {
>> CHECK_GLIBC_IFUNC_PREFERRED_BOTH (n, cpu_features,
>> - Fast_Copy_Backward,
>> - disable, 18);
>> + Fast_Copy_Backward, 18);
>> }
>> break;
>> case 19:
>> {
>> CHECK_GLIBC_IFUNC_PREFERRED_BOTH (n, cpu_features,
>> - Fast_Unaligned_Load,
>> - disable, 19);
>> + Fast_Unaligned_Load, 19);
>> CHECK_GLIBC_IFUNC_PREFERRED_BOTH (n, cpu_features,
>> - Fast_Unaligned_Copy,
>> - disable, 19);
>> + Fast_Unaligned_Copy, 19);
>> }
>> break;
>> case 20:
>> {
>> CHECK_GLIBC_IFUNC_PREFERRED_NEED_BOTH
>> - (n, cpu_features, Prefer_No_VZEROUPPER, AVX, disable,
>> - 20);
>> + (n, cpu_features, Prefer_No_VZEROUPPER, AVX, 20);
>> }
>> break;
>> case 23:
>> {
>> CHECK_GLIBC_IFUNC_PREFERRED_NEED_BOTH
>> - (n, cpu_features, AVX_Fast_Unaligned_Load, AVX,
>> - disable, 23);
>> + (n, cpu_features, AVX_Fast_Unaligned_Load, AVX, 23);
>> }
>> break;
>> case 24:
>> {
>> CHECK_GLIBC_IFUNC_PREFERRED_NEED_BOTH
>> - (n, cpu_features, MathVec_Prefer_No_AVX512, AVX512F,
>> - disable, 24);
>> + (n, cpu_features, MathVec_Prefer_No_AVX512, AVX512F, 24);
>> }
>> break;
>> case 26:
>> {
>> CHECK_GLIBC_IFUNC_PREFERRED_NEED_BOTH
>> - (n, cpu_features, Prefer_PMINUB_for_stringop, SSE2,
>> - disable, 26);
>> + (n, cpu_features, Prefer_PMINUB_for_stringop, SSE2, 26);
>> }
>> break;
>> }
>> - p += len + 1;
>> }
>> - while (*c != '\0');
>> }
>> #if CET_ENABLED
>> @@ -277,11 +245,11 @@ attribute_hidden
>> void
>> TUNABLE_CALLBACK (set_x86_ibt) (tunable_val_t *valp)
>> {
>> - if (memcmp (valp->strval, "on", sizeof ("on")) == 0)
>> + if (tunable_strcmp_cte (valp, "on"))
>> GL(dl_x86_feature_control).ibt = cet_always_on;
>> - else if (memcmp (valp->strval, "off", sizeof ("off")) == 0)
>> + else if (tunable_strcmp_cte (valp, "off"))
>> GL(dl_x86_feature_control).ibt = cet_always_off;
>> - else if (memcmp (valp->strval, "permissive", sizeof ("permissive")) == 0)
>> + else if (tunable_strcmp_cte (valp, "permissive"))
>> GL(dl_x86_feature_control).ibt = cet_permissive;
>> }
>> @@ -289,11 +257,11 @@ attribute_hidden
>> void
>> TUNABLE_CALLBACK (set_x86_shstk) (tunable_val_t *valp)
>> {
>> - if (memcmp (valp->strval, "on", sizeof ("on")) == 0)
>> + if (tunable_strcmp_cte (valp, "on"))
>> GL(dl_x86_feature_control).shstk = cet_always_on;
>> - else if (memcmp (valp->strval, "off", sizeof ("off")) == 0)
>> + else if (tunable_strcmp_cte (valp, "off"))
>> GL(dl_x86_feature_control).shstk = cet_always_off;
>> - else if (memcmp (valp->strval, "permissive", sizeof ("permissive")) == 0)
>> + else if (tunable_strcmp_cte (valp, "permissive"))
>> GL(dl_x86_feature_control).shstk = cet_permissive;
>> }
>> #endif
>> diff --git a/sysdeps/x86/tst-hwcap-tunables.c b/sysdeps/x86/tst-hwcap-tunables.c
>> new file mode 100644
>> index 0000000000..0d0a45fa93
>> --- /dev/null
>> +++ b/sysdeps/x86/tst-hwcap-tunables.c
>> @@ -0,0 +1,148 @@
>> +/* Tests for powerpc GLIBC_TUNABLES=glibc.cpu.hwcaps filter.
>
> s/powerpc/x86/
Ack.
>
>> + Copyright (C) 2023 Free Software Foundation, Inc.
>> + This file is part of the GNU C Library.
>> +
>> + The GNU C Library is free software; you can redistribute it and/or
>> + modify it under the terms of the GNU Lesser General Public
>> + License as published by the Free Software Foundation; either
>> + version 2.1 of the License, or (at your option) any later version.
>> +
>> + The GNU C Library is distributed in the hope that it will be useful,
>> + but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
>> + Lesser General Public License for more details.
>> +
>> + You should have received a copy of the GNU Lesser General Public
>> + License along with the GNU C Library; if not, see
>> + <http://www.gnu.org/licenses/>. */
>> +
>> +#include <array_length.h>
>> +#include <getopt.h>
>> +#include <ifunc-impl-list.h>
>> +#include <spawn.h>
>> +#include <stdio.h>
>> +#include <stdlib.h>
>> +#include <string.h>
>> +#include <intprops.h>
>> +#include <support/check.h>
>> +#include <support/support.h>
>> +#include <support/xunistd.h>
>> +#include <support/capture_subprocess.h>
>> +
>> +/* Nonzero if the program gets called via `exec'. */
>> +#define CMDLINE_OPTIONS \
>> + { "restart", no_argument, &restart, 1 },
>> +static int restart;
>> +
>> +/* Disable everything. */
>> +static const char *test_1[] =
>> +{
>> + "__memcpy_avx512_no_vzeroupper",
>> + "__memcpy_avx512_unaligned",
>> + "__memcpy_avx512_unaligned_erms",
>> + "__memcpy_evex_unaligned",
>> + "__memcpy_evex_unaligned_erms",
>> + "__memcpy_avx_unaligned",
>> + "__memcpy_avx_unaligned_erms",
>> + "__memcpy_avx_unaligned_rtm",
>> + "__memcpy_avx_unaligned_erms_rtm",
>> + "__memcpy_ssse3",
>> +};
>> +
>> +static const struct test_t
>> +{
>> + const char *env;
>> + const char *const *funcs;
>> + size_t nfuncs;
>> +} tests[] =
>> +{
>> + {
>> + /* Disable everything. */
>> + "-Prefer_ERMS,-Prefer_FSRM,-AVX,-AVX2,-AVX_Usable,-AVX2_Usable,"
>> + "-AVX512F_Usable,-SSE4_1,-SSE4_2,-SSSE3,-Fast_Unaligned_Load,-ERMS,"
>> + "-AVX_Fast_Unaligned_Load",
>> + test_1,
>> + array_length (test_1)
>> + },
>> + {
>> + /* Same as before, but with some empty suboptions. */
>> + ",-,-Prefer_ERMS,-Prefer_FSRM,-AVX,-AVX2,-AVX_Usable,-AVX2_Usable,"
>> + "-AVX512F_Usable,-SSE4_1,-SSE4_2,,-SSSE3,-Fast_Unaligned_Load,,-,-ERMS,"
>> + "-AVX_Fast_Unaligned_Load,-,",
>> + test_1,
>> + array_length (test_1)
>> + }
>> +};
>> +
>> +/* Called on process re-execution. */
>> +_Noreturn static void
>> +handle_restart (int ntest)
>> +{
>> + struct libc_ifunc_impl impls[32];
>> + int cnt = __libc_ifunc_impl_list ("memcpy", impls, array_length (impls));
>> + if (cnt == 0)
>> + _exit (EXIT_SUCCESS);
>> + TEST_VERIFY_EXIT (cnt >= 1);
>> + for (int i = 0; i < cnt; i++)
>> + {
>> + for (int f = 0; f < tests[ntest].nfuncs; f++)
>> + {
>> + if (strcmp (impls[i].name, tests[ntest].funcs[f]) == 0)
>> + TEST_COMPARE (impls[i].usable, false);
>> + }
>> + }
>> +
>> + _exit (EXIT_SUCCESS);
>> +}
>> +
>> +static int
>> +do_test (int argc, char *argv[])
>> +{
>> + /* We must have either:
>> + - One our fource parameters left if called initially:
>> + + path to ld.so optional
>> + + "--library-path" optional
>> + + the library path optional
>> + + the application name
>> + + the test to check */
>> +
>> + TEST_VERIFY_EXIT (argc == 2 || argc == 5);
>> +
>> + if (restart)
>> + handle_restart (atoi (argv[1]));
>> +
>> + char nteststr[INT_BUFSIZE_BOUND (int)];
>> +
>> + char *spargv[10];
>> + {
>> + int i = 0;
>> + for (; i < argc - 1; i++)
>> + spargv[i] = argv[i + 1];
>> + spargv[i++] = (char *) "--direct";
>> + spargv[i++] = (char *) "--restart";
>> + spargv[i++] = nteststr;
>> + spargv[i] = NULL;
>> + }
>> +
>> + for (int i = 0; i < array_length (tests); i++)
>> + {
>> + snprintf (nteststr, sizeof nteststr, "%d", i);
>> +
>> + printf ("[%d] Spawned test for %s\n", i, tests[i].env);
>> + char *tunable = xasprintf ("glibc.cpu.hwcaps=%s", tests[i].env);
>> + setenv ("GLIBC_TUNABLES", tunable, 1);
>> +
>> + struct support_capture_subprocess result
>> + = support_capture_subprogram (spargv[0], spargv);
>> + support_capture_subprocess_check (&result, "tst-tunables", 0,
>> + sc_allow_stderr);
>> + support_capture_subprocess_free (&result);
>> +
>> + free (tunable);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +#define TEST_FUNCTION_ARGV do_test
>> +#include <support/test-driver.c>
next prev parent reply other threads:[~2023-11-21 18:12 UTC|newest]
Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-11-06 20:25 [PATCH v3 00/19] Improve loader environment variable handling Adhemerval Zanella
2023-11-06 20:25 ` [PATCH v3 01/19] elf: Remove /etc/suid-debug support Adhemerval Zanella
2023-11-06 20:25 ` [PATCH v3 02/19] elf: Add GLIBC_TUNABLES to unsecvars Adhemerval Zanella
2023-11-06 20:25 ` [PATCH v3 03/19] elf: Ignore GLIBC_TUNABLES for setuid/setgid binaries Adhemerval Zanella
2023-11-06 20:25 ` [PATCH v3 04/19] elf: Add all malloc tunable to unsecvars Adhemerval Zanella
2023-11-06 20:25 ` [PATCH v3 05/19] elf: Do not process invalid tunable format Adhemerval Zanella
2023-11-06 20:25 ` [PATCH v3 06/19] elf: Do not parse ill-formatted strings Adhemerval Zanella
2023-11-20 21:48 ` Siddhesh Poyarekar
2023-11-06 20:25 ` [PATCH v3 07/19] elf: Fix _dl_debug_vdprintf to work before self-relocation Adhemerval Zanella
2023-11-06 20:25 ` [PATCH v3 08/19] elf: Emit warning if tunable is ill-formatted Adhemerval Zanella
2023-11-20 21:50 ` Siddhesh Poyarekar
2023-11-06 20:25 ` [PATCH v3 09/19] x86: Use dl-symbol-redir-ifunc.h on cpu-tunables Adhemerval Zanella
2023-11-06 20:25 ` [PATCH v3 10/19] s390: " Adhemerval Zanella
2023-11-06 20:25 ` [PATCH v3 11/19] elf: Do not duplicate the GLIBC_TUNABLES string Adhemerval Zanella
2023-11-20 22:44 ` Siddhesh Poyarekar
2023-11-21 18:12 ` Adhemerval Zanella Netto [this message]
2023-11-22 11:39 ` Adhemerval Zanella Netto
2023-11-22 12:23 ` Siddhesh Poyarekar
2023-11-22 13:03 ` Adhemerval Zanella Netto
2023-11-22 13:24 ` Siddhesh Poyarekar
2023-11-22 14:13 ` Adhemerval Zanella Netto
2023-11-06 20:25 ` [PATCH v3 12/19] elf: Ignore LD_PROFILE for setuid binaries Adhemerval Zanella
2023-11-20 22:47 ` Siddhesh Poyarekar
2023-11-06 20:25 ` [PATCH v3 13/19] elf: Remove LD_PROFILE for static binaries Adhemerval Zanella
2023-11-20 22:55 ` Siddhesh Poyarekar
2023-11-06 20:25 ` [PATCH v3 14/19] elf: Ignore loader debug env vars for setuid Adhemerval Zanella
2023-11-20 22:57 ` Siddhesh Poyarekar
2023-11-21 18:24 ` Adhemerval Zanella Netto
2023-11-06 20:25 ` [PATCH v3 15/19] elf: Remove any_debug from dl_main_state Adhemerval Zanella
2023-11-20 22:58 ` Siddhesh Poyarekar
2023-11-06 20:25 ` [PATCH v3 16/19] elf: Ignore LD_LIBRARY_PATH and debug env var for setuid for static Adhemerval Zanella
2023-11-20 22:59 ` Siddhesh Poyarekar
2023-11-06 20:25 ` [PATCH v3 17/19] elf: Add comments on how LD_AUDIT and LD_PRELOAD handle __libc_enable_secure Adhemerval Zanella
2023-11-06 20:25 ` [PATCH v3 18/19] elf: Ignore LD_BIND_NOW and LD_BIND_NOT for setuid binaries Adhemerval Zanella
2023-11-20 23:02 ` Siddhesh Poyarekar
2023-11-06 20:25 ` [PATCH v3 19/19] elf: Refactor process_envvars Adhemerval Zanella
2023-11-20 23:09 ` Siddhesh Poyarekar
2023-11-21 19:00 ` Adhemerval Zanella Netto
2023-11-20 23:12 ` [PATCH v3 00/19] Improve loader environment variable handling Siddhesh Poyarekar
2023-11-21 19:37 ` Adhemerval Zanella Netto
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ca6b4ce7-46ab-45d7-829d-12845982f8b4@linaro.org \
--to=adhemerval.zanella@linaro.org \
--cc=libc-alpha@sourceware.org \
--cc=siddhesh@sourceware.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).