[RFC PATCH v1 0/6] Add dynamic memory allocator support for nolibc

[RFC PATCH v1 0/6] Add dynamic memory allocator support for nolibc

[Ammar Faizi]

Hi,

This is the v1 of RFC to add dynamic memory allocator support for
nolibc.


## Background

The need to allocate memory dynamically has become a requirement for
the C programming language. Mainly it happens when the allocation size
is determined at runtime. Many other use cases also do it when the
object's lifetime is long-lived and needs to be recycled at runtime.

Currently, the nolibc header doesn't support such a type of allocation.
This series adds it.


## Implementation

Add basic functions to manage dynamic memory allocation:
  - malloc()
  - calloc()
  - realloc()
  - free()

The allocator uses mmap() syscall to allocate the memory and uses
munmap() syscall to free the allocated memory.

The metadata to keep track the length for munmap-ing is simply
defined as a struct below:
```
struct nolibc_heap {
        size_t  len;
        char    user_p[] __attribute__((__aligned__));
};
```
malloc(), realloc() and calloc() return a pointer to `user_p`.


## Add my_syscall6() support for x86 32-bit.

mmap() needs 6 arguments to work with. Not all architectures that
nolibc supports have the my_syscall6() wrapper. This series also
adds my_syscall6() wrapper support for i386.

Notes:

Both Clang and GCC cannot use %ebp in the clobber list and in the "r"
constraint without using -fomit-frame-pointer. To make it always
available for any kind of compilation, the below workaround is
implemented.

For clang (the Assembly statement can't clobber %ebp):
  1) Save the %ebp value to the redzone area -4(%esp).
  2) Load the 6-th argument from memory to %ebp.
  3) Subtract the %esp by 4.
  4) Do the syscall (int $0x80).
  5) Pop %ebp.

For GCC, fortunately it has a #pragma that can force a specific function
to be compiled with -fomit-frame-pointer, so it can use "r"(var) where
var is a variable bound to %ebp.


## Limitation

Currently, for mips and arm arch cannot use these dynamic memory allocator
functions because they're missing the my_syscall6() macro.

[ammarfaizi2: I would love to add the support for them too, but I don't
 have the hardware to play with MIPS and ARM. ]


## Test

The following simple program can be used to test this series:

  https://gist.github.com/ammarfaizi2/db0af6aa0b95a0c7478bce64e349f021


## Patchset Summary

1) Patch 1 is a fix for the System V ABI document link.

2) Patch 2 is a fix to support compile with clang.

3) Patch 3 adds my_syscall6() implementation for i386.

4) Patch 4 adds mmap() and munmap() functions.

5) Patch 5 adds malloc(), calloc(), realloc() and free().

6) Patch 6 adds strdup() and strndup().


Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
---
Ammar Faizi (6):
  tools/nolibc: x86-64: Update System V ABI document link
  tools/nolibc: Make the entry point not weak for clang
  tools/nolibc: i386: Implement syscall with 6 arguments
  tools/nolibc/sys: Implement `mmap()` and `munmap()`
  tools/nolibc/stdlib: Implement `malloc()`, `calloc()`, `realloc()` and `free()`
  tools/include/string: Implement `strdup()` and `strndup()`

 tools/include/nolibc/arch-aarch64.h |  2 +
 tools/include/nolibc/arch-arm.h     |  2 +
 tools/include/nolibc/arch-i386.h    | 66 ++++++++++++++++++++++++
 tools/include/nolibc/arch-mips.h    |  2 +
 tools/include/nolibc/arch-riscv.h   |  2 +
 tools/include/nolibc/arch-x86_64.h  |  4 +-
 tools/include/nolibc/stdlib.h       | 79 +++++++++++++++++++++++++++++
 tools/include/nolibc/string.h       | 68 +++++++++++++++++++++++++
 tools/include/nolibc/sys.h          | 62 ++++++++++++++++++++++
 9 files changed, 286 insertions(+), 1 deletion(-)


base-commit: fda0d5d1b79d8b7032be3d7720a481a9fde91baf
-- 
Ammar Faizi

[RFC PATCH v1 1/6] tools/nolibc: x86-64: Update System V ABI document link

[Ammar Faizi]

The old link no longer works, update it.

Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
---
 tools/include/nolibc/arch-x86_64.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/include/nolibc/arch-x86_64.h b/tools/include/nolibc/arch-x86_64.h
