Re: [PATCH] Replace rawmemchr (s, '\0') with strchr

[Adhemerval Zanella Netto <adhemerval.zanella@linaro.org>]

On 03/02/23 10:07, Wilco Dijkstra via Libc-alpha wrote:
> Almost all uses of rawmemchr find the end of a string.  Since most targets use a
> generic implementation, replacing it with strchr is better since that is optimized
> by compilers into strlen (s) + s.  Also fix generic rawmemchr implementation to use
> a cast to unsigned char on the char input.
> 
> Passes regress, OK for commit?

LGTM, only a small suggestion below.

Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>

> 
> ---
> 
> diff --git a/benchtests/bench-rawmemchr.c b/benchtests/bench-rawmemchr.c
> index f6398f86d9e5b1c95e2ec55b022a2913bfc51a46..088c516e59383f93e6cbcbe89825a866bebef518 100644
> --- a/benchtests/bench-rawmemchr.c
> +++ b/benchtests/bench-rawmemchr.c
> @@ -30,7 +30,7 @@ typedef char *(*proto_t) (const char *, int);
>  char *
>  generic_rawmemchr (const char *s, int c)
>  {
> -  if (c != 0)
> +  if ((unsigned char) c != 0)
>      return memchr (s, c, PTRDIFF_MAX);
>    return (char *)s + strlen (s);
>  }
> diff --git a/benchtests/bench-strtok.c b/benchtests/bench-strtok.c
> index 10260e9b47b5c6880eb2872a1d796e88dd6fddf4..711bdaab58ff297855b03d62057cefd066f4e1c5 100644
> --- a/benchtests/bench-strtok.c
> +++ b/benchtests/bench-strtok.c
> @@ -42,7 +42,7 @@ oldstrtok (char *s, const char *delim)
>    s = strpbrk (token, delim);
>    if (s == NULL)
>      /* This token finishes the string.  */
> -    olds = rawmemchr (token, '\0');
> +    olds = strchr (token, '\0');
>    else
>      {
>        /* Terminate the token and make OLDS point past it.  */
> diff --git a/elf/dl-support.c b/elf/dl-support.c
> index 9714f75db096fa6a946b95286733dc4a7ffe7aab..b1b6a1aa4caa4bfabcb8acbb5a671707282bb1b0 100644
> --- a/elf/dl-support.c
> +++ b/elf/dl-support.c
> @@ -326,7 +326,7 @@ _dl_non_dynamic_init (void)
>        while (cp < unsecure_envvars + sizeof (unsecure_envvars))
>  	{
>  	  __unsetenv (cp);
> -	  cp = (const char *) __rawmemchr (cp, '\0') + 1;
> +	  cp = (const char *) strchr (cp, '\0') + 1;

I think you can remove the spurious cast here.

>  	}
>  
>  #if !HAVE_TUNABLES
> diff --git a/elf/ldconfig.c b/elf/ldconfig.c
> index 166dccb5280cd8ef6bbcf98b4a77e24f7046bfe0..3f1b30c570263543701dfaee9f8e430077888670 100644
> --- a/elf/ldconfig.c
> +++ b/elf/ldconfig.c
> @@ -1201,7 +1201,7 @@ main (int argc, char **argv)
>    if (opt_chroot != NULL)
>      {
>        /* Normalize the path a bit, we might need it for printing later.  */
> -      char *endp = rawmemchr (opt_chroot, '\0');
> +      char *endp = strchr (opt_chroot, '\0');
>        while (endp > opt_chroot && endp[-1] == '/')
>  	--endp;
>        *endp = '\0';
> diff --git a/elf/rtld.c b/elf/rtld.c
> index b8467f37cf514e6dfd176bae18d61b49e4287143..93f333ec2a4aa31e9675ecda62f9590fe383b5bc 100644
> --- a/elf/rtld.c
> +++ b/elf/rtld.c
> @@ -1024,7 +1024,7 @@ ERROR: audit interface '%s' requires version %d (maximum supported version %d);
>  	newp->fptr[cnt] = NULL;
>        ++cnt;
>  
> -      cp = rawmemchr (cp, '\0') + 1;
> +      cp = strchr (cp, '\0') + 1;
>      }
>    while (*cp != '\0');
>    assert (cnt == naudit_ifaces);
> diff --git a/iconv/gconv_conf.c b/iconv/gconv_conf.c
> index 21165a558a17ae26b968b26b9e340fa1c4ab88d0..a75ac13e3f26a8a110b1f577666931a0a22a2e7b 100644
> --- a/iconv/gconv_conf.c
> +++ b/iconv/gconv_conf.c
> @@ -502,8 +502,8 @@ __gconv_read_conf (void)
>    do
>      {
>        const char *from = cp;
> -      const char *to = __rawmemchr (from, '\0') + 1;
> -      cp = __rawmemchr (to, '\0') + 1;
> +      const char *to = strchr (from, '\0') + 1;
> +      cp = strchr (to, '\0') + 1;
>  
>        add_alias2 (from, to, cp);
>      }
> diff --git a/iconvdata/iso646.c b/iconvdata/iso646.c
> index e044ed2cba806971612277ac2c835494a185dbe4..f7111a37591154eaacfe792845f9e1e8b7e29b31 100644
> --- a/iconvdata/iso646.c
> +++ b/iconvdata/iso646.c
> @@ -133,7 +133,7 @@ gconv_init (struct __gconv_step *step)
>  
>    enum variant var = 0;
>    for (const char *name = names; *name != '\0';
> -       name = __rawmemchr (name, '\0') + 1)
> +       name = strchr (name, '\0') + 1)
>      {
>        if (__strcasecmp (step->__from_name, name) == 0)
>  	{
> diff --git a/iconvdata/utf-7.c b/iconvdata/utf-7.c
> index 198ba99c2b0880a02eb64a5a85a999428365b5bf..babeb56f4e4fc4e073d5fbbe0ff8b5f6bd4dda07 100644
> --- a/iconvdata/utf-7.c
> +++ b/iconvdata/utf-7.c
> @@ -189,7 +189,7 @@ gconv_init (struct __gconv_step *step)
>  
>    enum variant var = 0;
>    for (const char *name = names; *name != '\0';
> -       name = __rawmemchr (name, '\0') + 1)
> +       name = strchr (name, '\0') + 1)
>      {
>        if (__strcasecmp (step->__from_name, name) == 0)
>  	{
> diff --git a/inet/getnetgrent_r.c b/inet/getnetgrent_r.c
> index 833fe21b9184ffccdc95e5f2b57abbc39f1ddf95..60f476b661fab64cf0db8a92f302401fb02d7fae 100644
> --- a/inet/getnetgrent_r.c
> +++ b/inet/getnetgrent_r.c
> @@ -217,11 +217,11 @@ nscd_getnetgrent (struct __netgrent *datap, char *buffer, size_t buflen,
>  
>    datap->type = triple_val;
>    datap->val.triple.host = get_nonempty_val (datap->cursor);
> -  datap->cursor = (char *) __rawmemchr (datap->cursor, '\0') + 1;
> +  datap->cursor = strchr (datap->cursor, '\0') + 1;
>    datap->val.triple.user = get_nonempty_val (datap->cursor);
> -  datap->cursor = (char *) __rawmemchr (datap->cursor, '\0') + 1;
> +  datap->cursor = strchr (datap->cursor, '\0') + 1;
>    datap->val.triple.domain = get_nonempty_val (datap->cursor);
> -  datap->cursor = (char *) __rawmemchr (datap->cursor, '\0') + 1;
> +  datap->cursor = strchr (datap->cursor, '\0') + 1;
>  
>    return NSS_STATUS_SUCCESS;
>  }
> diff --git a/intl/dcigettext.c b/intl/dcigettext.c
> index d0c62eec7aa0d09f917be97aa483e6780c149856..64de9d511ad03d627d45db45ebcb9bdcd74a0a34 100644
> --- a/intl/dcigettext.c
> +++ b/intl/dcigettext.c
> @@ -1420,11 +1420,7 @@ plural_lookup (struct loaded_l10nfile *domain, unsigned long int n,
>    p = translation;
>    while (index-- > 0)
>      {
> -#ifdef _LIBC
> -      p = __rawmemchr (p, '\0');
> -#else
>        p = strchr (p, '\0');
> -#endif
>        /* And skip over the NUL byte.  */
>        p++;
>  
> diff --git a/io/ftw.c b/io/ftw.c
> index 4d49fc7b1030ce00f05667b4e8e6f3f603cf8482..a72c7d517140d9ba70518678aa4397fa3053d585 100644
> --- a/io/ftw.c
> +++ b/io/ftw.c
> @@ -535,7 +535,7 @@ fail:
>  
>    /* Next, update the `struct FTW' information.  */
>    ++data->ftw.level;
> -  startp = __rawmemchr (data->dirbuf, '\0');
> +  startp = strchr (data->dirbuf, '\0');
>    /* There always must be a directory name.  */
>    assert (startp != data->dirbuf);
>    if (startp[-1] != '/')
> diff --git a/libio/strops.c b/libio/strops.c
> index 7e523ad7101aa88d1e56d3b8e84092ed31ed572c..b5c850b626a5a367741939a93c2d750aede58b1f 100644
> --- a/libio/strops.c
> +++ b/libio/strops.c
> @@ -38,7 +38,7 @@ _IO_str_init_static_internal (_IO_strfile *sf, char *ptr, size_t size,
>    char *end;
>  
>    if (size == 0)
> -    end = __rawmemchr (ptr, '\0');
> +    end = strchr (ptr, '\0');
>    else if ((size_t) ptr + size > (size_t) ptr)
>      end = ptr + size;
>    else
> diff --git a/manual/string.texi b/manual/string.texi
> index 7578aa1c265694b6861fd207e1bcddd57ff9b9da..e06433187eb1be512314b7cec2ce7c99abd90198 100644
> --- a/manual/string.texi
> +++ b/manual/string.texi
> @@ -1726,15 +1726,7 @@ made an error in assuming that the byte @var{c} is present in the block.
>  In this case the result is unspecified.  Otherwise the return value is a
>  pointer to the located byte.
>  
> -This function is of special interest when looking for the end of a
> -string.  Since all strings are terminated by a null byte a call like
> -
> -@smallexample
> -   rawmemchr (str, '\0')
> -@end smallexample
> -
> -@noindent
> -will never go beyond the end of the string.
> +When looking for the end of a string, use @code{strchr}.
>  
>  This function is a GNU extension.
>  @end deftypefun
> diff --git a/nis/nis_addmember.c b/nis/nis_addmember.c
> index f7d7f166070508bba09bd8ef98383929d485ba7f..89e7affee1abcdc1f00039ef84b2585670b6200e 100644
> --- a/nis/nis_addmember.c
> +++ b/nis/nis_addmember.c
> @@ -32,7 +32,7 @@ nis_addmember (const_nis_name member, const_nis_name group)
>        nis_error status;
>        char *cp, *cp2;
>  
> -      cp = rawmemchr (nis_leaf_of_r (group, buf, sizeof (buf) - 1), '\0');
> +      cp = strchr (nis_leaf_of_r (group, buf, sizeof (buf) - 1), '\0');
>        cp = stpcpy (cp, ".groups_dir");
>        cp2 = nis_domain_of_r (group, domainbuf, sizeof (domainbuf) - 1);
>        if (cp2 != NULL && cp2[0] != '\0')
> diff --git a/nis/nis_call.c b/nis/nis_call.c
> index ec19b21c5303a1a7d97a9059f24b627b0d83366d..75f2129c76bbaabcd7b72be50d75115a8c52859f 100644
> --- a/nis/nis_call.c
> +++ b/nis/nis_call.c
> @@ -483,7 +483,7 @@ rec_dirsearch (const_nis_name name, directory_obj *dir, nis_error *status)
>  	  }
>  	while (nis_dir_cmp (domain, dir->do_name) != SAME_NAME);
>  
> -	cp = rawmemchr (leaf, '\0');
> +	cp = strchr (leaf, '\0');
>  	*cp++ = '.';
>  	strcpy (cp, domain);
>  
> @@ -614,7 +614,7 @@ nis_server_cache_search (const_nis_name name, int search_parent,
>  	if (ret == NULL)
>  	  break;
>  
> -	addr = rawmemchr (nis_server_cache[i]->name, '\0') + 8;
> +	addr = strchr (nis_server_cache[i]->name, '\0') + 8;
>  	addr = (char *) ((uintptr_t) addr & ~(uintptr_t) 7);
>  	xdrmem_create (&xdrs, addr, nis_server_cache[i]->size, XDR_DECODE);
>  	if (!_xdr_directory_obj (&xdrs, ret))
> diff --git a/nis/nis_local_names.c b/nis/nis_local_names.c
> index 8fbe9ed80a358c64817c02f2364f6f4df220c8db..e68525530092ac2193d22162808eeda6511ac702 100644
> --- a/nis/nis_local_names.c
> +++ b/nis/nis_local_names.c
> @@ -63,7 +63,7 @@ nis_local_directory (void)
>  	__nisdomainname[0] = '\0';
>        else
>  	{
> -	  char *cp = rawmemchr (__nisdomainname, '\0');
> +	  char *cp = strchr (__nisdomainname, '\0');
>  
>  	  /* Missing trailing dot? */
>  	  if (cp[-1] != '.')
> @@ -154,7 +154,7 @@ nis_local_host (void)
>  	__nishostname[0] = '\0';
>        else
>  	{
> -	  char *cp = rawmemchr (__nishostname, '\0');
> +	  char *cp = strchr (__nishostname, '\0');
>  	  int len = cp - __nishostname;
>  
>  	  /* Hostname already fully qualified? */
> diff --git a/nis/nis_removemember.c b/nis/nis_removemember.c
> index 77702db30c5dc0ea2c7d6b3738ffb8c7e79ede1c..fbf9703b127a95d92487a3bd5706c651d7574099 100644
> --- a/nis/nis_removemember.c
> +++ b/nis/nis_removemember.c
> @@ -32,7 +32,7 @@ nis_removemember (const_nis_name member, const_nis_name group)
>        nis_error status;
>        char *cp, *cp2;
>  
> -      cp = rawmemchr (nis_leaf_of_r (group, buf, sizeof (buf) - 1), '\0');
> +      cp = strchr (nis_leaf_of_r (group, buf, sizeof (buf) - 1), '\0');
>        cp = stpcpy (cp, ".groups_dir");
>        cp2 = nis_domain_of_r (group, domainbuf, sizeof (domainbuf) - 1);
>        if (cp2 != NULL && cp2[0] != '\0')
> diff --git a/nscd/connections.c b/nscd/connections.c
> index b5fbb1a698e51aaa663aeb253e9860d81a383ee2..b1231a96dbe4d39433d9233b296413da07ce8ebb 100644
> --- a/nscd/connections.c
> +++ b/nscd/connections.c
> @@ -1359,7 +1359,7 @@ cannot open /proc/self/cmdline: %m; disabling paranoia mode"));
>    for (char *cp = cmdline; cp < cmdline + readlen;)
>      {
>        argv[argc++] = cp;
> -      cp = (char *) rawmemchr (cp, '\0') + 1;
> +      cp = strchr (cp, '\0') + 1;
>      }
>    argv[argc] = NULL;
>  
> diff --git a/nscd/grpcache.c b/nscd/grpcache.c
> index eb20c4f1abef0fd7215476ae093e3117852aa41a..cdd1071970db65b63031824b0c0970d5900e3dec 100644
> --- a/nscd/grpcache.c
> +++ b/nscd/grpcache.c
> @@ -259,7 +259,7 @@ cache_addgr (struct database_dyn *db, int fd, request_header *req,
>        /* Finally the stringified GID value.  */
>        memcpy (cp, buf, n);
>        char *key_copy = cp + key_offset;
> -      assert (key_copy == (char *) rawmemchr (cp, '\0') + 1);
> +      assert (key_copy == strchr (cp, '\0') + 1);
>  
>        assert (cp == dataset->strdata + total - offsetof (struct dataset,
>  							 strdata));
> diff --git a/nscd/netgroupcache.c b/nscd/netgroupcache.c
> index 439dd4ba38ad610598bfd574f43dd91266ce8dca..7f5258f81303697231883cdf6b8913a95b1d8852 100644
> --- a/nscd/netgroupcache.c
> +++ b/nscd/netgroupcache.c
> @@ -453,14 +453,14 @@ addinnetgrX (struct database_dyn *db, int fd, request_header *req,
>  	     struct datahead *dh)
>  {
>    const char *group = key;
> -  key = (char *) rawmemchr (key, '\0') + 1;
> +  key = strchr (key, '\0') + 1;
>    size_t group_len = key - group;
>    const char *host = *key++ ? key : NULL;
>    if (host != NULL)
> -    key = (char *) rawmemchr (key, '\0') + 1;
> +    key = strchr (key, '\0') + 1;
>    const char *user = *key++ ? key : NULL;
>    if (user != NULL)
> -    key = (char *) rawmemchr (key, '\0') + 1;
> +    key = strchr (key, '\0') + 1;
>    const char *domain = *key++ ? key : NULL;
>  
>    if (__glibc_unlikely (debug_level > 0))
> @@ -538,11 +538,11 @@ addinnetgrX (struct database_dyn *db, int fd, request_header *req,
>  	     match anything is stored in the netgroup cache.  */
>  	  if (host != NULL && *triplets != '\0')
>  	    success = strcmp (host, triplets) == 0;
> -	  triplets = (const char *) rawmemchr (triplets, '\0') + 1;
> +	  triplets = (const char *) strchr (triplets, '\0') + 1;
>  
>  	  if (success && user != NULL && *triplets != '\0')
>  	    success = strcmp (user, triplets) == 0;
> -	  triplets = (const char *) rawmemchr (triplets, '\0') + 1;
> +	  triplets = (const char *) strchr (triplets, '\0') + 1;
>  
>  	  if (success && (domain == NULL || *triplets == '\0'
>  			  || strcmp (domain, triplets) == 0))
> @@ -550,7 +550,7 @@ addinnetgrX (struct database_dyn *db, int fd, request_header *req,
>  	      dataset->resp.result = 1;
>  	      break;
>  	    }
> -	  triplets = (const char *) rawmemchr (triplets, '\0') + 1;
> +	  triplets = (const char *) strchr (triplets, '\0') + 1;
>  	}
>      }
>  
> diff --git a/nscd/pwdcache.c b/nscd/pwdcache.c
> index f546bb386ff7d56f614366c8aeca233e5b43930d..e1b579de6b1b5e5a2ef8574b39c6becf6d4fb285 100644
> --- a/nscd/pwdcache.c
> +++ b/nscd/pwdcache.c
> @@ -243,7 +243,7 @@ cache_addpw (struct database_dyn *db, int fd, request_header *req,
>        /* Finally the stringified UID value.  */
>        memcpy (cp, buf, n);
>        char *key_copy = cp + key_offset;
> -      assert (key_copy == (char *) rawmemchr (cp, '\0') + 1);
> +      assert (key_copy == strchr (cp, '\0') + 1);
>  
>        assert (cp == dataset->strdata + total - offsetof (struct dataset,
>  							 strdata));
> diff --git a/nss/nss_db/db-XXX.c b/nss/nss_db/db-XXX.c
> index a81bd7466bff75e3422b7502a1d5e365232476e7..64832e184388799117e57fc0a44e5295bf58dd00 100644
> --- a/nss/nss_db/db-XXX.c
> +++ b/nss/nss_db/db-XXX.c
> @@ -269,7 +269,7 @@ CONCAT(_nss_db_get,ENTNAME_r) (struct STRUCTURE *result, char *buffer,
>  			       + state.header->valstrlen);
>        while (entidx < end)
>  	{
> -	  const char *next = rawmemchr (entidx, '\0') + 1;
> +	  const char *next = (const char*) strchr (entidx, '\0') + 1;
>  	  size_t len = next - entidx;
>  
>  	  if (len > buflen)
> diff --git a/stdlib/canonicalize.c b/stdlib/canonicalize.c
> index 78beeef06390c897a4929f396dd1fe0ab5764edf..7e421ef11559049e8257f78cbc3e85474fd62553 100644
> --- a/stdlib/canonicalize.c
> +++ b/stdlib/canonicalize.c
> @@ -74,7 +74,6 @@
>  # endif
>  # define __mempcpy mempcpy
>  # define __pathconf pathconf
> -# define __rawmemchr rawmemchr
>  # define __readlink readlink
>  # define __stat stat
>  #endif
> @@ -232,7 +231,7 @@ realpath_stk (const char *name, char *resolved, struct realpath_bufs *bufs)
>  	    return NULL;
>            rname = bufs->rname.data;
>          }
> -      dest = __rawmemchr (rname, '\0');
> +      dest = strchr (rname, '\0');
>        start = name;
>        prefix_len = FILE_SYSTEM_PREFIX_LEN (rname);
>      }
> diff --git a/string/rawmemchr.c b/string/rawmemchr.c
> index b44ad79859e7450d52885242043d7a225239c01a..aa8f8f233665d8a912c8acf5711e0ad801bf127e 100644
> --- a/string/rawmemchr.c
> +++ b/string/rawmemchr.c
> @@ -39,7 +39,7 @@ DIAG_IGNORE_NEEDS_COMMENT (11, "-Wstringop-overread");
>  void *
>  RAWMEMCHR (const void *s, int c)
>  {
> -  if (c != '\0')
> +  if ((unsigned char) c != '\0')
>      return memchr (s, c, (size_t)-1);
>    return (char *)s + strlen (s);
>  }