Replace __bzero with memset
Commit Message
Replace all internal uses of __bzero with memset. This removes the need
to redirect it to a builtin and means memset is inlined whenever possible,
including with -Os.
ChangeLog:
2017-06-12 Wilco Dijkstra <wdijkstr@arm.com>
* string/bits/string2.h (__bzero): Remove define.
* sunrpc/bindrsvprt.c (bindresvport): Change __bzero to memset.
* sunrpc/clnt_gen.c (clnt_create): Likewise.
* sunrpc/des_impl.c (_des_crypt): Likewise.
* sunrpc/key_call.c (key_gendes): Likewise.
* sunrpc/pmap_rmt.c (clnt_broadcast): Likewise.
* sunrpc/svc_simple.c (universal): Likewise.
* sunrpc/svc_tcp.c (svctcp_create): Likewise.
* sunrpc/svc_udp.c (svcudp_bufcreate): Likewise.
* sysdeps/arm/aeabi_memclr.c (__aeabi_memclr): Likewise.
--
Comments
On Jun 12 2017, Wilco Dijkstra <Wilco.Dijkstra@arm.com> wrote:
> diff --git a/string/bits/string2.h b/string/bits/string2.h
> index 591533658dcef1dbde3d0579c795b72b8ef72f43..c3fc3eae592ecef5d2a0fdd94c0d65cd1407c9d6 100644
> --- a/string/bits/string2.h
> +++ b/string/bits/string2.h
> @@ -52,9 +52,6 @@
> #define __string2_1bptr_p(__x) \
> ((size_t)(const void *)((__x) + 1) - (size_t)(const void *)(__x) == 1)
>
> -/* Set N bytes of S to 0. */
> -#define __bzero(s, n) __builtin_memset (s, '\0', n)
> -
> /* Copy SRC to DEST, returning pointer to final NUL byte. */
> #ifndef __stpcpy
> # define __stpcpy(dest, src) __builtin_stpcpy (dest, src)
Does this depend on some other patch?
Andreas.
On 12/06/2017 08:42, Andreas Schwab wrote:
> On Jun 12 2017, Wilco Dijkstra <Wilco.Dijkstra@arm.com> wrote:
>
>> diff --git a/string/bits/string2.h b/string/bits/string2.h
>> index 591533658dcef1dbde3d0579c795b72b8ef72f43..c3fc3eae592ecef5d2a0fdd94c0d65cd1407c9d6 100644
>> --- a/string/bits/string2.h
>> +++ b/string/bits/string2.h
>> @@ -52,9 +52,6 @@
>> #define __string2_1bptr_p(__x) \
>> ((size_t)(const void *)((__x) + 1) - (size_t)(const void *)(__x) == 1)
>>
>> -/* Set N bytes of S to 0. */
>> -#define __bzero(s, n) __builtin_memset (s, '\0', n)
>> -
>> /* Copy SRC to DEST, returning pointer to final NUL byte. */
>> #ifndef __stpcpy
>> # define __stpcpy(dest, src) __builtin_stpcpy (dest, src)
>
> Does this depend on some other patch?
>
> Andreas.
>
It would be good also if you could update this based on your own patch [1],
which was already acked by me an Zack, since they seems to overlap.
[1] https://sourceware.org/ml/libc-alpha/2017-06/msg00113.html
Andreas Schwab wrote:
> -/* Set N bytes of S to 0. */
> -#define __bzero(s, n) __builtin_memset (s, '\0', n)
> -
> /* Copy SRC to DEST, returning pointer to final NUL byte. */
> #ifndef __stpcpy
> # define __stpcpy(dest, src) __builtin_stpcpy (dest, src)
> Does this depend on some other patch?
No, it's independent, but the patch tool often has difficulty if there are
unrelated changes nearby. There should not be a merge issue if the only
change is removal of lines which themselves match unchanged...
Wilco
On Jun 12 2017, Wilco Dijkstra <Wilco.Dijkstra@arm.com> wrote:
> Andreas Schwab wrote:
>> -/* Set N bytes of S to 0. */
>> -#define __bzero(s, n) __builtin_memset (s, '\0', n)
>> -
>> /* Copy SRC to DEST, returning pointer to final NUL byte. */
>> #ifndef __stpcpy
>> # define __stpcpy(dest, src) __builtin_stpcpy (dest, src)
>
>> Does this depend on some other patch?
>
> No, it's independent, but the patch tool often has difficulty if there are
> unrelated changes nearby. There should not be a merge issue if the only
> change is removal of lines which themselves match unchanged...
It doesn't match.
Andreas.
On Mon, Jun 12, 2017 at 6:45 AM, Wilco Dijkstra <Wilco.Dijkstra@arm.com> wrote:
> Replace all internal uses of __bzero with memset. This removes the need
> to redirect it to a builtin and means memset is inlined whenever possible,
> including with -Os.
+1 from me.
> * string/bits/string2.h (__bzero): Remove define.
You've already got a patch outstanding (and approved, I think) that
removes this header entirely, and people seem to be having problems
with this piece, so I suggest you scrap this bit for now ...
> * sunrpc/bindrsvprt.c (bindresvport): Change __bzero to memset.
> * sunrpc/clnt_gen.c (clnt_create): Likewise.
> * sunrpc/des_impl.c (_des_crypt): Likewise.
> * sunrpc/key_call.c (key_gendes): Likewise.
> * sunrpc/pmap_rmt.c (clnt_broadcast): Likewise.
> * sunrpc/svc_simple.c (universal): Likewise.
> * sunrpc/svc_tcp.c (svctcp_create): Likewise.
> * sunrpc/svc_udp.c (svcudp_bufcreate): Likewise.
> * sysdeps/arm/aeabi_memclr.c (__aeabi_memclr): Likewise.
... and just go ahead and commit these, then rev the removal of
bits/string2.h and maybe land that too?
> +/* Set memory like memset, but different argument order and no return
> + value required. Also only integer caller-saves may be used. */
> void
> __aeabi_memclr (void *dest, size_t n)
> {
> - __bzero (dest, n);
> + memset (dest, 0, n);
> }
It's a pre-existing condition, so it shouldn't hold up your patch, but
this comment concerns me; there is no guarantee that memset will avoid
using FP or vector registers. The existing arm/memset.S *doesn't*,
but it doesn't look like it's all that fine-tuned (compare
arm/armv7/multiarch/memcpy_impl.S).
zw
On Mon, 12 Jun 2017, Zack Weinberg wrote:
> > +/* Set memory like memset, but different argument order and no return
> > + value required. Also only integer caller-saves may be used. */
> > void
> > __aeabi_memclr (void *dest, size_t n)
> > {
> > - __bzero (dest, n);
> > + memset (dest, 0, n);
> > }
>
> It's a pre-existing condition, so it shouldn't hold up your patch, but
> this comment concerns me; there is no guarantee that memset will avoid
> using FP or vector registers. The existing arm/memset.S *doesn't*,
> but it doesn't look like it's all that fine-tuned (compare
> arm/armv7/multiarch/memcpy_impl.S).
__aeabi_memcpy* are (in the multiarch case) aliased to __memcpy_arm to
avoid such issues. (There is still an issue for the dynamic linker, see
bug 15792. Even with the dynamic linker built to use only core registers
there might be issues with e.g. interposed malloc called from the dynamic
linker, unless all relevant registers are saved and restored.)
The string2.h removal breaks build for 32-bit x86.
In file included from ../string/string-inlines.c:32:0,
from ../sysdeps/i386/string-inlines.c:21:
../sysdeps/x86/bits/string.h:678:0: error: "__stpcpy" redefined [-Werror]
# define __stpcpy(dest, src) \
In file included from ../string/string-inlines.c:27:0,
from ../sysdeps/i386/string-inlines.c:21:
../include/string.h:167:0: note: this is the location of the previous definition
# define __stpcpy(dest, src) __builtin_stpcpy (dest, src)
cc1: all warnings being treated as errors
Also, Wilco, please use git-style commit messages which start with a
single line with a short summary of the change, then a blank line, then
any further details (as in the mailing list message proposing a patch).
"2017-06-12 Wilco Dijkstra <wdijkstr@arm.com>" is not a good patch
summary.
On Mon, Jun 12, 2017 at 1:51 PM, Joseph Myers <joseph@codesourcery.com> wrote:
> The string2.h removal breaks build for 32-bit x86.
>
> In file included from ../string/string-inlines.c:32:0,
> from ../sysdeps/i386/string-inlines.c:21:
> ../sysdeps/x86/bits/string.h:678:0: error: "__stpcpy" redefined [-Werror]
> # define __stpcpy(dest, src) \
>
> In file included from ../string/string-inlines.c:27:0,
> from ../sysdeps/i386/string-inlines.c:21:
> ../include/string.h:167:0: note: this is the location of the previous definition
> # define __stpcpy(dest, src) __builtin_stpcpy (dest, src)
>
> cc1: all warnings being treated as errors
This should get fixed, but FYI, in about two hours I'm going to
propose a patch to zap bits/string.h. :)
zw
Zack Weinberg wrote:
On Mon, Jun 12, 2017 at 1:51 PM, Joseph Myers <joseph@codesourcery.com> wrote:
> The string2.h removal breaks build for 32-bit x86.
>
> In file included from ../string/string-inlines.c:32:0,
> from ../sysdeps/i386/string-inlines.c:21:
> ../sysdeps/x86/bits/string.h:678:0: error: "__stpcpy" redefined [-Werror]
> # define __stpcpy(dest, src) \
>
> In file included from ../string/string-inlines.c:27:0,
> from ../sysdeps/i386/string-inlines.c:21:
> ../include/string.h:167:0: note: this is the location of the previous definition
> # define __stpcpy(dest, src) __builtin_stpcpy (dest, src)
>
> cc1: all warnings being treated as errors
Oops, it seems the include order is different in string-inlines.c.
> This should get fixed, but FYI, in about two hours I'm going to
> propose a patch to zap bits/string.h. :)
I think adding a #undef next to the existing #undefs in string-inlines.c should do it.
Wilco
Wilco Dijkstra wrote:
> I think adding a #undef next to the existing #undefs in string-inlines.c should do it.
I have committed this as a trivial fix, x86 now builds:
Add an undef of __stpcpy in string-inlines.c to avoid a redefinition
error on x86.
2017-06-12 Wilco Dijkstra <wdijkstr@arm.com>
* string/string-inlines.c: Add undef of __stpcpy to fix build issue.
Wilco
@@ -52,9 +52,6 @@
#define __string2_1bptr_p(__x) \
((size_t)(const void *)((__x) + 1) - (size_t)(const void *)(__x) == 1)
-/* Set N bytes of S to 0. */
-#define __bzero(s, n) __builtin_memset (s, '\0', n)
-
/* Copy SRC to DEST, returning pointer to final NUL byte. */
#ifndef __stpcpy
# define __stpcpy(dest, src) __builtin_stpcpy (dest, src)
@@ -61,7 +61,7 @@ bindresvport (int sd, struct sockaddr_in *sin)
if (sin == (struct sockaddr_in *) 0)
{
sin = &myaddr;
- __bzero (sin, sizeof (*sin));
+ memset (sin, 0, sizeof (*sin));
sin->sin_family = AF_INET;
}
else if (sin->sin_family != AF_INET)
@@ -56,7 +56,7 @@ clnt_create (const char *hostname, u_long prog, u_long vers,
if (strcmp (proto, "unix") == 0)
{
- __bzero ((char *)&sun, sizeof (sun));
+ memset ((char *)&sun, 0, sizeof (sun));
sun.sun_family = AF_UNIX;
strcpy (sun.sun_path, hostname);
sock = RPC_ANYSOCK;
@@ -590,7 +590,7 @@ _des_crypt (char *buf, unsigned len, struct desparams *desp)
}
tin0 = tin1 = tout0 = tout1 = xor0 = xor1 = 0;
tbuf[0] = tbuf[1] = 0;
- __bzero (schedule, sizeof (schedule));
+ memset (schedule, 0, sizeof (schedule));
return (1);
}
@@ -218,7 +218,7 @@ key_gendes (des_block *key)
sin.sin_family = AF_INET;
sin.sin_port = 0;
sin.sin_addr.s_addr = htonl (INADDR_LOOPBACK);
- __bzero (sin.sin_zero, sizeof (sin.sin_zero));
+ memset (sin.sin_zero, 0, sizeof (sin.sin_zero));
socket = RPC_ANYSOCK;
client = clntudp_bufcreate (&sin, (u_long) KEY_PROG, (u_long) KEY_VERS,
trytimeout, &socket, RPCSMALLMSGSIZE,
@@ -256,7 +256,7 @@ clnt_broadcast (/* program number */
fd.fd = sock;
fd.events = POLLIN;
nets = getbroadcastnets (addrs, sizeof (addrs) / sizeof (addrs[0]));
- __bzero ((char *) &baddr, sizeof (baddr));
+ memset ((char *) &baddr, 0, sizeof (baddr));
baddr.sin_family = AF_INET;
baddr.sin_port = htons (PMAPPORT);
baddr.sin_addr.s_addr = htonl (INADDR_ANY);
@@ -154,7 +154,7 @@ universal (struct svc_req *rqstp, SVCXPRT *transp_l)
if (pl->p_prognum == prog && pl->p_procnum == proc)
{
/* decode arguments into a CLEAN buffer */
- __bzero (xdrbuf, sizeof (xdrbuf)); /* required ! */
+ memset (xdrbuf, 0, sizeof (xdrbuf)); /* required ! */
if (!svc_getargs (transp_l, pl->p_inproc, xdrbuf))
{
svcerr_decode (transp_l);
@@ -166,7 +166,7 @@ svctcp_create (int sock, u_int sendsize, u_int recvsize)
}
madesock = TRUE;
}
- __bzero ((char *) &addr, sizeof (addr));
+ memset ((char *) &addr, 0, sizeof (addr));
addr.sin_family = AF_INET;
if (bindresvport (sock, &addr))
{
@@ -137,7 +137,7 @@ svcudp_bufcreate (int sock, u_int sendsz, u_int recvsz)
}
madesock = TRUE;
}
- __bzero ((char *) &addr, sizeof (addr));
+ memset ((char *) &addr, 0, sizeof (addr));
addr.sin_family = AF_INET;
if (bindresvport (sock, &addr))
{
@@ -17,12 +17,12 @@
#include <string.h>
-/* Clear memory. Can't alias to bzero because it's not defined in the
- same translation unit. */
+/* Set memory like memset, but different argument order and no return
+ value required. Also only integer caller-saves may be used. */
void
__aeabi_memclr (void *dest, size_t n)
{
- __bzero (dest, n);
+ memset (dest, 0, n);
}
/* Versions of the above which may assume memory alignment. */