Replace rawmemchr (s, '\0') with strchr

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?

---

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);
>  }

On 2/3/23 03: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.

While the compiler does lower the strchr to strlen, why not use that replacement directly?


r~

Hi Richard,

> While the compiler does lower the strchr to strlen, why not use that replacement directly?

I'd prefer that and be explicit about what function you're using but like with mempcpy
(which also gets expanded into memcpy+size) there are always people arguing the
opposite... Since we taught the compiler to do the right thing in most of these cases,
it doesn't really matter as it's now syntactic sugar.

Cheers,
Wilco