index fe517c16cd4d..a7b70ea51b68 100644
--- a/tools/include/nolibc/arch-x86_64.h
+++ b/tools/include/nolibc/arch-x86_64.h
@@ -61,7 +61,7 @@ struct sys_stat_struct {
  *   - see also x86-64 ABI section A.2 AMD64 Linux Kernel Conventions, A.2.1
  *     Calling Conventions.
  *
- * Link x86-64 ABI: https://gitlab.com/x86-psABIs/x86-64-ABI/-/wikis/x86-64-psABI
+ * Link x86-64 ABI: https://gitlab.com/x86-psABIs/x86-64-ABI/-/wikis/home
  *
  */
 
-- 
Ammar Faizi

[RFC PATCH v1 2/6] tools/nolibc: Make the entry point not weak for clang

[Ammar Faizi]

Budilig with clang yields the following error:
```
  <inline asm>:3:1: error: _start changed binding to STB_GLOBAL
  .global _start
  ^
  1 error generated.
```
Don't make the entry point weak if we're compiling with clang.

Cc: llvm@lists.linux.dev
Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
---
 tools/include/nolibc/arch-aarch64.h | 2 ++
 tools/include/nolibc/arch-arm.h     | 2 ++
 tools/include/nolibc/arch-i386.h    | 2 ++
 tools/include/nolibc/arch-mips.h    | 2 ++
 tools/include/nolibc/arch-riscv.h   | 2 ++
 tools/include/nolibc/arch-x86_64.h  | 2 ++
 6 files changed, 12 insertions(+)

diff --git a/tools/include/nolibc/arch-aarch64.h b/tools/include/nolibc/arch-aarch64.h
index 87d9e434820c..5084cd58b429 100644
--- a/tools/include/nolibc/arch-aarch64.h
+++ b/tools/include/nolibc/arch-aarch64.h
@@ -183,7 +183,9 @@ struct sys_stat_struct {
 
 /* startup code */
 asm(".section .text\n"
+#if !defined(__clang__)
     ".weak _start\n"
+#endif
     ".global _start\n"
     "_start:\n"
     "ldr x0, [sp]\n"              // argc (x0) was in the stack
diff --git a/tools/include/nolibc/arch-arm.h b/tools/include/nolibc/arch-arm.h
index 001a3c8c9ad5..b3f135a615a6 100644
--- a/tools/include/nolibc/arch-arm.h
+++ b/tools/include/nolibc/arch-arm.h
@@ -176,7 +176,9 @@ struct sys_stat_struct {
 
 /* startup code */
 asm(".section .text\n"
+#if !defined(__clang__)
     ".weak _start\n"
+#endif
     ".global _start\n"
     "_start:\n"
 #if defined(__THUMBEB__) || defined(__THUMBEL__)
diff --git a/tools/include/nolibc/arch-i386.h b/tools/include/nolibc/arch-i386.h
index d7e4d53325a3..82bf797849ae 100644
--- a/tools/include/nolibc/arch-i386.h
+++ b/tools/include/nolibc/arch-i386.h
@@ -175,7 +175,9 @@ struct sys_stat_struct {
  *
  */
 asm(".section .text\n"
+#if !defined(__clang__)
     ".weak _start\n"
+#endif
     ".global _start\n"
     "_start:\n"
     "pop %eax\n"                // argc   (first arg, %eax)
diff --git a/tools/include/nolibc/arch-mips.h b/tools/include/nolibc/arch-mips.h
index c9a6aac87c6d..719d3808614d 100644
--- a/tools/include/nolibc/arch-mips.h
+++ b/tools/include/nolibc/arch-mips.h
@@ -190,7 +190,9 @@ struct sys_stat_struct {
 
 /* startup code, note that it's called __start on MIPS */
 asm(".section .text\n"
+#if !defined(__clang__)
     ".weak __start\n"
+#endif
     ".set nomips16\n"
     ".global __start\n"
     ".set    noreorder\n"
diff --git a/tools/include/nolibc/arch-riscv.h b/tools/include/nolibc/arch-riscv.h
index bc10b7b5706d..a9704affd7de 100644
--- a/tools/include/nolibc/arch-riscv.h
+++ b/tools/include/nolibc/arch-riscv.h
@@ -184,7 +184,9 @@ struct sys_stat_struct {
 
 /* startup code */
 asm(".section .text\n"
+#if !defined(__clang__)
     ".weak _start\n"
+#endif
     ".global _start\n"
     "_start:\n"
     ".option push\n"
diff --git a/tools/include/nolibc/arch-x86_64.h b/tools/include/nolibc/arch-x86_64.h
index a7b70ea51b68..f453f1a05a48 100644
--- a/tools/include/nolibc/arch-x86_64.h
+++ b/tools/include/nolibc/arch-x86_64.h
@@ -198,7 +198,9 @@ struct sys_stat_struct {
  *
  */
 asm(".section .text\n"
+#if !defined(__clang__)
     ".weak _start\n"
+#endif
     ".global _start\n"
     "_start:\n"
     "pop %rdi\n"                // argc   (first arg, %rdi)
-- 
Ammar Faizi

[RFC PATCH v1 3/6] tools/nolibc: i386: Implement syscall with 6 arguments

[Ammar Faizi]

In i386, the 6th argument of syscall goes in %ebp. However, both Clang
and GCC cannot use %ebp in the clobber list and in the "r" constraint
without using -fomit-frame-pointer. To make it always available for any
kind of compilation, the below workaround is implemented.

For clang (the Assembly statement can't clobber %ebp):
  1) Save the %ebp value to the redzone area -4(%esp).
  2) Load the 6-th argument from memory to %ebp.
  3) Subtract the %esp by 4.
  4) Do the syscall (int $0x80).
  5) Pop %ebp.

For GCC, fortunately it has a #pragma that can force a specific function
to be compiled with -fomit-frame-pointer, so it can always use "r"(var)
where `var` is a variable bound to %ebp.

Cc: x86@kernel.org
Cc: llvm@lists.linux.dev
Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
---
 tools/include/nolibc/arch-i386.h | 64 ++++++++++++++++++++++++++++++++
 1 file changed, 64 insertions(+)

diff --git a/tools/include/nolibc/arch-i386.h b/tools/include/nolibc/arch-i386.h
index 82bf797849ae..10de54d4b4d6 100644
--- a/tools/include/nolibc/arch-i386.h
+++ b/tools/include/nolibc/arch-i386.h
@@ -167,6 +167,70 @@ struct sys_stat_struct {
 	_ret;                                                                 \
 })
 
+
+/*
+ * Both Clang and GCC cannot use %ebp in the clobber list and in the "r"
+ * constraint without using -fomit-frame-pointer. To make it always
+ * available for any kind of compilation, the below workaround is
+ * implemented.
+ *
+ * For clang (the Assembly statement can't clobber %ebp):
+ *   1) Save the %ebp value to the redzone area -4(%esp).
+ *   2) Load the 6-th argument from memory to %ebp.
+ *   3) Subtract the %esp by 4.
+ *   4) Do the syscall (int $0x80).
+ *   5) Pop %ebp.
+ *
+ * For GCC, fortunately it has a #pragma that can force a specific function
+ * to be compiled with -fomit-frame-pointer, so it can use "r"(var) where
+ * var is a variable bound to %ebp.
+ *
+ */
+#if defined(__clang__)
+static inline long ____do_syscall6(long eax, long ebx, long ecx, long edx,
+				   long esi, long edi, long ebp)
+{
+	__asm__ volatile (
+		"movl	%%ebp, -4(%%esp)\n\t"
+		"movl	%[arg6], %%ebp\n\t"
+		"subl	$4, %%esp\n\t"
+		"int	$0x80\n\t"
+		"popl	%%ebp\n\t"
+		: "=a"(eax)
+		: "a"(eax), "b"(ebx), "c"(ecx), "d"(edx), "S"(esi), "D"(edi),
+		  [arg6]"m"(ebp)
+		: "memory", "cc"
+	);
+	return eax;
+}
+
+#else /* #if defined(__clang__) */
+#pragma GCC push_options
+#pragma GCC optimize "-fomit-frame-pointer"
+static inline long ____do_syscall6(long eax, long ebx, long ecx, long edx,
+				   long esi, long edi, long ebp)
+{
+	register long __ebp __asm__("ebp") = ebp;
+	__asm__ volatile (
+		"int	$0x80"
+		: "=a"(eax)
+		: "a"(eax), "b"(ebx), "c"(ecx), "d"(edx), "S"(esi), "D"(edi),
+		  "r"(__ebp)
+		: "memory", "cc"
+	);
+	return eax;
+}
+#pragma GCC pop_options
+#endif /* #if defined(__clang__) */
+
+#define my_syscall6(num, arg1, arg2, arg3, arg4, arg5, arg6) (   \
+	____do_syscall6((long)(num), (long)(arg1),               \
+			(long)(arg2), (long)(arg3),              \
+			(long)(arg4), (long)(arg5),              \
+			(long)(arg6))                            \
+)
+
+
 /* startup code */
 /*
  * i386 System V ABI mandates:
-- 
Ammar Faizi

[RFC PATCH v1 4/6] tools/nolibc/sys: Implement `mmap()` and `munmap()`

[Ammar Faizi]

Implement mmap() and munmap(). Currently, they are only available for
architecures that have my_syscall6 macro. For architectures that don't
have, this function will return -1 with errno set to -ENOSYS (Function
not implemented).

This has been tested on x86 and i386.

Notes for i386:
 1) The common mmap() syscall implementation uses __NR_mmap2 instead
    of __NR_mmap.
 2) The offset must be shifted-right by 12-bit.

Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
---
 tools/include/nolibc/sys.h | 62 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 62 insertions(+)

diff --git a/tools/include/nolibc/sys.h b/tools/include/nolibc/sys.h
index 28437863c63f..76510b19b7cf 100644
--- a/tools/include/nolibc/sys.h
+++ b/tools/include/nolibc/sys.h
@@ -14,6 +14,7 @@
 #include <asm/unistd.h>
 #include <asm/signal.h>  // for SIGCHLD
 #include <asm/ioctls.h>
+#include <asm/mman.h>
 #include <linux/fs.h>
 #include <linux/loop.h>
 #include <linux/time.h>
@@ -658,6 +659,67 @@ int mknod(const char *path, mode_t mode, dev_t dev)
 	return ret;
 }
 
+#ifndef MAP_SHARED
+#define MAP_SHARED		0x01	/* Share changes */
+#define MAP_PRIVATE		0x02	/* Changes are private */
+#define MAP_SHARED_VALIDATE	0x03	/* share + validate extension flags */
+#endif
+
+#ifndef MAP_FAILED
+#define MAP_FAILED ((void *)-1)
+#endif
+
+static __attribute__((unused))
+void *sys_mmap(void *addr, size_t length, int prot, int flags, int fd,
+	       off_t offset)
+{
+#ifndef my_syscall6
+	/* Function not implemented. */
+	return -ENOSYS;
+#else
+
+	int n;
+
+#if defined(__i386__)
+	n = __NR_mmap2;
+	offset >>= 12;
+#else
+	n = __NR_mmap;
+#endif
+
+	return (void *)my_syscall6(n, addr, length, prot, flags, fd, offset);
+#endif
+}
+
+static __attribute__((unused))
+void *mmap(void *addr, size_t length, int prot, int flags, int fd, off_t offset)
+{
+	void *ret = sys_mmap(addr, length, prot, flags, fd, offset);
+
+	if ((unsigned long)ret >= -4095UL) {
+		SET_ERRNO(-(long)ret);
+		ret = MAP_FAILED;
+	}
+	return ret;
+}
+
+static __attribute__((unused))
+int sys_munmap(void *addr, size_t length)
+{
+	return my_syscall2(__NR_munmap, addr, length);
+}
+
+static __attribute__((unused))
+int munmap(void *addr, size_t length)
+{
+	int ret = sys_munmap(addr, length);
+
+	if (ret < 0) {
+		SET_ERRNO(-ret);
+		ret = -1;
+	}
+	return ret;
+}
 
 /*
  * int mount(const char *source, const char *target,
-- 
Ammar Faizi

[RFC PATCH v1 5/6] tools/nolibc/stdlib: Implement `malloc()`, `calloc()`, `realloc()` and `free()`

[Ammar Faizi]

Implement basic dynamic allocator functions. These functions are
currently only available on architectures that have nolibc mmap()
syscall implemented. These are not a super-fast memory allocator,
but at least they can satisfy basic needs for having heap without
libc.

Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
---
 tools/include/nolibc/stdlib.h | 79 +++++++++++++++++++++++++++++++++++
 1 file changed, 79 insertions(+)

diff --git a/tools/include/nolibc/stdlib.h b/tools/include/nolibc/stdlib.h
index 733105c574ee..13600e73404d 100644
--- a/tools/include/nolibc/stdlib.h
+++ b/tools/include/nolibc/stdlib.h
@@ -10,8 +10,24 @@
 #include "std.h"
 #include "arch.h"
 #include "types.h"
+#include "string.h"
 #include "sys.h"
 
+struct nolibc_heap {
+	size_t	len;
+	char	user_p[] __attribute__((__aligned__));
+};
+
+#ifndef offsetof
+#define offsetof(TYPE, FIELD) ((size_t) &((TYPE *)0)->FIELD)
+#endif
+
+#ifndef container_of
+#define container_of(PTR, TYPE, FIELD) ({			\
+	__typeof__(((TYPE *)0)->FIELD) *__FIELD_PTR = (PTR);	\
+	(TYPE *)((char *) __FIELD_PTR - offsetof(TYPE, FIELD));	\
+})
+#endif
 
 /* Buffer used to store int-to-ASCII conversions. Will only be implemented if
  * any of the related functions is implemented. The area is large enough to
@@ -60,6 +76,18 @@ int atoi(const char *s)
 	return atol(s);
 }
 
+static __attribute__((unused))
+void free(void *ptr)
+{
+	struct nolibc_heap *heap;
+
+	if (!ptr)
+		return;
+
+	heap = container_of(ptr, struct nolibc_heap, user_p);
+	munmap(heap, heap->len);
+}
+
 /* Converts the unsigned long integer <in> to its hex representation into
  * buffer <buffer>, which must be long enough to store the number and the
  * trailing zero (17 bytes for "ffffffffffffffff" or 9 for "ffffffff"). The
@@ -182,6 +210,57 @@ char *ltoa(long in)
 	return itoa_buffer;
 }
 
+static inline __attribute__((unused))
+void *malloc(size_t len)
+{
+	struct nolibc_heap *heap;
+
+	heap = mmap(NULL, sizeof(*heap) + len, PROT_READ|PROT_WRITE,
+		    MAP_ANONYMOUS|MAP_PRIVATE, -1, 0);
+	if (__builtin_expect(heap == MAP_FAILED, 0))
+		return NULL;
+
+	heap->len = sizeof(*heap) + len;
+	return heap->user_p;
+}
+
+static inline __attribute__((unused))
+void *calloc(size_t size, size_t nmemb)
+{
+	void *orig;
+	size_t res = 0;
+
+	if (__builtin_expect(__builtin_mul_overflow(nmemb, size, &res), 0)) {
+		SET_ERRNO(ENOMEM);
+		return NULL;
+	}
+
+	/*
+	 * No need to zero the heap, the MAP_ANONYMOUS in malloc()
+	 * already does it.
+	 */
+	return malloc(res);
+}
+
+static inline __attribute__((unused))
+void *realloc(void *old_ptr, size_t new_size)
+{
+	struct nolibc_heap *heap;
+	void *ret;
+
+	if (!old_ptr)
+		return malloc(new_size);
+
+	ret = malloc(new_size);
+	if (__builtin_expect(!ret, 0))
+		return NULL;
+
+	heap = container_of(old_ptr, struct nolibc_heap, user_p);
+	memcpy(ret, heap->user_p, heap->len);
+	munmap(heap, heap->len);
+	return ret;
+}
+
 /* converts unsigned long integer <in> to a string using the static itoa_buffer
  * and returns the pointer to that string.
  */
-- 
Ammar Faizi

[RFC PATCH v1 6/6] tools/include/string: Implement `strdup()` and `strndup()`

[Ammar Faizi]

Add strdup and strndup support. These functions are only available on
architectures that have my_syscall6() macro from nolibc.

Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
---
 tools/include/nolibc/string.h | 68 +++++++++++++++++++++++++++++++++++
 1 file changed, 68 insertions(+)

diff --git a/tools/include/nolibc/string.h b/tools/include/nolibc/string.h
index 4554b6fcb400..413c65f7c853 100644
--- a/tools/include/nolibc/string.h
+++ b/tools/include/nolibc/string.h
@@ -9,6 +9,10 @@
 
 #include "std.h"
 
+static void free(void *ptr);
+static void *malloc(size_t len);
+static void *realloc(void *old_ptr, size_t new_size);
+
 /*
  * As much as possible, please keep functions alphabetically sorted.
  */
@@ -127,6 +131,70 @@ size_t nolibc_strlen(const char *str)
 		nolibc_strlen((str));           \
 })
 
+static __attribute__((unused))
+char *strdup(const char *str)
+{
+	size_t allocated = 2048;
+	size_t i;
+	char *ret;
+	char *tmp;
+
+	ret = malloc(allocated);
+	if (__builtin_expect(!ret, 0))
+		return NULL;
+
+	i = 0;
+	for (;;) {
+		char c = *str;
+		if (!c)
+			break;
+
+		if (i == allocated) {
+			allocated += 2048;
+			tmp = realloc(ret, allocated);
+			if (__builtin_expect(!tmp, 0)) {
+				free(ret);
+				return NULL;
+			}
+			ret = tmp;
+		}
+
+		ret[i++] = c;
+		str++;
+	}
+
+	ret[i] = '\0';
+	return ret;
+}
+
+static __attribute__((unused))
+char *strndup(const char *str, size_t maxlen)
+{
+	size_t i;
+	char *ret;
+
+	ret = malloc(maxlen + 1);
+	if (__builtin_expect(!ret, 0))
+		return NULL;
+
+	i = 0;
+	for (;;) {
+		char c = *str;
+		if (!c)
+			break;
+
+		if (i == maxlen)
+			break;
+
+		ret[i++] = c;
+		str++;
+	}
+
+	ret[i] = '\0';
+	return ret;
+}
+
+
 static __attribute__((unused))
 size_t strlcat(char *dst, const char *src, size_t size)
 {
-- 
Ammar Faizi

Re: [RFC PATCH v1 3/6] tools/nolibc: i386: Implement syscall with 6 arguments

[Alviro Iskandar Setiawan]

On Sun, Mar 20, 2022 at 4:37 PM Ammar Faizi wrote:
> In i386, the 6th argument of syscall goes in %ebp. However, both Clang
> and GCC cannot use %ebp in the clobber list and in the "r" constraint
> without using -fomit-frame-pointer. To make it always available for any
> kind of compilation, the below workaround is implemented.
>
> For clang (the Assembly statement can't clobber %ebp):
>   1) Save the %ebp value to the redzone area -4(%esp).
>   2) Load the 6-th argument from memory to %ebp.
>   3) Subtract the %esp by 4.
>   4) Do the syscall (int $0x80).
>   5) Pop %ebp.

I don't think you can safely use redzone from inline Assembly. The
compiler may also use redzone for a leaf function. In case the syscall
is done at the same time, your %ebp saving will clobber the redzone
that the compiler uses.

> For GCC, fortunately it has a #pragma that can force a specific function
> to be compiled with -fomit-frame-pointer, so it can always use "r"(var)
> where `var` is a variable bound to %ebp.
>
> Cc: x86@kernel.org
> Cc: llvm@lists.linux.dev
> Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
[...]
> +#if defined(__clang__)
> +static inline long ____do_syscall6(long eax, long ebx, long ecx, long edx,
> +                                  long esi, long edi, long ebp)
> +{
> +       __asm__ volatile (
> +               "movl   %%ebp, -4(%%esp)\n\t"
> +               "movl   %[arg6], %%ebp\n\t"
> +               "subl   $4, %%esp\n\t"
> +               "int    $0x80\n\t"
> +               "popl   %%ebp\n\t"
> +               : "=a"(eax)
> +               : "a"(eax), "b"(ebx), "c"(ecx), "d"(edx), "S"(esi), "D"(edi),
> +                 [arg6]"m"(ebp)
> +               : "memory", "cc"
> +       );
> +       return eax;
> +}
> +

-4(%esp) may be used by the compiler on a leaf call, you can't clobber that.

-- Viro

Re: [RFC PATCH v1 3/6] tools/nolibc: i386: Implement syscall with 6 arguments

[Alviro Iskandar Setiawan]

On Sun, Mar 20, 2022 at 5:33 PM Alviro Iskandar Setiawan wrote:
> On Sun, Mar 20, 2022 at 4:37 PM Ammar Faizi wrote:
> > In i386, the 6th argument of syscall goes in %ebp. However, both Clang
> > and GCC cannot use %ebp in the clobber list and in the "r" constraint
> > without using -fomit-frame-pointer. To make it always available for any
> > kind of compilation, the below workaround is implemented.
> >
> > For clang (the Assembly statement can't clobber %ebp):
> >   1) Save the %ebp value to the redzone area -4(%esp).
> >   2) Load the 6-th argument from memory to %ebp.
> >   3) Subtract the %esp by 4.
> >   4) Do the syscall (int $0x80).
> >   5) Pop %ebp.
>
> I don't think you can safely use redzone from inline Assembly. The
> compiler may also use redzone for a leaf function. In case the syscall
> is done at the same time, your %ebp saving will clobber the redzone
> that the compiler uses.
>
> > For GCC, fortunately it has a #pragma that can force a specific function
> > to be compiled with -fomit-frame-pointer, so it can always use "r"(var)
> > where `var` is a variable bound to %ebp.
> >
> > Cc: x86@kernel.org
> > Cc: llvm@lists.linux.dev
> > Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
> [...]
> > +#if defined(__clang__)
> > +static inline long ____do_syscall6(long eax, long ebx, long ecx, long edx,
> > +                                  long esi, long edi, long ebp)
> > +{
> > +       __asm__ volatile (
> > +               "movl   %%ebp, -4(%%esp)\n\t"
> > +               "movl   %[arg6], %%ebp\n\t"
> > +               "subl   $4, %%esp\n\t"
> > +               "int    $0x80\n\t"
> > +               "popl   %%ebp\n\t"
> > +               : "=a"(eax)
> > +               : "a"(eax), "b"(ebx), "c"(ecx), "d"(edx), "S"(esi), "D"(edi),
> > +                 [arg6]"m"(ebp)
> > +               : "memory", "cc"
> > +       );
> > +       return eax;
> > +}
> > +
>
> -4(%esp) may be used by the compiler on a leaf call, you can't clobber that.

