[PATCH v2] prefer memcpy() over strcpy()

[PATCH v2] prefer memcpy() over strcpy()

[James Tirta Halim]

Add zmemcpyz(), a memcpy() that nul-terminates the destination string.
This is meant to be used when we have the strlen() of the string.

We should prefer memcpy() when we know the length because it most libc
implementations provide an assembly implementation of memcpy but maybe
not strcpy(). Even if it is implemented in assembly, memcpy() is likely
to be faster than strcpy() since as the loop condition strcpy() needs
to check for zeros in SRC, whereas memcpy() can just decrement the size.

---
 Src/string.c | 56 +++++++++++++++++++++++++++++-----------------------
 Src/zsh.h    | 10 ++++++++++
 2 files changed, 41 insertions(+), 25 deletions(-)

diff --git a/Src/string.c b/Src/string.c
index 5f439926e..12da19f72 100644
--- a/Src/string.c
+++ b/Src/string.c
@@ -33,11 +33,13 @@ mod_export char *
 dupstring(const char *s)
 {
     char *t;
+    size_t l;
 
     if (!s)
 	return NULL;
-    t = (char *) zhalloc(strlen((char *)s) + 1);
-    strcpy(t, s);
+    l = strlen((char *)s);
+    t = (char *) zhalloc(l + 1);
+    zmemcpyz(t, s, l);
     return t;
 }
 
@@ -68,7 +70,7 @@ dupstring_glen(const char *s, unsigned *len_ret)
     if (!s)
 	return NULL;
     t = (char *) zhalloc((*len_ret = strlen((char *)s)) + 1);
-    strcpy(t, s);
+    zmemcpyz(t, s, *len_ret);
     return t;
 }
 
@@ -77,11 +79,13 @@ mod_export char *
 ztrdup(const char *s)
 {
     char *t;
+    size_t l;
 
     if (!s)
 	return NULL;
-    t = (char *)zalloc(strlen((char *)s) + 1);
-    strcpy(t, s);
+    l = strlen((char *)s);
+    t = (char *)zalloc(l + 1);
+    zmemcpyz(t, s, l);
     return t;
 }
 
@@ -116,11 +120,12 @@ tricat(char const *s1, char const *s2, char const *s3)
     char *ptr;
     size_t l1 = strlen(s1);
     size_t l2 = strlen(s2);
+    size_t l3 = strlen(s3);
 
-    ptr = (char *)zalloc(l1 + l2 + strlen(s3) + 1);
-    strcpy(ptr, s1);
-    strcpy(ptr + l1, s2);
-    strcpy(ptr + l1 + l2, s3);
+    ptr = (char *)zalloc(l1 + l2 + l3 + 1);
+    memcpy(ptr, s1, l1);
+    memcpy(ptr + l1, s2, l2);
+    zmemcpyz(ptr + l1 + l2, s3, l3);
     return ptr;
 }
 
@@ -131,11 +136,12 @@ zhtricat(char const *s1, char const *s2, char const *s3)
     char *ptr;
     size_t l1 = strlen(s1);
     size_t l2 = strlen(s2);
+    size_t l3 = strlen(s3);
 
     ptr = (char *)zhalloc(l1 + l2 + strlen(s3) + 1);
-    strcpy(ptr, s1);
-    strcpy(ptr + l1, s2);
-    strcpy(ptr + l1 + l2, s3);
+    memcpy(ptr, s1, l1);
+    memcpy(ptr + l1, s2, l2);
+    zmemcpyz(ptr + l1 + l2, s3, l3);
     return ptr;
 }
 
@@ -148,10 +154,11 @@ dyncat(const char *s1, const char *s2)
     /* This version always uses space from the current heap. */
     char *ptr;
     size_t l1 = strlen(s1);
+    size_t l2 = strlen(s2);
 
-    ptr = (char *)zhalloc(l1 + strlen(s2) + 1);
-    strcpy(ptr, s1);
-    strcpy(ptr + l1, s2);
+    ptr = (char *)zhalloc(l1 + l2 + 1);
+    memcpy(ptr, s1, l1);
+    zmemcpyz(ptr + l1, s2, l2);
     return ptr;
 }
 
@@ -162,10 +169,11 @@ bicat(const char *s1, const char *s2)
     /* This version always uses permanently-allocated space. */
     char *ptr;
     size_t l1 = strlen(s1);
+    size_t l2 = strlen(s2);
 
-    ptr = (char *)zalloc(l1 + strlen(s2) + 1);
-    strcpy(ptr, s1);
-    strcpy(ptr + l1, s2);
+    ptr = (char *)zalloc(l1 + l2 + 1);
+    memcpy(ptr, s1, l1);
+    zmemcpyz(ptr + l1, s2, l2);
     return ptr;
 }
 
@@ -177,9 +185,7 @@ dupstrpfx(const char *s, int len)
 {
     char *r = zhalloc(len + 1);
 
-    memcpy(r, s, len);
-    r[len] = '\0';
-    return r;
+    return zmemcpyz(r, s, len);
 }
 
 /**/
@@ -189,9 +195,7 @@ ztrduppfx(const char *s, int len)
     /* This version always uses permanently-allocated space. */
     char *r = zalloc(len + 1);
 
-    memcpy(r, s, len);
-    r[len] = '\0';
-    return r;
+    return zmemcpyz(r, s, len);
 }
 
 /* Append a string to an allocated string, reallocating to make room. */
