[v3,11/19] elf: Do not duplicate the GLIBC_TUNABLES string

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

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.

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

> +  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.

> +	  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.

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

>     _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?

>   		{
>   		  /* 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/

> +   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>

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>

>>> +
>>> +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.
> 

On a second though, assert would work here.

On 2023-11-21 13:12, Adhemerval Zanella Netto wrote:
>>> -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:

Sure, but the strval here should not be the entire envvar, just the 
value portion of it.  Granted that _dl_strtoul will bail out on the 
first non-numeric character, so maybe it's not

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

Why isn't it required? Couldn't n.str be smaller than 6 chars?

Thanks,
Sid

On 22/11/23 09:23, Siddhesh Poyarekar wrote:
> On 2023-11-21 13:12, Adhemerval Zanella Netto wrote:
>>>> -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:
> 
> Sure, but the strval here should not be the entire envvar, just the value portion of it.  Granted that _dl_strtoul will bail out on the first non-numeric character, so maybe it's not

For the GLIBC_TUNABLES itself, parse_tunables will pass the correct length
for each tunable.  For the alias case, get_next_env will pass the value
length (which I fixed on v4 btw). So I think there is no need to copy the
value on a temporary value, parsing can be done in-place since kernel ensures
the string will be null-terminated.

> 
>>>> -      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.
> 
> Why isn't it required? Couldn't n.str be smaller than 6 chars?

No because on the start of the loop the switch checks for 'n.len'.

On 2023-11-22 08:03, Adhemerval Zanella Netto wrote:
> 
> 
> On 22/11/23 09:23, Siddhesh Poyarekar wrote:
>> On 2023-11-21 13:12, Adhemerval Zanella Netto wrote:
>>>>> -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:
>>
>> Sure, but the strval here should not be the entire envvar, just the value portion of it.  Granted that _dl_strtoul will bail out on the first non-numeric character, so maybe it's not
> 
> For the GLIBC_TUNABLES itself, parse_tunables will pass the correct length
> for each tunable.  For the alias case, get_next_env will pass the value
> length (which I fixed on v4 btw). So I think there is no need to copy the
> value on a temporary value, parsing can be done in-place since kernel ensures
> the string will be null-terminated.

Yes, but the length is not passed into _dl_strtoul, which will then 
parse until it finds a non-numeric character.  That's probably not an 
issue for a properly validated tunable string (and your patch to bail 
out in case of invalid tunable strings ensures that), but at the minimum 
there should be a comment stating that assumption for future you, or me.

Thanks,
Sid

On 22/11/23 10:24, Siddhesh Poyarekar wrote:
> On 2023-11-22 08:03, Adhemerval Zanella Netto wrote:
>>
>>
>> On 22/11/23 09:23, Siddhesh Poyarekar wrote:
>>> On 2023-11-21 13:12, Adhemerval Zanella Netto wrote:
>>>>>> -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:
>>>
>>> Sure, but the strval here should not be the entire envvar, just the value portion of it.  Granted that _dl_strtoul will bail out on the first non-numeric character, so maybe it's not
>>
>> For the GLIBC_TUNABLES itself, parse_tunables will pass the correct length
>> for each tunable.  For the alias case, get_next_env will pass the value
>> length (which I fixed on v4 btw). So I think there is no need to copy the
>> value on a temporary value, parsing can be done in-place since kernel ensures
>> the string will be null-terminated.
> 
> Yes, but the length is not passed into _dl_strtoul, which will then parse until it finds a non-numeric character.  That's probably not an issue for a properly validated tunable string (and your patch to bail out in case of invalid tunable strings ensures that), but at the minimum there should be a comment stating that assumption for future you, or me.
Right, so maybe it would be better to add a check for 'endptr' and bail 
out if the value is not a number.  And maybe it would be also better to
extend _dl_strtoul to return if the value is out-of-bounds.  I will add
an extra patch on set for this.