Using xchgl to preserve %ebp in the same place where the arg6 is
stored in memory is a better solution and doesn't clobber anything.

    xchgl %ebp, %[arg6]
    int    $0x80
    xchgl %ebp, %[arg6]

-- Viro

RE: [RFC PATCH v1 3/6] tools/nolibc: i386: Implement syscall with 6 arguments

[David Laight]

From: Ammar Faizi
> Sent: 20 March 2022 09:38
> 
> In i386, the 6th argument of syscall goes in %ebp. However, both Clang
> and GCC cannot use %ebp in the clobber list and in the "r" constraint
> without using -fomit-frame-pointer. To make it always available for any
> kind of compilation, the below workaround is implemented.
> 
> For clang (the Assembly statement can't clobber %ebp):
>   1) Save the %ebp value to the redzone area -4(%esp).

i386 doesn't have a redzone.
If you get a signal it will trash -4(%sp)

>   2) Load the 6-th argument from memory to %ebp.
>   3) Subtract the %esp by 4.
>   4) Do the syscall (int $0x80).
>   5) Pop %ebp.
> 
> For GCC, fortunately it has a #pragma that can force a specific function
> to be compiled with -fomit-frame-pointer, so it can always use "r"(var)
> where `var` is a variable bound to %ebp.

How is that going to work for an inlined functon?