@@ -200,7 +204,9 @@ ztrduppfx(const char *s, int len)
 mod_export char *
 appstr(char *base, char const *append)
 {
-    return strcat(realloc(base, strlen(base) + strlen(append) + 1), append);
+    size_t bl = strlen(base);
+    size_t al = strlen(append);
+    return zmemcpyz((char *)realloc(base, bl + al + 1) + bl, append, al);
 }
 
 /* Return a pointer to the last character of a string,
diff --git a/Src/zsh.h b/Src/zsh.h
index fae62b8d0..b10705356 100644
--- a/Src/zsh.h
+++ b/Src/zsh.h
@@ -3367,3 +3367,13 @@ typedef int convchar_t;
 #define ZWS(s)	s
 
 #endif /* MULTIBYTE_SUPPORT */
+
+#include <stddef.h>
+#include <string.h>
+
+/* memcpy that nul-terminates DST. SRC need not be nul-terminated. */
+static inline char *zmemcpyz(char *dst, const char *src, size_t n)
+{
+    *((char *)memcpy(dst, src, n) + n) = '\0';
+    return dst;
+}
-- 
2.44.0

Re: [PATCH v2] prefer memcpy() over strcpy()

[Oliver Kiddle]

On 18 Mar, James Tirta Halim wrote:
> Add zmemcpyz(), a memcpy() that nul-terminates the destination string.
> This is meant to be used when we have the strlen() of the string.

I was hoping someone else would review this. It touches on areas I'm
unsure of and I needed to check on how static inline differs between C
and C++.

> We should prefer memcpy() when we know the length because it most libc
> implementations provide an assembly implementation of memcpy but maybe
> not strcpy(). Even if it is implemented in assembly, memcpy() is likely
> to be faster than strcpy() since as the loop condition strcpy() needs
> to check for zeros in SRC, whereas memcpy() can just decrement the size.

Have you verified that this does actually have a positive effect on
performance?

If we're bothering to do this, I'd also assume that mempcpy() might also
be a teany tiny bit faster on systems that support it. Autoconf should
make that easy to check for.

> --- a/Src/zsh.h

> +#include <stddef.h>
> +#include <string.h>

Currently there are no #includes in zsh.h so I'd be uneasy about adding
some without being fully sure I understood the reasoning behind the
current setup for prototypes and headers. For now, it is only used in
string.c. It'd be easier to add there only for now and address our
use of inline functions separately. The makepro.awk mechanism largely
predates inline functions so I'm not quite sure how we can arrange for
inline functions to appear in full in string.epro or whatever zsh.mdh
includes. Anyone know?

Oliver

Re: [PATCH v2] prefer memcpy() over strcpy()

[James]

[-- Attachment #1: Type: text/plain, Size: 1973 bytes --]

On Tue, Mar 26, 2024, 7:42 AM Oliver Kiddle <opk@zsh.org> wrote:

> On 18 Mar, James Tirta Halim wrote:
> > Add zmemcpyz(), a memcpy() that nul-terminates the destination string.
> > This is meant to be used when we have the strlen() of the string.
>
> I was hoping someone else would review this. It touches on areas I'm
> unsure of and I needed to check on how static inline differs between C
> and C++.
>
> > We should prefer memcpy() when we know the length because it most libc
> > implementations provide an assembly implementation of memcpy but maybe
> > not strcpy(). Even if it is implemented in assembly, memcpy() is likely
> > to be faster than strcpy() since as the loop condition strcpy() needs
> > to check for zeros in SRC, whereas memcpy() can just decrement the size.
>
> Have you verified that this does actually have a positive effect on
> performance?

Musl doesn't provide an assembly implementation for strcpy, so it should be
as fast as asm memcpy is to C memcpy. Glibc does, so performance difference
is theoretical.

> If we're bothering to do this, I'd also assume that mempcpy() might also
> be a teany tiny bit faster on systems that support it. Autoconf should
> make that easy to check for.
>
Sure.

> > --- a/Src/zsh.h
>
> > +#include <stddef.h>
> > +#include <string.h>
>
> Currently there are no #includes in zsh.h so I'd be uneasy about adding
> some without being fully sure I understood the reasoning behind the
> current setup for prototypes and headers. For now, it is only used in
> string.c. It'd be easier to add there only for now and address our
> use of inline functions separately. The makepro.awk mechanism largely
> predates inline functions so I'm not quite sure how we can arrange for
> inline functions to appear in full in string.epro or whatever zsh.mdh
> includes. Anyone know?
>
I think it would be better to make it a macro for now. The function call
overhead may become a pessimization for small strings.

> Oliver
>

[-- Attachment #2: Type: text/html, Size: 3006 bytes --]

Re: [PATCH v2] prefer memcpy() over strcpy()

[Bart Schaefer]

On Mon, Mar 25, 2024 at 7:18 PM James <tirtajames45@gmail.com> wrote:
>
> I think it would be better to make it a macro for now. The function call overhead may become a pessimization for small strings.

Didn't we go over this (and have a discussion about naming the macro)
a few months ago?

Re: [PATCH v2] prefer memcpy() over strcpy()

[James]

[-- Attachment #1: Type: text/plain, Size: 410 bytes --]

On Tue, Mar 26, 2024, 9:26 AM Bart Schaefer <schaefer@brasslantern.com>
wrote:

> On Mon, Mar 25, 2024 at 7:18 PM James <tirtajames45@gmail.com> wrote:
> >
> > I think it would be better to make it a macro for now. The function call
> overhead may become a pessimization for small strings.
>
> Didn't we go over this (and have a discussion about naming the macro)
> a few months ago?
>
Yes.

>

[-- Attachment #2: Type: text/html, Size: 966 bytes --]