And using xchg is slow - it is always locked.

One possibility might be to do:
	push arg6
	push %ebp
	mov  %ebp, 4(%sp)
	int  0x80
	pop  %ebp
	add  %esp,4

Although I'm not sure you really want to allocate 4k pages
for every malloc() call.

Probably better to write a mini 'libc' that uses sbrk()
and a best fit scan of a linear free list.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [RFC PATCH v1 3/6] tools/nolibc: i386: Implement syscall with 6 arguments

[Willy Tarreau]

Hi David,

On Sun, Mar 20, 2022 at 01:10:33PM +0000, David Laight wrote:
> And using xchg is slow - it is always locked.

Note that we don't really care here, as it remains minimal compared to
an mmap() call.

> One possibility might be to do:
> 	push arg6
> 	push %ebp
> 	mov  %ebp, 4(%sp)
> 	int  0x80
> 	pop  %ebp
> 	add  %esp,4
> 
> Although I'm not sure you really want to allocate 4k pages
> for every malloc() call.

Well, it depends. I would argue that we don't even need malloc() but
on the other hand this is essentially used to write small regtests so
we don't really care about the waste here if someone really needs it.

I'd rather get Ammar's motivations for malloc() in the first place.

Willy

Re: [RFC PATCH v1 3/6] tools/nolibc: i386: Implement syscall with 6 arguments

[Ammar Faizi]

On 3/20/22 8:10 PM, David Laight wrote:
> From: Ammar Faizi
>> Sent: 20 March 2022 09:38
>>
>> In i386, the 6th argument of syscall goes in %ebp. However, both Clang
>> and GCC cannot use %ebp in the clobber list and in the "r" constraint
>> without using -fomit-frame-pointer. To make it always available for any
>> kind of compilation, the below workaround is implemented.
>>
>> For clang (the Assembly statement can't clobber %ebp):
>>    1) Save the %ebp value to the redzone area -4(%esp).
> 
> i386 doesn't have a redzone.
> If you get a signal it will trash -4(%sp)

OK, I missed that one. Thanks for reviewing this.

>> For GCC, fortunately it has a #pragma that can force a specific function
>> to be compiled with -fomit-frame-pointer, so it can always use "r"(var)
>> where `var` is a variable bound to %ebp.
> 
> How is that going to work for an inlined functon?

It works, but obviously the inline modifier here is just a hint for the
compiler, not a requirement. I just took a look at the GCC generated code.
It doesn't inline the ____do_syscall6() despite it's marked as inline.

I think this one shouldn't be marked as inline. I will remove the inline
in the next version.

> And using xchg is slow - it is always locked.
> 
> One possibility might be to do:
> 	push arg6
> 	push %ebp
> 	mov  %ebp, 4(%sp)

Did you mean `mov 4(%esp), %ebp`?

> 	int  0x80
> 	pop  %ebp
> 	add  %esp,4

I think your solution is better than the xchg approach (with the 3rd line
fixed). Will take this in for the next version.

> Although I'm not sure you really want to allocate 4k pages
> for every malloc() call.
> 
> Probably better to write a mini 'libc' that uses sbrk()
> and a best fit scan of a linear free list.

^ This part is addressed by Willy's response. I will still go with mmap()
in the next version.

And yes, we do waste space for small allocations here, but we don't hit
the scale that the waste space will cause problem.

-- 
Ammar Faizi

Re: [RFC PATCH v1 3/6] tools/nolibc: i386: Implement syscall with 6 arguments

[Ammar Faizi]

On 3/20/22 5:42 PM, Alviro Iskandar Setiawan wrote:
> On Sun, Mar 20, 2022 at 5:33 PM Alviro Iskandar Setiawan wrote:
[...]
>> I don't think you can safely use redzone from inline Assembly. The
>> compiler may also use redzone for a leaf function. In case the syscall
>> is done at the same time, your %ebp saving will clobber the redzone
>> that the compiler uses.

It turned out we don't have a redzone for 32-bit.

>>
>> -4(%esp) may be used by the compiler on a leaf call, you can't clobber that.

Yeah, this is still wrong even with a redzone.

> Using xchgl to preserve %ebp in the same place where the arg6 is
> stored in memory is a better solution and doesn't clobber anything.
> 
>      xchgl %ebp, %[arg6]
>      int    $0x80
>      xchgl %ebp, %[arg6]

Addressed your review in my response to David.

Thanks!

-- 
Ammar Faizi

Re: [RFC PATCH v1 5/6] tools/nolibc/stdlib: Implement `malloc()`, `calloc()`, `realloc()` and `free()`

[Alviro Iskandar Setiawan]

On Sun, Mar 20, 2022 at 4:37 PM Ammar Faizi wrote:
> +void *realloc(void *old_ptr, size_t new_size)
> +{
> +       struct nolibc_heap *heap;
> +       void *ret;
> +
> +       if (!old_ptr)
> +               return malloc(new_size);
> +
> +       ret = malloc(new_size);
> +       if (__builtin_expect(!ret, 0))
> +               return NULL;
> +
> +       heap = container_of(old_ptr, struct nolibc_heap, user_p);
> +       memcpy(ret, heap->user_p, heap->len);
> +       munmap(heap, heap->len);
> +       return ret;
> +}

This better be simplified like this, so only have 1 malloc() call that
applies to both branches.

  void *realloc(void *old_ptr, size_t new_size)
  {
         struct nolibc_heap *heap;
         void *ret;

         ret = malloc(new_size);
         if (__builtin_expect(!ret, 0))
                 return NULL;

         if (!old_ptr)
                 return ret;

         heap = container_of(old_ptr, struct nolibc_heap, user_p);
         memcpy(ret, heap->user_p, heap->len);
         munmap(heap, heap->len);
         return ret;
  }

-- Viro

Re: [RFC PATCH v1 6/6] tools/include/string: Implement `strdup()` and `strndup()`

[Alviro Iskandar Setiawan]

On Sun, Mar 20, 2022 at 4:37 PM Ammar Faizi wrote:
> +}
> +
> +

(Trivial) Got double newlines here, one newline should be good.

>  static __attribute__((unused))
>  size_t strlcat(char *dst, const char *src, size_t size)
>  {

-- Viro

Re: [RFC PATCH v1 5/6] tools/nolibc/stdlib: Implement `malloc()`, `calloc()`, `realloc()` and `free()`

[Ammar Faizi]

On 3/20/22 10:50 PM, Alviro Iskandar Setiawan wrote:
> This better be simplified like this, so only have 1 malloc() call that
> applies to both branches.
> 
>    void *realloc(void *old_ptr, size_t new_size)
>    {
>           struct nolibc_heap *heap;
>           void *ret;
> 
>           ret = malloc(new_size);
>           if (__builtin_expect(!ret, 0))
>                   return NULL;
> 
>           if (!old_ptr)
>                   return ret;
> 
>           heap = container_of(old_ptr, struct nolibc_heap, user_p);
>           memcpy(ret, heap->user_p, heap->len);
>           munmap(heap, heap->len);
>           return ret;
>    }

That looks better, will take this for the v2. Thanks!

-- 
Ammar Faizi

Re: [RFC PATCH v1 6/6] tools/include/string: Implement `strdup()` and `strndup()`

[Ammar Faizi]

On 3/20/22 10:55 PM, Alviro Iskandar Setiawan wrote:
> On Sun, Mar 20, 2022 at 4:37 PM Ammar Faizi wrote:
>> +}
>> +
>> +
> 
> (Trivial) Got double newlines here, one newline should be good.

OK, fixed.

>>   static __attribute__((unused))
>>   size_t strlcat(char *dst, const char *src, size_t size)
>>   {

-- 
Ammar Faizi

Re: [RFC PATCH v1 5/6] tools/nolibc/stdlib: Implement `malloc()`, `calloc()`, `realloc()` and `free()`

[Willy Tarreau]

Ammar,

a few points below:

On Sun, Mar 20, 2022 at 04:37:49PM +0700, Ammar Faizi wrote:
> +struct nolibc_heap {
> +	size_t	len;
> +	char	user_p[] __attribute__((__aligned__));
> +};

Note that many programs assume that malloc() returns a field aligned
to 2*sizeof(pointer) and unless I'm mistaken, above the user pointer
will only be aligned by one pointer. This may have an impact when the
compiler decides to use SIMD instructions.

> +#ifndef offsetof
> +#define offsetof(TYPE, FIELD) ((size_t) &((TYPE *)0)->FIELD)
> +#endif
> +
> +#ifndef container_of
> +#define container_of(PTR, TYPE, FIELD) ({			\
> +	__typeof__(((TYPE *)0)->FIELD) *__FIELD_PTR = (PTR);	\
> +	(TYPE *)((char *) __FIELD_PTR - offsetof(TYPE, FIELD));	\
> +})
> +#endif

These ones are independent on the malloc code and should move to a
different patch and likely to a different file. I'm seeing we already
have a few macros in types.h and since it's shared by almost everything
it might be more suitable there.

Thanks!
Willy

Re: [RFC PATCH v1 5/6] tools/nolibc/stdlib: Implement `malloc()`, `calloc()`, `realloc()` and `free()`

[Ammar Faizi]

On 3/20/22 11:16 PM, Willy Tarreau wrote:
> Ammar,
> 
> a few points below:
> 
> On Sun, Mar 20, 2022 at 04:37:49PM +0700, Ammar Faizi wrote:
>> +struct nolibc_heap {
>> +	size_t	len;
>> +	char	user_p[] __attribute__((__aligned__));
>> +};
> 
> Note that many programs assume that malloc() returns a field aligned
> to 2*sizeof(pointer) and unless I'm mistaken, above the user pointer
> will only be aligned by one pointer. This may have an impact when the
> compiler decides to use SIMD instructions.

Section 7.20.3 of C99 states this about `malloc()`:
"""
   The pointer returned if the allocation succeeds is suitably aligned
   so that it may be assigned to a pointer to any type of object.
"""

And this is what GCC doc says about __attribute__((__aligned__)):
"""
   The aligned attribute specifies a minimum alignment for the variable
   or structure field, measured in bytes. When specified, alignment must
   be an integer constant power of 2. Specifying no alignment argument
   implies the maximum alignment for the target, which is often, but by
   no means always, 8 or 16 bytes.
"""

Link: https://gcc.gnu.org/onlinedocs/gcc-11.2.0/gcc/Common-Variable-Attributes.html#Common-Variable-Attributes

Simple experiment on Linux x86-64...

```
ammarfaizi2@integral2:/tmp$ cat > test.c
#include <stdio.h>
int main(void)
{
	printf("alignof = %zu\n", __alignof__(long double));
	return 0;
}
ammarfaizi2@integral2:/tmp$ gcc -o test test.c
ammarfaizi2@integral2:/tmp$ ./test
alignof = 16
ammarfaizi2@integral2:/tmp$
```

We have `long double` which requires 16 byte alignment. So
__attribute__((__aligned__)) should cover this. And yes, it's true that
it's 2*sizeof(void*), but importantly for the above reason.

>> +#ifndef offsetof
>> +#define offsetof(TYPE, FIELD) ((size_t) &((TYPE *)0)->FIELD)
>> +#endif
>> +
>> +#ifndef container_of
>> +#define container_of(PTR, TYPE, FIELD) ({			\
>> +	__typeof__(((TYPE *)0)->FIELD) *__FIELD_PTR = (PTR);	\
>> +	(TYPE *)((char *) __FIELD_PTR - offsetof(TYPE, FIELD));	\
>> +})
>> +#endif
> 
> These ones are independent on the malloc code and should move to a
> different patch and likely to a different file. I'm seeing we already
> have a few macros in types.h and since it's shared by almost everything
> it might be more suitable there.

OK, will do it in the v2. Thanks!

-- 
Ammar Faizi

Re: [RFC PATCH v1 5/6] tools/nolibc/stdlib: Implement `malloc()`, `calloc()`, `realloc()` and `free()`

[Willy Tarreau]

On Sun, Mar 20, 2022 at 11:36:55PM +0700, Ammar Faizi wrote:
> And this is what GCC doc says about __attribute__((__aligned__)):
> """
>   The aligned attribute specifies a minimum alignment for the variable
>   or structure field, measured in bytes. When specified, alignment must
>   be an integer constant power of 2. Specifying no alignment argument
>   implies the maximum alignment for the target, which is often, but by
>   no means always, 8 or 16 bytes.
> """
> 
> Link: https://gcc.gnu.org/onlinedocs/gcc-11.2.0/gcc/Common-Variable-Attributes.html#Common-Variable-Attributes

OK then that's fine, thank you. I thought it would force the alignment
to the type itself.

> Simple experiment on Linux x86-64...

That's even easier checked like this:

  $ cat > c.c <<EOF
  struct blah {
          char a;
          char b __attribute__((__aligned__));
  } blah;
  EOF
  $ gcc -g -c c.c
  $ pahole c.o
  struct blah {
          char                       a;                    /*     0     1 */
  
          /* XXX 15 bytes hole, try to pack */
  
          char                       b __attribute__((__aligned__(16))); /*    16     1 */
  
          /* size: 32, cachelines: 1, members: 2 */
          /* sum members: 2, holes: 1, sum holes: 15 */
          /* padding: 15 */
          /* forced alignments: 1, forced holes: 1, sum forced holes: 15 */
          /* last cacheline: 32 bytes */
  } __attribute__((__aligned__(16)));

Thank you!
Willy

RE: [RFC PATCH v1 3/6] tools/nolibc: i386: Implement syscall with 6 arguments

[David Laight]

From: Ammar Faizi
> Sent: 20 March 2022 15:04
> On 3/20/22 8:10 PM, David Laight wrote:
> > From: Ammar Faizi
> >> Sent: 20 March 2022 09:38
> >>
> >> In i386, the 6th argument of syscall goes in %ebp. However, both Clang
> >> and GCC cannot use %ebp in the clobber list and in the "r" constraint
> >> without using -fomit-frame-pointer. To make it always available for any
> >> kind of compilation, the below workaround is implemented.
> >>
> >> For clang (the Assembly statement can't clobber %ebp):
> >>    1) Save the %ebp value to the redzone area -4(%esp).
> >
> > i386 doesn't have a redzone.
> > If you get a signal it will trash -4(%sp)
> 
> OK, I missed that one. Thanks for reviewing this.
> 
...
> >
> > One possibility might be to do:
> > 	push arg6
> > 	push %ebp
> > 	mov  %ebp, 4(%sp)
> 
> Did you mean `mov 4(%esp), %ebp`?
> 
> > 	int  0x80
> > 	pop  %ebp
> > 	add  %esp,4
> 
> I think your solution is better than the xchg approach (with the 3rd line
> fixed). Will take this in for the next version.

It has to be said that although I've been writing x86 asm
for 40 years (and others for longer) I can never actually
remember the exact syntax or order of the operands!
Probably because it is randomly different between assemblers.
You want the 'memory read' instruction: 8b /r.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [RFC PATCH v1 2/6] tools/nolibc: Make the entry point not weak for clang

[Willy Tarreau]

Hi Ammar,

I've had a look at this one.

On Sun, Mar 20, 2022 at 04:37:46PM +0700, Ammar Faizi wrote:
> Budilig with clang yields the following error:
  ^^^^^^^
BTW please fix the typo in the final commit message.

> @@ -183,7 +183,9 @@ struct sys_stat_struct {
>  
>  /* startup code */
>  asm(".section .text\n"
> +#if !defined(__clang__)
>      ".weak _start\n"
> +#endif
>      ".global _start\n"
>      "_start:\n"

So it seems that I was wrong and that .weak is an alternate for .global
and not a complement. As such, instead of adding all these #if, please
simply remove all .global.

Thanks!
Willy

Re: [RFC PATCH v1 6/6] tools/include/string: Implement `strdup()` and `strndup()`

[Willy Tarreau]

Hi Ammar,

On Sun, Mar 20, 2022 at 04:37:50PM +0700, Ammar Faizi wrote:
> Add strdup and strndup support. These functions are only available on
> architectures that have my_syscall6() macro from nolibc.
> 
> Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
> ---
>  tools/include/nolibc/string.h | 68 +++++++++++++++++++++++++++++++++++
>  1 file changed, 68 insertions(+)
> 
> diff --git a/tools/include/nolibc/string.h b/tools/include/nolibc/string.h
> index 4554b6fcb400..413c65f7c853 100644
> --- a/tools/include/nolibc/string.h
> +++ b/tools/include/nolibc/string.h
> @@ -9,6 +9,10 @@
>  
>  #include "std.h"
>  
> +static void free(void *ptr);
> +static void *malloc(size_t len);
> +static void *realloc(void *old_ptr, size_t new_size);

Better include the required h files here.

>  /*
>   * As much as possible, please keep functions alphabetically sorted.
>   */
> @@ -127,6 +131,70 @@ size_t nolibc_strlen(const char *str)
>  		nolibc_strlen((str));           \
>  })
>  
> +static __attribute__((unused))
> +char *strdup(const char *str)
> +{
> +	size_t allocated = 2048;
> +	size_t i;
> +	char *ret;
> +	char *tmp;
> +
> +	ret = malloc(allocated);
> +	if (__builtin_expect(!ret, 0))
> +		return NULL;
> +
> +	i = 0;
> +	for (;;) {
> +		char c = *str;
> +		if (!c)
> +			break;
> +
> +		if (i == allocated) {
> +			allocated += 2048;
> +			tmp = realloc(ret, allocated);
> +			if (__builtin_expect(!tmp, 0)) {
> +				free(ret);
> +				return NULL;
> +			}
> +			ret = tmp;
> +		}
> +
> +		ret[i++] = c;
> +		str++;
> +	}
> +
> +	ret[i] = '\0';
> +	return ret;
> +}

This version is suboptimal in terms of code size, CPU usage and memory
usage. And it even seems it contains a buffer overflow: if the string
is exactly a multiple of 2048, it seems to me that you'll write the
trailing zero past the end. Please instead use the more intuitive form
below (not tested but you get the idea):

	size_t len = strlen(str);
	char *ret = malloc(len + 1);
	if (ret)
		memcpy(ret, str, len);
	return ret;

> +static __attribute__((unused))
> +char *strndup(const char *str, size_t maxlen)
> +{
> +	size_t i;
> +	char *ret;
> +
> +	ret = malloc(maxlen + 1);
> +	if (__builtin_expect(!ret, 0))
> +		return NULL;
> +
> +	i = 0;
> +	for (;;) {
> +		char c = *str;
> +		if (!c)
> +			break;
> +
> +		if (i == maxlen)
> +			break;
> +
> +		ret[i++] = c;
> +		str++;
> +	}
> +
> +	ret[i] = '\0';
> +	return ret;
> +}

Here it can cost quite a lot for large values of maxlen. Please just use
a variant of the proposal above like this one:

	size_t len;
	char *ret;

	len = strlen(str);
	if (len > maxlen)
		len = maxlen;
	ret = malloc(len + 1);
	if (ret)
		memcpy(ret, str, len);
	return ret;

Thanks,
Willy

Re: [RFC PATCH v1 6/6] tools/include/string: Implement `strdup()` and `strndup()`

[Alviro Iskandar Setiawan]

On Mon, Mar 21, 2022 at 2:53 PM Willy Tarreau wrote:
> Here it can cost quite a lot for large values of maxlen. Please just use
> a variant of the proposal above like this one:
>
>         size_t len;
>         char *ret;
>
>         len = strlen(str);
>         if (len > maxlen)
>                 len = maxlen;
>         ret = malloc(len + 1);
>         if (ret)
>                 memcpy(ret, str, len);
>         return ret;

Maybe better to use strnlen(), see the detail at man 3 strnlen.

  size_t strnlen(const char *s, size_t maxlen);

The strnlen() function returns the number of bytes in the string
pointed to by s, excluding the terminating null byte ('\0'), but at
most maxlen. In doing this, strnlen() looks only at the first maxlen
characters in the string pointed to by s and never beyond s[maxlen-1].

Should be trivial to add strnlen() with a separate patch before this patch.

So it can be:

    size_t len;
    char *ret;

    len = strnlen(str, maxlen);
    ret = malloc(len + 1);
    if (__builtin_expect(ret != NULL, 1)) {
        memcpy(ret, str, len);
        ret[len] = '\0';
    }
    return ret;

Thoughts?

-- Viro

Re: [RFC PATCH v1 6/6] tools/include/string: Implement `strdup()` and `strndup()`

[Willy Tarreau]

On Mon, Mar 21, 2022 at 03:16:54PM +0700, Alviro Iskandar Setiawan wrote:
> On Mon, Mar 21, 2022 at 2:53 PM Willy Tarreau wrote:
> > Here it can cost quite a lot for large values of maxlen. Please just use
> > a variant of the proposal above like this one:
> >
> >         size_t len;
> >         char *ret;
> >
> >         len = strlen(str);
> >         if (len > maxlen)
> >                 len = maxlen;
> >         ret = malloc(len + 1);
> >         if (ret)
> >                 memcpy(ret, str, len);
> >         return ret;
> 
> Maybe better to use strnlen(), see the detail at man 3 strnlen.
> 
>   size_t strnlen(const char *s, size_t maxlen);
> 
> The strnlen() function returns the number of bytes in the string
> pointed to by s, excluding the terminating null byte ('\0'), but at
> most maxlen. In doing this, strnlen() looks only at the first maxlen
> characters in the string pointed to by s and never beyond s[maxlen-1].
> 
> Should be trivial to add strnlen() with a separate patch before this patch.
> 
> So it can be:
> 
>     size_t len;
>     char *ret;
> 
>     len = strnlen(str, maxlen);
>     ret = malloc(len + 1);
>     if (__builtin_expect(ret != NULL, 1)) {
>         memcpy(ret, str, len);
>         ret[len] = '\0';
>     }
>     return ret;
> 
> Thoughts?

I thought about it as well and while I was seeking the simplest route,
I agree it would indeed be cleaner.

Willy

Re: [RFC PATCH v1 6/6] tools/include/string: Implement `strdup()` and `strndup()`

[Ammar Faizi]

On 3/21/22 2:53 PM, Willy Tarreau wrote:
> Hi Ammar,
[...]
>> +static void free(void *ptr);
>> +static void *malloc(size_t len);
>> +static void *realloc(void *old_ptr, size_t new_size);
> 
> Better include the required h files here.

I can't do that, in nolibc.h, we have something like this:

```
   #include "stdlib.h" <--- We inlcude string.h from here
   #include "string.h" <--- This is a no-op.
```

Note, stdlib.h is included first before string.h, next, in stdlib.h, we
have this:

```
   #include "string.h"

   // malloc, calloc, free here
```

If I include "stdlib.h" in "string.h", it will just be a no-op, and the
declarations will not be taken, because the declarations happen after
#include "string.h". So it doesn't work. It's somewhat circular dependency.

   stdlib.h needs string.h
   string.h needs stdlib.h

One of them must fully see the other before they can use the defined functions
in another header.

Suggestion welcome...

I am thinking of creating a new header just for the forward declarations
where all function declarations live there, we just split off the real
functions' body.

[...]
> 
> This version is suboptimal in terms of code size, CPU usage and memory
> usage. And it even seems it contains a buffer overflow: if the string
> is exactly a multiple of 2048, it seems to me that you'll write the
> trailing zero past the end. Please instead use the more intuitive form
> below (not tested but you get the idea):
> 
> 	size_t len = strlen(str);
> 	char *ret = malloc(len + 1);
> 	if (ret)
> 		memcpy(ret, str, len);
> 	return ret;

Ah right, that's indeed overflow. Will fold this in as you suggested.

[...]
> Here it can cost quite a lot for large values of maxlen. Please just use
> a variant of the proposal above like this one:
> 
> 	size_t len;
> 	char *ret;
> 
> 	len = strlen(str);
> 	if (len > maxlen)
> 		len = maxlen;
> 	ret = malloc(len + 1);
> 	if (ret)
> 		memcpy(ret, str, len);
> 	return ret;

Will take the Alviro's suggestion for this part...

-- 
Ammar Faizi

Re: [RFC PATCH v1 2/6] tools/nolibc: Make the entry point not weak for clang

[Ammar Faizi]

On 3/21/22 2:16 AM, Willy Tarreau wrote:
> Hi Ammar,
> 
> I've had a look at this one.
> 
> On Sun, Mar 20, 2022 at 04:37:46PM +0700, Ammar Faizi wrote:
>> Budilig with clang yields the following error:
>    ^^^^^^^
> BTW please fix the typo in the final commit message.
> 
>> @@ -183,7 +183,9 @@ struct sys_stat_struct {
>>   
>>   /* startup code */
>>   asm(".section .text\n"
>> +#if !defined(__clang__)
>>       ".weak _start\n"
>> +#endif
>>       ".global _start\n"
>>       "_start:\n"
> 
> So it seems that I was wrong and that .weak is an alternate for .global
> and not a complement. As such, instead of adding all these #if, please
> simply remove all .global.

Will fix this in the next version.

-- 
Ammar Faizi

Re: [RFC PATCH v1 6/6] tools/include/string: Implement `strdup()` and `strndup()`

[Willy Tarreau]

On Mon, Mar 21, 2022 at 06:36:37PM +0700, Ammar Faizi wrote:
> On 3/21/22 2:53 PM, Willy Tarreau wrote:
> > Hi Ammar,
> [...]
> > > +static void free(void *ptr);
> > > +static void *malloc(size_t len);
> > > +static void *realloc(void *old_ptr, size_t new_size);
> > 
> > Better include the required h files here.
> 
> I can't do that, in nolibc.h, we have something like this:
> 
> ```
>   #include "stdlib.h" <--- We inlcude string.h from here
>   #include "string.h" <--- This is a no-op.
> ```
> 
> Note, stdlib.h is included first before string.h, next, in stdlib.h, we
> have this:
> 
> ```
>   #include "string.h"
> 
>   // malloc, calloc, free here
> ```
> 
> If I include "stdlib.h" in "string.h", it will just be a no-op, and the
> declarations will not be taken, because the declarations happen after
> #include "string.h". So it doesn't work. It's somewhat circular dependency.
> 
>   stdlib.h needs string.h
>   string.h needs stdlib.h
> 
> One of them must fully see the other before they can use the defined functions
> in another header.

OK, usual stuff indeed.

> Suggestion welcome...

Then just leave it as-is.

> I am thinking of creating a new header just for the forward declarations
> where all function declarations live there, we just split off the real
> functions' body.

That's why I'm doing in my projects for the exact reason above. But
here we only have static functions so this will increase the burden to
contribute. Better wait for the situation to reach a point where we're
certain there will be some benefit in doing that.

Thanks,
Willy

Re: [RFC PATCH v1 2/6] tools/nolibc: Make the entry point not weak for clang

[Nick Desaulniers]

On Mon, Mar 21, 2022 at 4:38 AM Ammar Faizi <ammarfaizi2@gnuweeb.org> wrote:
>
> On 3/21/22 2:16 AM, Willy Tarreau wrote:
> > Hi Ammar,
> >
> > I've had a look at this one.
> >
> > On Sun, Mar 20, 2022 at 04:37:46PM +0700, Ammar Faizi wrote:
> >> Budilig with clang yields the following error:
> >    ^^^^^^^
> > BTW please fix the typo in the final commit message.
> >
> >> @@ -183,7 +183,9 @@ struct sys_stat_struct {
> >>
> >>   /* startup code */
> >>   asm(".section .text\n"
> >> +#if !defined(__clang__)
> >>       ".weak _start\n"
> >> +#endif
> >>       ".global _start\n"
> >>       "_start:\n"
> >
> > So it seems that I was wrong and that .weak is an alternate for .global
> > and not a complement. As such, instead of adding all these #if, please
> > simply remove all .global.

See also:
commit 4d6ffa27b8e5 ("x86/lib: Change .weak to SYM_FUNC_START_WEAK for
arch/x86/lib/mem*_64.S")
commit ec9d78070de9 ("arm64: Change .weak to SYM_FUNC_START_WEAK_PI
for arch/arm64/lib/mem*.S")

Also, please note in the commit message that this diagnostic comes
from using clang as the assembler (which is clang's default behavior
unless -fno-integrated-as is passed).

>
> Will fix this in the next version.
>
> --
> Ammar Faizi
>


-- 
Thanks,
~Nick Desaulniers